Closed Bug 1035007 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: Networking, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
firefox30 --- wontfix
firefox31 --- wontfix
firefox32 + verified
firefox33 + verified
firefox34 + verified
firefox-esr24 --- unaffected
firefox-esr31 32+ verified
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: jruderman, Assigned: valentin)

References

(Blocks 1 open bug)

Details

(Keywords: crash, sec-critical, testcase, Whiteboard: [adv-main32+][adv-esr31.1+])

Attachments

(4 files)

Attached file testcase
No description provided.
Attached file stack
Group: core-security
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)
Group: network-core-security
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.
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(valentin.gosu)
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/238ab54e98e3
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Attached patch TestSplinter Review
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
Attachment #8451391 - Flags: approval-mozilla-beta?
Attachment #8451391 - Flags: approval-mozilla-aurora?
Comment on attachment 8451391 [details]
testcase

Let's move the request over to the actual patch :-)
Attachment #8451391 - Flags: approval-mozilla-beta?
Attachment #8451391 - Flags: approval-mozilla-aurora?
Attachment #8454175 - Flags: approval-mozilla-esr31?
Attachment #8454175 - Flags: approval-mozilla-beta?
Attachment #8454175 - Flags: approval-mozilla-aurora?
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+
Whiteboard: [adv-main32+][adv-esr31.1+]
Confirmed crash on Fx33, 2014-07-02.
Verified fixed in Fx32, Fx33 and Fx34, 2014-08-22.
Verified fixed in Fx31.1.0esr, release.
Group: network-core-security
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.