Extension gets CSP error appending an inline <style> node with uninitialized textContent (CSP: style-src 'self')
Categories
(WebExtensions :: General, defect, P5)
Tracking
(Not tracked)
People
(Reporter: saschanaz, Unassigned)
References
()
Details
Attachments
(2 files)
- Install the latest Node.js
- Unzip the attachment
- Run
node run.json the directory - Go
about:debugging#/runtime/this-firefox - Click "Load Temporary Add-on" and select
extensions/manifest.json. - Open
0.0.0.0:3000 - Open devtools and see there is a CSP error
Expected: No issue
Actual: Content Security Policy: The page’s settings blocked the loading of a resource at inline (“style-src”).
|
||
Comment 1•3 years ago
•
|
||
Why is that a bug? The CSP check is run before we "load" things so we don't know it's an empty <style> block at that point. But the worst that happens is that CSP doesn't process the block that does nothing, so either way nothing happens.
I suspect this bug is invalid according to the spec (which is writen in terms of "fetch"), but either way we're not going to fix it given the rewrite it would take for essentially no benefit (except less reporting I guess).
|
Reporter | |
Comment 2•3 years ago
|
||
By "empty" I mean an element created by document.createElement("style") without any src, so I think it should be able to check the emptyness? I mean, it shouldn't fetch anything, right?
|
|
Reporter | |
Comment 3•3 years ago
|
||
Just for easier comment, this is what the extension script do. (extension/index.js from the zip file)
const elementWithTextContent = document.createElement("style");
elementWithTextContent.textContent = 'p { color: red }';
document.head.appendChild(elementWithTextContent) // No CSP errors.
const normalElement = document.createElement("style");
document.head.appendChild(normalElement) // Observer CSP errors.
| Comment hidden (obsolete) |
|
|
||
Comment 5•3 years ago
•
|
||
And now that I've looked at the GH issue I see that you've already run this code from Gusted so I guess it's safe?
The node script is 7 lines -- that's small enough to see that it's safe. when the GH issue talked about "hardened" sites I imagined lots of impenetrable library code. I'm going to delete my previous comment because I missed the point entirely. both injections are into the <head>. Both should be allowed because it's an extension doing the injecting. The difference is that one IS empty in the sense I thought you meant originally. It shouldn't matter, but it does. (Is it empty, or is it an initialization problem? if we explicitly set textContent = "";, which should be the default, does the error go away?)
Why would an empty or uninitialized textContent even matter? CSP shouldn't care what is in the node; decisions are made based on where it is (in this case "inline") and who created it (an extension, not limited by the page's CSP), not what's in it.
|
|
Reporter | |
Comment 6•3 years ago
|
||
Can you package this into a minimally-permissioned addon I can run in a test profile without all that?
Comment #0 lists the way to load the package with the attached zip file. Following the steps loads the script as a temporary addon.
Is the addon itself doing the style injection?
Is it the same script doing the injecting into head and body?
Yes, the addon runs a content script where all the injection happens.
|
|
Reporter | |
Comment 7•3 years ago
•
|
||
And now that I've looked at the GH issue I see that you've already run this code from Gusted so I guess it's safe?
Yeah, I read it and it does nothing harmful 😀 It's just a 22-line file anyway. (And the Node.js script is just a minimal code to open a local server which sends CSP header.)
|
||
Comment 8•3 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #5)
Gusted
Hmm that's a name I know, It's me!
Just going to chip in to steer this bug into hopefully the right direction and give some insight what is going on.
So in plain text this bug is:
Extension x want's to inject a normal style without any textContent into page x that has within thr CSP header: style-src 'self'; and get's an CSP error thrown at you, while when you provide a textContent it won't.
So hence the run.js to simulate the style-src: 'self'; as with any normal website it won't happend, it's specific to the style-src: 'self'; behavior so far I can reproduce. If you like,
some sites that I came across with this style-src: 'self'; (so won't have to node on the run.js, which I perfectly understand), archlinux . org and privacytools . io has this header. So the extension can be modified to either of those sites for permission and it will still reproduce this error. I've attached the extension modified to those 2 sites.
So yes, the addon is doing the injection and should like you mention bypass this CSP behavior as otherwise we couldn't deliver our extensions' purpose to those sites. But in this case when textContent is empty, and please note that when you set textContent to '' it WONT fail. < This test-case is also now included in the new attached extension.
|
|
||
Comment 9•3 years ago
|
||
Look at that, it IS an initialization issue? Looking https://github.com/darkreader/darkreader/pull/2359 their fix was literally just that:
function fixFirefoxCSPIssue() {
// Some websites get CSP warning,
// when `textContent` is not set
if (corsCopy && !corsCopy.textContent) {
corsCopy.textContent = '';
}
...
Wild!
|
|
Reporter | |
Comment 10•3 years ago
|
||
My wild guess is that mTriggeringPrincipal allows us to bypass CSP within addon scripts, and setting .textContent gives it a proper value.
In the case with no .textContent I guess it's not getting a valid mTriggeringPrincipal so no bypass?
|
|
||
Comment 11•3 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #9)
Look at that, it IS an initialization issue? Looking https://github.com/darkreader/darkreader/pull/2359 their fix was literally just that:
[...]
Wild!
Seems like it, but just little off-topic, that fix wasn't bullet-proof, due to the weird issues of having sheet not set on elements, see commit:
https://github.com/darkreader/darkreader/commit/4ed02c27b770e43b884b68cfe8945f5034660567
|
|
||
Updated•3 years ago
|
|
|
||
Comment 12•3 years ago
|
||
(In reply to Kagami :saschanaz from comment #10)
In the case with no
.textContentI guess it's not getting a validmTriggeringPrincipalso no bypass?
Looks like mTriggeringPrincipal is explicitly set to null to trigger use of the document principal, from bug 1415352
https://searchfox.org/mozilla-central/rev/4648b6ee31c2519b1753023e4f4853b14fdd16e5/dom/html/HTMLStyleElement.cpp#76
But if the TriggeringPrincipal is null we use the document's CSP
https://searchfox.org/mozilla-central/source/layout/style/nsStyleUtil.cpp#289-297
... which is wrong when an extension has created the node.
|
||
Comment 13•3 years ago
|
||
The extension created the node, but what we care about is who set the node's contents, since we wouldn't want to apply the extension's CSP to style content that a page script changed after the node was created.
In this case, though, nothing set the content. I suppose it might arguably make sense to treat the node creator as having set the contents to an empty string in that case, but it hasn't actually come up in practice.
|
|
||
Comment 14•3 years ago
|
||
CSP is correct here. Not sure if the problem is the Style element code or if this is an exception that WebExtensions want to handle. Realistically this is a tiny corner case (why create a <style> element if it's got literally nothing in it?) with an easy workaround so I'm not sure the complexity of a solution is worth the risk of opening up security holes.
|
|
||
Comment 15•3 years ago
|
||
Well, people write code with different taste and different orders etc. It just so happens that we(Dark reader extension) happened to first create such element and insert it into the dom and then do the heavy lifting of what needs to be within the textContent and the workaround(having textContent before inserting the element) was pure on luck that we stumbled on that solution.
|
||
Comment 16•3 years ago
|
||
At this point it's just an issue of logspam. There aren't significant other consequences, and extensions have a way to work around the issue.
This bug could potentially be fixed by special-casing behavior of the case where the style element is uninitialized, but I don't see ourselves spending time on doing that. Patches are welcome though.
|
||
Comment 17•6 months ago
|
||
This bug makes Firefox behave like Chrome which intentionally exempts the DOM created by an ISOLATED content script from the CSP of the page. Cross-browser developers of WebExtensions wish that Firefox would behave the same for all the other types of elements in this specific scenario, which doesn't seem likely though judging by bug 1267027.
|
|
||
Comment 18•6 months ago
|
||
To clarify, this is not a log spam issue: the actual bug here is that CSP is ignored for a style with textContent, and I like that!
Description
•