Bug 1350460 (CVE-2017-7790)

Non-null-terminated strings in crashreporter_win.cpp may potentially disclose memory

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: xiaoyin.l, Assigned: gsvelto)

Tracking

(Blocks 1 bug, {csectype-disclosure, sec-low})

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr45 wontfix, firefox52 wontfix, firefox-esr52 wontfix, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [adv-main55+][post-critsmash-triage])

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
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
Reporter

Comment 1

2 years ago
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.
Reporter

Updated

2 years ago
Assignee: xiaoyin.l → nobody

Comment 2

2 years ago
Gabriele or Jim, can you take a look?
Flags: needinfo?(jmathies)
Flags: needinfo?(gsvelto)
Assignee

Comment 3

2 years ago
Clearing the destination strings should be enough to prevent any issues.
Assignee: nobody → gsvelto
Status: NEW → ASSIGNED
Flags: needinfo?(gsvelto)
Attachment #8851232 - Flags: review?(ted)
Reporter

Comment 4

2 years ago
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
Assignee

Comment 5

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

Comment 6

2 years ago
This is the right one.
Attachment #8851232 - Attachment is obsolete: true
Attachment #8851232 - Flags: review?(ted)
Attachment #8851319 - Flags: review?(ted)
Reporter

Comment 7

2 years ago
(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.
Assignee

Comment 8

2 years ago
(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.
Assignee

Comment 9

2 years ago
Updated patch, I also noticed that in the invocation of RegQueryValueExW() we were passing the size parameter uninitialized, ouch.
Attachment #8851319 - Attachment is obsolete: true
Attachment #8851319 - Flags: review?(ted)
Attachment #8851330 - Flags: review?(ted)
Reporter

Comment 10

2 years ago
(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?
Assignee

Comment 11

2 years ago
(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.

Updated

2 years ago
Flags: needinfo?(jmathies)
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 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!
Attachment #8851330 - Flags: review?(ted) → review+
Blocks: 1352140
sec-low for this specific issue. I've cloned bug 1352140 for the more general investigation of other places we need to fix.
Reporter

Updated

2 years ago
Duplicate of this bug: 1348843
Assignee

Comment 16

2 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/ba5a351909cc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Group: toolkit-core-security → core-security-release
Whiteboard: [adv-main55+]
Alias: CVE-2017-7790
Flags: qe-verify-
Whiteboard: [adv-main55+] → [adv-main55+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.