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

Gather impact of supporting non file based modules through ESM #520

Closed
bmeck opened this issue Apr 17, 2019 · 8 comments
Closed

Gather impact of supporting non file based modules through ESM #520

bmeck opened this issue Apr 17, 2019 · 8 comments
Labels

Comments

@bmeck
Copy link
Member

bmeck commented Apr 17, 2019

Currently, the experimental ESM loader supports file: backed URL based loading. It does not allow other schemes. It would be good to review what we should be concerned about when going through the Node.js loading mechanisms and how we can discuss support for other protocols, in particular data: and https:.

I've gathered some slides around this related to the experimental policies implementation.

These slides can be framed under a few hypotheses:

  • The ability to mutate core and lack of constraints being applied to modules being loaded leads to needing to treat sources as having implicit authority if loaded; this is the same as all code loaded currently in Node.js. Ideally in the future with API constrains core limiting the authority of a module should be possible.
  • Guarding against mutable core is not necessary if all code must first be loaded in a trusted manner in order to mutate core in problematic ways. The burden of not loading code that invalidates parts of core (including bypassing policies) is left to users for now. Ideally in the future with API constraints being applied this burden could be mitigated.
  • Policies only need ensure that they are properly read-only and do not give false positives of being approved when loading unknown resources. Policies do not enforce that a location and body cannot be recreated/forged at runtime. Unknown resources can be defined as having a location or a body not contained within the policy.

Given that mindset I believe that https: and data: based loading should be doable from a security perspective, but would like to have some review. There is however a slight difference from CJS loading due to the deterministic loading of modules when loaded via ESM. This means that modules are permanently in a map once loaded and has a more reliable way to ensure that a module reference is a singleton than in CJS which has a mutable require.cache leading to an increased concern about if a module reference can be recreated before another module uses it; note, this is similar to what happened in event-stream and also applies to CJS usually.

With all that in mind, I was wanting to gather any problems there might be in supporting https: and data: loading from the ESM loader. If we can agree on solutions or concessions I would like to PR core with https: functionality at least.

@bmeck
Copy link
Member Author

bmeck commented May 15, 2019

We have had this brought up a couple of times on the Realms calls via TC39 and no major pushback there as well except some of the concerns listed in the slides which were not enough to be considered objectionable. If nothing is apparent security wise that seems a problem, we should move onto other interested areas of Node.

CC: @nodejs/modules

@Fishrock123
Copy link

Fishrock123 commented May 15, 2019

data: I am ok with but I don't think we should ever have default unchecked networked module loading functionality.

Edit: i.e. if you had to provide a shasum, maybe that would be ok.

@bmeck
Copy link
Member Author

bmeck commented May 15, 2019

@Fishrock123 can you explain relative to the slides what is different about networked access here vs generated by code? e.g. Why is using a trusted transport (including the CA checks that node does by default) not sufficient but using runtime codegen without integrity checks is?

@benjamingr
Copy link
Member

@bmeck can only speak for myself: for the same reason subresource integrity is useful - I might trust my own code more than the CDN.

Imagine the following scenario:

  • Some package I use that is barely maintained loads a file from domain A
  • Domain A expires, attacker buys domain A and gets a valid certificate
  • Attacker replaces the file I load from domain A with whatever payload they want

@bmeck
Copy link
Member Author

bmeck commented May 15, 2019

@benjamingr that is an argument for policies to allow these URLs, but doesn't seem to necessitate that these URLs be unusable without being in the policy? Without integrity policies code in general isn't safe from mutation as we saw in event-stream where one package mutated a different one purely using file based modules.

@benjamingr
Copy link
Member

@bmeck you are right, package-lock files mitigate this for modules where you don't accidentally update a module. An SRI like mechanism for URL imports would essentially be solving the same problem.

@bmeck
Copy link
Member Author

bmeck commented May 15, 2019

@benjamingr this integrity check already exists in an experimental flag via a file manifest (see a blog post) but isn't using package level data as that would require calculating the integrity of all files within a directory on startup and might not be valid if there are compile steps on the local machine like for C++ modules, if anything needs to be added to that it should probably be done against core as a separate issue.

@github-actions
Copy link
Contributor

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

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

No branches or pull requests

3 participants