Closed
Bug 1267130
Opened 8 years ago
Closed 8 years ago
nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer, #5
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
People
(Reporter: jruderman, Assigned: baku)
References
(Blocks 1 open bug)
Details
(Keywords: crash, sec-critical, testcase, Whiteboard: [necko-active][adv-main47+][adv-esr45.2+][post-critsmash-triage])
Attachments
(4 files, 2 obsolete files)
226 bytes,
text/html
|
Details | |
9.82 KB,
text/plain
|
Details | |
15.10 KB,
patch
|
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
1.12 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → amarchesini
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8744839 -
Flags: review?(valentin.gosu)
Assignee | ||
Comment 3•8 years ago
|
||
Just to give more information about what this bug is about. Let's create this crazy URL: var url = new URL("https://@@@@@@@@"); This creates a nsStandardURL and it populates the members here: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#1273 Then we also do: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#1277 Where we encode each segment of the URL: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#535 We change the value of the segments but we don't update the mPos of the following ones. This makes so that mHost.mPos still points to the value set by ParseURL. When we call url.hostname = "something", this calculates the position for the next segments: https://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsStandardURL.cpp#1730 and we end up having a negative value stored in a uint32. In our example, the mHost.mPos is 17 but the mUsername.mPos + mUsername.mLen is 22: This makes so that we write in random memory (17-22 = 4294967291).
Comment 4•8 years ago
|
||
See also https://github.com/whatwg/url/issues/31 (though if we do that we should do it for all URLs).
Comment 5•8 years ago
|
||
Comment on attachment 8744839 [details] [diff] [review] crash.patch Review of attachment 8744839 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the quick patch. ::: netwerk/base/nsStandardURL.cpp @@ +478,4 @@ > if (seg.mLen > 0) { > if (useEscaped) { > + MOZ_ASSERT(diff); > + *diff = escapedStr->Length() - seg.mLen; We need to make sure this doesn't result in diff being a negative number and overflowing. See below. @@ +608,5 @@ > if (!mSpec.SetLength(approxLen+1, mozilla::fallible)) // buf needs a trailing '\0' below > return NS_ERROR_OUT_OF_MEMORY; > char *buf; > mSpec.BeginWriting(buf); > + uint32_t i = 0, diff = 0; nit: One declaration per line. @@ +640,5 @@ > if (mHost.mLen > 0) { > + i = AppendSegmentToBuf(buf, i, spec, mHost, &encHost, useEncHost, > + &diff); > + if (diff) { > + ShiftFromHost(diff); The fact that we unescape percent encoded values we find in the hostname will cause diff to be a negative value in this case. This causes test_standardurl.js::test_percentDecoding() to crash. We need to handle this possibility as well.
Attachment #8744839 -
Flags: review?(valentin.gosu) → feedback+
Updated•8 years ago
|
Whiteboard: [necko-active]
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8744839 -
Attachment is obsolete: true
Attachment #8745051 -
Flags: review?(valentin.gosu)
Comment 7•8 years ago
|
||
Comment on attachment 8745051 [details] [diff] [review] crash.patch Review of attachment 8745051 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, the code looks good, although it seems to get more complex with every change. Hopefully we can get rid of all of it soon, in favor of rust-url.
Attachment #8745051 -
Flags: review?(valentin.gosu) → review+
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8745051 [details] [diff] [review] crash.patch [Security approval request comment] How easily could an exploit be constructed based on the patch? I think it's easy. We write out of a memory buffer something that comes from the encoding/decoding of URL segments. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? There is a crashtest attached. And the patch does a lot of checks about what and when we write URL segments. Which older supported branches are affected by this flaw? All. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? How likely is this patch to cause regressions; how much testing does it need? Easy to backport, I guess. This patch is green and I don't think we are going to have regressions, but still, it touches a critical component: nsStandardURL.
Attachment #8745051 -
Flags: sec-approval?
Comment 9•8 years ago
|
||
Sec-approval for checkin on May 10 (two weeks into the cycle) on trunk. We'll want this on Aurora, Beta, and ESR45 after it goes into trunk so you'll want to prepare and nominate branch patches as well.
status-firefox46:
--- → wontfix
status-firefox47:
--- → affected
status-firefox49:
--- → affected
status-firefox-esr45:
--- → affected
Whiteboard: [necko-active] → [necko-active][checkin on 5/10]
Updated•8 years ago
|
Attachment #8745051 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 10•8 years ago
|
||
Good. I start to upload patches with comments and so on. Btw, would be nice to have a auto NI-cancelation with timers in bugzilla, so that I don't forget to land it the 10th of May.
Attachment #8745051 -
Attachment is obsolete: true
Comment 11•8 years ago
|
||
sec-critical, tracking.
tracking-firefox47:
--- → +
tracking-firefox48:
--- → +
tracking-firefox49:
--- → +
tracking-firefox-esr45:
--- → ?
Updated•8 years ago
|
Group: core-security → network-core-security
Assignee | ||
Comment 13•8 years ago
|
||
Comment 14•8 years ago
|
||
No, please don't land the test. We don't land tests for security bugs until *after* the release containing the fix is shipped. Otherwise, you run the risk of 0day'ing ourselves, especially since you comment that the issue is easy to spot. I should have mentioned this in my approval so thanks for asking.
Flags: needinfo?(abillings)
https://hg.mozilla.org/mozilla-central/rev/ed9265023c32
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Baku, could you please nominate patches for uplift to Beta, Aurora, ESR45? Thanks!
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8745505 [details] [diff] [review] patch for m-i Approval Request Comment [Feature/regressing bug #]: URL parsing [User impact if declined]: a crash: we write and read memory that has not allocated. [Describe test coverage new/current, TreeHerder]: test is included. but we don't want to land it yet. [Risks and why]: the patch is not small. But it doesn't show any regression in m-i/m-c and URL parsing is a very common step everywhere in gecko. [String/UUID change made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #8745505 -
Flags: approval-mozilla-esr45?
Attachment #8745505 -
Flags: approval-mozilla-beta?
Attachment #8745505 -
Flags: approval-mozilla-aurora?
Comment on attachment 8745505 [details] [diff] [review] patch for m-i Sec-crit, approved for uplift to all affected branches.
Attachment #8745505 -
Flags: approval-mozilla-esr45?
Attachment #8745505 -
Flags: approval-mozilla-esr45+
Attachment #8745505 -
Flags: approval-mozilla-beta?
Attachment #8745505 -
Flags: approval-mozilla-beta+
Attachment #8745505 -
Flags: approval-mozilla-aurora?
Attachment #8745505 -
Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f49523be5c1 https://hg.mozilla.org/releases/mozilla-beta/rev/0636f9b7e9a2 ESR45's closed, so I didn't uplift this there yet.
Updated•8 years ago
|
Updated•8 years ago
|
Group: network-core-security → core-security-release
(In reply to Andrew Overholt [:overholt] from comment #20) > What's left to be uplifted here? Hi Andrew, this hasn't landed on ESR45 branch yet. NI Tomcat, Wes to ensure this gets uplifted now.
Flags: needinfo?(wkocher)
Flags: needinfo?(rkothari)
Flags: needinfo?(cbook)
Updated•8 years ago
|
Whiteboard: [necko-active][checkin on 5/10] → [necko-active][adv-main47+][adv-esr45.2+]
Updated•8 years ago
|
Flags: qe-verify-
Whiteboard: [necko-active][adv-main47+][adv-esr45.2+] → [necko-active][adv-main47+][adv-esr45.2+][post-critsmash-triage]
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
•