1350460 - (CVE-2017-7790) Non-null-terminated strings in crashreporter_win.cpp may potentially disclose memory

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, enhancement)

Description

[Xiaoyin Liu]

In toolkit\crashreporter\client\crashreporter_win.cpp, function GetStringValue, a Windows registry value is queried. If the type is REG_SZ (i.e. a string), it copies the content of a wchar_t stack buffer to a wstring object, which is returned to the caller.

However, in Microsoft's document [1], it warns us:
> 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 terminating
> null characters. 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.

In our case, because we don't prefill the |buf| with all-zero, suppose the data we read is not null-terminated, then the wstring will contain the stack memory data (which may include old eip value), until it reaches a NULL.

Whether this issue is exploitable depends on how the caller uses the wstring object and what registry value it queries. But no matter what, not ensuring the string is null-terminated is problematic.

The doc suggests to use RegGetValue function.

[1] https://msdn.microsoft.com/en-us/library/windows/desktop/ms724911(v=vs.85).aspx

Comment 1

[Xiaoyin Liu]

And there are a lots of others, like:
 * xpcom\io\SpecialSystemDirectory.cpp: GetRegWindowsAppDataFolder: |path| may not be null-terminated, and thus 
         int len = wcslen(path);
   may not give right value.

   After that,
         path[len]   = L'\\';
   may overwrite a 2-byte memory at unintended address.

I will search it more thoroughly if you confirm this is a vulnerability.

Comment 2

[:Gijs (he/him)]

Gabriele or Jim, can you take a look?

Comment 3

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Ensure that strings read from the registry are nul-terminated

Clearing the destination strings should be enough to prevent any issues.

Comment 4

[Xiaoyin Liu]

It seems the patch you just attached isn't for this bug. That is for Bug 1345153 - Save crash pings to disk so that if they could not be sent by the pingsender they're persisted for later

Comment 5

[Gabriele Svelto [:gsvelto]]

Ouch, sorry, I must have used the wrong revision when he export'ing the patch.

Comment 6

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] Ensure that strings read from the registry are nul-terminated

This is the right one.

Comment 7

[Xiaoyin Liu]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> Created attachment 8851319 [details] [diff] [review]
> [PATCH] Ensure that strings read from the registry are nul-terminated
> 
> This is the right one.

Thank you, Gabriele! But I think there is still a small problem: if RegQueryValueEx copies exactly 2048 wide chars to |buf| and it is not null-terminated, then constructing wstring will still over-read.

This can be fixed by setting dataSize = sizeof(buf) - 1; which ensures buf has at least one \0 at the end.

Comment 8

[Gabriele Svelto [:gsvelto]]

(In reply to Xiaoyin Liu from comment #7)
> Thank you, Gabriele! But I think there is still a small problem: if
> RegQueryValueEx copies exactly 2048 wide chars to |buf| and it is not
> null-terminated, then constructing wstring will still over-read.
> 
> This can be fixed by setting dataSize = sizeof(buf) - 1; which ensures buf
> has at least one \0 at the end.

Nice catch, I hadn't thought of that, updated patch coming.

Comment 9

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] Ensure that strings read from the registry are nul-terminated

Updated patch, I also noticed that in the invocation of RegQueryValueExW() we were passing the size parameter uninitialized, ouch.

Comment 10

[Xiaoyin Liu]

(In reply to Gabriele Svelto [:gsvelto] from comment #9)
> Created attachment 8851330 [details] [diff] [review]
> [PATCH v2] Ensure that strings read from the registry are nul-terminated
> 
> Updated patch, I also noticed that in the invocation of RegQueryValueExW()
> we were passing the size parameter uninitialized, ouch.

Actually I had filed Bug 1348843 for the uninitialized parameter issue. I didn't realized this null-terminated string issue back then. Do need to merge that bug to this one?

Comment 11

[Gabriele Svelto [:gsvelto]]

(In reply to Xiaoyin Liu from comment #10)
> Actually I had filed Bug 1348843 for the uninitialized parameter issue. I
> didn't realized this null-terminated string issue back then. Do need to
> merge that bug to this one?

Yes, there's no point in clearing the string if we leave that parameter uninitialized.

Comment 12

[(not currently active) Ted Mielczarek]

FWIW this seems pretty unlikely to be exploitable given that we only query two fixed registry keys ("Software\\Mozilla\\Crash Reporter" and possibly "Software\\Microsoft\\Windows\\CurrentVersion\\Explorer\\Shell Folders" if SHGetFolderPath fails).

Additionally, this code is in the crashreporter client which only runs when the entire Firefox process has crashed, not the Firefox binary itself.

Thanks for the bug report!

Comment 13

[(not currently active) Ted Mielczarek]

Comment on attachment 8851330 [details] [diff] [review]
[PATCH v2] Ensure that strings read from the registry are nul-terminated

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

Humorously, I have https://tonyarcieri.com/it-s-time-for-a-memory-safety-intervention open in the tab right next to this one!

Comment 14

[Daniel Veditz [:dveditz]]

sec-low for this specific issue. I've cloned bug 1352140 for the more general investigation of other places we need to fix.

Comment 16

[Gabriele Svelto [:gsvelto]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d673a07d5aed14620976f5d0ff07b5e9842b7067

Sorry for the delay here but I was on parental leave the past two weeks.

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba5a351909cc

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Doesn't seem worth backporting given comment 12 and comment 14. Feel free to set things back to affected and request approval if you feel otherwise, though.