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)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox46 --- wontfix
firefox47 + fixed
firefox48 + fixed
firefox49 + fixed
firefox-esr45 47+ fixed

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)

Attached file testcase
      No description provided.
Attached file stack
Assignee: nobody → amarchesini
Attached patch crash.patch (obsolete) — Splinter Review
Attachment #8744839 - Flags: review?(valentin.gosu)
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).
See also https://github.com/whatwg/url/issues/31 (though if we do that we should do it for all URLs).
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+
Whiteboard: [necko-active]
Attached patch crash.patch (obsolete) — Splinter Review
Attachment #8744839 - Attachment is obsolete: true
Attachment #8745051 - Flags: review?(valentin.gosu)
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+
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?
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.
Whiteboard: [necko-active] → [necko-active][checkin on 5/10]
Attachment #8745051 - Flags: sec-approval? → sec-approval+
Attached patch patch for m-iSplinter Review
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
sec-critical, tracking.
Group: core-security → network-core-security
al, should I land the test too?
Flags: needinfo?(abillings)
Attached patch m-i testSplinter Review
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)
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+
Group: network-core-security → core-security-release
What's left to be uplifted here?
Flags: needinfo?(rkothari)
(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)
Whiteboard: [necko-active][checkin on 5/10] → [necko-active][adv-main47+][adv-esr45.2+]
Flags: qe-verify-
Whiteboard: [necko-active][adv-main47+][adv-esr45.2+] → [necko-active][adv-main47+][adv-esr45.2+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.