Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 17

What's wrong with this code, part 17

  • Comments 29
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.

 

  • 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
Page 1 of 2 (29 items) 12