Closed Bug 1422456 (CVE-2018-12401) Opened 3 years ago Closed 2 years ago

about:home crashes whole browser (possible memory corruption)

Categories

(Core :: Storage: IndexedDB, defect, P3)

58 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- verified
firefox64 --- verified

People

(Reporter: qab, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: csectype-dos, sec-low, Whiteboard: [post-critsmash-triage][adv-main63+])

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.94 Safari/537.36

Steps to reproduce:

Visit the following URL: about:home?1:q

"MOZ_CRASH(Should never get here!)" is the reason.



Actual results:


Report:
https://crash-stats.mozilla.com/report/index/5bd57607-cb08-4606-b085-9755d0171202#tab-details


Expected results:

No crash. Also I will mark as sensitive since I am seeing the words "MEMORY_CORRUPTION_LARGE_80000003_memory_corruption!KERNEL32" in my windbg, though I'm not sure if it is. Also, about:home is linkable from web content, so if this was exploitable its even more dangerous as it requires no user interaction.
Quota manager -> baku, can you take a look?
Flags: needinfo?(amarchesini)
janv knows more about QM.
Flags: needinfo?(amarchesini) → needinfo?(jvarga)
Group: firefox-core-security → dom-core-security
Component: Untriaged → DOM: IndexedDB
Product: Firefox → Core
Attached file homeCrashDump.txt
FWIW this is the windbg crash dump that contains 'MEMORY_CORRUPTION_LARGE_80000003_memory_corruption!KERNEL32'
Crashes the whole browser for me -- boom down in IndexedDB bp-90668379-16a2-4fbf-8e83-32b7e0180419

Could not reproduce in 58, 59 or 60 with a fresh profile which is kind of odd. Maybe I didn't have enough stored in IndexedDB for it to need to check the per-site quota? Haven't yet tried a fresh profile in nightly.

Doesn't seem exploitable in the malware sense, just a DoS -- but a bad one because it kills the parent process and not just the tab.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Flags: needinfo?(jvarga)
Blocks: 1482662
Priority: -- → P3
Assignee: jvarga → shes050117
I can verify current Nightly hits the assertion here [1] in debug build. If I comment [1], then it will crash in [2], which looks like the place comment #0 mentioned. (Somehow I couldn't see the crash report)

[1] https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/quota/ActorsParent.cpp#8637
[2] https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/quota/ActorsParent.cpp#8723
I checked the origin [1] from GetInfoFromPrincipal() if typing "about:home?1:q". I expected that origin should be "about:home", but, actually, I got "about:home?1:q".

It seems that about protocol doesn't strip the ref and query which isn't expected by QuotaManager. So that if typing "about:home?1" or "about:home#2", I will get "about+home+1", and "about+home+2" in my profile.

I reckon in this bug we should do things below:
1. Make origin parser not be so strict. At least, it shouldn't let FF crash [2] if the format of handling protocol isn't as expected. We may still want to capture issues on Nightly via crashing it. But, as for Release and Beta, just return false should be fine.
2. Strip '?xxx' or '#xxx' for about protocol in GetInfoFromPrincipal() or returning false earlier [3] if mSchemeType is eNone in OriginParser.

[1] https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/quota/ActorsParent.cpp#5613
[2] https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/quota/ActorsParent.cpp#8723
[3] https://searchfox.org/mozilla-central/rev/de7676288a78b70d2b9927c79493adbf294faad5/dom/quota/ActorsParent.cpp#8636-8663
(In reply to Tom Tung [:tt] from comment #6)
> I checked the origin [1] from GetInfoFromPrincipal() if typing
> "about:home?1:q". I expected that origin should be "about:home", but,
> actually, I got "about:home?1:q".

According to the nsIPrincipal's interface [1] and the MDN documents for origin [2] (also the reference documents inside it), I expected the origin which getting from the Principal object shouldn't contain '?'. However, "about:home" seems to be an internal implementation for Gecko, so I'm not really sure should I fix internally in dom/quota/ActorsParent.cpp or somewhere else when the principal object for "about:xxx" is created.

baku, do you have any suggestion about that?

[1] https://searchfox.org/mozilla-central/source/caps/nsIPrincipal.idl#220-226
[2] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Origin
Flags: needinfo?(amarchesini)
Attached patch origin_about.patch (obsolete) — Splinter Review
Here a patch to fix the origin for about/moz-safe-about/indexeddb URLs.
Flags: needinfo?(amarchesini)
Attachment #9008795 - Flags: review?(bugs)
Comment on attachment 9008795 [details] [diff] [review]
origin_about.patch

We must get tests for this.

As of now, easiest might be some browser test which loads about:robots?foo and its contentDocument.nodePrincipal.URI.spec should be then about:robots. Need to test also # url with both # and ?.

I'd like to review this with the test.

Can the crash be somehow triggered by web page?
Attachment #9008795 - Flags: review?(bugs)
(In reply to Olli Pettay [:smaug] from comment #9)
> Can the crash be somehow triggered by web page?

At the time of the report, yes. But ever since about:home has become unlinkable from web, so as of now, no.
Assignee: shes050117 → amarchesini
Attachment #9008795 - Attachment is obsolete: true
Attachment #9008853 - Flags: review?(bugs)
Attachment #9008853 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/37d3e0a27dfe4afa23f067ff06fb309bdbf422ae
https://hg.mozilla.org/mozilla-central/rev/37d3e0a27dfe
Group: dom-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Tom, so the URL fix has landed, but are we sure that there can't be any origin directories like about+home+1 ?
Maybe such directories could have been created, in that case we need to extend origin parser to support it.
Flags: needinfo?(shes050117)
I open bug 1491637 for discussing and investigating that if they were created, we might want to support them in that bug.
Flags: needinfo?(shes050117)
Is this worth backporting to Beta? If so, go ahead and do the request, it grafts cleanly.
Flags: needinfo?(amarchesini)
Flags: in-testsuite+
Comment on attachment 9008853 [details] [diff] [review]
origin_about.patch

Approval Request Comment
[Feature/Bug causing the regression]: origin computation for about: URLs
[User impact if declined]: ff crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes, see the description of the bug 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: The fix consists in the removal of the ref and query part of about: URLs, exactly how we do for http/https/ftp/.. URLs.
[String changes made/needed]: none
Flags: needinfo?(amarchesini)
Attachment #9008853 - Flags: approval-mozilla-beta?
Comment on attachment 9008853 [details] [diff] [review]
origin_about.patch

Crash fix verified on Nightly, approved for 63 Beta 8, thanks.
Attachment #9008853 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
Reproduced this crash on Windows 10x64 using Fx 63.0b1, buildID 20180824192747.

Confirmed the fix on latest Nightly, build ID: 20180919003800.

Leaving the qe+ flag in place until I`ll also cover this fix on Beta 63.
QA Contact: cornel.ionce
Also confirming the fix for Fx 63.0b8 (buildID: 20180920135444).
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+]
Alias: CVE-2018-12401
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.