1511903 - about:crashparent as a favicon still crashes nightly

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[:Gijs (he/him)]

(In reply to Hanno Boeck from bug 1507702 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.

This seems to be because the about: URI gets passed verbatim to the parent, then we stick it in a XUL image and try to load it (so it gets loaded with system principal, so it passes security checks). This is surprising to me because per the security flags on it, I don't think the data URI should be allowed to load it, and I wouldn't expect it to make it through to the parent at all. Mossop, can you clarify what I'm missing, maybe?

Comment 1

[:Gijs (he/him)]

Looks like we bypass the load in the content process for some schemes:

https://searchfox.org/mozilla-central/source/browser/modules/FaviconLoader.jsm#40-46

which unfortunately also means we bypass the security checks implied by loading them in the content process with the right triggering principal.

This wouldn't have any serious sec impact if it wasn't for about:crashparent - about:logo is the only about: thing that'll load as an image, and is world-loadable. I guess web pages can use a chrome: URI they couldn't otherwise access as a favicon, but it's hard to see how that'd be a serious problem.

Anyway, clearly the correct thing to do is to ensure the relevant security checks happen either way - also for things like CSP to apply to data: URIs for favicons.

Comment 2

[:Gijs (he/him)]

I think I have a fix.

Comment 3

[:Gijs (he/him)]

Attachment: Bug 1511903 - r?johannh

Comment 4

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
bug 1507702 will hopefully be uplifted, but it landed on m-c with a full descriptive comment, and therefore I don't think we should release 64 with the issue described in that comment not being completely fixed.

Comment 5

[:Gijs (he/him)]

So this got backed out - it seems our own about: pages depend on some of this infrastructure.

https://treeherder.mozilla.org/testview.html?repo=autoland&revision=5516f3e7b817f0ec3543d0fa6295c01e4363238b

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #5)
> So this got backed out - it seems our own about: pages depend on some of
> this infrastructure.
> 
> https://treeherder.mozilla.org/testview.
> html?repo=autoland&revision=5516f3e7b817f0ec3543d0fa6295c01e4363238b

Ah right, I'd forgotten that we use ALLOW_CHROME for some loads. As the comments in https://searchfox.org/mozilla-central/rev/3fdc51e66c9487b39220ad58dcee275fca070ccd/caps/nsScriptSecurityManager.cpp#898-907 indicate, this should still only allow such URIs from chrome/resource pages, and in my testing this seems to work. If it didn't, I'd also expect the <img src=about:crashcontent> case to still be broken, as I'm pretty sure we use the same flag there.

Comment 7

[:Gijs (he/him)]

Relanded: https://hg.mozilla.org/integration/autoland/rev/c10ccebf6647c0f398024fc72c55551ec3a6a575

Comment 8

[Julien Cristau [:jcristau]]

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

Please request uplift so we can get this and bug 1507702 in 64 rc2.

Comment 9

[:Gijs (he/him)]

Comment on attachment 9029365 [details]
Bug 1511903 - r?johannh

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1462702

User impact if declined: Arbitrary webpage can crash the parent process / browser

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 1. Copy: data:text/html,<link rel="icon" href="about:crashparent"/>
2. open new tab
3. paste in URL bar
4. hit enter

Expected: don't crash.

List of other uplifts needed: Bug 1507702

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Adds usual restrictions for image loading for about/chrome/resource URLs for favicons, too, so they can't arbitrarily be requested by web content. This shouldn't impact any web pages that aren't trying to be nefarious.

String changes made/needed: None

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 9029365 [details]
Bug 1511903 - r?johannh

approved for 64 rc2

Comment 11

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/319052b37637c5472406ea7ee330d2cd50dca3e0

Comment 12

[Hani Yacoub, Desktop QA]

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