Bug 1168959 (CVE-2015-4517)

Memory-safety bugs in NetworkUtils.cpp generally

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: q1, Assigned: dimi)

Tracking

({csectype-bounds, sec-moderate})

unspecified
2.2 S14 (12june)
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox39 wontfix, firefox40 wontfix, firefox41 fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [b2g-adv-main2.2?][post-critsmash-triage][adv-main41+])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

4 years ago
User Agent: Mozilla/5.0 (Windows; rv:***) Gecko/20100101 Firefox/**.*
Build ID: 20150305021524

Steps to reproduce:

38.0.1\dom\system\gonk\NetworkUtils.cpp has several memory-safety bugs resulting from the use of snprintf. The problem is that it assumes that snprintf always null-terminates the stack-based destination string. This is not, however, guaranteed unless the result string length is < the specified character count, and, of course, automatic variables are generally uninitialized, so the result string won't be null-terminated by default.

There are 3 good examples of this bug in NetworkUtils::setAccessPoint, and several others throughout the module.

If any of the strings used as input to snprintf are under external control, this bug could cause the disclosure of sensitive information and/or allow an attacker to corrupt Firefox's address space and/or send attacker-designated network commands and/or allow execution of attacker-chosen code.
Reporter

Comment 1

4 years ago
I've found other instances of this bug:

38.0.1\dom\camera\GonkCameraHwMgr.cpp:

GonkCameraHardware::Init line 173 will create an unterminated string if the decimal representation of mCameraId (a uint32_t) is >= 6, and then line 174 will pass it to the Android OS function __system_property_get, with unpredictable results.

38.0.1\dom\wifi\WifiUtils.cpp:

do_wifi_command potentially creates and sends to OS an unterminated string.


Really snprintf should be replaced app-wide with a variant that always null-terminates the resulting string, and that throws or returns status if the string was truncated.
Component: Untriaged → Networking
Flags: sec-bounty?
Product: Firefox → Core
Component: Networking → Wifi
Flags: needinfo?(dlee)
Product: Core → Firefox OS
Version: 38 Branch → unspecified
(In reply to q1 from comment #1)
> Really snprintf should be replaced app-wide with a variant that always
> null-terminates the resulting string, and that throws or returns status if
> the string was truncated.

We have one of those in NSPR, PL_strncpy(), and at one point people went through and converted all of the PL_ calls to ones that didn't require NSPR :-(
Assignee

Comment 3

4 years ago
In this patch I use PR_snprintf to replace snprintf because PR_snprintf will handle null-terminated string.
I only update files I am familiar with, including NetworkUtils.cpp & WifiUtils.cpp
Flags: needinfo?(dlee)
Attachment #8614580 - Flags: feedback?(mcmanus)
Comment on attachment 8614580 [details] [diff] [review]
Patch v1 - use PR_snprintf replace snprintf

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

sounds reasonable enough at first glance, but I think you need a gonk peer (this isn't platform code) to comment on how they feel about more nspr calls in their code.
Attachment #8614580 - Flags: feedback?(mcmanus)
Assignee

Updated

4 years ago
Assignee: nobody → dlee
Assignee

Comment 5

4 years ago
Comment on attachment 8614580 [details] [diff] [review]
Patch v1 - use PR_snprintf replace snprintf

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

Hi Michael,
As Patrick suggested in comment 4, could you help review this patch ? Thanks!
Attachment #8614580 - Flags: review?(mwu)

Comment 6

4 years ago
Comment on attachment 8614580 [details] [diff] [review]
Patch v1 - use PR_snprintf replace snprintf

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

Fabrice probably knows this code better than I do.

I think in general we like to avoid dependencies on NSPR and switch to XPCOM or MFBT things. nsTextFormatter provides a snprintf impl. No idea if it behaves better than normal snprintf though.
Attachment #8614580 - Flags: review?(mwu) → review?(fabrice)
Comment on attachment 8614580 [details] [diff] [review]
Patch v1 - use PR_snprintf replace snprintf

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

Please use nsTextFormatter.snprintf instead. It guarantees null termination and error checking.
Attachment #8614580 - Flags: review?(fabrice) → review-
Assignee

Comment 8

4 years ago
Hi Fabrice,
nsTextFormatter::snprintf takes char16_t as input, but NetworkUtils.cpp use char for most cases, do you suggest use nsTextFormatter::snprintf and then convert char16_t to char8_t or there is other better option other than nsTextFormatter::snprintf ?
Flags: needinfo?(fabrice)
Hm, yeah that's annoying. The mfbt one is just a fix for windows, so doesn't help us there, and most of our code (including in dom/) uses PR_snprintf.
Flags: needinfo?(fabrice)
Comment on attachment 8614580 [details] [diff] [review]
Patch v1 - use PR_snprintf replace snprintf

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

Let's go with that then.
Attachment #8614580 - Flags: review- → review+
Assignee

Comment 11

4 years ago
Posted patch Patch v2Splinter Review
Rebased to latest code
Attachment #8614580 - Attachment is obsolete: true
Attachment #8616550 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
At a minimum, this needs a security rating before it can land. Beyond that, it may also require additional approvals depending the severity and branches affected. See the link below:
https://wiki.mozilla.org/Security/Bug_Approval_Process
Keywords: checkin-needed
Assignee

Comment 14

4 years ago
Comment on attachment 8616550 [details] [diff] [review]
Patch v2

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I am not security expert so not pretty sure about this. Some of these 'snprintf' fixed in this patch are used internally by gecko so it should be fine, but in some cases data is read from android property system.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
b2g version after 1.3

If not all supported branches, which bug introduced the flaw?
bug 864931

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I think it will be straightforward

How likely is this patch to cause regressions; how much testing does it need?
This solution is straightforward, just replace snprintf by PR_snprintf so it unlikely cause regression. I have run Wifi emulator testcases locally and tests are passed
Attachment #8616550 - Flags: sec-approval?
(In reply to Dimi Lee[:dimi][:dlee] from comment #3)
> I only update files I am familiar with, including NetworkUtils.cpp &
> WifiUtils.cpp

Not patching code you're not comfortable in is fine, but black-holing part of a security bug isn't. As the assignee for this bug if you're not going to patch part of it you need to make sure that part is passed along to someone else so it doesn't get lost. (Actually that should be tru for multi-symptom non-security bugs, too). For instance, you could clone this bug into a security bug in the Camera component and make this bug "depend on" that new one. The the security team will search for an appropriate owner of the new bug.
Confirming, the presence of the patch clearly proves we agree there's a problem here.

I'm tempted to call this sec-low because I'm dubious wifi protocols support fields long enough to add up to overflowing a 4K MAX_COMMAND_SIZE buffer. The ones using the SSID might get that from user input but we're not that worried about self-hacking (Mozilla is all about hacking your own stuff). Still, the potential for creating unterminated strings and reading extra Heartbleed-style is there should something be that long, and I don't know wifi protocol details enough to be sure so we'll go with sec-moderate.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8616550 [details] [diff] [review]
Patch v2

Clearing sec-approval? on the patch since sec-moderate issues do not require sec-approval+
Attachment #8616550 - Flags: sec-approval?
Assignee

Updated

4 years ago
Blocks: 1173640
Assignee

Updated

4 years ago
No longer blocks: 1173640
Assignee

Updated

4 years ago
Depends on: 1173640
Assignee

Comment 18

4 years ago
(In reply to Daniel Veditz [:dveditz] from comment #15)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #3)
> > I only update files I am familiar with, including NetworkUtils.cpp &
> > WifiUtils.cpp
> 
> Not patching code you're not comfortable in is fine, but black-holing part
> of a security bug isn't. As the assignee for this bug if you're not going to
> patch part of it you need to make sure that part is passed along to someone
> else so it doesn't get lost. (Actually that should be tru for multi-symptom
> non-security bugs, too). For instance, you could clone this bug into a
> security bug in the Camera component and make this bug "depend on" that new
> one. The the security team will search for an appropriate owner of the new
> bug.

create Bug 1173640 for Camera component
Thanks!
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/13f5dda0175b

Does this affect older B2G releases too?
status-b2g-v2.2: --- → ?
Flags: needinfo?(dlee)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/13f5dda0175b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S14 (12june)
Flags: sec-bounty? → sec-bounty+
Assignee

Comment 22

4 years ago
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #20)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/13f5dda0175b
> 
> Does this affect older B2G releases too?

Sorry for late reply.
Yes, as mentioned in Comment 14, this also affect B2G release after 1.3 (1.3 not included)
Flags: needinfo?(dlee)
Josh, how far back do you want to take this?
Hi Ryan,
Let's just uplift to b2g-37 for this issue as no plan for 2.0/2.1 device now.
Thanks!
Flags: needinfo?(jocheng)

Updated

4 years ago
Flags: needinfo?(ryanvm)
Comment on attachment 8616550 [details] [diff] [review]
Patch v2

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 864931
User impact if declined: possibly sec issues
Testing completed: On master for nearly 3 weeks now without any known regressions
Risk to taking this patch (and alternatives if risky): Per comment 14, low-risk
String or UUID changes made by this patch: None
Flags: needinfo?(ryanvm)
Attachment #8616550 - Flags: approval-mozilla-b2g37?

Updated

4 years ago
Attachment #8616550 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Ryan, has the patch made it into the 2.2 release?
Flags: needinfo?(ryanvm)
Whiteboard: [b2g-adv-main2.2?]
Comment 26 right above yours and status-b2g-v2.2: fixed say that it did.
Flags: needinfo?(ryanvm)
Whiteboard: [b2g-adv-main2.2?] → [b2g-adv-main2.2?][post-critsmash-triage]

Updated

4 years ago
Group: core-security → core-security-release
Whiteboard: [b2g-adv-main2.2?][post-critsmash-triage] → [b2g-adv-main2.2?][post-critsmash-triage][adv-main41+]
Alias: CVE-2015-4517
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.