Larry Osterman's WebLog

Let's not call all sorts of bugs potential security problems just because security is a hot issue right now. Most bugs can be potential security problem, let's stick to the old style, most descriptive terms like buffer overrun.

It is blindingly obvious, the article explaining the issue is only a few entries down :)

if (returnCode) {

should be

if (returnCode == ERROR_SUCCESS) {

Crap, nevermind. Didn't see == ERROR_SUCCESS at the end of the RegQueryValueEx call.

Hm, I doubt it works at all...
RegQueryValueEx() - is non-unicode, whereas
ExpandEnvironmentStringsW() is unicode...

Another bug is that registry value is placed to in fixed buffer.

MSDN says: "If the data has the REG_SZ, REG_MULTI_SZ or REG_EXPAND_SZ type, the string may not have been stored with the proper null-terminating characters. For example, if the string data is 12 characters and the buffer is larger than that, the function will add the null character and the size of the data returned is 13*sizeof(TCHAR) bytes. However, if the buffer is 12*sizeof(TCHAR) bytes, the data is stored successfully but does not include a terminating null. Therefore, even if the function returns ERROR_SUCCESS, the application should ensure that the string is properly terminated before using it; otherwise, it may overwrite a buffer. (Note that REG_MULTI_SZ strings should have two null-terminating characters, but the function only attempts to add one.)"

1. Doesn't check that the returned string is null terminated.
2. Doesn't check that the returned string is of a length that is a multiple of WCHARs.
3. Relies on an implementation detail of ExpandEnvironmentStringsW and assumes that it won't use the output buffer as scratch data and will leave it untouched if the function fails.

Also, this code will break badly if UNICODE is not defined. It's mixing "W" functions with the UNICODE-flag-dependant versions, so without the proper preprocessor options the function will fail to compile. Not exactly a "bug" per se, but bad coding practice.

Not sure about the security hole. Obviously it won't compile without UNICODE defined.

My guess is along the lines of the ExpandEnvironmentStrings function. If that function fails, there is no guarantee the return will be null terminated. Generally I believe the output of Win32 string functions is undefined on fail, so the initial [0] = 0 won't save you.

That's all I got for now.

I think I see 2 problems with this...

1) RegQueryValueEx is used which could map to RegQueryValueExA or RegQueryValueExW. Could be a typo, but as ExpandEnvironmentStringsW is explicitly UNICODE maybe not and passing an ANSI string to ExpandEnvironmentStringsW should give you garbage, and passing it to CopyMemory gives back the caller a string that is essentially garbage.

2) CopyMemory((PVOID)lpszValue,
(PVOID)buffer,
dwLen * sizeof(WCHAR));

If the caller passed a buffer larger than sizeof(buffer) this will end up copying stack data beyond 'buffer'. Shouldn't it be... ?

CopyMemory((PVOID)lpszValue,
(PVOID)buffer,
min(bufferSize, (dwLen * sizeof(WCHAR)));

lpszValue is not first checked to make sure it is not null before trying to assign a value to it.

ExpandEnvironmentStringsW is asking for the number of characters lpszValue can hold, not the length in bytes of the buffer. So by passing 1024, you are saying that the 512 character UNICODE buffer can hold 1024 UNICODE characters.

I'll have a stab at it ...

First, its not known whether or not the code was compiled for UNICODE or not, I'll assume it is not, since ExpandEnvironmentStringsW was explicitly called.

1. bufferSize = sizeof(buffer) is the size in wchar's, not bytes, so bufferSize starts life equalling 128 and not 256.

2. RegQueryValueEx is called, possibly the A version, which will write an ANSI string into the wchar buffer.

2a. RegQueryValueEx given a buffer size of 128, may not null terminate the string if the string happens to fit exactly in the buffer with no room for a null at the end.

3. lpszValue[0] = TEXT('\0') doesn't check if lpszValue is non-NULL first, and doesn't check dwLen to make sure its not zero bytes.

4. CopyMemory() also same problem of not checking for a non-null lpszValue, but at least if dwLen is zero, nothing is copied.

1. CopyMemory is given the wrong length (destination's instead of source's which at this point is *not* known to be less than destination's). It may copy whatever garbage is there in buffer after the read value. Or it may copy from whatever is in memory *after* the buffer.
2. You don't check return value of ExpandEnvironmentStrings. There may be not enough space in the output buffer in which case the function will fail. What will happen depends on whether ExpandEnvironmentStrings is in habit of garbaging the output buffer on failure. If it does you will return true with garbage in the buffer
3. The use of TEXT and RegQueryValueEx contradicts use of wchar_t and ExpandEnvironmentStringsW. Not necessarily an error but very poor practice.

Moral: switch to C++

Oops! Correction: bufferSize is the size in BYTES not wchar's, which means bufferSize starts out life at 512 bytes instead of 256.

As for fixes, I might do something like this:

1. Fix ExpandEnvironmentStringsW usage.

1a. If the actual size returned by ExpandEnvironmentStringsW (with null terminator) is too large for the caller supplied buffer, then 1) fail the request, or 2) allocate a buffer of the appropriate size, do the expansion again (and verify the return value on the second call!), and return a truncated string depending on how the caller expects the routine to work (this wasn't provided in the information you gave).

2. Explicitly verify that the value returned by RegQueryValueExW is a well formed null terminated Unicode string. In addition to the existing checks...

2a. If the length is not a multiple of sizeof(WCHAR), fail the function.
2b. If the length is equal to the given buffer length, then check the last WCHAR in the returned string. Depending on how the function is supposed to work, you could either 1) truncate the string and turn the last character into L'\0' or 2) fail the function call.
2c. Check buffer[bufferSize/sizeof(WCHAR)-1] for a L'\0'. If not found, either 1) write a L'\0' to the next character, or 2) fail the function call. This is dependent on how the function is supposed to work. Again, because you did not provide a sufficient description of the function, I'm not sure which behavior the caller would be expecting.

The main piece of information missing from your function documentation is whether this function is supposed to try and fix invalid input (missing null terminator, string was too long and needs to be truncated, etc...) or just fail immediately without trying.

BTW, allowing the caller to supply a buffer length and then limiting the returned string (except in the case of expanded strings) to 256 characters including terminator is a bit silly. I would say that the function is arguably buggy in this respect, and should attempt to honor the callers buffer size by allocating instead of capping it at 256 characters under some circumstances.

On my last post I was thinking dwLen was the bufferSize variable, I didn't notice that dwLen was an argument. So my last post (which is still being moderated) I was completely off track. :)

So ignore my last post. :)

Ouch! Not obvious?

BTW, should the cast on the EESW line read LPTSTR?

Mark

I might have not been totally off track.... Is the not doing a simple check for lpszValue==NULL still valid?

Also it seems that the checking of returnCode is invalid. RegQueryValueEx returns ERROR_SUCCESS (which is 0) if successful. So the code is performing what it is deamed a successful operation only on failures.

RegQueryValueEx does not guarantee the resulting string in buffer is NULL terminated.

ExpandEnvironmentStringsW says to pass in a NULL terminated string as the first parameter, but doesn't say what happens if you do not. I'm guessing there's room for problems there.

When copied back to lpszValue, the calling function may not check for a NULL and cause a buffer overflow.

The use of CopyMemory in the bottom else should use the smaller of dwLen * sizeof(WCHAR) or bufferSize, since anything still in buffer past bufferSize is not defined. Also, if lpszValue is larger than buffer (which is not hard since buffer is only 256 characters), then CopyMemory is going to copy extra stuff well beyond buffer. I'm guessing that's the security hole.

How'd I do?

Nowhere is dwLen actually compared with what buffer[] can hold (256 WCHARs), which has the potential for overflow in CopyMemory.

CopyMemory is copying dwLen "bytes" when it should only be copying bufferSize (the size indicated by RegQueryValue) "bytes".

The same can be said of ExpandEnvironmentString, it's using dwLen as it's size instead of bufferSize.

While there is a check for bufferSize>dwLen, there is no recipricol check for bufferSize<dwLen, which is, IMHO, the security hole here.

Also, there's something that strikes me as odd about casting an array of WCHAR to LPBYTE (BYTE *). I can't back that up right now, but it's just, I dunno, odd. And recasting lpszValue to LPWSTR when it's already that type isn't necessary, is it?

Jeremy Boschen: Good catch there with the potentially copying from past the local stack space allocated for the function. It would never overflow the callers buffer with that CopyMemory call, but it is conceivable that you could cause the thread to die with a stack overflow if you passed a larger buffer length as dwLen than the thread has remaining stack space. This is definitely a bug and should be fixed, although in practice I think it would be highly unlikely for this to turn into an exploitable bug. You never know without having access to the rest of the programs source code, though.

Of course, that's a bit architecture specific, but at least on x86 and x64 (can't say much about IA64, as I don't have much experience with it) I think it would be an extremely unlikely occurance that you would suffer any ill effects from that bug.

If the code in question was modified to allocate the buffer on the heap instead of using a stack based buffer for the buffer passed to RegQueryValueEx, this would be much more likely to crash, especially if you had the appropriate page heap options enabled. By default on x86/x64 though, you would likely have to pass an enourmous (1MB or so) buffer length to trip the problem, and I suspect this function is probably going to be used in scenarios where you have buffer lengths known at compile time (given that it has no provision to return to the caller what kind of buffer size it should expect for the requested key) and those are probably not going to be in excess (or anywhere near) 1MB.

GetStringValueFromRegistry seems like a general purpose helper to read strings from registry. However, it has a severe limitation that it doesn't return any strings greater than 256 bytes. Given that this is compiled as UNICODE, it will read only 128 UNICODE chars. This behavior is not obvious from the interface. Moreover, when it reads exactly 128 chars, the returned string won't be NULL terminated.

Actually, scratch the LPTSTR thing - if this was written during the transition to W, there's an argument for making it clear (I think it should compile without UNICODE).

But the function's just a bit messy - in addition to Skywings points, EES should be called before checking the length (expanded environment strings can be shorter as well as longer).

And if we're talking about hidden behaviour, what if an lpszValue of length 0x80000040 is passed in? It shouldn't happen, but could (I think)!

Great series,

Mark

The CopyMemory issue that Jeremy Boschen mentioned seems like a big danger as well. If I send in a buffer that's 64KB and a corresponding dwLen=65536, a huge chunk of stack will be copied to the buffer--although I suppose it might do something worse.

The dwLen argument really grates on me. First, I tend to think of the Length of something as the dynamic/current size versus a Count or Size which is the static/max size. Second, the Hungarian says it's a DWORD but it's declared as ULONG--sure I know there's no difference to the compiler. Finally, the name gives no indication that it's a character count versus a byte count. If you're going to use Hungarian--even the parameters aren't consistent--it would be better to use cchSize or something to that effect.

The security hole is that, if dwLen is large (much larger than sizeof(buffer)), you have information leakage.
The function is copying data from its own stack into memory that it doesn't control. Since the stack grows down in memory, the CopyMemory may pick up local variables from functions higher up the call stack, which may contain sensitive info (character arrays with usernames/passwords, for example).

The actual security problems are already beaten to death, but there still seems to be confusion about the other bugs. Mixture of TCHAR style and wchar_t style in a source file that's only supposed to be compilable as Unicode isn't a security problem by itself. It's bad style, it's a warning sign that the programmer needs more training, it can potentially contribute to security problems when maintainers guess wrong about the original programmer's intentions, but by itself it is not a security problem. A source file that doesn't compile (as this one would be if attempted as ANSI) isn't a security problem by itself. If no executable file is produced from it, then no exploitable executable file is produced from it. Though again it can contribute to security problems if the programmer doesn't notice they still have an old executable hanging around because it didn't get replaced, or if maintainers guess wrong about the original programmer's intentions.

Mike Dunn: Not sure that I agree with that. The buffer would already still be filled with uninitialized, potentially sensitive information when it was allocated by the caller (either on the heap or the stack) - even if this function only copied the exact amount necessary to have the full string and null terminator. So either way, you still are going to very probably have uninitialized data following the terminator. This is a very common thing in practice and is absolutely harmless unless you have another bug elsewhere that sends a fixed size buffer to say a client instead of just a null terminated string.

Skywing, consider that the caller passed a very large buffer and this function is called with less stack space than the caller allocated buffer. If this function doesn't have read access to the data beyond the stack it's going to cause an access violation.

PingBack from http://winblogs.security-feed.com/2006/01/27/whats-wrong-with-this-code-part-17/

PingBack from http://winblogs.security-feed.com/2006/01/27/whats-wrong-with-this-code-part-17/