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

Import maps should be controlled by CSP #105

Closed
domenic opened this issue Feb 21, 2019 · 11 comments
Closed

Import maps should be controlled by CSP #105

domenic opened this issue Feb 21, 2019 · 11 comments

Comments

@domenic
Copy link
Collaborator

domenic commented Feb 21, 2019

I believe that, since the threat model of CSP is "someone is able to inject script into my page but I still want to prevent bad things from happening", we need import maps to be controlled by CSP as well. If a malicious actor can inject an import map, then they can change the behavior of scripts on the page (similar to overwriting self.fetch(), or inserting a <base> tag, or similar). So CSP should stop that as well.

This issue can be closed when there is an actual spec which includes these protections.

@developit
Copy link

developit commented Feb 21, 2019

I think Import Maps should be subject to script-src, but should not be subject to unsafe-inline. An Import Map is not itself executable, it only allows redirecting script URLs to other URLs. Ultimately, those script URLs are external scripts and should be subject to any present script-src policy.

For example: say I have a CSP defined that blocks scripts not in a given list of sources:

Content-Security-Policy: unsafe-inline; script-src main.js libs.js

As long as the resulting mapped import is subject to CSP constraints, it's still possible to prevent loading of any unknown code via Import Maps:

<script type="importmap">
{ "imports": {
  "@std/kv-storage": [
    "@std/kv-storage",
    "/kvs-polyfill.mjs"
  ]
} }
</script>
import 'main.js'  // ok
import 'libs.js'   // ok
import '@std/kv-storage'  // violation

What's unclear to me, however, is whether the violation is triggered for @std/kv-storage, or /kvs-polyfill.mjs. Intuitively, it seems like Built-in Modules should be unconditionally allowed and should not be affected by CSP, since they're part of the platform itself.

@jeremyroman
Copy link

CSS also isn't executable, but IIUC <style> is still blocked if unsafe-inline is not in style-src.

Would it be so bad to just expect authors to use a hash or nonce in such cases (or to put import maps in a different CSP bucket, if that would be too cumbersome)?

@hiroshige-g
Copy link
Collaborator

I feel we should apply at least all CSP restrictions that are currently applied to inline scripts also to import maps, because even though import maps are not "executable" in a strict meaning, they still can affect fetching and execution of other scripts.
(Not so sure this is sufficient though)

@mikewest
Copy link
Member

For script, CSP generally acts in two places:

  1. Inline script blocks are checked against nonces and 'unsafe-inline'.
  2. Fetched resources are checked against the script-src safelist.

Import maps adds a third, which are the "resources" quasi-fetched as part of the standard library. I think I accept the notion that built-ins aren't actually fetched, and are part of the platform, and I'm not sure there's value in trying to govern their accessibility via CSP.

@developit's example above matches my understanding of how we'd want to handle the second case. If kvs-polyfill.mjs isn't safelisted, then it's going to be blocked by the page's policy during Fetch, after the mapping takes place. That means that kvs-polyfill.mjs is the filename developers will see in the violation report, etc, which makes sense to me.

Governing the <script type="importmap"> via CSP as well seems reasonable (for the same reasons that <base> is governed by base-uri. I don't have a strong opinion about how to spell the policy, though. I'm not sure a new directive is appropriate, and I think I could get behind @hiroshige-g's suggestion to treat it as any other inline script (by e.g. applying nonces and 'unsafe-inline'). That seems reasonable to me.

@shhnjk
Copy link

shhnjk commented Mar 18, 2019

I think I accept the notion that built-ins aren't actually fetched, and are part of the platform,
and I'm not sure there's value in trying to govern their accessibility via CSP.

I've commented about this here. If platform will fix all vulnerabilities or gadgets in the standard library, then I'm fine with not adding standard library under CSP governance. Though,if DOM manipulation in standard library increases (e.g. things like std:virtual-scroller), then we might also introduce gadgets to bypass Trusted Types (And that's also a part of CSP) 🙂

@mikewest
Copy link
Member

If platform will fix all vulnerabilities or gadgets in the standard library, then I'm fine with not adding standard library under CSP governance.

If we have a standard library, it seems to me that we need to treat it as we would any other API provided by the browser. I'm 100% sure that we'll introduce bugs along the way; I'm looking forward to folks like you holding our feet to the fire to fix them. :)

@koto
Copy link

koto commented Mar 20, 2019

Import maps, if they are in the DOM like proposed, should be covered by the standard CSP script-src, including unsafe-inline. I don't think they should be in the DOM in the first place, but exempting them leads to at least CSP bypasses (e.g. under nonce-only CSP an injection is able to load an arbitrary script by injection of an import map).

I don't think we should treat import maps as anything less than regular scripts.

@mikesamuel
Copy link

mikesamuel commented Mar 21, 2019

To synthesize what has been said above, and articulate a success condition:

Problem Statement

  • Policies that use script-src nonce-... or default-src nonce-... separate the decision of whether to load from the decision of what to load.
  • An attacker who controls a <script type=importmap> controls what to load decisions.
  • Unless import maps are subject to the whether to load aspect of nonce-based Content-Security-Policies then importmaps bypass existing, recommended CSP policies for any site that adds one use of a <script type=module>.

Success condition: Compatibility in the face of injection for non-adopters

It'd be nice to see an argument in the explainer about how a document that

  • uses Content-Security-Policy: script-src nonce-xyz
  • loads at least one <script type=module nonce=xyz>
  • intentionally loads no <script type=importmap>
  • allows an HTML injection

loads equivalent JavaScript regardless of whether the browser supports import mapping.

@mikesamuel
Copy link

mikesamuel commented Mar 21, 2019

I think Domenic's proposed fix addresses the compatibility argument, as long as it follows the same (script-src falling back to default-src) path used for <script type=module ...> and <script ...>.

The Security Considerations verbiage might look like:

  1. Because the [CSP3] Script directive checks finds that <script type=importmap ...>...</script> is script-like, an attacker can cause no side effects that they could not cause by injecting <script type=module ...>...</script> or <script ...>...</script>.
    • If the policy has a nonce, and the attacker can predict a nonce, then they can inject JavaScript directly.
    • If the policy has integrity expressions, and assuming that it's no easier to generate hash-colliding JSON than JS, then they can as easily inject JavaScript directly.
    • If the policy allows parser-inserted scripts via 'strict-dynamic', there's no reason to presume it's easier to trick a script into creating an HTMLScriptElement with type=importmap than without.
    • If the policy has a source list, then see caveat hosted JSON .
    • Otherwise, an attacker's injected import map will be rejected.
  2. Therefore existing policies are unlikely to Allow importmaps without also allowing direct script injection so an attacker gains little leverage by targeting user-agents that supports import mapping.

Some white-listed origins host user-contributed content. Often they check extensions or content-types, and are careful not to serve user-contributed content with Content-type: text/javascript.

The proposal could mitigated this by requiring a Content-type that is not unadorned application/json.
Acquiring Import Maps says

What should be the MIME type? Let's say application/json+importmap? Maybe accept any JSON MIME type.

We should probably not accept any JSON MIME type.

mikesamuel added a commit to mikesamuel/import-maps that referenced this issue Mar 22, 2019
This incorporates the conclusion at the end of WICG#105 (comment)

> We should probably not accept any JSON MIME type.

based on the assumptions made by existing hosting providers and JSON-producing web services as to what can be safely hosted on origins that appear on CSP source white lists.
@hiroshige-g
Copy link
Collaborator

By the way, I'm interested in @koto's following comment:

I don't think they should be in the DOM in the first place

Do you have alternative designs for this? Do you have a github issue to discuss this point?

@domenic
Copy link
Collaborator Author

domenic commented Jul 3, 2019

The spec now covers this, I believe, as do the web platform tests. Happy to reopen if we missed something, but if not, closing for now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
10 participants