Larry Osterman's WebLog

Confessions of an Old Fogey
Blog - Title

What's wrong with this code, part 17 - the answer

What's wrong with this code, part 17 - the answer

  • Comments 8

Yesterday's post discussed a hypothetical API to retrieve data from the registry.  The security hole in the original code is that if the value in the registry is exactly 512 bytes long, the buffer isn't null terminated.  That means that the caller, who is expecting a null terminated string, won't always get a null terminated string.  As 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.)


There's another, more subtle problem, the routine's parameters (in particular the lpszValue parameter) isn't SAL annotated.  This means that static analysis tools like Prefast can't really correctly analyze the function.  So the developer fixed the security bug by ensuring that the returned string is null terminated.

BOOL GetStringValueFromRegistry(HANDLE KeyHandle,
                                    LPCWSTR ValueName,
                                    ULONG dwLen,
                                    __out_ecount(dwLen) LPWSTR lpszValue)
    BOOL returnCode;
    WCHAR buffer[512];
    DWORD bufferSize = sizeof(buffer);
    DWORD valueType;
    returnCode = RegQueryValueExW(KeyHandle,
                                 &bufferSize) == ERROR_SUCCESS;

    if (returnCode) {
         ** Check we got the right type of data and not too much

        if (bufferSize > dwLen * sizeof(WCHAR) ||
            bufferSize % sizeof(WCHAR) != 0 ||
            (valueType != REG_SZ &&
             valueType != REG_EXPAND_SZ))
            returnCode = FALSE;
             ** Copy back the data
            if (valueType == REG_EXPAND_SZ)
                DWORD requiredBufferSize;
                lpszValue[0] = TEXT('\0');
                requiredBufferSize = ExpandEnvironmentStringsW(buffer,
                if ((requiredBufferSize == 0) || (requiredBufferSize > dwLen))
                    returnCode = FALSE;
                DWORD cchString;
                cchString = bufferSize/ sizeof(WCHAR);
                WinAssert(cchString < dwLen);
                lpszValue[cchString-1] = TEXT('\0');
    return returnCode;

Mea Culpas:  Raymond caught the fact that function won't compile if you don't #define UNICODE because it doesn't explicitly call the W version of the RegQueryValueEx API.  He also noticed that the code doesn't check for failure in ExpandEnvironmentStringsW.  Both fixes are applied above. In the "not a bug, but wierd" category, he noted that the function will never fill more than 256 characters in the output buffer, which needs to be clearly documented.

One final mea culpa: When I originally wrote this code, I wanted to show off how the above fix was itself broken.  Unfortunately I can't, because I believe that this code is correct :).  The original code from which this example was taken was  broken but in rewriting this for publication, I inadvertently fixed the 2nd bug (the original code used other APIs than the APIs shown in this example).  I'm going to try to come up with a similar example using other APIs that will show the two step problem.

Vassili Bourdo also caught the ExpandEnvironmentStrings issue

Kudos: Skywing found the root problem - that the length of the returned string isn't correctly checked and the string isn't correctly null terminated.

Other comments:

DanT questioned the security hole issue.  This is a security hole, but it requires another piece of code to call strcpy on the returned data.  But this is the root cause of that problem - if it had returned a null terminated string, then the other code wouldn't be a security hole.  That's how root cause analysis works - you find the root of the bug, not just the code that's in error.  In hindsight, the RegQueryValueEx API should have been fixed, but since that function was introduced in NT 3.1, it's too late to make such a sweeping change to the API - stuff WILL break if the fix is applied at that level.  That's why RegGetValue was introduced - it fixes the problem entirely.

  • Actually, I believe there is still a bug: lpszValue[cchString-1] = TEXT('\0'); That statement truncates the last character if the string is 256 characters. Also it is pointless if the string is less than 256 characters, because it will already have a null.
  • aa: That's absolutely true. But it ensures that the routine's contract (returning a null terminated string) is enforced.

    One alternative would be to fail if the value wasn't null terminated, but that's not what was done here.

  • The REG_MULTI_SZ regval 'ProductSuite' under HKLM\System\CurrentControlSet\Control\Prod uctOptions sometimes omits \0\0 termination. (It is sometimes terminated with only a single \0.) This is on XP Pro SP2.
  • Can guaranteeing termination ever cause a security problem? If the string was too long, and, to ensure termination, the string is truncated, then you could be introducing a subtle bug. Suppose the Registry string that comes back has the path to a DLL you want to load. Say "C:\...\Foob". If the string is truncated to fit into a buffer (rather than returning an error), you might end up with "C:\...\Foo". Malicious software could leave a corrupted DLL in Foo and convince your program to load the wrong one. I believe your example will chop the last character off in order to return a properly terminated string. I realize this is a bit contrived, but it's not out of the question. It reminds me of the _Raiders of the Lost Ark_. To find the Ark, one had to construct a staff of a specific length given by the inscription on an amulet. Bellock (sp?) made the staff too long, because he got a truncated copy of the inscription (only the beginning from the front side of the amulet). The back side said to subtract a certain amount off from the length. Truncation of the message caused the Germans to look in the wrong place.
  • If there is a length limit to registry string values, the writing and reading methods should agree on it.

    And yes, errors should be fatal (no value returned) rather than silent (the right value was too long, so here's a wrong value)

  • I was surprised to see your link to "Prefast" point to the Windows CE version instead of server or desktop version. But then searching MSDN for "prefast analysis tool", 100% of the matches found were in the Windows CE version. I'm pretty sure that a tool I used last year in the DDK or WDK was also called "Prefast". That was for a driver targeting Windows XP not CE. Is there a "Prefast" for user mode programs in Windows server or desktop environments?
  • Norman, I don't get the MSDN search algorithms... Prefast is a static analysis tool shipped with VS 2K5, so...

  • OK, so it strictly passes verification, but it's still not right on the edge case.

    The correct response to the 512-character return from RegQVEx should be to fail, just as a 513-character return would. If it doesn't fit in the buffer, you can't just cut off the last character! This kind of unpredictable behaviour is what leads to security issues - what if this registry value contained a SID at the end, and cutting off the last digit resulted in a different SID?

    Furthermore, if a dwLen of 512 or above is passed, and the value is a 512-byte REG_EXPAND_SZ, it will pass it *unterminated* to EES. What the buffer then spills over into is up to the compiler (I think it would normally be bufferSize, so you'll get \u0200 stuck on the end), and could be any amount of gibberish. Again, it's also a bit unfair to return nothing when dwLen is enough for the "expanded" string, but not the unexpanded.

    And why do you have an assert that bufferSize < dwLen * sizeof(WCHAR)? What if the caller guessed the right length?

    Sorry to be nit-picky... :-)

Page 1 of 1 (8 items)