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.