-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sizeof vs. ARRAYSIZE mismatches in SystemConfigurationProvider::GetSettingsFromLink #11761
Comments
The initial release of the source code had the same bugs already: terminal/src/interactivity/win32/SystemConfigurationProvider.cpp Lines 155 to 162 in d4d59fa
|
Classic - there were tons of these in the codebase when we first inherited the console code. Seems we didn't find all of them. I suspect this isn't the only bug like this. |
Did the codebase use ANSI functions originally? |
I believe it did, but then like, half of it was also surrounded by |
As a rough rule, code that came from Win9x started life as ANSI; code from NT started life as Unicode. The consoles are completely different - this one was Unicode from the start. Looking back at history, what happened here is the code was prepared for the open source release, which involved replacing undocumented NT calls with documented Win32 APIs. One of the differences is the underlying NT APIs often use bytes for buffer size, even for WCHARs. So it looks like the initial change to use SearchPathW still used sizeof, which failed tests, and this was changed to ARRAYSIZE a week later. Unfortunately the return value also needed to be changed. SearchPathW is multiplying the value by sizeof(WCHAR), calling NT, and dividing the result by sizeof(WCHAR), so sizeof was correct for both cases before that change. |
Is there something that can detect bugs like this automatically? The SearchPathW function has a parameter annotated as |
Hello guys, i just submitted the fix for this issue. I did this for a class assignment now it would be great if someone can tell how to find the function wszIconLocation. Thank you |
wszIconLocation is a local variable, not a function. I already posted a link to its definition. The |
Closed this because #12273 was merged and released. |
Windows Terminal version (or Windows build number)
1.12.2931.0
Other Software
No response
Steps to reproduce
Both
sizeof(wszIconLocation)
expressions in the following part ofSystemConfigurationProvider::GetSettingsFromLink
should beARRAYSIZE(wszIconLocation)
, instead.terminal/src/interactivity/win32/SystemConfigurationProvider.cpp
Lines 172 to 179 in 39b72f7
wszIconLocation is a local variable here:
terminal/src/interactivity/win32/SystemConfigurationProvider.cpp
Line 67 in 39b72f7
Expected Behavior
No response
Actual Behavior
In the
dwLinkLen > sizeof(wszIconLocation)
comparison,dwLinkLen
comes from SearchPathW, which returns the number of WCHARs.sizeof(wszIconLocation)
however is the number of bytes. The effect of this mismatch is that, if the string fromSearchPathW
does not fit inwszIconLocation
, thenSystemConfigurationProvider::GetSettingsFromLink
might not notice the problem. In principle, the content of thewszIconLocation
array is then undefined, but the array was filled with zeroes initially and it seems likely that at least one of them is still there and prevents any out-of-bounds read.In the
wcslen(pwszTitle) < sizeof(wszIconLocation)
comparison,wcslen
returns the number ofwchar_t
values, butsizeof(wszIconLocation)
is the number of bytes. The effect of this mismatch is that, ifpwszTitle
is too long,SystemConfigurationProvider::GetSettingsFromLink
might callStringCchCopyW(wszIconLocation, ARRAYSIZE(wszIconLocation), pwszTitle)
regardless. StringCchCopyW would then fail withSTRSAFE_E_INSUFFICIENT_BUFFER
and truncate the string, andSystemConfigurationProvider::GetSettingsFromLink
would attempt to load icons from the truncated path. This does not seem exploitable because anyone providing the overlongpwszTitle
could have provided the truncated string instead.The text was updated successfully, but these errors were encountered: