1412345 - Crash in ExpandedPrincipal::GetHashValue, with MOZ_CRASH(extended principal should never be used as key in a hash map)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is 
report bp-a333a2ea-3f81-4dfe-89b1-2c02d0171027.
=============================================================

Seen while looking at nightly crash stats: http://bit.ly/2gOb4mw. Windows and Linux crash which started using 20171019100107. To date 100 crashes/9 installs.

Possible regression range based on Build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a29052590fc6538b89641e690d11731ca8e78120&tochange=b6ba3e919f561bfabbf4bec7f0f53a23c360adee

Comment 1

[Nicholas Nethercote [inactive]]

They all have "MOZ_CRASH(extended principal should never be used as key in a hash map)".

baku, do you have any ideas about this one? It's the #5 Windows topcrash in Nightly 20171027100103.

Comment 2

[Andrea Marchesini [:baku]]

I'm not too familiar with ExpandedPrincipal. NI bz.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Looks like we ended up with a font-face rule in a sheet, and the sheet has an expanded principal?

Comment 4

[Boris Zbarsky [:bzbarsky]]

It doesn't look like we did anything for fonts per se in bug 1406278.  And I don't think we changed anything for <style> so far either...

Presumably we're going to need to make this work for expanded principals if we expect extension-injected <style> with font-face rules to do the CSP-bypass thing.  But I'm still not sure how we're getting in this situation _now_.

Comment 5

[Kris Maglione [:kmag]]

My guess is that we have a <link rel="stylesheet" text="data:..."> showing up in an extension content script somewhere, and it's inheriting an expanded principal.

I think we probably just want to inherit the extension principal rather than the expanded principal, in that case. Fixing inheritance for expanded principals is actually at the top of my priority list for this week, along with capturing triggering principals for inline styles.

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1412345: Downgrade expanded principals before inheriting.

There are several ways that expanded principals can be used as triggering
principals for requests. While that works fine for security checks, it also
sometimes causes them to be inherited, and used as result principals in
contexts where expanded principals aren't allowed.

This patch changes our inheritance behavior so that expanded principals are
downgraded to the most appropriate constituent principal when they would
otherwise be inherited.

The logic for choosing the most appropriate principal is a bit suspect, and
may eventually need to be changed to always select the last whitelist
principal, but I chose it to preserve the current principal downgrade behavior
used by XMLHttpRequest for the time being.

Review commit: https://reviewboard.mozilla.org/r/196054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196054/

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8924821 [details]
Bug 1412345: Downgrade expanded principals before inheriting.

https://reviewboard.mozilla.org/r/196054/#review201260

::: caps/BasePrincipal.h:128
(Diff revision 1)
> +  // Returns the principal to inherit when a caller with this principal the
> +  // given URL.

"loads the given URL" perhaps?

::: caps/BasePrincipal.h:132
(Diff revision 1)
>  
> +  // Returns the principal to inherit when a caller with this principal the
> +  // given URL.
> +  //
> +  // For most principal types, this returns the principal itself. For expanded
> +  // principals, it returns the first sub-principal which subsumes the given URI

Please be consistents for URI vs URL here?

::: caps/BasePrincipal.h:135
(Diff revision 1)
> +  //
> +  // For most principal types, this returns the principal itself. For expanded
> +  // principals, it returns the first sub-principal which subsumes the given URI
> +  // (or, if no URI is given, the last whitelist principal).
> +  nsIPrincipal* PrincipalToInherit(nsIURI* aRequestedURI = nullptr,
> +                                   bool aAllowIfInheritsPrincipal = true);

Document what the second arg does, please.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8924821 [details]
Bug 1412345: Downgrade expanded principals before inheriting.

https://reviewboard.mozilla.org/r/196054/#review201384

::: caps/ExpandedPrincipal.cpp:195
(Diff revision 1)
> +                                               aAllowIfInheritsPrincipal))) {
> +        return principal;
> +      }
> +    }
> +  }
> +  return mPrincipals.LastElement();

When ExpandedPrincipal was introduced I felt the need to specify a 'default' principal from the white list at construction time. After a long discussion I could not reason it back then but now we do have a deafult one (here and in nsDocument the same downgrade logic applies).

This is kind of what I wanted to avoid back then though, that we have one, but it's not obvious from the API or from the class definition / ctor. It's also not helping that for origin attribute we follow different logic: http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/js/xpconnect/src/Sandbox.cpp#1347

This should be a separate issue from this bug, but what do you think about introducing a default principal for nsEPs? Option B would be to be a bit more explicit about the role of the last entry in the white list in the idl file at least:  http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/caps/nsIPrincipal.idl#336

I think for this patch a short comment about this in the idl would be nice.

::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:269
(Diff revision 1)
> +    // Basic smoke test to check that we don't try to create stylesheets with
> +    // with an expanded principal, which would cause a crash when loading font

Nit: with with

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8924821 [details]
Bug 1412345: Downgrade expanded principals before inheriting.

https://reviewboard.mozilla.org/r/196054/#review201384

> When ExpandedPrincipal was introduced I felt the need to specify a 'default' principal from the white list at construction time. After a long discussion I could not reason it back then but now we do have a deafult one (here and in nsDocument the same downgrade logic applies).
> 
> This is kind of what I wanted to avoid back then though, that we have one, but it's not obvious from the API or from the class definition / ctor. It's also not helping that for origin attribute we follow different logic: http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/js/xpconnect/src/Sandbox.cpp#1347
> 
> This should be a separate issue from this bug, but what do you think about introducing a default principal for nsEPs? Option B would be to be a bit more explicit about the role of the last entry in the white list in the idl file at least:  http://searchfox.org/mozilla-central/rev/af86a58b157fbed26b0e86fcd81f1b421e80e60a/caps/nsIPrincipal.idl#336
> 
> I think for this patch a short comment about this in the idl would be nice.

Yeah, I thought about adding an attribute for it, but in the end it seemed like overkill. I'd rather just document that the last principal is always used as the fallback when inheriting.

For origin attributes, the logic isn't really different. All principals need to have the same origin attributes. The originAttributes option should really only be used when constructing principals from strings. If it's passed, we should really ensure that it matches the origin attributes of all principal args... But that's another bug.

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4350a326a49805c6138aabd0ed68136498bf97cd
Bug 1412345: Downgrade expanded principals before inheriting. r=bz,krizsa

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/4350a326a498

Comment 12

[Cervantes Yu [:cyu] [:cervantes]]

This is affecting release build. I experienced this on my workstation https://crash-stats.mozilla.org/report/index/0b903c30-b6e8-46ad-82f3-b9b7f0171110

I can 100% reproduce this crash with browsing https://slack.com/downloads/linux with "Dark Background and Light Text" https://addons.mozilla.org/en-US/firefox/addon/dark-background-light-text/ addon enabled.

We should backport this to release build.

Comment 13

[Julien Cristau [:jcristau]]

The crash volume doesn't seem to justify dot release inclusion.  Too bad this wasn't caught in time for 57.

Comment 14

[Kris Maglione [:kmag]]

This shouldn't be an issue in 57. There's no simple way to create stylesheets with expanded principals there.

Comment 15

[Kris Maglione [:kmag]]

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #12)
> This is affecting release build. I experienced this on my workstation
> https://crash-stats.mozilla.org/report/index/0b903c30-b6e8-46ad-82f3-
> b9b7f0171110

Sorry, I missed this comment. It would be interesting to know what's causing this, but it appears to only be an issue on 56, and all affected users seem to have several legacy extensions installed, so I'm not especially concerned about 57.

Comment 16

[Khvoinitsky Mikhail [:m_khvoinitsky]]

Dark Background and Light Text add-on developer here.

My add-on indeed causes this issue to reproduce. I suppose the problem is in workaround of bug 1393022: I have to substitute page's <link rel="stylesheet" href="cross_origin_url"> with fetch()'ed data:url to workaround SecurityError while accessing stylesheets. The issue raises when I'm resolving relative url()s in @font-face in CSS to absolute ones (I need this, again, because of workaround of bug 1393022: relative url()s in workarounded stylesheets resolve relative to current document rather than original stylesheets so they need to be fixed).

While Desktop Firefox 57 isn't affected, Firefox for Android 57 *is*. Any chance for me to workaround this issue?

Comment 17

[Kris Maglione [:kmag]]

I guess we may wind up with a blob: URL with an expanded principal if it's created via fetch() from a content script in 57. There's not really anything we can do about it now, though.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Can we replace the MOZ_CRASH with something that doesn't crash and backport it?

The blob/data issue doesn't sound like a new issue in 57, right?

Comment 19

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #18)
> Can we replace the MOZ_CRASH with something that doesn't crash and backport
> it?

We could make it a diagnostic assert. I'd be happy with that, but I doubt we'd get approval to chemspill for this.

> The blob/data issue doesn't sound like a new issue in 57, right?

I don't think so. There were actually higher crash volumes in 56 than 57, but I don't know if they had the same cause. Either way, a blob URI with an expanded principal would have caused this in any remotely recent release.

Comment 20

[Khvoinitsky Mikhail [:m_khvoinitsky]]

It seems that I've managed to eliminate the crashes: I used to resolve all relative urls in @font-face including the ones in stylesheets that I haven't changed which is not needed. When I skip such ones, no crashes occurs on affected pages. I haven't stumbled upon a crash so far.