nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer, #5

RESOLVED FIXED in Firefox 47



a year ago
11 months ago


(Reporter: Jesse Ruderman, Assigned: baku)


(Blocks: 1 bug, {crash, sec-critical, testcase})

crash, sec-critical, testcase
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox46 wontfix, firefox47+ fixed, firefox48+ fixed, firefox49+ fixed, firefox-esr4547+ fixed)


(Whiteboard: [necko-active][adv-main47+][adv-esr45.2+][post-critsmash-triage])


(4 attachments, 2 obsolete attachments)



a year ago
Created attachment 8744782 [details]

Comment 1

a year ago
Created attachment 8744783 [details]


a year ago
Assignee: nobody → amarchesini

Comment 2

a year ago
Created attachment 8744839 [details] [diff] [review]
Attachment #8744839 - Flags: review?(valentin.gosu)

Comment 3

a year 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:


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:

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

a year ago
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]

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]

Comment 6

a year ago
Created attachment 8745051 [details] [diff] [review]
Attachment #8744839 - Attachment is obsolete: true
Attachment #8745051 - Flags: review?(valentin.gosu)
Comment on attachment 8745051 [details] [diff] [review]

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 8

a year ago
Comment on attachment 8745051 [details] [diff] [review]

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


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.
status-firefox46: --- → wontfix
status-firefox47: --- → affected
status-firefox49: --- → affected
status-firefox-esr45: --- → affected
Whiteboard: [necko-active] → [necko-active][checkin on 5/10]
Attachment #8745051 - Flags: sec-approval? → sec-approval+

Comment 10

a year ago
Created attachment 8745505 [details] [diff] [review]
patch for m-i

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.
tracking-firefox47: --- → +
tracking-firefox48: --- → +
tracking-firefox49: --- → +
tracking-firefox-esr45: --- → ?
Group: core-security → network-core-security

Comment 12

a year ago
al, should I land the test too?
Flags: needinfo?(abillings)

Comment 13

a year ago
Created attachment 8750682 [details] [diff] [review]
m-i test
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)
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Baku, could you please nominate patches for uplift to Beta, Aurora, ESR45? Thanks!
Flags: needinfo?(amarchesini)

Comment 17

a year 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+


ESR45's closed, so I didn't uplift this there yet.
status-firefox47: affected → fixed
status-firefox48: affected → fixed
tracking-firefox-esr45: ? → 47+
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)
status-firefox-esr45: affected → fixed
Flags: needinfo?(wkocher)
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]


11 months ago
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.