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

Proposal: CSP-compatible hooks #100

Closed
guybedford opened this issue Dec 7, 2015 · 6 comments
Closed

Proposal: CSP-compatible hooks #100

guybedford opened this issue Dec 7, 2015 · 6 comments

Comments

@guybedford
Copy link

This is in response to #69 (comment).

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 flag rather:

      System.loader.unsafeTranslate = true;
      System.loader[Reflect.loader.translate] = function() {
        // ...
      };

    Without unsafeTranslate set, the loader by default skips the translate stage.

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.

@guybedford
Copy link
Author

The unsafeTranslate or loaderTranslate flag would of course need to be an environment-level option.

Perhaps simply following the CSP policy unsafe-inline itself, or even via a custom HTML meta tag.

@caridy
Copy link
Contributor

caridy commented Dec 7, 2015

why a new issue? let's keep the discussion in one place.

@caridy
Copy link
Contributor

caridy commented Dec 7, 2015

How is this:

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

different from

System.loader[Reflect.Loader.fetch] = function(entry, key) {
    return 'here is my content to be evaluated?';
};

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.

@guybedford
Copy link
Author

Sure, that's understandable - I just didn't want the proposal to be missed at the bottom of #69 which is marked as a question, while I believe this is an important issue, but I can copy it across if you would prefer?

In the two cases you show above they are different in that in case (1), with translate disabled via a flag, the source that is executed can't be modified, while in case (2) the content string is an execution injection vector, unless the fetch method on the loader can be somehow protected or frozen reliably.

@matthewp
Copy link

matthewp commented Dec 7, 2015

Won't the environment fetch use window.fetch in the browser? If so you can use Service Workers to violate the fetch integrity anyways. I'm not a fan of getting rid of fetch (and turning it into locate).

@guybedford guybedford mentioned this issue Dec 7, 2015
4 tasks
@guybedford
Copy link
Author

@matthewp internal browser optimizations may find it better to skip the fetch spec, which is primarily a user-facing interface. That's a good point, but the scope of the service worker isn't the page itself so that there isn't a direct injection vector, just like for normal script loads.

@caridy closing to track in #69.

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

No branches or pull requests

3 participants