Potential buffer overrun in Windows ICE interface name code

RESOLVED FIXED in Firefox 44



4 years ago
3 years ago


(Reporter: drno, Assigned: drno)


({csectype-bounds, sec-high})


Firefox Tracking Flags

(firefox42 wontfix, firefox43 wontfix, firefox44+ fixed, firefox45+ fixed, firefox46 unaffected, firefox-esr3844+ fixed, firefox-esr45 fixed, b2g-v2.0 wontfix, b2g-v2.0M wontfix, b2g-v2.1 wontfix, b2g-v2.1S wontfix, b2g-v2.2 affected, b2g-v2.5 fixed, b2g-v2.2r affected, b2g-master fixed)


(Whiteboard: [post-critsmash-triage][adv-main44+][adv-esr38.6+])


(1 attachment, 2 obsolete attachments)

This call to snprintf does not check its return value:
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.
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.
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.
I'm unsure if MSVC 2013 has a compliant snprintf.
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.
Posted patch bug1233346.patch (obsolete) — Splinter Review
I assume it can not harm to put in an extra safety measure here, just in case.
Attachment #8699574 - Flags: review?(ekr)
Comment on attachment 8699574 [details] [diff] [review]

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
Attachment #8699574 - Flags: review?(ekr)
Posted patch bug1233346.patch (obsolete) — Splinter Review
Attachment #8699574 - Attachment is obsolete: true
Attachment #8699584 - Flags: review?(ekr)
Comment on attachment 8699584 [details] [diff] [review]

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.
Attachment #8699584 - Flags: review?(ekr)
Attachment #8699584 - Attachment is obsolete: true
Attachment #8699662 - Flags: review?(ekr)
Comment on attachment 8699662 [details] [diff] [review]

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

Attachment #8699662 - Flags: review?(ekr) → review+
Nils: let's ask for s-a on this.  I assume it affects pretty much everything.
Assignee: nobody → drno
Group: media-core-security
backlog: --- → webrtc/webaudio+
Rank: 10
Flags: needinfo?(drno)
Priority: -- → P1
(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.
Flags: needinfo?(drno)
Comment on attachment 8699662 [details] [diff] [review]

[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?


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).
Attachment #8699662 - Flags: sec-approval?
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.
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.
Whiteboard: [checkin on 12/29]
Attachment #8699662 - Flags: sec-approval? → sec-approval+
I'll handle landing it around the 29th since Nils will be on PTO
Randell, could you fill the uplift requests? Thanks
Flags: needinfo?(rjesup)
Keywords: checkin-needed
Whiteboard: [checkin on 12/29]
Comment on attachment 8699662 [details] [diff] [review]

[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.
Flags: needinfo?(rjesup)
Attachment #8699662 - Flags: approval-mozilla-esr38?
Attachment #8699662 - Flags: approval-mozilla-beta?
Attachment #8699662 - Flags: approval-mozilla-aurora?
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.
Attachment #8699662 - Flags: approval-mozilla-esr38?
Attachment #8699662 - Flags: approval-mozilla-esr38+
Attachment #8699662 - Flags: approval-mozilla-beta?
Attachment #8699662 - Flags: approval-mozilla-beta+
Attachment #8699662 - Flags: approval-mozilla-aurora?
Attachment #8699662 - Flags: approval-mozilla-aurora+
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).
Closed: 4 years ago
Resolution: --- → FIXED
tomcat: Can you land this on ESR?  Thanks
Flags: needinfo?(cbook)
(In reply to Randell Jesup [:jesup] from comment #21)
> tomcat: Can you land this on ESR?  Thanks


Flags: needinfo?(cbook)
Keywords: checkin-needed
Target Milestone: --- → mozilla45
Group: media-core-security, core-security → core-security-release
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+][adv-esr38.6+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.