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

CSP compatibility #69

Open
4 tasks
matthewbauer opened this issue Aug 18, 2015 · 12 comments
Open
4 tasks

CSP compatibility #69

matthewbauer opened this issue Aug 18, 2015 · 12 comments
Labels

Comments

@matthewbauer
Copy link

Discussed originally in systemjs/systemjs#704.

Eventually, we're going to need to figure out how to make the loader spec compatible with CSP. This is just a stub for now because I don't think it's an urgent problem but I want to make sure it's being considered.

Here are some basic questions to resolve:

  • Does the loader need to know about the CSP?
  • How can a loader "instantiate" without unsafe-inline or unsafe-eval?
  • Do loaders need special permissions above CSP? (either through a CSP option or some other method)
  • Will external HTML resources like img and link eventually be put through a loader as well?

Right now, SystemJS requires "unsafe-inline" for anything except AMD which doesn't require a translate. We can give SystemJS (and other loaders) special permissions through the "nonce" option for some XSS safety.

@caridy
Copy link
Contributor

caridy commented Aug 18, 2015

I haven't spent too much time on this topic, but in principle, loader should treat the module scripts as regular scripts, whether they are inline or externals, just like regular scripts tags. The way I see it:

  • Loader does not expose the internals of the pipeline to parse and execute a source text, users will have to define their own mechanism to evaluate custom module formats, and that will be subject to the CSP rules as well (e.g.: whether or not you can use eval in your custom loader hook).
  • In the case of external resources, initially you will have to have some custom hooks in place to use fetch or something else, which is subject to the CSP rules as well (as it is today).
  • As for the nonces and hashes, they should work the same for <script type="module">.

@matthewbauer
Copy link
Author

Ok, so a polyfill of loader like SystemJS will inevitably have issues with CSP because the internals are exposed?

That makes sense too me. I think most of my confusion comes from that the Reflect APIs are intended to be implemented by vendors not user scripts.

You can close this if it's not a serious concern.

@caridy
Copy link
Contributor

caridy commented Aug 18, 2015

let's keep in around until we get to this topic.

@caridy
Copy link
Contributor

caridy commented Dec 4, 2015

Few notes (some initial brainstorming):

Evaluation of arbitrary code

Creating a loader and setting the right hooks will effectible allow you to execute any arbitrary code. This needs to be considered since it will be equivalent to eval(). e.g.:

let translate = System.loader[Reflect.Loader.translate];
System.loader[Reflect.Loader.translate] = (entry, payload) => {
    return translate.call(this, entry, payload).then((source) => 'doSomethingUgly();' + source);
}; 

We might need to have a way to mark source text fetched by the engine's default loading mechanism, to allow execution of it only if the source text hasn't changed thru the pipeline.

Nonces and Hashes

Executing an arbitrary source text as a module when inserting an inline <module> should probably be equivalent to <script>, same rules apply. But what about entry.resolve()? If a csp hash matches, should we allow the execution?

@caridy caridy changed the title CSP compatability CSP compatibility Dec 7, 2015
@guybedford
Copy link

(Originally posted at #100)

The current loader hooks effectively provide inline eval support and do not provide the ability to ensure source text integrity.

In order to support CSP, SubResource Integrity and nonces, it is important to be able to provide these guarantees.

I've included a suggestion here, to illustrate how this might work with an adjusted fetch hook to aid discussion along these lines.

Illustrative Proposal

  1. Instead of having the fetch hook return a source string, we can have the fetch hook itself return either a URL to fetch or undefined if there is nothing to fetch:

      System.loader[Reflect.Loader.fetch] = function(entry, key) {
        return encodeURI(key);
      };

    It is important that we can return a URL that is different to the key, as while the key will likely be a URL, there may still be some modifications necessary before fetching such as encoding and stripping plugin metadata.

    The environment Fetch implementation is then delegated to for loading that URL entirely. Perhaps with some default headers or fetch options.

    The fetch hook payload is then populated as the fetched source (despite the hook itself returning a URL). This way the source is available to the subsequent translate and instantiate hooks as the fetch hook result.

  2. A flag can then conditionally disable the translate hook. Perhaps from a security perspective translate should be disabled by default, and turned on with a meta tag in the head or some other environment specifier, like knowing that unsafe-eval is enabled:

      // translate hook is only called when `unsafeTranslate` environment flag is set
      System.loader[Reflect.loader.translate] = function() {
        // ...
      };

    Without the flag set, the loader by default skips the translate stage, effectively treating it as the identity operation.

With the above, we're able to maintain full support for the current hook use cases, while providing a guarantee in the loader that the source that was fetched is the source that was executed. These guarantees then give a path to inline <module> nonce support, SubResource Integrity support for <module integrity="..."> and CSP compatibility.

@matthewbauer
Copy link
Author

I don't think a flag is needed:

Advise browsers to make the "translate" and "fetch" builtin hooks not writable when "unsafe-inline" or "unsafe-eval" is off because that's going to be the equivalent of eval. Since some JavaScript environments like Node don't even have a concept of CSP we don't have to tell them to implement something that doesn't make sense in their use case.

It is important that we can return a URL that is different to the key, as while the key will likely be a URL, there may still be some modifications necessary before fetching such as encoding and stripping plugin metadata.

Is it possible to move this logic into "resolve"? My understanding is hooks should be able to stack one onto another so you would just need that "fetch" functionality to be at the end. I guess I really like the way the "fetch" hook corresponds to the new global fetch API and hope we won't have to abandon that.

The biggest problem with this is that there are lots of good uses of translate that we want available in CSP mode. SystemJS currently requires everything to be translated ahead of time to JS to enable it to work and maintain CSP. So currently there's not really an easy way to get it to work (what's to stop a malicious loader from translating to malicious code like @caridy mentions).

My solution to this would be to add a new CSP directive called "loader-src" that allows a page to specify ahead of time what loaders it's going to want to use. This maintains CSP compatibility because the loader would be a static module that couldn't be modified after initial script load. The loader could be imported in any module within the CSP through "import Loader from http://...". That Loader would be then able to override the translate and fetch methods in this case only. Because it is whitelisted a malicious script couldn't define its own (or override Loader).

@matthewp
Copy link

matthewp commented Dec 9, 2015

Advise browsers to make the "translate" and "fetch" builtin hooks not writable when "unsafe-inline" or "unsafe-eval" is off because that's going to be the equivalent of eval.

Wouldn't you have to do that for all of the hooks? Being able to change the URL (in resolve) is equivalent to eval as well. Probably the loader should just be frozen in those environments.

@guybedford
Copy link

@matthewp script execution URLs are bound by the origin policy of the page, so that script location is much more secure than eval.

So the origin security policy of the page would be taken into account when using the URL returned by fetch, which might throw a security error while running the fetch hook.

@domenic
Copy link
Member

domenic commented Dec 9, 2015

Changing the URL is 100% equivalent to eval. The SOP is not relevant here.

@guybedford
Copy link

Sorry the term origin wasn't meant to relate to same origin policy. I'm talking about the CSP whitelisting of hosts that are able to provide eg script-src on the page.

@matthewbauer
Copy link
Author

Changing the URL is 100% equivalent to eval. The SOP is not relevant here.

So, the logic for CSP checks will need to be after the resolve hook when we have an absolute URL. As long as the final result of resolve satisfies the CSP we should be good.

The fetch and translate hooks are the only ones that end up returning "content" that will later be evaluated (as opposed to URLs for resolve and JavaScript objects for instantiate).

@caridy
Copy link
Contributor

caridy commented Dec 9, 2015

posting my original answer to the proposal from @guybedford:

I don't think this problem is in the realm of the pipeline, this security measures can be implemented at the lowest level by just marking the content that can be executed, whether that content was fetched via default browser, or just a fetch process on the user-land with the proper content type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

5 participants