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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded or on shutdown)

Comment 1

[Jesse Ruderman]

Attachment: stack (lldb)

Comment 2

[Jesse Ruderman]

Attachment: stack (minidump_stackwalk)

Comment 3

[Jesse Ruderman]

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

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

Comment 4

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

Attachment: Restrict charcters which are allowed in the URL hash (ref)

Comment 6

[Andrew McCreight [:mccr8]]

Marking sec-high because the dupe is sec-high.

Comment 7

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

Attachment: Restrict charcters which are allowed in the URL hash (ref)

Comment 8

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

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

Comment 9

[Patrick McManus [: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.

Comment 10

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

Try is green
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d93488e5548

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Needs sec-approval.
https://wiki.mozilla.org/Security/Bug_Approval_Process

Comment 12

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

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.

Comment 13

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

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.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c334736aa584

Comment 15

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

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c334736aa584

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/35b6e9d50a36