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

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,                                 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) ||            bufferSize % sizeof(WCHAR) != 0 ||            (valueType != REG_SZ &&             valueType != REG_EXPAND_SZ))        {            returnCode = FALSE;        }        else        {            /*             ** Copy back the data             */            if (valueType == REG_EXPAND_SZ)             {                DWORD requiredBufferSize;                lpszValue[0] = TEXT('\0');                requiredBufferSize = ExpandEnvironmentStringsW(buffer,                    (LPWSTR)lpszValue,                    dwLen);                if ((requiredBufferSize == 0) || (requiredBufferSize > dwLen))                {                    returnCode = FALSE;                }            }            else             {                DWORD cchString;                CopyMemory((PVOID)lpszValue,                            buffer,                            bufferSize);                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.

Comments

• Anonymous
  January 30, 2006
  Actually, I believe there is still a bug: lpszValue[cchString-1] = TEXT('�');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.
• Anonymous
  January 30, 2006
  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.
  
• Anonymous
  January 30, 2006
  The REG_MULTI_SZ regval 'ProductSuite' under HKLMSystemCurrentControlSetControlProductOptions sometimes omits �� termination. (It is sometimes terminated with only a single �.) This is on XP Pro SP2.
• Anonymous
  January 30, 2006
  The comment has been removed
• Anonymous
  January 30, 2006
  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)
  
  
• Anonymous
  January 30, 2006
  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?
• Anonymous
  January 30, 2006
  Norman, I don't get the MSDN search algorithms... Prefast is a static analysis tool shipped with VS 2K5, so...
  
• Anonymous
  February 04, 2006
  The comment has been removed