Hacker News Reader

Comment by 323 | Hacker News Reader

You have buffer overflows in your code, `length` and `length+1` are past the buffer end.

That's code copied from the linked gist. But, it omits some key lines in the middle, and does not actually overflow:

    DWORD required_length;
    auto rc = RegQueryValueExW(key, version, NULL, NULL, NULL, &required_length);
    if (rc != 0)  return NULL;

    DWORD length = required_length + 2;  // The +2 is for the maybe optional zero later on. Probably we are over-allocating.
    wchar_t *value = (wchar_t *)malloc(length);
    if (!value) return NULL;

    rc = RegQueryValueExW(key, version, NULL, NULL, (LPBYTE)value, &length);  // We know that version is zero-terminated...
    if (rc != 0)  return NULL;

    // The documentation says that if the string for some reason was not stored
    // with zero-termination, we need to manually terminate it. Sigh!!

    if (value[length]) {
        value[length+1] = 0;
    }

`length` starts at 2 greater than naively needed, then is updated to the real value (again, always 2 less I guess).

Pray tell, what is the size of a wchar_t on your system?

hah, more than 1 byte for sure, I missed that detail ...

EDIT: I develop on unix systems, not windows, so I'm rusty here ... looking up RegQueryValueExW() the `length` ("lpcbData") is in bytes so everything goes fine until `value[length]` which is indexing by wchar_t (16-bit on windows I guess, 32-bit on linux systems) so it's double the byte offset intended, that's way out.

That only works if someone does not race and modify the content of that registry key between the two function calls. And that, my friend, is how buffer overflow exploits are born.

Don't poke beyond your end. Don't poke using a value that was returned by a function you don't control. The code shown does both. Such quality, it Jonathan Blow.

Yeah, this function has a few surprising bits of sloppyness ... but:

> NOTE(Kalinovcic): I have translated the original implementation to C

Another point, this is a build-time tool: some assumptions of good-faith input are reasonable and necessary. If an attacker can modify paths to visual studio components in your registry, you have bigger problems (just running the attacker's code directly regardless of safe string handling).

I’m sure that’s why this person posted this snippet.

wchar_t is also more than one byte, so the buffer is at most half as big as the code appears to expect.

I thought it was parent's code.

Source seems to take into account the buffer overflow:

    DWORD length = required_length + 2;  // The +2 is for the maybe optional zero later on. Probably we are over-allocating.

Does it? If required_length is 10, it allocates 12 bytes, and writes element 13, which is byte 26 and 27.

There are two off-by-one errors. Should be checking value[length-1].

`RegQueryValueExW`'s last parameter is in and out, so that `length` is set to the actual written length after the call.

It might cause an OOB write though, with a data race on the registry key (if the key's value happens to grow in length by a char or two between the calls, time of check time of use yada yada).

> It might cause an OOB write though

No, because `RegQueryValueExW` will return ERROR_MORE_DATA and the code bails out on error (also leaking the memory).

'by a char or two', they said.

1. first call to `RegQueryValueExW` returns a value length of 10

2. length is set to 12

3. external change causes the value to now be 12

4. second call to ``RegQueryValueExW` succeeds, as 12 <= 12, no ERROR_MORE_DATA here; length stays 12

5. length + 1 and length + 2 are now OOB

[in, out, optional] lpcbData (aka &length)

A pointer to a variable that specifies the size of the buffer pointed to by the lpData parameter, in bytes. When the function returns, this variable contains the size of the data copied to lpData.

In other words, it's rewritten to be the actual length before it's incremented for null termination.

Reply on news.ycombinator.com