1348843 - uninitialized local variable in GetRegWindowsAppDataFolder function

Status: RESOLVED DUPLICATE of bug 1350460

(Toolkit :: Startup and Profile System, defect)

Description

[Xiaoyin Liu]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.79 Safari/537.36 Edge/14.14393




Expected results:

The GetRegWindowsAppDataFolder function, defined in SpecialSystemDirectory.cpp, tries to find the current user's APPDATA or LOCALAPPDATA folder by querying Windows Registry. In this function, the last parameter passed to RegQueryValueExW, |size|, is not initialized. Therefore, the call to RegQueryValueExW may overflow the local buffer |path|, if |size| happens to be larger than the buffer length, and the registry value of "HKCU\SOFTWARE\Microsoft\Windows\CurrentVersion\Explorer\Shell Folders" is longer than the buffer length.

This bug is hardly exploitable because it would require attackers to modify a registry key, which is impossible unless they can already execute arbitrary code on the victim's system, so feel free to make this report unrestricted. I make it hidden since I am not 100% sure whether there is a potential security problem.

I will attach a patch.

Comment 1

[Xiaoyin Liu]

Found another uninitialized variable passed to RegQueryValueExW:
 * toolkit\crashreporter\client\crashreporter_win.cpp: UIGetSettingsPath

Document of RegQueryValueExW: https://msdn.microsoft.com/en-us/library/windows/desktop/ms724911(v=vs.85).aspx

Comment 2

[Xiaoyin Liu]

Attachment: initialize local variables before calling RegQueryValueExW

The local variable |size| in function UIGetSettingsPath and
GetRegWindowsAppDataFolder were not initialized to the size of input buffer
before calling RegQueryValueExW, which may cause buffer overflow if the
registry value is longer than the buffer size.
This patch sets size = sizeof(path).

Comment 3

[Xiaoyin Liu]

Attachment: initialize local variables before calling RegQueryValueExW

The local variable |size| in function UIGetSettingsPath and
GetRegWindowsAppDataFolder were not initialized to the size of input buffer
before calling RegQueryValueExW, which may cause buffer overflow if the
registry value is longer than the buffer size.
This patch sets size = sizeof(path).

Comment 4

[Robert Strong (they/them - no direct email)]

Comment on attachment 8849184 [details] [diff] [review]
initialize local variables before calling RegQueryValueExW

Punting this over to Matt

Comment 5

[Molly Howell (she/her) (mostly inactive)]

Comment on attachment 8849184 [details] [diff] [review]
initialize local variables before calling RegQueryValueExW

Review of attachment 8849184 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good! Thanks for taking care of this.

Comment 6

[Xiaoyin Liu]

Thank you for reviewing. Unfortunately because of a different issue I discovered, this issue has to be fixed as part of Bug 1350460 and Bug 1352140. So I am closing this bug as duplicated.