1233346 - Potential buffer overrun in Windows ICE interface name code

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect, P1)

Description

[Nils Ohlmeier [:drno]]

This call to snprintf does not check its return value:
  https://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nICEr/src/stun/addrs.c#192
If the |FriendlyName| is bigger then |IFNAMSIZ|, |munged_ifname| is not going to be null terminated and the subsequent while loops will go off in the weeds.

Comment 1

[Eric Rescorla (:ekr)]

Are you sure? I don't have the C standard to hand but here's what the Mac man page
says about snprintf:

     The snprintf() and vsnprintf() functions will write at most n-1 of the
     characters printed into the output string (the n'th character then gets
     the terminating `\0'); if the return value is greater than or equal to
     the n argument, the string was too short and some of the printed charac-
     ters were discarded.  The output is always null-terminated.

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

It depends on which snprintf is used.  If this is #defined or otherwise devolves to _snprintf on Windows, it will NOT null-terminate if the formatted string is too large.
https://msdn.microsoft.com/en-us/library/2ts7cx93.aspx
I'm unsure if MSVC 2013 has a compliant snprintf.

Comment 3

[Nils Ohlmeier [:drno]]

What Randel said. My assumptions are based on the MSDN documentation. What got me suspicious is that the snprintf call seems to try to append an additional 0 at the end. Nice to learn that snprintf behave differently per OS.

Comment 4

[Nils Ohlmeier [:drno]]

Attachment: bug1233346.patch

I assume it can not harm to put in an extra safety measure here, just in case.

Comment 5

[Eric Rescorla (:ekr)]

Comment on attachment 8699574 [details] [diff] [review]
bug1233346.patch

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

::: media/mtransport/third_party/nICEr/src/stun/addrs.c
@@ +190,5 @@
>          continue;
>  
> +      n = snprintf(munged_ifname, IFNAMSIZ, "%S%c", tmpAddress->FriendlyName, 0);
> +      if (n == IFNAMSIZ) {
> +        *munged_ifname[IFNAMSIZ] = '\0';

This is writing past the end of the buffer. You need IFNAMSIZ-1

Also, I wouldn't make this conditional. It just makes life complicated

Comment 6

[Nils Ohlmeier [:drno]]

Attachment: bug1233346.patch

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 8699584 [details] [diff] [review]
bug1233346.patch

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

::: media/mtransport/third_party/nICEr/src/stun/addrs.c
@@ +189,5 @@
>        if (tmpAddress->OperStatus != IfOperStatusUp)
>          continue;
>  
>        snprintf(munged_ifname, IFNAMSIZ, "%S%c", tmpAddress->FriendlyName, 0);
> +      *munged_ifname[IFNAMSIZ-1] = '\0';

What is the * doing here? [] already dereferences this.

Comment 8

[Nils Ohlmeier [:drno]]

Attachment: bug1233346.patch

Comment 9

[Eric Rescorla (:ekr)]

Comment on attachment 8699662 [details] [diff] [review]
bug1233346.patch

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

LGTM

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Nils: let's ask for s-a on this.  I assume it affects pretty much everything.

Comment 11

[Nils Ohlmeier [:drno]]

(In reply to Randell Jesup [:jesup] from comment #10)
> Nils: let's ask for s-a on this.  I assume it affects pretty much everything.

The fix for bug 1229633 removes all of the faulty code. I'm planing on landing that in Nightly soon. But I had no intentions to uplift the code from bug 1229633.
So I guess that means this fir here should go into all the other releases before 46.

Comment 12

[Nils Ohlmeier [:drno]]

Comment on attachment 8699662 [details] [diff] [review]
bug1233346.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The patch makes it obvious what can go wrong here. But an attacker would need to be able to rename the network interface on the target Windows machine, which I assume is not possible from within the browser.

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?

All versions up to Firefox 18.

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

Initial import of WebRTC code in bug 790517.

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

I assume the patch applies cleanly to all branches. If not a backport is trivial.

How likely is this patch to cause regressions; how much testing does it need?

Potential for regression is basically zero, as we are only zero terminating a string buffer which is not guaranteed to be zero terminated (depending on the platform compiler and header includes).

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Per conversation with Nils, this isn't really triggerable remotely, as you'd have to control the interface name in order to do more than maybe crash.  Thus it's even arguable if it's a sec-high, but I also see low if any need to uplift.  OTOH it's a trivial, safe 1-liner patch.

Comment 14

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

sec-approval+ for checkin on 12/29 (two weeks into the next cycle). We'll want Aurora, Beta, and ESR38 patches made and nominated by then as well so we can take it everywhere as a sec-high rated issue.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

I'll handle landing it around the 29th since Nils will be on PTO

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Randell, could you fill the uplift requests? Thanks

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8699662 [details] [diff] [review]
bug1233346.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Low risk of exploit - requires user to rename an interface typically to have a risk

Fix Landed on Version: 46

Risk to taking this patch (and alternatives if risky): super-low risk (null-terminate buffer)

String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Note: the landing of this bug for Firefox 46 is overtaken-by-events because of bug 1229633 (which replaced this string with a hash).  It's still relevant for older versions (45 and earlier) unless we want to uplift the hashing to Aurora/45.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0ab687772056
https://hg.mozilla.org/releases/mozilla-beta/rev/bc679c4aeb59

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Marking fixed since the bug 1229633 landing fixed this on central/46.  We still need to land ESR (I don't have a local ESR tree; if needed I can install one).

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

tomcat: Can you land this on ESR?  Thanks

Comment 22

[Carsten Book [:Tomcat]]

(In reply to Randell Jesup [:jesup] from comment #21)
> tomcat: Can you land this on ESR?  Thanks

done

-->https://hg.mozilla.org/releases/mozilla-esr38/rev/925215cae26f

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bc679c4aeb59