Closed Bug 1035007 Opened 6 years ago Closed 6 years ago
Standard URL::Set Host called net _To Lower Case with a bogus pointer, #4
178 bytes, text/html
9.14 KB, text/plain
1.60 KB, patch
|Details | Diff | Splinter Review|
997 bytes, patch
|Details | Diff | Splinter Review|
No description provided.
I'll take this. It seems the problem comes up when setting a path containing a colon on a URL with no hostname.
Assignee: nobody → valentin.gosu
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.
Attachment #8454175 - Flags: review?(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
Attachment #8454175 - Flags: review?(mcmanus) → review+
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.
Attachment #8454175 - Flags: sec-approval?
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
Attachment #8454175 - Flags: sec-approval? → sec-approval+
Do we need this on ESR31 as well? It is a critical rated security bug.
This is okay to check in now.
You are going to need a try run linked in the bug to get checked in.
Whiteboard: [checkin on 8/5]
Sure thing. I think I did at some point, but I can't find it anymore :) Thanks for the heads up.
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 on attachment 8451391 [details] testcase Let's move the request over to the actual patch :-)
Oops. Thanks Ryan!
Attachment #8454175 - Flags: approval-mozilla-esr31?
Attachment #8454175 - Flags: approval-mozilla-esr31+
Attachment #8454175 - Flags: approval-mozilla-beta?
Attachment #8454175 - Flags: approval-mozilla-beta+
Attachment #8454175 - Flags: approval-mozilla-aurora?
Attachment #8454175 - Flags: approval-mozilla-aurora+
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
Confirmed crash on Fx33, 2014-07-02. Verified fixed in Fx32, Fx33 and Fx34, 2014-08-22. Verified fixed in Fx31.1.0esr, release.
You need to log in before you can comment on or make changes to this bug.