Override page CSP for inline styles injected by extension content scripts
Categories
(Core :: DOM: Security, enhancement, P2)
Tracking
()
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: kmag, Assigned: kmag)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [domsecurity-active])
Attachments
(13 files)
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
ckerschb
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
gkrizsanits
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
bzbarsky
:
review+
|
Details |
This should apply to both HTML `style` attributes and the contents of HTML <style> nodes. The inline styles themselves should be exempt from CSPs that prohibit inline styles, and the content that they load should likewise be exempt from CSPs that would prevent loading that content.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•6 years ago
|
||
mozreview-review |
Comment on attachment 8926147 [details] Bug 1415352: Part 1a - Pass subject principal through to ParseAttribute. https://reviewboard.mozilla.org/r/197388/#review202596 C/C++ static analysis found 0 defects in this patch (only the first 30 are reported here). You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment 15•6 years ago
|
||
mozreview-review |
Comment on attachment 8926148 [details] Bug 1415352: Part 1b - Store the subject principal when parsing style attributes. https://reviewboard.mozilla.org/r/197390/#review202600 C/C++ static analysis found 0 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment 16•6 years ago
|
||
mozreview-review |
Comment on attachment 8926149 [details] Bug 1415352: Part 1c - Store the subject principal when setting Element.style properties. https://reviewboard.mozilla.org/r/197392/#review202604 C/C++ static analysis found 0 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment 17•6 years ago
|
||
mozreview-review |
Comment on attachment 8926150 [details] Bug 1415352: Part 1d - Use correct subject principal in CSS environment when modifying attr declarations. https://reviewboard.mozilla.org/r/197394/#review202610 C/C++ static analysis found 1 defect in this patch (only the first 30 are reported here). You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` ::: layout/style/StyleRule.cpp:1069 (Diff revision 1) > explicit DOMCSSDeclarationImpl(css::StyleRule *aRule); > > NS_IMETHOD GetParentRule(nsIDOMCSSRule **aParent) override; > virtual DeclarationBlock* GetCSSDeclaration(Operation aOperation) override; > virtual nsresult SetCSSDeclaration(DeclarationBlock* aDecl) override; > - virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv) override; > + virtual void GetCSSParsingEnvironment(CSSParsingEnvironment& aCSSParseEnv, Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]
Comment 18•6 years ago
|
||
mozreview-review |
Comment on attachment 8926151 [details] Bug 1415352: Part 2 - Exempt inline CSS from extension principals from CSP. https://reviewboard.mozilla.org/r/197396/#review202620 C/C++ static analysis found 0 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment 19•6 years ago
|
||
mozreview-review |
Comment on attachment 8926152 [details] Bug 1415352: Part 3a - Add preference to increase max length of CSP report source sample. https://reviewboard.mozilla.org/r/197398/#review202624 C/C++ static analysis found 0 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`
Comment 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8926152 [details] Bug 1415352: Part 3a - Add preference to increase max length of CSP report source sample. https://reviewboard.mozilla.org/r/197398/#review203084 yeah, I can live with that. ::: dom/security/nsCSPContext.h:113 (Diff revision 1) > > + static int32_t sScriptSampleMaxLength; > + > + static uint32_t ScriptSampleMaxLength() > + { > + return std::max(sScriptSampleMaxLength, 0); why not initialize the static from above to 0 and then just return sScriptSampleMaxLength?
Assignee | ||
Comment 21•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926152 [details] Bug 1415352: Part 3a - Add preference to increase max length of CSP report source sample. https://reviewboard.mozilla.org/r/197398/#review203084 > why not initialize the static from above to 0 and then just return sScriptSampleMaxLength? Static data is initialized to 0 by default. The problem is that we have no concept of unsigned int preferences, so it's possible for the preference value to be negative. In practice, that would just overflow to an insanely large positive number, and cause us not to ever truncate. Which probably wouldn't be a real problem. But it makes me uncomfortable.
Comment 22•6 years ago
|
||
mozreview-review |
Comment on attachment 8926158 [details] Bug 1415352: Part 5b - Always use the last component principal as principal to inherit. https://reviewboard.mozilla.org/r/197410/#review203228 ::: caps/ExpandedPrincipal.cpp:192 (Diff revision 1) > - } > - } > - } > + // However, since we only get to the point of using an inherited principal if > + // the load would otherwise succeed, and when the requested URI would inherit > + // the principal, that logic winds up always selecting the first principal in > + // our whitelist, since any principal would, by definition, be able to load > + // the URI. > return mPrincipals.LastElement(); I'm not quite sure about the "always selecting the first principal" part in all the possible (non webextension) cases. Anyway, I like this logic, but you should either assert CheckMayLoad for the last principal (to be future proof), or just remove both arguments of the function completly (since the args would only be used for the assert I would probably do this).
Assignee | ||
Comment 23•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926158 [details] Bug 1415352: Part 5b - Always use the last component principal as principal to inherit. https://reviewboard.mozilla.org/r/197410/#review203228 > I'm not quite sure about the "always selecting the first principal" part in all the possible (non webextension) cases. Anyway, I like this logic, but you should either assert CheckMayLoad for the last principal (to be future proof), or just remove both arguments of the function completly (since the args would only be used for the assert I would probably do this). I think I'll add the assertion. I thought about removing the args completely, but wanted to avoid the churn in case we need to change the principal selection logic in the future and need to re-add them.
Assignee | ||
Comment 24•6 years ago
|
||
mozreview-review |
Comment on attachment 8926157 [details] Bug 1415352: Part 5a - Allow extension codebase principals to override CSP. https://reviewboard.mozilla.org/r/197408/#review203468 ::: caps/BasePrincipal.h:156 (Diff revision 1) > + return FastSubsumes(aDocumentPrincipal); > + } > + // Extension principals always override CSP. This is primarily for the sake > + // of their stylesheets, which are usually loaded from channels and cannot > + // have expanded principals. > + return AddonPolicy(); Note: I'm thinking about changing this to `AddonPolicy() && !BasePrincipal::Cast(aDocumentPrincipal)->AddonPolicy()` I initially left the second part out on the basis that a document with an add-on principal will generally only have content added by the same add-on principal, with the same CSP. But any of its own external stylesheets that it loads will have a principal with the same origin, but without the document CSP, and we probably don't want them to be exempt from the add-on's CSP.
Comment 25•6 years ago
|
||
mozreview-review |
Comment on attachment 8926147 [details] Bug 1415352: Part 1a - Pass subject principal through to ParseAttribute. https://reviewboard.mozilla.org/r/197388/#review206660 I'm really sorry for the lag here... r=me with the comment nit. ::: dom/base/Element.h:1546 (Diff revision 1) > * for attributes in the null namespace (kNameSpaceID_None). > * > * @param aNamespaceID the namespace of the attribute to convert > * @param aAttribute the attribute to convert > * @param aValue the string value to convert > + * @param aMaybeScriptedPrincipal the principal of the script setting the This should probably say something about what null and non-null might mean, like BeforeSetAttr/AfterSetAttr do.
Comment 26•6 years ago
|
||
mozreview-review |
Comment on attachment 8926148 [details] Bug 1415352: Part 1b - Store the subject principal when parsing style attributes. https://reviewboard.mozilla.org/r/197390/#review206662 ::: dom/base/nsAttrValue.h:438 (Diff revision 1) > * > * @param aString the style attribute value to be parsed. > * @param aElement the element the attribute is set on. > */ > bool ParseStyleAttribute(const nsAString& aString, > + nsIPrincipal* aMaybeScriptedPrincipal, Please document. ::: dom/base/nsStyledElement.h:67 (Diff revision 1) > * > * @param aValue the value to parse > * @param aResult the resulting HTMLValue [OUT] > */ > void ParseStyleAttribute(const nsAString& aValue, > + nsIPrincipal* aMaybeScriptedPrincipal, Document, please.
Comment 27•6 years ago
|
||
mozreview-review |
Comment on attachment 8926149 [details] Bug 1415352: Part 1c - Store the subject principal when setting Element.style properties. https://reviewboard.mozilla.org/r/197392/#review206668 r=me, I think, but please double-check the CSS var thing, and I don't think this makes the cssText setter actually work as desired so far. ::: commit-message-333ae:4 (Diff revision 1) > +Bug 1415352: Part 1c - Store the subject principal when setting Element.style properties. r?bz > + > +This causes the subject principal that was responsible for setting a CSS > +property, or the full cssText of an attribute, to be used as the triggering I don't see where we use the principal in SetCSSText. ::: dom/interfaces/css/nsIDOMCSSStyleDeclaration.idl:21 (Diff revision 1) > +interface nsIPrincipal; > + > [uuid(a6cf90be-15b3-11d2-932e-00805f8add32)] > interface nsIDOMCSSStyleDeclaration : nsISupports > { > - attribute DOMString cssText; > + %{C++ I assume this is a C++ block so you can do the "= nullptr" thing, because xpidl can't express that? In that case, please document clearly that this is why we're doing it, and that it's OK because this is not a scriptable interface. I did look into just killing this interface, and it's not trivial, unfortunately. :( ::: layout/style/nsDOMCSSDeclaration.cpp:242 (Diff revision 1) > // XXX silent failure? > return NS_OK; > } > > if (propID == eCSSPropertyExtra_variable) { > - return ParseCustomPropertyValue(aPropertyName, aValue, important); > + return ParseCustomPropertyValue(aPropertyName, aValue, important, This is actually possibly worrisome. What happens when vars are set by extensions and used by content or vice versa?
Comment 28•6 years ago
|
||
mozreview-review |
Comment on attachment 8926150 [details] Bug 1415352: Part 1d - Use correct subject principal in CSS environment when modifying attr declarations. https://reviewboard.mozilla.org/r/197394/#review206676 r=me if the performance is ok. ::: dom/base/FragmentOrElement.cpp:464 (Diff revision 1) > // This also ignores the case that SVG inside XBL binding. > // But it is probably fine. > return OwnerDoc()->GetDocBaseURI(); > } > > -URLExtraData* > +already_AddRefed<URLExtraData> This is slightly annoying. URLExtraData has threadsafe refcounting, which might actually show up in profiles here. Have you done any performance measurements with these patches?
Comment 29•6 years ago
|
||
mozreview-review |
Comment on attachment 8926151 [details] Bug 1415352: Part 2 - Exempt inline CSS from extension principals from CSP. https://reviewboard.mozilla.org/r/197396/#review206684
Comment 30•6 years ago
|
||
mozreview-review |
Comment on attachment 8926153 [details] Bug 1415352: Part 3b - Add tests for triggering principal and CSP subjection of style attributes. https://reviewboard.mozilla.org/r/197400/#review206688 ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:286 (Diff revision 1) > + > + let {href} = new URL(`css-${i++}.png?origin=${encodeURIComponent(origin)}&source=${encodeURIComponent(source)}`, > + location.href); > + > + urls.push(Object.assign({}, opts, {href, origin, source})); > + return `url(${href})`; `url("${href}")` because nothing prevents there being a ')' in the name, say. encodeURIComponent encodes '"', so this will be safe. ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:581 (Diff revision 1) > * test object. > * @param {Object<string, object>} [forbiddenSources = {}] > * A set of sources for which requests should never be sent. Any > * matching requests from these sources will cause the test to > * fail. > - * @returns {object} > + * @returns {RequestedURLs} ElementTestCase, not RequestedURLs, right?
Comment 31•6 years ago
|
||
mozreview-review |
Comment on attachment 8926154 [details] Bug 1415352: Part 4a - Capture subject principal in innerHTML setters. https://reviewboard.mozilla.org/r/197402/#review206694
Comment 32•6 years ago
|
||
mozreview-review |
Comment on attachment 8926155 [details] Bug 1415352: Part 4b - Capture the subject principal in textContent setters. https://reviewboard.mozilla.org/r/197404/#review206698
Comment 33•6 years ago
|
||
mozreview-review |
Comment on attachment 8926156 [details] Bug 1415352: Part 4c - Use subject principal as the triggering principal for inline <style> nodes. https://reviewboard.mozilla.org/r/197406/#review206702 r=me with the one nit below. ::: layout/style/Loader.cpp:1915 (Diff revision 1) > + principal = BasePrincipal::Cast(aTriggeringPrincipal)->PrincipalToInherit(); > + } > + > SheetLoadData* data = new SheetLoadData(this, aTitle, nullptr, sheet, > owningElement, *aIsAlternate, > - aObserver, nullptr, static_cast<nsINode*>(aElement)); > + aObserver, principal, Why this change? Inline sheet loads shouldn't ever use mLoadingPrincipal for anything. I'd rather keep the old behavior here.
Comment 34•6 years ago
|
||
mozreview-review |
Comment on attachment 8926157 [details] Bug 1415352: Part 5a - Allow extension codebase principals to override CSP. https://reviewboard.mozilla.org/r/197408/#review206704 ::: caps/BasePrincipal.h:156 (Diff revision 1) > + return FastSubsumes(aDocumentPrincipal); > + } > + // Extension principals always override CSP. This is primarily for the sake > + // of their stylesheets, which are usually loaded from channels and cannot > + // have expanded principals. > + return AddonPolicy(); I'd rather do the more restrictive thing here for now and then think about how we can loosen it up.
Comment 35•6 years ago
|
||
mozreview-review |
Comment on attachment 8926158 [details] Bug 1415352: Part 5b - Always use the last component principal as principal to inherit. https://reviewboard.mozilla.org/r/197410/#review206710 ::: caps/ExpandedPrincipal.cpp:193 (Diff revision 1) > - for (const auto& principal : mPrincipals) { > - if (NS_SUCCEEDED(principal->CheckMayLoad(aRequestedURI, false, > - aAllowIfInheritsPrincipal))) { > - return principal; > - } > - } > + // principal. The previous logic here was inherited from even older logic for > + // selecting the triggering principal for a load, and used the first > + // sub-principal which was able to load the requested URI. > + // > + // However, since we only get to the point of using an inherited principal if > + // the load would otherwise succeed, and when the requested URI would inherit This isn't true, actually. We could land in this code and use its result with a non-inheriting URI via InheritAndSetCSPOnPrincipalIfNeeded or the force-inherit flags on loadinfo. I _think_ all those cases are OK with the new behavior, but please double-check that and adjust the comment.
Comment 36•6 years ago
|
||
mozreview-review |
Comment on attachment 8926159 [details] Bug 1415352: Part 6 - Test triggering principals and CSP subjection for inline <style> nodes. https://reviewboard.mozilla.org/r/197412/#review206712 ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:405 (Diff revision 1) > + // eslint-disable-next-line no-unsanitized/method > + style.insertAdjacentHTML("beforeend", css); > + }); > + > + // And again using insertAdjacentText. > + // to it using insertAdjacentHTML, with the same rules as above. Nix this line of comment? ::: toolkit/components/extensions/test/xpcshell/test_ext_contentscript_triggeringPrincipal.js:410 (Diff revision 1) > + // to it using insertAdjacentHTML, with the same rules as above. > + testModifyAfterInject("insertAdjacentText", (style, css) => { > + style.insertAdjacentText("beforeend", css); > + }); > + > + // Test creating a script and then accessing its CSSStyleSheet object. s/script/style element/?
Updated•6 years ago
|
Assignee | ||
Comment 37•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926149 [details] Bug 1415352: Part 1c - Store the subject principal when setting Element.style properties. https://reviewboard.mozilla.org/r/197392/#review206668 > I don't see where we use the principal in SetCSSText. Sorry, that's in the next part. I'll fix the comment. > I assume this is a C++ block so you can do the "= nullptr" thing, because xpidl can't express that? > > In that case, please document clearly that this is why we're doing it, and that it's OK because this is not a scriptable interface. > > I did look into just killing this interface, and it's not trivial, unfortunately. :( Yeah, I wanted a default value for the principal arguments, and since this interface is non-scriptable, there's really no need for it to be in xpidl. It would probably be better to just convert the whole thing to a .h at this point, but I wanted to avoid adding the churn. > This is actually possibly worrisome. What happens when vars are set by extensions and used by content or vice versa? We wind up storing the triggering principal of whichever sheet set the variable, and using that principal for the load. I'm a bit wary of that myself, but we already have this problem for variables set in a sheet with one origin, and used in another. In fact, I've seen it come up several times in Firefox front-end code, when someone tries to set a variable in a skin stylesheet, and then finds that it can't load certain resources.
Assignee | ||
Comment 38•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926150 [details] Bug 1415352: Part 1d - Use correct subject principal in CSS environment when modifying attr declarations. https://reviewboard.mozilla.org/r/197394/#review206676 > This is slightly annoying. URLExtraData has threadsafe refcounting, which might actually show up in profiles here. > > Have you done any performance measurements with these patches? I'll do some tests. I'd be surprised if the refcounting overhead is significant compared to the overhead of setting style properties, but I guess it's worth checking. The only other option I can think of, though, is to cache the URLData object for each triggering principal in a per-document hashmap.
Assignee | ||
Comment 39•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926156 [details] Bug 1415352: Part 4c - Use subject principal as the triggering principal for inline <style> nodes. https://reviewboard.mozilla.org/r/197406/#review206702 > Why this change? Inline sheet loads shouldn't ever use mLoadingPrincipal for anything. I'd rather keep the old behavior here. I don't remember exactly why I made this change, but looking again, I think you're right that it shouldn't be necessary.
Assignee | ||
Comment 40•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926153 [details] Bug 1415352: Part 3b - Add tests for triggering principal and CSP subjection of style attributes. https://reviewboard.mozilla.org/r/197400/#review206688 > ElementTestCase, not RequestedURLs, right? Hm. Nope, the name is right here, but wrong in the @typedef.
Assignee | ||
Comment 41•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926150 [details] Bug 1415352: Part 1d - Use correct subject principal in CSS environment when modifying attr declarations. https://reviewboard.mozilla.org/r/197394/#review206676 > I'll do some tests. I'd be surprised if the refcounting overhead is significant compared to the overhead of setting style properties, but I guess it's worth checking. > > The only other option I can think of, though, is to cache the URLData object for each triggering principal in a per-document hashmap. Hm. So just with a quick micro-benchmark: function run(doc, inject) { let div = document.createElement("div"); if (inject) { doc.body.appendChild(div); } let iterations = 1024 * 128; let start = performance.now(); for (let i = 0; i < iterations; i++) { div.style.backgroundColor = "black"; } let end = performance.now(); let delta = (end - start) * 1000 * 1000; doc.body.appendChild(document.createTextNode(`Per-iteration (${inject}): ${Math.round(delta / iterations)}ns`)); doc.body.appendChild(document.createElement("br")); } there doesn't seem to be any difference between the addref case and the raw pointer case. But the numbers are pretty noisy, and the performance here appears to be pretty bad, at about 6,500ns per iteration.
Assignee | ||
Comment 42•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8926158 [details] Bug 1415352: Part 5b - Always use the last component principal as principal to inherit. https://reviewboard.mozilla.org/r/197410/#review206710 > This isn't true, actually. We could land in this code and use its result with a non-inheriting URI via InheritAndSetCSPOnPrincipalIfNeeded or the force-inherit flags on loadinfo. > > I _think_ all those cases are OK with the new behavior, but please double-check that and adjust the comment. OK. I went for a compromise, and went back to using the first principal that could load the URI without inheritance, and updated the comment.
Assignee | ||
Comment 43•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f63760c1dbb3d413a850c8b12ad29646399e7d7 Bug 1415352: Part 1a - Pass subject principal through to ParseAttribute. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/9cbae3a211596c34de3541f3e757d488a735d221 Bug 1415352: Part 1b - Store the subject principal when parsing style attributes. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/e38a3fe5b832d8305c69cbd7944b732ba2dc9660 Bug 1415352: Part 1c - Store the subject principal when setting Element.style properties. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/c16650358929b0c55fa865301314a1aa28787a68 Bug 1415352: Part 1d - Use correct subject principal in CSS environment when modifying attr declarations. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/26dd740f73e162c47afbdb29c7369d6474103e2a Bug 1415352: Part 2 - Exempt inline CSS from extension principals from CSP. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/df07d62d21d5cb74965ad698fc73a4681828879d Bug 1415352: Part 3a - Add preference to increase max length of CSP report source sample. r=ckerschb https://hg.mozilla.org/integration/mozilla-inbound/rev/e1fb3ede55c43aaab61eb58b0f5e0725db916a24 Bug 1415352: Part 3b - Add tests for triggering principal and CSP subjection of style attributes. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/c77bc1a5218e984623f384dc0c9f72b644c4f093 Bug 1415352: Part 4a - Capture subject principal in innerHTML setters. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/67c8196e367520a37172480a9eacdd9a965b920d Bug 1415352: Part 4b - Capture the subject principal in textContent setters. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/77a3e035aca6aa11293db4514bc0c930d3c2f054 Bug 1415352: Part 4c - Use subject principal as the triggering principal for inline <style> nodes. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/0f044857350891d27712d3be932bf1ae10f4e5ce Bug 1415352: Part 5a - Allow extension codebase principals to override CSP. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/96ffd32355aecf7d2999ff767c8ca09abb6f3ea0 Bug 1415352: Part 5b - Use the last component principal as principal to inherit for data: URLs. r=bz,krizsa https://hg.mozilla.org/integration/mozilla-inbound/rev/d811ce4ebcd9a7bfce3e6ac0ec5d006f9e2ff82c Bug 1415352: Part 6 - Test triggering principals and CSP subjection for inline <style> nodes. r=bz
Comment 44•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4f63760c1dbb https://hg.mozilla.org/mozilla-central/rev/9cbae3a21159 https://hg.mozilla.org/mozilla-central/rev/e38a3fe5b832 https://hg.mozilla.org/mozilla-central/rev/c16650358929 https://hg.mozilla.org/mozilla-central/rev/26dd740f73e1 https://hg.mozilla.org/mozilla-central/rev/df07d62d21d5 https://hg.mozilla.org/mozilla-central/rev/e1fb3ede55c4 https://hg.mozilla.org/mozilla-central/rev/c77bc1a5218e https://hg.mozilla.org/mozilla-central/rev/67c8196e3675 https://hg.mozilla.org/mozilla-central/rev/77a3e035aca6 https://hg.mozilla.org/mozilla-central/rev/0f0448573508 https://hg.mozilla.org/mozilla-central/rev/96ffd32355ae https://hg.mozilla.org/mozilla-central/rev/d811ce4ebcd9
Comment 45•6 years ago
|
||
Is manual testing required on this? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Comment 46•6 years ago
|
||
Please provide an extension or steps in order to test the scenarios that are covered by this fix and possibly other CSP related maters.
Comment 47•6 years ago
|
||
(In reply to marius.santa from comment #46) > Please provide an extension or steps in order to test the scenarios that are > covered by this fix and possibly other CSP related maters. As per Kris, this does not require QA verification.
Updated•6 years ago
|
Updated•2 years ago
|
Comment 48•1 year ago
•
|
||
I think style nodes have regressed despite the tests.
I'm permitted in a content script to inject <link href=data:
and style="..."
but not <style>....</style>
into the page.
For example I would expect the following to work:
const styleTag = document.createElement('style')
styleTag.innerText = '* {background: red;}'
document.head.appendChild(styleTag)
This is with a CSP of:
<meta http-equiv="Content-Security-Policy" content="script-src 'self' https: http:; object-src 'none'; base-uri 'none'; style-src 'self' ; font-src 'self'">
Use of wrappedJSObject to access the DOM makes no difference here.
Even more fun is with the link data URL I'm still seeing CSP warnings but it's certainly loading.
:ckerschb do you know of anything that might cause this? Perhaps subjectPrincipal in setInnerHTML isn't the codebase principal?
Comment 49•1 year ago
|
||
(In reply to Jonathan Kingston [:jkt] he/him from comment #48)
@jkt - thanks for reporting. Hope things are going well for you.
:ckerschb do you know of anything that might cause this? Perhaps subjectPrincipal in setInnerHTML isn't the codebase principal?
Tom is in a better position to answer here because he did a bunch of CSP updates lately. Tom, what's your take?
Comment 50•1 year ago
|
||
I went back all the way to Firefox 90 (older builds tend to just crash) and the inline style was already blocked. Either a pretty old regression or maybe this specific case never worked. Could you open a new bug for this?
Comment 51•1 year ago
•
|
||
I've raised https://bugzilla.mozilla.org/show_bug.cgi?id=1822067
@jkt - thanks for reporting. Hope things are going well for you.
Yeah great thank you, I hope you're good also 😄
Thanks both for looking into this 😄
Updated•1 year ago
|
Description
•