Closed Bug 1412345 Opened 2 years ago Closed 2 years ago

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

Categories

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

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: marcia, Assigned: kmag)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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
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.
Flags: needinfo?(amarchesini)
Summary: Crash in ExpandedPrincipal::GetHashValue → Crash in ExpandedPrincipal::GetHashValue, with MOZ_CRASH(extended principal should never be used as key in a hash map)
I'm not too familiar with ExpandedPrincipal. NI bz.
Flags: needinfo?(amarchesini) → needinfo?(bzbarsky)
Looks like we ended up with a font-face rule in a sheet, and the sheet has an expanded principal?
Flags: needinfo?(bzbarsky) → needinfo?(kmaglione+bmo)
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_.
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.
Flags: needinfo?(kmaglione+bmo)
Assignee: nobody → kmaglione+bmo
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.
Attachment #8924821 - Flags: review?(bzbarsky) → review+
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
Attachment #8924821 - Flags: review?(gkrizsanits) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/4350a326a498
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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.
The crash volume doesn't seem to justify dot release inclusion.  Too bad this wasn't caught in time for 57.
This shouldn't be an issue in 57. There's no simple way to create stylesheets with expanded principals there.
(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.
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?
Flags: needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
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?
(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.
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.
See Also: → 1417790
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.