5 ms·
wchar_t *value = (wchar_t *)malloc(length); // The documentation says that if the string for some reason was not stored with zero-termination, we need to manua
by malft 5y ago
wchar_t *value = (wchar_t *)malloc(length);
// 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; }
- 323 5y agoYou have buffer overflows in your code, `length` and `length+1` are past the buffer end.
- creato 5y agoI’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.
- 323 5y agoI 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.
- malf 5y agoDoes it? If required_length is 10, it allocates 12 bytes, and writes element 13, which is byte 26 and 27.
- ectopod 5y agoThere are two off-by-one errors. Should be checking value[length-1].
- seritools 5y ago`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).
- 323 5y ago> 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).
- malft 5y ago'by a char or two', they said.
- seritools 5y ago1. 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
- lkey 5y ago[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.
- ploxiln 5y agoThat'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).
- malft 5y agoPray tell, what is the size of a wchar_t on your system?
- ploxiln 5y agohah, 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.
- pierrebai 5y agoThat 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.
- ploxiln 5y agoYeah, 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).
- deleted 5y ago[deleted]
- CGamesPlay 5y agoThere's a comment highlighting this at the bottom of the gist, and apparently an updated version.