Closed Bug 1215944 Opened 4 years ago Closed 4 years ago

crash in nsStandardURL::BuildNormalizedSpec

Categories

(Core :: Networking, defect, critical)

Unspecified
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox41 --- unaffected
firefox42 + fixed
firefox43 + fixed
firefox44 --- fixed
firefox-esr38 42+ fixed

People

(Reporter: gfritzsche, Assigned: valentin.gosu)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main42-][adv-esr38.4-])

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is 
report bp-2d06acbd-2ff0-464e-bb3a-f745b2151018.
=============================================================

This was consistently reproducible over multiple tries and Fx restarts by just opening a new tab and CMD-V pasting a https:// URL that was inserted into the clipboard by Dropbox.
After a few restarts this suddenly stopped crashing, but i could potentially try around in case the stack and/or dumps are not sufficient.

Making it security-sensitive until it is clear whether this is exploitable.
Setting needinfo per hg blame information around the crash location.

Examples:
bp-880147bb-1bb9-48f7-bc7d-5fc2c2151018
bp-2d06acbd-2ff0-464e-bb3a-f745b2151018
bp-8f293365-482f-4b14-b0e7-fdb5d2151018
bp-873504c3-4ecb-47c7-96a9-dd3d22151018
bp-71828d78-0983-412d-a211-7e8102151018
Flags: needinfo?
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(mcmanus)
Flags: needinfo?
Thanks Georg,
I've identified the bug. Could you provide the URL you pasted so we could have a reliable test case?
Thanks!
Flags: needinfo?(valentin.gosu)
Keywords: sec-high
Flags: needinfo?(mcmanus)
This reintroduces the empty string check I had mistakenly removed when I added the length parameter in bug 1199430
Assignee: nobody → valentin.gosu
Attachment #8675505 - Flags: review?(mcmanus)
(In reply to Valentin Gosu [:valentin] from comment #1)
> Thanks Georg,
> I've identified the bug. Could you provide the URL you pasted so we could
> have a reliable test case?
> Thanks!

This was the URL i was trying to copy-paste: https://www.dropbox.com/s/5j4p5mpodv3pie3/sektore_v3.mp3?dl=0
However, this might also have been Dropbox putting unexpected contents in the clipboard as sometimes i got an an empty string trying to paste elsewhere.
The best thing would be to check the submitted dumps for what data was actually passed here.
Attachment #8675505 - Flags: review?(mcmanus) → review+
Keywords: regression
Comment on attachment 8675505 [details] [diff] [review]
bug1215944-validhostname-empty.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Very difficult. The crash is caused by an out-of-bounds read when the length of the string is 0.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
Present in aurora, beta and esr38 (not in release)

If not all supported branches, which bug introduced the flaw?
Introduced in bug 1199430 which was uplifted to beta, aurora and esr38.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Yes. The patch is very simple.

How likely is this patch to cause regressions; how much testing does it need?
This shouldn't cause any regressions as this check was there before 1199430 but I mistakenly removed it.
Attachment #8675505 - Flags: sec-approval?
Comment on attachment 8675505 [details] [diff] [review]
bug1215944-validhostname-empty.patch

sec-approval+ for trunk. This needs nominated patches for Aurora, Beta, and ESR38. Sooner is better than later!
Attachment #8675505 - Flags: sec-approval? → sec-approval+
Keywords: checkin-needed
Comment on attachment 8675505 [details] [diff] [review]
bug1215944-validhostname-empty.patch

Approval Request Comment
[Feature/regressing bug #]:
Bug 1199430

[User impact if declined]:
Possible crashes when loading ignored-single-character-IDNA URLs or other single character hostname URLs.

[Describe test coverage new/current, TreeHerder]:
Attached test does not cause a crash after this patch is applied.

[Risks and why]:
Low risk, as this check was there before bug 1199430.

[String/UUID change made/needed]:
None
Attachment #8675505 - Flags: approval-mozilla-esr38?
Attachment #8675505 - Flags: approval-mozilla-beta?
Attachment #8675505 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1216338
Duplicate of this bug: 1215837
Group: core-security → network-core-security
https://hg.mozilla.org/mozilla-central/rev/8dc73ca2ec85
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Group: network-core-security → core-security-release
Comment on attachment 8675505 [details] [diff] [review]
bug1215944-validhostname-empty.patch

Seems safe and fixes a crash, taking it. Should be in 42 beta 9.
Attachment #8675505 - Flags: approval-mozilla-beta?
Attachment #8675505 - Flags: approval-mozilla-beta+
Attachment #8675505 - Flags: approval-mozilla-aurora?
Attachment #8675505 - Flags: approval-mozilla-aurora+
Georg, if you have a minute, would you want to take a look at this to confirm that it was fixed? Thank you.
Flags: needinfo?(gfritzsche)
Comment on attachment 8675505 [details] [diff] [review]
bug1215944-validhostname-empty.patch

Taking it for esr38.4.0 as it's a sec-high and a simple fix.
Attachment #8675505 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38+
(In reply to Matt Wobensmith from comment #15)
> Georg, if you have a minute, would you want to take a look at this to
> confirm that it was fixed? Thank you.

I'm not able to reproduce this right now.
Flags: needinfo?(gfritzsche)
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main42-][adv-esr38.4-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.