Closed
Bug 1233346
Opened 8 years ago
Closed 8 years ago
Potential buffer overrun in Windows ICE interface name code
Categories
(Core :: WebRTC: Networking, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox42 | --- | wontfix |
firefox43 | --- | wontfix |
firefox44 | + | fixed |
firefox45 | + | fixed |
firefox46 | --- | unaffected |
firefox-esr38 | 44+ | 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 |
backlog | webrtc/webaudio+ |
People
(Reporter: drno, Assigned: drno)
Details
(Keywords: csectype-bounds, sec-high, Whiteboard: [post-critsmash-triage][adv-main44+][adv-esr38.6+])
Attachments
(1 file, 2 obsolete files)
855 bytes,
patch
|
ekr
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
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•8 years ago
|
||
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.
Assignee | ||
Comment 3•8 years ago
|
||
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.
Assignee | ||
Comment 4•8 years ago
|
||
I assume it can not harm to put in an extra safety measure here, just in case.
Attachment #8699574 -
Flags: review?(ekr)
Comment 5•8 years ago
|
||
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
Attachment #8699574 -
Flags: review?(ekr)
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8699574 -
Attachment is obsolete: true
Attachment #8699584 -
Flags: review?(ekr)
Comment 7•8 years ago
|
||
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.
Attachment #8699584 -
Flags: review?(ekr)
Assignee | ||
Comment 8•8 years ago
|
||
Attachment #8699584 -
Attachment is obsolete: true
Attachment #8699662 -
Flags: review?(ekr)
Comment 9•8 years ago
|
||
Comment on attachment 8699662 [details] [diff] [review] bug1233346.patch Review of attachment 8699662 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8699662 -
Flags: review?(ekr) → review+
Comment 10•8 years ago
|
||
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
status-firefox42:
--- → wontfix
status-firefox43:
--- → wontfix
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox-esr38:
--- → affected
status-firefox-esr45:
--- → affected
Flags: needinfo?(drno)
Keywords: csectype-bounds,
sec-high
Priority: -- → P1
Assignee | ||
Comment 11•8 years ago
|
||
(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)
Assignee | ||
Comment 12•8 years ago
|
||
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).
Attachment #8699662 -
Flags: sec-approval?
Comment 13•8 years ago
|
||
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•8 years ago
|
||
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.
tracking-firefox44:
--- → +
tracking-firefox45:
--- → +
tracking-firefox46:
--- → +
tracking-firefox-esr38:
--- → 44+
Whiteboard: [checkin on 12/29]
Updated•8 years ago
|
Attachment #8699662 -
Flags: sec-approval? → sec-approval+
Comment 15•8 years ago
|
||
I'll handle landing it around the 29th since Nils will be on PTO
Comment 16•8 years ago
|
||
Randell, could you fill the uplift requests? Thanks
Comment 17•8 years ago
|
||
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.
Flags: needinfo?(rjesup)
Attachment #8699662 -
Flags: approval-mozilla-esr38?
Attachment #8699662 -
Flags: approval-mozilla-beta?
Attachment #8699662 -
Flags: approval-mozilla-aurora?
Comment 18•8 years ago
|
||
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.
tracking-firefox46:
+ → ---
Updated•8 years ago
|
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+
Comment 19•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/0ab687772056 https://hg.mozilla.org/releases/mozilla-beta/rev/bc679c4aeb59
Comment 20•8 years ago
|
||
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).
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 22•8 years ago
|
||
(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
Flags: needinfo?(cbook)
Comment 23•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/bc679c4aeb59
status-b2g-v2.5:
--- → fixed
Updated•8 years ago
|
status-b2g-v2.0:
--- → wontfix
status-b2g-v2.0M:
--- → wontfix
status-b2g-v2.1:
--- → wontfix
status-b2g-v2.1S:
--- → wontfix
status-b2g-v2.2:
--- → affected
status-b2g-v2.2r:
--- → affected
status-b2g-master:
--- → fixed
Keywords: checkin-needed
Target Milestone: --- → mozilla45
Updated•8 years ago
|
Group: media-core-security, core-security → core-security-release
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage]
Updated•8 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main44+][adv-esr38.6+]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•