1035007 - nsStandardURL::SetHost called net_ToLowerCase with a bogus pointer, #4

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Valentin Gosu [:valentin] (he/him)]

I'll take this.
It seems the problem comes up when setting a path containing a colon on a URL with no hostname.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

Attachment: url_sethost.patch

It seems this crash is somewhat my fault (Bug 991471, changeset 178001:8e20983ae82d).

Note that while this patch does fix the crash, the behaviour we get in the end isn't exactly correct. For example:

var url = new URL("e:");
url.pathname = "";
url.protocol = "https:"; // We now have a url with no host
url.pathname = "1:\\2"; 
// this calls SetSpec with the pathname appended at the end
// SetSpec fails, because there is no hostname
// When it fails, all of the fields are cleared, so we now have an empty url
url.host = "mozilla.org";
// This doesn't crash anymore, but the url is "mozilla.org" instead of "https://mozilla.org"

Bug 1009648 would somewhat fix this issue by reverting the url in case SetSpec fails. I was reluctant to push the patch until we fixed all of the ways the url could get to an "unparsable" state, because those would easily lead to a recursive loop & stack overflow.

Comment 4

[Patrick McManus [:mcmanus]]

Comment on attachment 8454175 [details] [diff] [review]
url_sethost.patch

Review of attachment 8454175 [details] [diff] [review]:
-----------------------------------------------------------------

this seems like a reasonable workaround while we wait for 1009648

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8454175 [details] [diff] [review]
url_sethost.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
While the fix is pretty obvious, figuring out how to exploit it using content code would be quite difficult. 

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No. The crash depends on creating a URL with no host, and then doing something that clears the URL. There is nothing to suggest this in the patch.

Which older supported branches are affected by this flaw?
All branches which integrated Bug 991471 are affected by this. So firefox29 and later.

If not all supported branches, which bug introduced the flaw?
It was introduced by Bug 991471

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?
Unlikely. It deals with very specific edge cases which it fixes. Passes existing unit tests.

Comment 6

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

This is going to be delayed for checkin for a few weeks because we just made the Firefox 31 builds for release on this coming tuesday.

sec-approval+ for checkin on trunk after 8/5

Comment 7

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

Do we need this on ESR31 as well? It is a critical rated security bug.

Comment 8

[Andrew McCreight [:mccr8]]

This is okay to check in now.

Comment 9

[Andrew McCreight [:mccr8]]

You are going to need a try run linked in the bug to get checked in.

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Sure thing. I think I did at some point, but I can't find it anymore :)
Thanks for the heads up.

Comment 11

[Valentin Gosu [:valentin] (he/him)]

https://tbpl.mozilla.org/?tree=Try&rev=d0795a900228

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/238ab54e98e3

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/238ab54e98e3

Comment 14

[Valentin Gosu [:valentin] (he/him)]

Attachment: Test

Comment 15

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8451391 [details]
testcase

Approval Request Comment
[Feature/regressing bug #]: Bug 991471
[User impact if declined]: Potential crash and security concerns
[Describe test coverage new/current, TBPL]: On m-c, all green on try, fixes attached test and testcase
[Risks and why]: Fixing this crash allows nsStandardURL to have slightly incorrect values at times. Also fixing Bug 1009648 would eliminate this concern.
[String/UUID change made/needed]: none

Comment 16

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8451391 [details]
testcase

Let's move the request over to the actual patch :-)

Comment 17

[Valentin Gosu [:valentin] (he/him)]

Oops. Thanks Ryan!

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9306dcf4eb39
https://hg.mozilla.org/releases/mozilla-beta/rev/e6cee3b7907e
https://hg.mozilla.org/releases/mozilla-esr31/rev/ebefe851e8a1
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/bef95cbe17be
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/17060408fc3f

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e6cee3b7907e

Comment 20

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

Confirmed crash on Fx33, 2014-07-02.
Verified fixed in Fx32, Fx33 and Fx34, 2014-08-22.
Verified fixed in Fx31.1.0esr, release.