What's wrong with this code, part 17

Time for another "What's wrong with this code".  This time, it's an exercise in how a fix for a potential security problem has the potential to go horribly wrong.  This is a multi-part bug, so we'll start with the original code.

We start the exercise with some really old code:

BOOL GetStringValueFromRegistry(HKEY KeyHandle,                    LPCWSTR ValueName,                    ULONG dwLen,                    LPWSTR lpszValue){    BOOL returnCode;    WCHAR buffer[256];    DWORD bufferSize = sizeof(buffer);    DWORD valueType;    returnCode = RegQueryValueEx(KeyHandle,                                                            ValueName,                                                            NULL,                                                            &valueType,                                                            (LPBYTE)buffer,                                                            &bufferSize) == ERROR_SUCCESS;    if (returnCode) {        /*         ** Check we got the right type of data and not too much         */        if (bufferSize > dwLen * sizeof(WCHAR) ||            (valueType != REG_SZ &&             valueType != REG_EXPAND_SZ))        {            returnCode = FALSE;        }        else        {            /*             ** Copy back the data             */            if (valueType == REG_EXPAND_SZ)             {                lpszValue[0] = TEXT('\0');                ExpandEnvironmentStringsW(buffer,                                        (LPWSTR)lpszValue,                                        dwLen);            }            else             {                CopyMemory((PVOID)lpszValue,                            (PVOID)buffer,                            dwLen * sizeof(WCHAR));            }        }    }    return returnCode;}

There's a security hole in this code, but it's not really obvious.  If you've been paying attention and it's blindingly obvious what's going on, please give others a chance :)

As always, kudos and mea culpas on each step of the way.

Comments

• Anonymous
  January 27, 2006
  The comment has been removed
• Anonymous
  January 27, 2006
  It is blindingly obvious, the article explaining the issue is only a few entries down :)
• Anonymous
  January 27, 2006
  if (returnCode) {
  
  should be
  
  if (returnCode == ERROR_SUCCESS) {
• Anonymous
  January 27, 2006
  The comment has been removed
• Anonymous
  January 27, 2006
  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 13sizeof(TCHAR) bytes. However, if the buffer is 12sizeof(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.)"
  
• Anonymous
  January 27, 2006

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.

• Anonymous
  January 27, 2006
  The comment has been removed
• Anonymous
  January 27, 2006
  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)));
  
  
• Anonymous
  January 27, 2006
  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.
• Anonymous
  January 27, 2006
  The comment has been removed
• Anonymous
  January 27, 2006

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++
   

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

• Anonymous
  January 27, 2006
  The comment has been removed

• Anonymous
  January 27, 2006
  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. :)

• Anonymous
  January 27, 2006
  The comment has been removed

• Anonymous
  January 27, 2006
  The comment has been removed

• Anonymous
  January 27, 2006
  The comment has been removed

• Anonymous
  January 27, 2006
  The comment has been removed

• Anonymous
  January 27, 2006
  The comment has been removed

• Anonymous
  January 27, 2006
  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.

• Anonymous
  January 27, 2006
  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.
  

• Anonymous
  January 27, 2006
  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

• Anonymous
  January 27, 2006
  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.

• Anonymous
  January 27, 2006
  The comment has been removed

• Anonymous
  January 29, 2006
  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.

• Anonymous
  January 30, 2006
  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.

• Anonymous
  January 30, 2006
  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.

• Anonymous
  March 13, 2007
  PingBack from http://winblogs.security-feed.com/2006/01/27/whats-wrong-with-this-code-part-17/

• Anonymous
  March 13, 2007
  PingBack from http://winblogs.security-feed.com/2006/01/27/whats-wrong-with-this-code-part-17/