1254194 - Support CSP in WebExtensions

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Kris Maglione [:kmag]]

WebExtension pages should have a fairly strict default CSP. We should probably re-use the same policy we currently use for privileged apps.

Chrome also supports a `content_security_policy` manifest directive, to allow extensions to specify their own policies. We should probably add support for that at the same time.

Comment 1

[Andrew Williamson [:eviljeff]]

The Chrome default CSP is pretty strict (and almost the same as privileged apps), so for compatibility we may want to use that one.
Chrome uses "script-src 'self'; object-src 'self'" and privileged apps use[d] "default-src *; script-src 'self'; object-src 'none'; style-src 'self' 'unsafe-inline'".

Comment 2

[Andy McKay]

It feels like we should do this sooner rather than later so we don't surprise people by changing the CSP in a later release.

Comment 3

[Kris Maglione [:kmag]]

It's easy enough to do. We should definitely be able to get it in by 48.

Comment 4

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254194: [webext] Fix tests that violate the default CSP. r?aswan

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

Comment 5

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254194: Allow iterating over and inspecting sources of parsed CSP directives. r?ckerschb

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

Comment 6

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254194: Add a validator for custom add-on content security policies. r?dveditz r?billm f?aswan

Dan, can you please do a security review, and Bill a general review?

Thanks

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

Comment 7

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254194: [webext] Add 'onError' schema option to make manifest errors non-fatal. r?aswan

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

Comment 8

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254194: Add XPCOMUtils.defineLazyPreferenceGetter. r?billm

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

Comment 9

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254194: [webext] Allow extensions to register custom content security policies. r?billm f?aswan

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

Comment 10

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254194: Apply a content security policy to all WebExtension documents. r?gabor

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

Comment 11

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1254194: [webext] Test that content security policies are applied to WebExtension documents. r?aswan

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

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8741574 [details]
MozReview Request: Bug 1254194: [webext] Fix tests that violate the default CSP. r?aswan

https://reviewboard.mozilla.org/r/46583/#review43387

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8741577 [details]
MozReview Request: Bug 1254194: [webext] Add 'onError' schema option to make manifest errors non-fatal. r?aswan

https://reviewboard.mozilla.org/r/46589/#review43419

I don't have any quibbles on the implementation but I think we'll have to be really careful about where we use this to avoid surprising or confusing developers.
I guess the application of this capability to CSP comes in a subsequent patch.  But my gut reaction is that it would be better to make an invalid policy an immediate error instead of an easy-to-overlook warning but I suspect you've thought this through and have a solid rationale.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #13)
> I don't have any quibbles on the implementation but I think we'll have to be
> really careful about where we use this to avoid surprising or confusing
> developers.
> I guess the application of this capability to CSP comes in a subsequent
> patch.  But my gut reaction is that it would be better to make an invalid
> policy an immediate error instead of an easy-to-overlook warning but I
> suspect you've thought this through and have a solid rationale.

I agree, and I think we should probably make invalid policies a fatal error at
some point in the future. My concern at this point, though, is that we're
going to wind up breaking otherwise valid extensions if they specify a policy
that's valid in Chrome but not Firefox (e.g., one containing chrome-extension:
or chrome-extension-resource: sources). This has already been an issue in
other places (e.g., bug 1263560), so I'd rather introduce this as a soft error
until we figure out the interoperability implications.

Comment 15

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8741580 [details]
MozReview Request: Bug 1254194: Apply a content security policy to all WebExtension documents. r?gabor

https://reviewboard.mozilla.org/r/46595/#review43295

r=me with removing one of the addonID getters or explaining to me why you need both.

::: caps/BasePrincipal.h:234
(Diff revision 1)
>    CreateCodebasePrincipal(nsIURI* aURI, const PrincipalOriginAttributes& aAttrs);
>    static already_AddRefed<BasePrincipal> CreateCodebasePrincipal(const nsACString& aOrigin);
>  
>    const PrincipalOriginAttributes& OriginAttributesRef() { return mOriginAttributes; }
>    uint32_t AppId() const { return mOriginAttributes.mAppId; }
> +  inline const nsAString& AddonId() const { return mOriginAttributes.mAddonId; }

Is there any reason that you add an interface method and an inline version for the same thing? I don't think that an extra virtual call matters where you use this, so I'd keep the interface method and remove the inline version, but I'm not particularly picky about which one to remove.

Comment 16

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/46595/#review43295

> Is there any reason that you add an interface method and an inline version for the same thing? I don't think that an extra virtual call matters where you use this, so I'd keep the interface method and remove the inline version, but I'm not particularly picky about which one to remove.

Mainly for the sake of consistency with the other origin attribute getters, which are all infallible. I'd have liked to have just made this an ordinary infallible getter, but xpidl doesn't support infallible string getters (even though the call is actually infallible).

I'm less worried about the virtual call in this case (although I think we'd add more than one extra virtual call, and possibly a string copy if not careful) then about adding extra verbosity and different conventions for just one getter. But I still want the `addonId` getter to be available from JS. So maybe we should just go with a [notxpcom] `AddonId` method, rather than an inline method?

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Kris, I was browsing through this bug but I couldn't find any design discussions of this new security model for CSP in regards for web extensions. So far I have seen that you try to massage the CSP API to use a visitor pattern. Is there a specific reason you can't use the regular API for nsIContentPolicy we already have in place? If we are going to use the newly added visitor pattern users will not get any web-console error messages when CSP is blocking a load as far as I can tell, is this by design and desired behavior? The same is true for CSP reports (not sure if that is an issue for web extensions).

I would appreciate if you could add a paragraph explaining the design before we can add valuable feedback about the implementation. Thank you!

Comment 18

[Kris Maglione [:kmag]]

Hi Christoph,

Thanks for the feedback.

This is a near-parity implementation of the content security policy system used for Chrome extensions[1]. It applies, by default, a fairly restrictive security policy to all pages belonging to an extension. It also allows extensions to specify a custom security policy, if the default is too restrictive for their needs, but the custom policy needs to meet certain security benchmarks.

These patches do use the ordinary mechanisms to apply security policies to documents, regardless of whether a default or custom policy is used. The visitor interface is only used to verify the security of any custom policies, and reject them if they're too weak. The third patch in this series implements that validation, and contains comments explaining the details[2].

[1]: https://developer.chrome.com/extensions/contentSecurityPolicy
[2]: https://reviewboard.mozilla.org/r/46587/diff/1#chunk6.21

Comment 19

[Andrew Swan [:aswan]]

https://reviewboard.mozilla.org/r/46593/#review44629

::: toolkit/components/utils/simpleServices.js:156
(Diff revision 1)
>    /*
> +   * Sets the custom CSP string to be used for the add-on. Not accessible over
> +   * XPCOM - callers should use .wrappedJSObject on the service to call it
> +   * directly.
> +   */
> +  setAddonCSP(aAddonId, aCSPString) {

Is it deliberate that this method isn't declared in nsIAddonPolicyService.idl?

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8741581 [details]
MozReview Request: Bug 1254194: [webext] Test that content security policies are applied to WebExtension documents. r?aswan

https://reviewboard.mozilla.org/r/46597/#review44633

The mechanics of `testPolicy()` make sense after reading, though a comment explaining briefly what it does would be good.  Znd obviously using it to test the default policy makes sense but can you add some comments explaining the rationale for the other policies that get tested?

Comment 21

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/46593/#review44629

> Is it deliberate that this method isn't declared in nsIAddonPolicyService.idl?

Yes. It's only meant to be accessed from JS. All of the related setters are handled in the same way.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8741575 [details]
MozReview Request: Bug 1254194: Allow iterating over and inspecting sources of parsed CSP directives. r?ckerschb

https://reviewboard.mozilla.org/r/46585/#review44839

Kris, thanks for the explanation in Comment 18. Please make sure that it's not needed to log CSP errors to the console. I would imagine that it feels weired for web extension uses if something doesn't behave correctly but there is no corresponding console message.

Other than that I am fine with the change, r=me

::: dom/security/nsCSPUtils.h:342
(Diff revision 1)
> +    virtual bool visitNonceSrc(const nsCSPNonceSrc& src) = 0;
> +
> +    virtual bool visitHashSrc(const nsCSPHashSrc& src) = 0;
> +
> +  protected:
> +    nsCSPSrcVisitor() {};

I know it's redundant on default constructors, but who knows what the future brings? Would you mind adding 'explicit'?

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8741578 [details]
MozReview Request: Bug 1254194: Add XPCOMUtils.defineLazyPreferenceGetter. r?billm

https://reviewboard.mozilla.org/r/46591/#review45343

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8741576 [details]
MozReview Request: Bug 1254194: Add a validator for custom add-on content security policies. r?dveditz r?billm f?aswan

https://reviewboard.mozilla.org/r/46587/#review45347

This looks good to me. I think there might be some follow-ups, but what you have here is definitely better than what we have now (i.e., nothing).

::: toolkit/components/extensions/test/xpcshell/test_csp_validator.js:15
(Diff revision 1)
> +
> +    let result = cps.validateAddonCSP(policy);
> +    equal(result, expectedResult);
> +  };
> +
> +  checkPolicy("script-src 'self'; object-src 'self';",

Does Chrome allow "default-src 'self'"? The documentation suggests it does [1], while this patch forbids it.

[1] https://developer.chrome.com/extensions/contentSecurityPolicy#tightening

::: toolkit/components/extensions/test/xpcshell/test_csp_validator.js:39
(Diff revision 1)
> +              "'script-src' must include the source 'self'");
> +
> +  checkPolicy("script-src 'self'; object-src 'none';",
> +              null);
> +
> +  checkPolicy("script-src 'self' 'unsafe-inline'; object-src 'self';",

The Chrome docs suggest that 'unsafe-inline' is ignored rather than forbidden [1]. Could this cause issues where Chrome enforces the rest of the policy while we drop the entire thing?

[1] https://developer.chrome.com/extensions/contentSecurityPolicy#relaxing-inline-script

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp:206
(Diff revision 1)
> +    }
> +
> +
> +    // Visitors
> +
> +    bool visitSchemeSrc(const nsCSPSchemeSrc& src) override

Generally the formatting here is pretty weird: method names start with lowercase letters and there a lot of instances of multiple empty lines in a row. I guess the CSP code itself maybe looks like that? Please file a bug to fix that and use standard Gecko style as much as you can here, even if it causes inconsistencies. The only way we'll ever get to a uniform style is by enforcing it rigorously without allowing exceptions to make things "fit in".

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp:267
(Diff revision 1)
> +        return true;
> +
> +      default:
> +        NS_ConvertASCIItoUTF16 keyword(CSP_EnumToKeyword(src.getKeyword()));
> +
> +        formatError("csp.error.illegal-keyword", keyword);

Chrome allows inline scripts if the hash of the source code is provided. We should at least have a bug on file to support that.

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp:303
(Diff revision 1)
> +
> +    template <typename... T>
> +    inline void formatError(const char* aName, const T ...aParams)
> +    {
> +      const char16_t* params[] = { mDirective.get(), aParams.get()... };
> +      formatErrorParams(aName, params, PR_ARRAY_SIZE(params));

MOZ_ARRAY_LENGTH

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp:311
(Diff revision 1)
> +  private:
> +    // Validators
> +
> +    bool hostIsAllowed(nsAString& host)
> +    {
> +      if (host.First() == '*') {

Is there any way that |host| could be empty here?

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp:337
(Diff revision 1)
> +
> +      return true;
> +    };
> +
> +    template <typename T>
> +    bool schemeIsAllowed(nsAString& scheme, T& allowedSchemes)

Perhaps this could be called schemeInList or something. The way this is used, "allowed" isn't always the right word.

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp:339
(Diff revision 1)
> +    };
> +
> +    template <typename T>
> +    bool schemeIsAllowed(nsAString& scheme, T& allowedSchemes)
> +    {
> +      for (uint32_t i = 0; i < PR_ARRAY_SIZE(allowedSchemes); i++) {

MOZ_ARRAY_LENGTH is a little more up to date. However, a slightly nicer way to do this is to end the array with nullptr. Then you can do for(i=0;allowedSchemes[i];i++) and you don't need the template.

::: toolkit/mozapps/extensions/AddonContentPolicy.cpp:423
(Diff revision 1)
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    char idString[NSID_LENGTH];
> +    id.ToProvidedString(idString);
> +
> +    url.AppendASCII(idString + 1, NSID_LENGTH - 2);

Can you please MOZ_RELEASE_ASSERT that idString has the form {...}? That makes this a little more understandable and also less vulerable to memory corruption issues if we somehow get an ID that has an unexpected form.

Comment 25

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8741579 [details]
MozReview Request: Bug 1254194: [webext] Allow extensions to register custom content security policies. r?billm f?aswan

https://reviewboard.mozilla.org/r/46593/#review45379

::: toolkit/components/extensions/Extension.jsm:1209
(Diff revision 1)
> -    try {
> +    let started = false;
> +    return this.readManifest().then(() => {
>        ExtensionManagement.startupExtension(this.uuid, this.addonData.resourceURI, this);
> -    } catch (e) {
> +      started = true;
> -      return Promise.reject(e);
> -    }
>  
> -    return this.readManifest().then(() => {

Why this change?

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

One more thing: is there anything that ensures that content scripts correctly bypass a web page's CSP [1]? Bug 1207394 was supposed to cover this. I see it has some tests that seem relevant, but I'm really surprised that this "just works" without us having to do anything.

[1] https://developer.chrome.com/extensions/contentSecurityPolicy#interactions

Comment 27

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/46587/#review45347

> Does Chrome allow "default-src 'self'"? The documentation suggests it does [1], while this patch forbids it.
> 
> [1] https://developer.chrome.com/extensions/contentSecurityPolicy#tightening

It does. This shouldn't put any restrictions on default-src, but I can add a test.

> The Chrome docs suggest that 'unsafe-inline' is ignored rather than forbidden [1]. Could this cause issues where Chrome enforces the rest of the policy while we drop the entire thing?
> 
> [1] https://developer.chrome.com/extensions/contentSecurityPolicy#relaxing-inline-script

It might, but I hope there aren't enough add-ons using it for it to matter. Either way, rejecting it seems more consistent with the rest of the policy enforcement.

It might be worth adding a telemetry probe to get some idea of how content policies are used in the real world.

> Generally the formatting here is pretty weird: method names start with lowercase letters and there a lot of instances of multiple empty lines in a row. I guess the CSP code itself maybe looks like that? Please file a bug to fix that and use standard Gecko style as much as you can here, even if it causes inconsistencies. The only way we'll ever get to a uniform style is by enforcing it rigorously without allowing exceptions to make things "fit in".

Yeah, that's the capitalization style used in the CSP code, so I decided to stick with it for the visitor interface, which meant I wound up using it for the other method names in that class. I'll change the local method names in this patch and file a bug for the CSP code.

The multiple empty lines can go too, I guess. They just made it easier for me to keep track of the related sections of code.

> Chrome allows inline scripts if the hash of the source code is provided. We should at least have a bug on file to support that.

I believe that we should handle this. There's a test to make sure that we allow the 'hash-*' sources, but I haven't tested whether they work.

> Is there any way that |host| could be empty here?

No, in that case it's parsed as a scheme source rather than a host source.

> Perhaps this could be called schemeInList or something. The way this is used, "allowed" isn't always the right word.

Yeah, the first implementation used a static list, and I just didn't change the name when I repurposed it.

> MOZ_ARRAY_LENGTH is a little more up to date. However, a slightly nicer way to do this is to end the array with nullptr. Then you can do for(i=0;allowedSchemes[i];i++) and you don't need the template.

Yeah, I suppose. I tend to worry that I'll forget to add the `nullptr` to the end, but it probably isn't a huge issue here.

Comment 28

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/46593/#review45379

> Why this change?

`startupExtension` needs access to the manifest so it can register the "content_security_policy" value. Without this change, it gets called before the manifest has been parsed.

Comment 29

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #26)
> One more thing: is there anything that ensures that content scripts
> correctly bypass a web page's CSP [1]? Bug 1207394 was supposed to cover
> this. I see it has some tests that seem relevant, but I'm really surprised
> that this "just works" without us having to do anything.

There isn't, and I'm still not sure if that works. Bug 1207394 only tests that CSP restrictions don't apply to resources served from moz-extension: URLs. I suspect that injecting other resources that the content policy forbids will still fail. I'll file another bug for that.

Comment 30

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Kris Maglione [:kmag] from comment #27)
> https://reviewboard.mozilla.org/r/46587/#review45347
> 
> > Does Chrome allow "default-src 'self'"? The documentation suggests it does [1], while this patch forbids it.
> > 
> > [1] https://developer.chrome.com/extensions/contentSecurityPolicy#tightening
> 
> It does. This shouldn't put any restrictions on default-src, but I can add a
> test.

I'm not sure I understand. What I meant is that Chrome presumably allows "default-src 'self'" as the entire CSP while we will reject it (I think) because there aren't specific script-src and object-src directives.

Comment 31

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #30)
> (In reply to Kris Maglione [:kmag] from comment #27)
> > https://reviewboard.mozilla.org/r/46587/#review45347
> > 
> > > Does Chrome allow "default-src 'self'"? The documentation suggests it does [1], while this patch forbids it.
> > > 
> > > [1] https://developer.chrome.com/extensions/contentSecurityPolicy#tightening
> > 
> > It does. This shouldn't put any restrictions on default-src, but I can add a
> > test.
> 
> I'm not sure I understand. What I meant is that Chrome presumably allows
> "default-src 'self'" as the entire CSP while we will reject it (I think)
> because there aren't specific script-src and object-src directives.

Oh. I just tested, and it seems that they do. I guess this is easy enough to solve by just testing if we have a valid default-src directive, and allowing a missing script-src or object-src directive if we do.

Comment 32

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/66ce70f225330c89731d4c8f7a916a27a7ca6b3f
Bug 1254194: [webext] Fix tests that violate the default CSP. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c7140ccd2e86b8ad75e54db5ae7d3c21c66819
Bug 1254194: Allow iterating over and inspecting sources of parsed CSP directives. r=ckerschb

https://hg.mozilla.org/integration/mozilla-inbound/rev/58fc5fb221e0eb4dfb23779215873fb870e980c4
Bug 1254194: Add a validator for custom add-on content security policies. r=billm f=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/f099f37db1e4853395f1e67e9514be28e253ea00
Bug 1254194: [webext] Add 'onError' schema option to make manifest errors non-fatal. r=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/64a14b5509fcbc1da50eddabcd526cbfe0e57e00
Bug 1254194: Add XPCOMUtils.defineLazyPreferenceGetter. r=billm

https://hg.mozilla.org/integration/mozilla-inbound/rev/40310d2455d5f5dba0c4de2a192808b57509b942
Bug 1254194: [webext] Allow extensions to register custom content security policies. r=billm f=aswan

https://hg.mozilla.org/integration/mozilla-inbound/rev/701c0a43f593039d47d385ee103f060a8cd3e570
Bug 1254194: Apply a content security policy to all WebExtension documents. r=gabor

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd06fa4221949ade2f42498c48554e498bf2d15a
Bug 1254194: [webext] Test that content security policies are applied to WebExtension documents. r=aswan

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/66ce70f22533
https://hg.mozilla.org/mozilla-central/rev/f9c7140ccd2e
https://hg.mozilla.org/mozilla-central/rev/58fc5fb221e0
https://hg.mozilla.org/mozilla-central/rev/f099f37db1e4
https://hg.mozilla.org/mozilla-central/rev/64a14b5509fc
https://hg.mozilla.org/mozilla-central/rev/40310d2455d5
https://hg.mozilla.org/mozilla-central/rev/701c0a43f593
https://hg.mozilla.org/mozilla-central/rev/bd06fa422194

Comment 34

[Francesco Lodolo [:flod]]

Passing by note: the .properties file should have the usual MPL license on top.

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Fx48 Aurora release notes

Comment 36

[Will Bamberg [:wbamberg]]

I've added:

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Content_Security_Policy
- this briefly introduces CSP, describes the default policy and its implications

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/content_security_policy
- how to use the key to change the default policy

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Anatomy_of_a_WebExtension
- some pointers to the CSP doc

Let me know if this covers it.

Comment 37

[Kris Maglione [:kmag]]

Looks good. Thanks!