1005578 - nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer, #2

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Same symptoms as bug 991471, different testcase.

Comment 1

[Jesse Ruderman]

Attachment: stack (ASan)

Comment 2

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Ok, so the problem here is as follows:

var url = new URL("http://mozilla.org/"); // creates an nsStandardURL object

url.href = "javascript: 5";
// it first checks that ioService->NewURI(href,...) succeeds. It does.
// then it does aRv = mURI->SetSpec(href); This fails, as mURI is nsStandardURL, not a nsJSURI
// so all of the parameters in the nsStandardURL object get cleared

url.hostname = "allizom.org";
// tries to set the hostname, but the url is missing everything, including a scheme. From here we have a bad offset and crash.

Comment 3

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Attachment: url_crash.patch

According to the spec, we should discard the old url object when calling url.href

http://url.spec.whatwg.org/#concept-uu-set-the-input

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8417035 [details] [diff] [review]
url_crash.patch

Yeah, makes sense.

Comment 5

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8417035 [details] [diff] [review]
url_crash.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The changes made in the patch don't immediately reveal the vulnerability. It would be very difficult to create an exploit based on this patch.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The comment does say what the problem is, and it might need to be changed.

Which older supported branches are affected by this flaw?
All branches are affected.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
It should be easy to apply this patch on all branches.


How likely is this patch to cause regressions; how much testing does it need?

Comment 6

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

(In reply to Valentin Gosu [:valentin] from comment #5)
> How likely is this patch to cause regressions; how much testing does it need?
Unlikely. Not a risky change, and it is the specified behavior in the spec.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8417035 [details] [diff] [review]
url_crash.patch

sec-approval+ for trunk.

Please create Aurora, Beta, and ESR24 patches and nominate them once it is in trunk.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffa88afe433

Please request Aurora/Beta/ESR24 approval on this ASAP to expedite it being uplifted everywhere else it needs to go.

Comment 9

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

It seems I misread the output of hg log, and that the error was actually introduced by http://hg.mozilla.org/mozilla-central/rev/f2199d73aef6
ESR24 is not affected.

Comment 10

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

Comment on attachment 8417035 [details] [diff] [review]
url_crash.patch

[Approval Request Comment]
Issue caused by (bug #): Bug 887364
User impact if declined: Security concerns
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 11

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8417035 [details] [diff] [review]
url_crash.patch

Valentin, don't hesitate to be a bit more verbose next time ;)

Comment 12

[Valentin Gosu [:valentin] (he/him) {{ AFK in July }}]

(In reply to Sylvestre Ledru [:sylvestre] from comment #11)
> Valentin, don't hesitate to be a bit more verbose next time ;)
Thanks! I will :)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ffa88afe433

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0f40e6c752c8
https://hg.mozilla.org/releases/mozilla-beta/rev/0c8b19312abf

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/0c8b19312abf
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/6d6cf2f3166e
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/bc77a1b00676

Comment 16

[Matt Wobensmith [:mwobensmith][:matt:]]

Confirmed crash on 2014-04-21, debug build of Fx30.
Verified fixed 2014-06-03, ASan builds of Fx30, Fx31 and Fx32.

Comment 17

[Justin Wood (:Callek)]

Applied ontop of a relbranch for SeaMonkey 2.26.1 (Gecko 29 based):

https://hg.mozilla.org/releases/mozilla-release/rev/908f619b9122