1168938 - Memory safety bug in NetworkUtils::postTetherInterfaceList

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[q1]

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

Steps to reproduce:

NetworkUtils::postTetherInterfaceList (38.0.1\dom\system\gonk\NetworkUtils.cpp lines 809-11) copies a variable-length string into a fixed-length stack buffer without checking that it fits. If the variable-length string's length can be manipulated by an attacker, this bug could cause a crash and/or execution of attacker-chosen code:

801: void NetworkUtils::postTetherInterfaceList(CommandChain* aChain,
802:                                           CommandCallback aCallback,
803:                                           NetworkResultOptions& aResult)
804: {
805:   // Send the dummy command to continue the function chain.
806:   char command[MAX_COMMAND_SIZE];
807:   snprintf(command, MAX_COMMAND_SIZE - 1, "%s", DUMMY_COMMAND);
808:
809:   char buf[BUF_SIZE];
810:   NS_ConvertUTF16toUTF8 reason(aResult.mResultReason);
811:   memcpy(buf, reason.get(), reason.Length() + 1);
812:   ...

Comment 1

[Andrew McCreight [:mccr8]]

Fabrice, do you know who might be able to investigate this? Thanks.

Comment 2

[[:fabrice] Fabrice Desré]

Dimi, can you take a look?

Comment 3

[Dimi Lee [:dimi]]

Attachment: Patch v1

In this patch I just make sure the length copied will not exceed buf size. If |reason| is somehow too big, it will only be copied partially and error will occur when netd execute it.

Comment 4

[q1]

(In reply to Dimi Lee[:dimi][:dlee] from comment #3)
> Created attachment 8628164 [details] [diff] [review]
> Patch v1
> 
> In this patch I just make sure the length copied will not exceed buf size.
> If |reason| is somehow too big, it will only be copied partially and error
> will occur when netd execute it.

You might also want to fix the snprintf on line 807, which won't terminate command if it truncates.

Comment 5

[Dimi Lee [:dimi]]

(In reply to q1 from comment #4)
> (In reply to Dimi Lee[:dimi][:dlee] from comment #3)
> > Created attachment 8628164 [details] [diff] [review]
> > Patch v1
> > 
> > In this patch I just make sure the length copied will not exceed buf size.
> > If |reason| is somehow too big, it will only be copied partially and error
> > will occur when netd execute it.
> 
> You might also want to fix the snprintf on line 807, which won't terminate
> command if it truncates.

This issue is handled in bug 1168959 which using PR_snprintf instead of snprintf

Comment 6

[[:fabrice] Fabrice Desré]

Comment on attachment 8628164 [details] [diff] [review]
Patch v1

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

::: dom/system/gonk/NetworkUtils.cpp
@@ +817,5 @@
>  
>    char buf[BUF_SIZE];
>    NS_ConvertUTF16toUTF8 reason(aResult.mResultReason);
> +
> +  uint32_t length = reason.Length() + 1 < BUF_SIZE ? reason.Length() + 1 : BUF_SIZE;

nit: use size_t instead of uint32_t

Comment 7

[Dimi Lee [:dimi]]

Attachment: Patch v2

Address nits

Comment 8

[Dimi Lee [:dimi]]

try : https://treeherder.mozilla.org/#/jobs?repo=try&revision=77a942b4b77e

Comment 9

[Dimi Lee [:dimi]]

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

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easy, this security issue requires hack to the netd and fake the return result from previous command of postTetherInterfaceList

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?
It is hard to cause regression because the solution is quite simple that we
just check copied size before doing memory copy.

Comment 10

[Daniel Veditz [:dveditz]]

Defense in depth is good, but this appears to be messages we send to ourselves and not something an attacker could send us.

Comment 11

[Daniel Veditz [:dveditz]]

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

Check in any time, sec-approval not needed.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Probably worth getting this on v2.2 given the low risk. Not as sure about v2.1/v2.1s.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/05afb37c7f62

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05afb37c7f62

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Do you want this backported to any other branches, Josh?

Comment 16

[Josh Cheng [:josh]]

Hi Ryan,
It seems the issue is very edge per comment 9 from Dimi. 
I am fine with not uplift to previous version unless someone have different opinion.
Thanks!

Comment 17

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

Minusing for bounty because this isn't regarded to be a security issue by us since they are internal messages only and can't be used for escalation.