URL with embedded null: crash [@ _platform_strncmp | nsStandardURL::SegmentIs | nsStandardURL::EqualsInternal]

RESOLVED FIXED in Firefox 38, Firefox OS master

Status

()

Core
Networking
--
critical
RESOLVED FIXED
3 years ago
a year ago

People

(Reporter: Jesse Ruderman, Assigned: valentin)

Tracking

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

Trunk
mozilla39
x86_64
Mac OS X
crash, sec-high, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox37 unaffected, firefox38+ fixed, firefox39+ fixed, firefox-esr31 unaffected, b2g-v1.4 unaffected, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master fixed)

Details

(crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8578969 [details]
testcase (crashes Firefox when loaded or on shutdown)
(Reporter)

Comment 1

3 years ago
Created attachment 8578973 [details]
stack (lldb)
(Reporter)

Comment 2

3 years ago
Created attachment 8578976 [details]
stack (minidump_stackwalk)
(Reporter)

Comment 3

3 years ago
First stack:
* Crash on load
* Frame #10 is mozilla::places::History::RegisterVisitedCallback

Second stack:
* Crash on unload
* Frame #5 is mozilla::places::History::UnregisterVisitedCallback
(Assignee)

Comment 4

3 years ago
Created attachment 8579391 [details] [diff] [review]
Restrict charcters which are allowed in the URL hash (ref)
Attachment #8579391 - Flags: review?(mcmanus)
(Assignee)

Updated

3 years ago
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1129855
Marking sec-high because the dupe is sec-high.
Keywords: sec-high
(Assignee)

Comment 7

3 years ago
Created attachment 8579434 [details] [diff] [review]
Restrict charcters which are allowed in the URL hash (ref)
Attachment #8579434 - Flags: review?(mcmanus)
(Assignee)

Comment 8

3 years ago
Comment on attachment 8579391 [details] [diff] [review]
Restrict charcters which are allowed in the URL hash (ref)

I removed the percent decoding bit, because of a web-platform regression:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8605f9110db
Attachment #8579391 - Attachment is obsolete: true
Attachment #8579391 - Flags: review?(mcmanus)
Comment on attachment 8579434 [details] [diff] [review]
Restrict charcters which are allowed in the URL hash (ref)

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

this adds another linear scan of the whole url.. but valentin convinced me its the only way to do it safely without massively ripping things up and the url parser engages in far too many copies and scans already so this change doesn't significantly change the complexity of a url parse.

since a stale version of this has already gone to try once, please send it again and get a green run before committing.
Attachment #8579434 - Flags: review?(mcmanus) → review+
(Assignee)

Comment 10

3 years ago
Try is green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d93488e5548
Keywords: checkin-needed
Needs sec-approval.
https://wiki.mozilla.org/Security/Bug_Approval_Process
Keywords: checkin-needed
(Assignee)

Comment 12

3 years ago
Comment on attachment 8579434 [details] [diff] [review]
Restrict charcters which are allowed in the URL hash (ref)

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
- Easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
- Yes, it is fairly obvious what the patch does. It restricts several characters from appearing in the ref part of the URL.

Which older supported branches are affected by this flaw?
- Nightly and Aurora

If not all supported branches, which bug introduced the flaw?
- Introduced by bug 1093611.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
- This patch should be easy to apply on all affected branches (nightly + aurora)

How likely is this patch to cause regressions; how much testing does it need?
- Unlikely. It introduces an extra check which should have been there from the beginning.
Attachment #8579434 - Flags: sec-approval?
Comment on attachment 8579434 [details] [diff] [review]
Restrict charcters which are allowed in the URL hash (ref)

sec-approval+ for trunk.

Let's get a patch nominated for aurora as well.
Attachment #8579434 - Flags: sec-approval? → sec-approval+
status-firefox37: --- → unaffected
status-firefox38: --- → affected
status-firefox39: --- → affected
status-firefox-esr31: --- → unaffected
tracking-firefox38: --- → +
tracking-firefox39: --- → +
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c334736aa584
Flags: in-testsuite?
Keywords: checkin-needed
(Assignee)

Comment 15

3 years ago
Comment on attachment 8579434 [details] [diff] [review]
Restrict charcters which are allowed in the URL hash (ref)

Approval Request Comment
[Feature/regressing bug #]:
Introduced by bug 1093611.
[User impact if declined]:
Possible security concerns.
[Describe test coverage new/current, TreeHerder]:
Green on try. Pushed to m-c. Manual testing.
[Risks and why]:
Low risk patch. It introduces a check for invalid characters.
The extra check may pose a performance issue, but it is negligible for the average case (we already do many scans of the URL spec).
[String/UUID change made/needed]:
None.
Attachment #8579434 - Flags: approval-mozilla-aurora?
Attachment #8579434 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/c334736aa584
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-master: --- → fixed
status-firefox39: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
https://hg.mozilla.org/releases/mozilla-aurora/rev/35b6e9d50a36
status-firefox38: affected → fixed

Updated

2 years ago
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.