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.