1415352 - Override page CSP for inline styles injected by extension content scripts

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Description

[Kris Maglione [:kmag]]

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 1

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 1a - Pass subject principal through to ParseAttribute.

This is necessary in order to parse style attributes using the subject
principal of the caller, rather than defaulting to the page principal.

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

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 1b - Store the subject principal when parsing style attributes.

This change stores the subject principal in the URLExtraData when parsing
style attributes, which causes it to be used as the triggering principal for
those loads rather than defaulting to the document principal.

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

Comment 3

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 1c - Store the subject principal when setting Element.style properties.

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
principal when loading URLs for that property.

Note that this allows for different properties defined in the same style
attribute to have different triggering principals, depending on the caller
which originally set them, as long as the cssText of that attribute is not
modified. Once it is, all properties revert to the principal of the caller
that modified the CSS text.

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

Comment 4

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 1d - Use correct subject principal in CSS environment when modifying attr declarations.

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

Comment 5

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 2 - Exempt inline CSS from extension principals from CSP.

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

Comment 6

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 3a - Add preference to increase max length of CSP report source sample.

This is necessary for tests which need to verify that reports are being sent
for the correct inline sources, where the current sample size is not enough to
completely distinguish them.

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

Comment 7

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 3b - Add tests for triggering principal and CSP subjection of style attributes.

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

Comment 8

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 4a - Capture subject principal in innerHTML setters.

This is necessary in order to capture the correct triggering principal for
inline <style> nodes.

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

Comment 9

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 4b - Capture the subject principal in textContent setters.

This is necessary in order to capture the correct triggering principal for
inline <style> nodes.

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

Comment 10

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 4c - Use subject principal as the triggering principal for inline <style> nodes.

This change captures the subject principal when a scripted caller sets the
textContent or innerHTML property of a <style> node, and uses it as the
triggering principal for the resulting stylesheet.

If the node contents are modified in any way other than through textContent or
innerHTML, the triggering principal is forgotten (which is an intentional
design feature).

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

Comment 11

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 5a - Allow extension codebase principals to override CSP.

We currently use plain extension codebase principals for most of the extension
stylesheets that we inject into content pages. Since we want the content
loaded by those stylesheets to be exempt from CSP, and can't safely use
expanded principals for their loads, we need to make plain extension codebase
principals exempt from CSP.

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

Comment 12

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 5b - Always use the last component principal as principal to inherit.

The logic for choosing the principal here was originally written before
loadInfo had a separate principalToInherit field, and we needed to specify it
via the triggeringPrincipal instead. At that point, we had to choose a
component principal with permission to load the URI at the start of the
request.

However, now that we have a separate field for the principal to inherit, it's
only needed after access checks have passed and we know that we have a URI
which inherits a principal. In that case, the current logic causes us to
always inherit the first principal in the whitelist (which is the page
principal), where we really want to inherit the last (which is the extension
principal).

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

Comment 13

[Kris Maglione [:kmag]]

Attachment: Bug 1415352: Part 6 - Test triggering principals and CSP subjection for inline <style> nodes.

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

Comment 14

[Code Review Bot [:reviewbot]]

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

[Code Review Bot [:reviewbot]]

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

[Code Review Bot [:reviewbot]]

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

[Code Review Bot [:reviewbot]]

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

[Code Review Bot [:reviewbot]]

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

[Code Review Bot [:reviewbot]]

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

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 21

[Kris Maglione [:kmag]]

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

[Gabor Krizsanits (INACTIVE)]

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).

Comment 23

[Kris Maglione [:kmag]]

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.

Comment 24

[Kris Maglione [:kmag]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8926154 [details]
Bug 1415352: Part 4a - Capture subject principal in innerHTML setters.

https://reviewboard.mozilla.org/r/197402/#review206694

Comment 32

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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

[Boris Zbarsky [:bzbarsky]]

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/?

Comment 37

[Kris Maglione [:kmag]]

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.

Comment 38

[Kris Maglione [:kmag]]

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.

Comment 39

[Kris Maglione [:kmag]]

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.

Comment 40

[Kris Maglione [:kmag]]

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.

Comment 41

[Kris Maglione [:kmag]]

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.

Comment 42

[Kris Maglione [:kmag]]

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.

Comment 43

[Kris Maglione [:kmag]]

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

[Cosmin Sabou [:CosminS]]

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

[marius.santa]

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

[marius.santa]

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

[krupa raj [:krupa--use this to needinfo]]

(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.

Comment 48

[Jonathan Kingston [:jkt] he/him]

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

[Christoph Kerschbaumer [:ckerschb]]

(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

[Tom Schuster [PTO]]

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

[Jonathan Kingston [:jkt] he/him]

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 😄