1422456 - (CVE-2018-12401) about:home crashes whole browser (possible memory corruption)

Status: VERIFIED FIXED

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

Description

[Abdulrahman Alqabandi]

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.

Comment 1

[:Gijs (he/him)]

Quota manager -> baku, can you take a look?

Comment 2

[Andrea Marchesini [:baku]]

janv knows more about QM.

Comment 3

[Abdulrahman Alqabandi]

Attachment: homeCrashDump.txt

FWIW this is the windbg crash dump that contains 'MEMORY_CORRUPTION_LARGE_80000003_memory_corruption!KERNEL32'

Comment 4

[Daniel Veditz [:dveditz]]

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.

Comment 5

[Tom Tung [:tt, :ttung]]

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

Comment 6

[Tom Tung [:tt, :ttung]]

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

Comment 7

[Tom Tung [:tt, :ttung]]

(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

Comment 8

[Andrea Marchesini [:baku]]

Attachment: origin_about.patch

Here a patch to fix the origin for about/moz-safe-about/indexeddb URLs.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 10

[Abdulrahman Alqabandi]

(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.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: origin_about.patch

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37d3e0a27dfe4afa23f067ff06fb309bdbf422ae
https://hg.mozilla.org/mozilla-central/rev/37d3e0a27dfe

Comment 13

[Jan Varga [:janv]]

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.

Comment 14

[Tom Tung [:tt, :ttung]]

I open bug 1491637 for discussing and investigating that if they were created, we might want to support them in that bug.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this worth backporting to Beta? If so, go ahead and do the request, it grafts cleanly.

Comment 16

[Andrea Marchesini [:baku]]

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

Comment 17

[Pascal Chevrel:pascalc]

Comment on attachment 9008853 [details] [diff] [review]
origin_about.patch

Crash fix verified on Nightly, approved for 63 Beta 8, thanks.

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7b6a90c66a6c

Comment 19

[Cornel Ionce [:noni] [Hubs QA]]

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.

Comment 20

[Cornel Ionce [:noni] [Hubs QA]]

Also confirming the fix for Fx 63.0b8 (buildID: 20180920135444).