1507702 - (CVE-2018-18510) "about:crashcontent" and "about:crashparent" can be triggered from web content

Status: VERIFIED FIXED

(Firefox :: Security, defect, P1)

Description

[Hanno Boeck]

The firefox special URL "about:crashcontent" will trigger a crash of the current tab for testing purposes.

It is possible to trigger that URL from within a webpage, e.g. via an <img> tag. This also extends to being able to crash a browser from within an iframe, which I believe is particularly problematic as iframes are often used to embed content that a site not necessarily trusts.

(For a practical example look at https://twitter.com/xsamaster/status/1063347833271713792 - a tweet that embeds a "video" that will crash the browser tab.)

I believe a special debugging feature URL like about:crashcontent should never be accessible from a webpage, it should only be possible to activate it if the user actually types in that URL.

Comment 1

[Johann Hofmann [:johannh]]

Wow, thank you for reporting this. It gets even worse, you can crash the entire browser with data:text/html,<link rel="icon" href="about:crashparent"/>, so that means any web content can trivially trigger it.

Combined with other issues (e.g. gaming frecency to become a top site) this can be catastrophic.

Snorp, we should not have about:crashparent in the first place. I remember a proposal for having an about:restart page being denied in an email discussion based on the effort we would have to go through to avoid loopholes that cause web content to load it. Even if we fix this issue IMO the risk is too high to keep it.

I'm less sure about about:crashcontent, though my vote would be for removing it as well.

Considering that this is a tweet already, we might need to act fast before someone discovers about:crashparent...

Comment 2

[Hanno Boeck]

I'm deleting the tweet + the loaded code for now. (I didn't think a lot about creating this, this is a test account from me, I doubt anyone knows about it publicly.)

I'd also like to voice my opinion that I absolutely think this is a useful feature and should *not* be removed. I wish I knew about it, then I could've found #1507142 sooner. However I think it might be an idea to put it behind a flag that comes with a warning that this should only be used for debugging.

Comment 3

[:Gijs (he/him)]

At a minimum, this should have been nightly/testing-only, maybe mobile-only if that's what it's for, and it should have been tested thoroughly to make sure web content couldn't trip it in any way. I'm not sure if just removing URI_SAFE_FOR_UNTRUSTED_CONTENT is enough here, given the way the implementation is done (ie in NewChannel, rather than when the content of the URI actually loads).

Comment 4

[Christian Holler (:decoder)]

(In reply to :Gijs (he/him) from comment #3)
> At a minimum, this should have been nightly/testing-only, maybe mobile-only
> if that's what it's for, and it should have been tested thoroughly to make
> sure web content couldn't trip it in any way.

I agree that web content shouldn't be able to trigger this, but I disagree about this being e.g. mobile-only. this feature is required to test automation across different platforms.

If possible, we should make it such that it cannot be loaded as a resource and not redirected to. But I don't know if that is easily doable.

Comment 5

[:Gijs (he/him)]

(In reply to Christian Holler (:decoder) from comment #4)
> (In reply to :Gijs (he/him) from comment #3)
> > At a minimum, this should have been nightly/testing-only, maybe mobile-only
> > if that's what it's for, and it should have been tested thoroughly to make
> > sure web content couldn't trip it in any way.
> 
> I agree that web content shouldn't be able to trigger this, but I disagree
> about this being e.g. mobile-only. this feature is required to test
> automation across different platforms.

If we need it just for testing automation, can we make it only available when MOZ_AUTOMATION / Cu.isInAutomation is true?

Comment 6

[(not currently active) Ted Mielczarek]

Fundamentally this is also a replacement for the "Crash me now" extension that stopped working when we dropped support for non-web extensions. Being able to test our crash reporting pipeline is pretty important. If we disable these URLs then we should figure out a path to support something like that extension again. rhelmer had a prototype web extension + experiment but we had a hard time getting people on board with shipping it: https://github.com/rhelmer/webext-experiment-crashme/

Comment 7

[Christian Holler (:decoder)]

(In reply to :Gijs (he/him) from comment #5)

> If we need it just for testing automation, can we make it only available
> when MOZ_AUTOMATION / Cu.isInAutomation is true?


Yes, I believe going for this and/or a set of prefs is the right approach. Johann also proposed this. In Fuzzing, I don't think we have Cu.isInAutomation set, but we do have a fuzzing pref and FUZZING builds. So it is certainly possible to tie this to some pref/env variables.

Comment 8

[Johann Hofmann [:johannh]]

Yeah, I think just preffing this off and fixing consumers is the easiest solution here :)

Comment 9

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #8)
> Yeah, I think just preffing this off and fixing consumers is the easiest
> solution here :)

Well, we can probably/hopefully enforce that the channel is a toplevel document load as well... belt and suspenders and all that.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Ugh, what a mess. I would prefer a solution that doesn't require prefs, if doable. GeckoView consumers don't have a direct way of setting prefs and they also want to be able to test crash reporting since there is some downstream stuff that they need to implement.

My preference is to restrict this to toplevel, user-entered loads if possible. It looks like we should be able to ensure it's a toplevel load by checking `nsILoadInfo::loadingDocument` in `nsAboutRedirector::NewChannel`, but I'm not sure about the user-entered part. I thought we had some flags for that, but I guess they're not present in nsILoadInfo? The comments in nsAboutModule.idl indicate you need MAKE_LINKABLE to allow links from content, so maybe checking for toplevel is enough?

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1507702 - Don't make about:crash* accessible to web content r?Ehsan,bzbarsky

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Unlikely we'll fix this in 63 at this point, but we should definitely do something for 64+.

Comment 13

[:Gijs (he/him)]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #10)
> Ugh, what a mess. I would prefer a solution that doesn't require prefs, if
> doable. GeckoView consumers don't have a direct way of setting prefs and
> they also want to be able to test crash reporting since there is some
> downstream stuff that they need to implement.

Can those consumers not set the right env vars to trip Cu.isInAutomation? Otherwise, could geckoview expose an API that consumers can call to toggle this functionality on/off? (ni for this)

> My preference is to restrict this to toplevel, user-entered loads if
> possible. It looks like we should be able to ensure it's a toplevel load by
> checking `nsILoadInfo::loadingDocument` in `nsAboutRedirector::NewChannel`,
> but I'm not sure about the user-entered part.

You can check for triggering principal == system principal, I guess, but I'm not aware of us having anything that *actually* means user-entered.

> I thought we had some flags
> for that, but I guess they're not present in nsILoadInfo? The comments in
> nsAboutModule.idl indicate you need MAKE_LINKABLE to allow links from
> content, so maybe checking for toplevel is enough?

I don't think that alone is enough, given that this exploit uses <iframe> or <video> or <img> sourcing. From just checking a data:text/html,<img src=about:crashcontent>,  apparently we first create the channel and then do security checks on that channel (from inside AsyncOpen). In which case, the implementation of the crash may need to move out of NewChannel so the crash only happens *after* the security checks instead of *before*. Someone who knows CAPS/docshell better than me can comment, I guess - :ckerschb ?

Comment 14

[(not currently active) Ted Mielczarek]

I don't see that anyone has mentioned it yet but for the record--these are safe crashes that are not exploitable. This is simply a DoS vector.

Comment 15

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to :Gijs (he/him) from comment #13)
> (In reply to James Willcox (:snorp) ([e-mail]) from comment #10)
> > Ugh, what a mess. I would prefer a solution that doesn't require prefs, if
> > doable. GeckoView consumers don't have a direct way of setting prefs and
> > they also want to be able to test crash reporting since there is some
> > downstream stuff that they need to implement.
> 
> Can those consumers not set the right env vars to trip Cu.isInAutomation?
> Otherwise, could geckoview expose an API that consumers can call to toggle
> this functionality on/off? (ni for this)

We could, but it's a PITA. It's just that much more work that every app needs to duplicate. If it comes to this, fine, but avoiding would be preferable.

> 
> > My preference is to restrict this to toplevel, user-entered loads if
> > possible. It looks like we should be able to ensure it's a toplevel load by
> > checking `nsILoadInfo::loadingDocument` in `nsAboutRedirector::NewChannel`,
> > but I'm not sure about the user-entered part.
> 
> You can check for triggering principal == system principal, I guess, but I'm
> not aware of us having anything that *actually* means user-entered.
> 
> > I thought we had some flags
> > for that, but I guess they're not present in nsILoadInfo? The comments in
> > nsAboutModule.idl indicate you need MAKE_LINKABLE to allow links from
> > content, so maybe checking for toplevel is enough?
> 
> I don't think that alone is enough, given that this exploit uses <iframe> or
> <video> or <img> sourcing. From just checking a data:text/html,<img
> src=about:crashcontent>,  apparently we first create the channel and then do
> security checks on that channel (from inside AsyncOpen). In which case, the
> implementation of the crash may need to move out of NewChannel so the crash
> only happens *after* the security checks instead of *before*. Someone who
> knows CAPS/docshell better than me can comment, I guess - :ckerschb ?

The patch + test seems to be working for these cases, but I could be missing something there. I would definitely appreciate an expert opinion.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #13)
> I don't think that alone is enough, given that this exploit uses <iframe> or
> <video> or <img> sourcing. From just checking a data:text/html,<img
> src=about:crashcontent>,  apparently we first create the channel and then do
> security checks on that channel (from inside AsyncOpen). In which case, the
> implementation of the crash may need to move out of NewChannel so the crash
> only happens *after* the security checks instead of *before*. Someone who
> knows CAPS/docshell better than me can comment, I guess - :ckerschb ?

Right, so currently whenever we create a new Channel the NewChannel() implementation, for this case in particular [1] is run before we open the channel. In other words, no security have been performed at that point. I think it makes sense to move that code [1] somewhere else. A common point would be the ContentSecurityManager after all the security checks have performed, somewhere at the end of DoContentSecurityChecks().

Alternatively, probably better, we could move it to the right ::asyncOpen2() implementation and put it there after the security checks have been performed. I think about channels call nsBaseChannel::AsyncOpen2() [2].

Additionally, as Gijs suggested, I think it makes sense to not ship that code in release. We should use |#ifdef debug| or something similar.


[1] https://searchfox.org/mozilla-central/source/docshell/base/nsAboutRedirector.cpp#177
[2] https://searchfox.org/mozilla-central/source/netwerk/base/nsBaseChannel.cpp#752

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Oh, one last note that might be of importance. Do we want to allow that crash to be triggered by web content? E.g. for testing purposes? The right way to figure that out is by inspecting the TriggeringPrincipal within the LoadInfo. Calling  |triggeringPrincipal->GetIsCodebasePrincipal()| would reveal that information if needed.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Which effect will those upcoming changes have for tests? Marionette specifically has a unit test for checking crashes of Firefox and is using both the content and parent page to crash Firefox in test_crash.py.

Comment 19

[(no longer active)]

(In reply to Henrik Skupin (:whimboo) from comment #18)
> Which effect will those upcoming changes have for tests? Marionette
> specifically has a unit test for checking crashes of Firefox and is using
> both the content and parent page to crash Firefox in test_crash.py.

I don't know about the environment where those tests run in, but made a comment about this in the review.

Comment 20

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

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/50171af401fc178eb7ebc37e95730791b2328f24

Backed out for bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/beffa3a1e854d93a60caef290c03b4fb33a627a0

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Crunnable&group_state=expanded&revision=50171af401fc178eb7ebc37e95730791b2328f24
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=214616102&repo=mozilla-inbound
[task 2018-11-29T14:44:49.001Z] 14:44:49    ERROR -  /builds/worker/workspace/build/src/docshell/base/nsAboutRedirector.cpp:34:3: error: bad implicit conversion constructor for 'CrashChannel'
[task 2018-11-29T14:44:49.001Z] 14:44:49     INFO -    CrashChannel(nsIURI* aURI)
[task 2018-11-29T14:44:49.001Z] 14:44:49     INFO -    ^
[task 2018-11-29T14:44:49.001Z] 14:44:49     INFO -  /builds/worker/workspace/build/src/docshell/base/nsAboutRedirector.cpp:34:3: note: consider adding the explicit keyword to the constructor
[task 2018-11-29T14:44:49.002Z] 14:44:49     INFO -    CrashChannel(nsIURI* aURI)
[task 2018-11-29T14:44:49.002Z] 14:44:49     INFO -    ^
[task 2018-11-29T14:44:49.002Z] 14:44:49     INFO -    explicit

Comment 21

[(no longer active)]

Hmm, didn't this need sec-approval?

Comment 22

[Alex Gaynor [:Alex_Gaynor]]

No, sec-approval is only required for sec-high and sec-critical.

Comment 23

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

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

Comment 24

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

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. Thanks for writing a mochitest for this too.

Comment 26

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9025681 [details]
Bug 1507702 - Don't make about:crash* accessible to web content r?Ehsan,bzbarsky

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1462702

User impact if declined: Possible DoS from web content by causing intentional crashes.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Can't make it any worse, right?

String changes made/needed: None

Comment 27

[Hanno Boeck]

I just checked this in nightly and I don't think this is fully fixed.

The <img src="about:crashcontent"> no longer works, but the example from comment #1 - <link rel="icon" href="about:crashparent"/> - still crashes nightly.

Comment 28

[:Gijs (he/him)]

(In reply to Hanno Boeck from comment #27)
> I just checked this in nightly and I don't think this is fully fixed.
> 
> The <img src="about:crashcontent"> no longer works, but the example from
> comment #1 - <link rel="icon" href="about:crashparent"/> - still crashes
> nightly.

Thanks for pointing this out. We should deal with this in a separate bug. I filed bug 1511903.

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 9025681 [details]
Bug 1507702 - Don't make about:crash* accessible to web content r?Ehsan,bzbarsky

approved for 64 rc2

Comment 30

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/0d9074318f9c9ae05696710b3fe4d578c8253ea2

Comment 31

[Hani Yacoub, Desktop QA]

Accessing "about:crashcontent" in the latest Firefox Nightly and Firefox beta displays "Gah. Your tab just crashed." with the options to "Close Tab" or "Restore This tab" without closing Firefox.

But accessing "about:crashparent" crashes Firefox and close Firefox.

I'm not sure if this is the desired behavior here. Could you please clarify this to me?

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Hani Yacoub from comment #31)
> Accessing "about:crashcontent" in the latest Firefox Nightly and Firefox
> beta displays "Gah. Your tab just crashed." with the options to "Close Tab"
> or "Restore This tab" without closing Firefox.
> 
> But accessing "about:crashparent" crashes Firefox and close Firefox.
> 
> I'm not sure if this is the desired behavior here. Could you please clarify
> this to me?

Accessing these URIs via the location bar should do as you described. This patch prevents web content from loading them (as resources, etc).

Comment 33

[Hani Yacoub, Desktop QA]

Verified as fixed on Firefox Nightly 65.0a1 (2018-12-10) and Firefox 64.0 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.13 and on Ubuntu 16.04 x64.

Comment 34

[Hanno Boeck]

This wasn't mentioned in the security advisory for Firefox 64:
https://www.mozilla.org/en-US/security/advisories/mfsa2018-29/

Comment 35

[(not currently active) Ted Mielczarek]

I don't know what the qualifications are to wind up in that advisory, but this was only a denial of service, there's no security impact.

Comment 36

[Hanno Boeck]

Can we make this bug public?

Comment 37

[Daniel Veditz [:dveditz]]

(In reply to Hanno Boeck from comment #34)

This wasn't mentioned in the security advisory for Firefox 64:

That's a better question addressed to security@ mail, not buried in this bug which the people writing advisories obviously somehow missed (which is probably why it didn't have an advisory in the first place). RC2 of a release (comment 29) is typically after we've done the advisories and no one let us know.

New about: pages, especially web-accessible ones, really ought to have had a security review.

Comment 38

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

The advisory is up at https://www.mozilla.org/en-US/security/advisories/mfsa2018-29/#CVE-2018-18510 now.