1168959 - (CVE-2015-4517) Memory-safety bugs in NetworkUtils.cpp generally

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[q1]

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.

Comment 1

[q1]

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.

Comment 2

[Daniel Veditz [:dveditz]]

(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 :-(

Comment 3

[Dimi Lee [:dimi]]

Attachment: Patch v1 - use PR_snprintf replace snprintf

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

Comment 4

[Patrick McManus [: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.

Comment 5

[Dimi Lee [:dimi]]

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!

Comment 6

[Michael Wu [:mwu]]

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.

Comment 7

[[:fabrice] Fabrice Desré]

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.

Comment 8

[Dimi Lee [:dimi]]

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 ?

Comment 9

[[:fabrice] Fabrice Desré]

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.

Comment 10

[[:fabrice] Fabrice Desré]

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.

Comment 11

[Dimi Lee [:dimi]]

Attachment: Patch v2

Rebased to latest code

Comment 12

[Dimi Lee [:dimi]]

try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a1cc27507a5

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 14

[Dimi Lee [:dimi]]

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

Comment 15

[Daniel Veditz [:dveditz]]

(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.

Comment 16

[Daniel Veditz [:dveditz]]

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.

Comment 17

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8616550 [details] [diff] [review]
Patch v2

Clearing sec-approval? on the patch since sec-moderate issues do not require sec-approval+

Comment 18

[Dimi Lee [:dimi]]

(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

Comment 19

[Daniel Veditz [:dveditz]]

Thanks!

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13f5dda0175b

Does this affect older B2G releases too?

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/13f5dda0175b

Comment 22

[Dimi Lee [:dimi]]

(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)

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Josh, how far back do you want to take this?

Comment 24

[Josh Cheng [:josh]]

Hi Ryan,
Let's just uplift to b2g-37 for this issue as no plan for 2.0/2.1 device now.
Thanks!

Comment 25

[Ryan VanderMeulen [: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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3029b7ed01ef

Comment 27

[Christiane Ruetten [INACTIVE]]

Ryan, has the patch made it into the 2.2 release?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment 26 right above yours and status-b2g-v2.2: fixed say that it did.

Comment 29

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion