sizeof vs. ARRAYSIZE mismatches in SystemConfigurationProvider::GetSettingsFromLink

[KalleOlaviNiemitalo]

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 of SystemConfigurationProvider::GetSettingsFromLink should be ARRAYSIZE(wszIconLocation), instead.

terminal/src/interactivity/win32/SystemConfigurationProvider.cpp

Lines 172 to 179 in 39b72f7

	// search for the application along the path so that we can load its icons (if we didn't find one explicitly in
	// the shortcut)
	const DWORD dwLinkLen = SearchPathW(pwszCurrDir, pwszAppName, nullptr, ARRAYSIZE(wszIconLocation), wszIconLocation, nullptr);
	// If we cannot find the application in the path, then try to fall back and see if the window title is a valid path and use that.
	if (dwLinkLen <= 0 || dwLinkLen > sizeof(wszIconLocation))
	{
	if (PathFileExistsW(pwszTitle) && (wcslen(pwszTitle) < sizeof(wszIconLocation)))

wszIconLocation is a local variable here:

terminal/src/interactivity/win32/SystemConfigurationProvider.cpp

Line 67 in 39b72f7

WCHAR wszIconLocation[MAX_PATH] = { 0 };

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 from SearchPathW does not fit in wszIconLocation, then SystemConfigurationProvider::GetSettingsFromLink might not notice the problem. In principle, the content of the wszIconLocation 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 of wchar_t values, but sizeof(wszIconLocation) is the number of bytes. The effect of this mismatch is that, if pwszTitle is too long, SystemConfigurationProvider::GetSettingsFromLink might call StringCchCopyW(wszIconLocation, ARRAYSIZE(wszIconLocation), pwszTitle) regardless. StringCchCopyW would then fail with STRSAFE_E_INSUFFICIENT_BUFFER and truncate the string, and SystemConfigurationProvider::GetSettingsFromLink would attempt to load icons from the truncated path. This does not seem exploitable because anyone providing the overlong pwszTitle could have provided the truncated string instead.

[KalleOlaviNiemitalo]

The initial release of the source code had the same bugs already:

terminal/src/interactivity/win32/SystemConfigurationProvider.cpp

Lines 155 to 162 in d4d59fa

	// search for the application along the path so that we can load its icons (if we didn't find one explicitly in
	// the shortcut)
	const DWORD dwLinkLen = SearchPathW(pwszCurrDir, pwszAppName, nullptr, ARRAYSIZE(wszIconLocation), wszIconLocation, nullptr);
	// If we cannot find the application in the path, then try to fall back and see if the window title is a valid path and use that.
	if (dwLinkLen <= 0 || dwLinkLen > sizeof(wszIconLocation))
	{
	if (PathFileExistsW(pwszTitle) && (wcslen(pwszTitle) < sizeof(wszIconLocation)))

[zadjii-msft]

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.

[KalleOlaviNiemitalo]

Did the codebase use ANSI functions originally?

[zadjii-msft]

I believe it did, but then like, half of it was also surrounded by #ifdef CJK or something, which was a total mess, so a lot of the ANSI and wchar support all got merged into one codebase when we first inherited it.

[malxau]

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.

[KalleOlaviNiemitalo]

Is there something that can detect bugs like this automatically? The SearchPathW function has a parameter annotated as _Out_writes_to_opt_(nBufferLength,return + 1) LPWSTR lpBuffer, from which an analyzer could deduce that the return value counts WCHARs and does not make sense to compare to sizeof a WCHAR array.

[abdoulkkonate]

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

[KalleOlaviNiemitalo]

wszIconLocation is a local variable, not a function. I already posted a link to its definition.

The wsz prefix is Microsoft's traditional "Hungarian notation" and means it is a null-terminated string of wide characters.

[KalleOlaviNiemitalo]

Closed this because #12273 was merged and released.