Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove object-src from the CSP (at least in MV3) #204

Open
Rob--W opened this issue Apr 27, 2022 · 10 comments
Open

Remove object-src from the CSP (at least in MV3) #204

Rob--W opened this issue Apr 27, 2022 · 10 comments
Labels
implemented: chrome Implemented in Chrome implemented: firefox Implemented in Firefox supportive: safari Supportive from Safari topic: csp Related to content security policy enforcement

Comments

@Rob--W
Copy link
Member

Rob--W commented Apr 27, 2022

Currently, in MV2 and MV3 the CSP requires the script-src and object-src directives to be specified with secure sources only.
script-src is restricted to avoid unsafe code execution,
object-src was restricted to avoid unsafe plugin code execution (https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/object-src).

These days, with plugin support having been removed from web browsers*, object-src is not useful any more.
But at least Firefox and Chrome still require object-src (or default-src as a fallback) with "secure" sources to be specified if extensions want to specify a custom CSP, despite object-src not being useful.

I propose that we remove object-src from the default CSP (which I suggested at #98 (comment)), at least in MV3. This makes the CSP more readable and makes it easier to override the custom CSP.

* Firefox is not supporting NPAPI plugins any more, including the Flash plugin.
Chrome is not supporting NPAPI plugins either, and (P)NaCl (NativeClient) have been deprecated and support is ending (https://blog.chromium.org/2020/01/moving-forward-from-chrome-apps.html). Even if there is somehow a desire to support (P)NaCl in the future, the implementation does not need to rely on CSP to block access by extensions.

@Rob--W Rob--W added the topic: csp Related to content security policy enforcement label Apr 27, 2022
@Rob--W
Copy link
Member Author

Rob--W commented Apr 28, 2022

There is consensus on removing object-src from the CPS.

I have filed the following issues on the individual browser repositories to track this effort:

@rdcronin
Copy link
Contributor

object-src applies to , , and . I think s and s can still be legitimately used for things like embedding PDFs or videos. While this isn't necessarily best practice (videos should probably be embedded with the

@rdcronin
Copy link
Contributor

Wow - github apparently doesn't like it when you type HTML elements, and dropped three quarters of the comment. Trying again...

object-src applies to object, embed, and applet tags. I think object and embed tags can still be legitimately used for things like embedding PDFs or videos. While this isn't necessarily best practice (videos should probably be embedded with the video tag, for instance), I don't think this is something we'd disallow in the browser or on the platform.

Is the suggestion to remove this directive because these elements are no longer used, or because we no longer need to specify them with secure sources only? My read from this and the entry in the chromium bug tracker was the former, but I don't think that's accurate for the reason above. In theory, I think the latter may be okay - but I'm hesitant to remove this restriction until all the code to run plugins is fully removed (and I believe, as you mention, Chrome still has some support for (P)NaCl).

WDYT?

@Rob--W
Copy link
Member Author

Rob--W commented May 2, 2022

Is the suggestion to remove this directive because these elements are no longer used, or because we no longer need to specify them with secure sources only?

The suggestion is to stop requiring extensions to specify an object-src directive when they set a custom CSP. Currently, "content_security_policy": "script-src 'self'" triggers the following warning:

'content_security_policy': CSP directive 'object-src' must be specified (either explicitly, or implicitly via 'default-src') and must allowlist only secure resources.

but I'm hesitant to remove this restriction until all the code to run plugins is fully removed (and I believe, as you mention, Chrome still has some support for (P)NaCl).

If you'd still like to enforce the restriction, you could define a base policy that disallows remote sources in object-src (or "minimum" CSP to match with your description at https://crbug.com/1318922#c6). Extensions wouldn't be able to loosen the object-src policy, but at least they won't be forced to add a boilerplate object-src in the extension.

@rdcronin
Copy link
Contributor

rdcronin commented May 3, 2022

When we have a proper "minimum CSP", I'm supportive of removing this restriction. In fact, I'd be in favor of removing all restrictions - I see no reason we'd require an extension to specify something for those directives (though we could maybe warn them for directives that will be effectively ignored by our minimum?).

Until then, though, I think we'll want to keep this in place (at least on the Chrome side).

I'm also still curious for other vendor's thoughts on its presence in the minimum CSP, given it's applied to more than plugins (which is what the original post was targeting).

@xeenon xeenon changed the title Remove object-src from the CSP (at least in MV3) Remove object-src from the CSP (at least in MV3) Aug 31, 2022
@xeenon
Copy link
Collaborator

xeenon commented Sep 23, 2022

Safari currently implicitly adds object-src 'self' if it is missing from the extension's CSP — no warning/error is given. I am supportive of dropping the requirement for object-src to be specified.

@Rob--W Rob--W added implemented: firefox Implemented in Firefox supportive: safari Supportive from Safari labels Sep 23, 2022
@carlosjeurissen
Copy link
Contributor

@xeenon does Safari and other browsers do additional transformations besides adding object-src 'self'? Would be good to have documentation on how the browsers deal with the CSP given by the extension author.

@xeenon
Copy link
Collaborator

xeenon commented Oct 2, 2022

@carlosjeurissen Safari is working to change our implementation to a different parser mode in WebKit, so the current way we modify the CSP string is not a great indication on what we will do in the future.

The WebKit code can be found via: https://github.com/WebKit/WebKit/search?q=ContentSecurityPolicyModeForExtension

@Rob--W
Copy link
Member Author

Rob--W commented Nov 9, 2022

Marking as supportive: chrome given that the proposal here has been included in their plan shared at #98 (comment)

aarongable pushed a commit to chromium/chromium that referenced this issue Dec 21, 2022
Currently, if an extension provides a custom content security policy,
we require that both script-src and object-src are specified (either
explicitly or through fallbacks like default-src). Since we now apply
a minimum CSP to all extension pages, this is unnecessary. Additionally,
since object-src is primarily used to control plugins, its presence is
somewhat misleading and noisy.

With this CL, we no longer require extensions to specify object-src
explicitly in MV3, and instead rely on the minimum CSP to enforce the
restriction. If an extension *does* specify object-src, we still require
it to be secure and disallow remotely-hosted code. Because object-src
inherits from default-src if it's not specified, this also means we
*will* require object-src to be explicitly specified if a non-secure
default-src is used (that is, the effective value of object-src in
an extension-provided CSP must either be secure or be undefined).

This CL does not change the default CSP; this can be visited in a
follow-up.

WECG Issue: w3c/webextensions#204

Bug: 1320785
Change-Id: Ib64ab23c1f8721f433a5313e1658b0c3b0e0263f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4116239
Commit-Queue: Devlin Cronin <[email protected]>
Reviewed-by: David Bertoni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1086114}
@Rob--W
Copy link
Member Author

Rob--W commented Jan 3, 2023

Implemented in Chrome 111 by https://bugs.chromium.org/p/chromium/issues/detail?id=1320785

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
implemented: chrome Implemented in Chrome implemented: firefox Implemented in Firefox supportive: safari Supportive from Safari topic: csp Related to content security policy enforcement
Projects
None yet
Development

No branches or pull requests

4 participants