HTTPS Imports

[bmeck]

Github PR UI is not well suited to this level of conversation

Discussing PR #36328

Discussing implications of HTTPS imports as a whole

[bmeck]

Concern To Mitigate: IO Timeouts

[jasnell]

Closely related to this is a never-ending import (a malicious server sending a never-ending stream of data).

Likely need a max import size as well as timeouts.

[bmeck]

@jasnell agreed, but there are multiple concepts of granularity here. Graph size vs individual source text size. I think we have to solve for both either by mitigating or scoping out. I'm thinking for this topic we are really talking about mitigations on the individual level, but the infinite recursion might be better for the strongly connected static graph mitigations. I think at least for the initial approach to size, byte limits might be enough as laid out in https://github.com/bmeck/node/tree/policy-max-body . Timeouts are more complex.

[bmeck]

Concern To Mitigate: Infinite Recursive Importing

[targos]

How can that happen?

[bmeck]

A server could dynamically generate a payload that ~= import "import.meta.url + 1 ". This would prevent the ESM graph from ever evaluating any code and memory leak until the process dies.

[bmeck]

It is possible to do this in CJS already, but with ESM this is a bit different. CJS can write files as they are loaded. ESM must load files after they are written using dynamic import. With the introduction of Loaders and/or 3rd party actors like servers that makes static import also able to cause infinite recursion.

I'm not entirely sure how we want to limit this, could limit the dep graph source size of an import or the total distance of the graph from entry or from the main entry point. When we get to that point if the problem was through static imports it won't be too much of an issue since we can GC the modules prior to linking (linking makes them live for the life of the global unfortunately [cantfix / upstream]). With dynamic imports, we couldn't reclaim that lost memory.

[jasnell]

POC:

const fastify = require('fastify')({ logger: true })

let counter = 1;

fastify.get('/foo.ts', (request, reply) => {
  reply
    .code(200)
    .header('Content-Type', 'application/x-typescript')
    .send(`import * as foo from 'http://localhost:3002/foo.ts?${counter++}'`);
})

const start = async () => {
  try {
    await fastify.listen(3002)
    fastify.log.info(`server listening on ${fastify.server.address().port}`)
  } catch (err) {
    fastify.log.error(err)
    process.exit(1)
  }
}

start();

[bmeck]

The more I stare at this them more it looks like just placing a config on graphs likely ends up limiting the whole program size. If we just limit statically linked graphs E.G. import "a"; we don't cover a server sending import("a"). If we do cover import("a") there isn't a clear boundary I can discern on what the limiting factor is since they could do import("https://alias/a") instead.

[bmeck]

Concern To Mitigate: Server Downtime

[bmeck]

We can somewhat do this already with URL redirects, see a policy with:

{
  "scopes": {
    "file:": {
       "integrity": true,
       "dependencies": {
           "https://example.com/foo": "file://cache/https/example.com/foo"
       }
    }
  }
}

That would not cover the HTTP redirect semantics, leak the wrong import.meta.url, and would not respect cache / updating. It does bring up some questions though about what the update semantics should be for a cache. Should it ever be automatic? I somewhat lean on "no it should always be a manual step to sync the cache".

[targos]

I agree the cache update should be manual. I think we need a good CLI to work with it though.

[bmeck]

Concern To Mitigate: Alteration of Source Code

[mcollina]

let's imagine a workflow:

1. I start using a new dependency, I quickly take a look at it and it fits my need, therefore I trust this dependency and its author
2. I configure my "update mechanisms" to pick up any new version of that library authored by the same author
3. If/when the author changes, I'm getting notified to look at that again

This workflow works the same for both certificate pinning and code signing at the module level. I prefer the module level because it's out-of-band of the transport mechanism and it can be easily proxied through. Using TLS certs for this would mean that proxy would need to do something special to keep provenance.

[bmeck]

I think we disagree then.

I configure my "update mechanisms" to pick up any new version of that library authored by the same author

I don't think trust based upon social audits of specific maintainers is either practical or even reliable.

1. We have examples of actors publishing on behalf of others like https://eslint.org/blog/2018/07/postmortem-for-malicious-package-publishes . This is a general workflow for most libraries with multiple collaborators these days and wouldn't be helped by trying to treat individual collaborators as a level of trust.
2. We have examples of actors turning malicious https://snyk.io/blog/a-post-mortem-of-the-malicious-event-stream-backdoor/ . Updating during that good period would seem safe.
3. The trust provided per module would be a sum of all dependencies of that module, leading to huge amounts of auditing needed.
4. This could also be solved on the remote level by giving each trust level its own cert . This would likely mimic https://aws.amazon.com/blogs/aws/amazon-s3-path-deprecation-plan-the-rest-of-the-story/ 's epic migration.

I still am unclear on the actual wins of signing a module itself. Especially if when dumped on disk it might not match what was signed due to scripts or fs mutation.

[wperron]

I agree with @bmeck here, I don't think these issues are new to URL modules, they are already present in the current ecosystem. Putting my security tin foil hat on for a minute, I'd argue it's best practice to use a lock-file with a hash of the source code downloaded, or to vendor dependencies with source code and to run static analysis on the built artifact.

I think an allow-list for specific domains and TLS verification + using an ETag header are sufficient - the rest can be addressed in userland

---

Should also point out that the exploit detailed in the linked article wouldn't have been possible without post-install scripts, which are a feature of the npm cli. URL imports wouldn't be able to support those anyway.

[mcollina]

I agree with @bmeck here, I don't think these issues are new to URL modules, they are already present in the current ecosystem. Putting my security tin foil hat on for a minute, I'd argue it's best practice to use a lock-file with a hash of the source code downloaded, or to vendor dependencies with source code and to run static analysis on the built artifact.

Regarding https://medium.com/@alex.birsan/dependency-confusion-4a5d60fec610, my whole point was that URL modules where as susceptible as a central registry is, mainly due to the expiration of domain names. Calling this a known vulnerability is totally legit, at least folks could use countermeasures.

I don't think this is easy to mitigate in userland without some support from the runtime.

I think we could all agree to have some level of certificate pinning (inside a lockfile) to mitigate this issue.

[ljharb]

Specifically, that attack was a vulnerability in a trusted internal registry, not in the public npm registry nor inherently in npm/registries, except in the inherent dangers of URLs that @mcollina outlines (which HTTPS imports increase the usage of).

[wperron]

I guess a good starting point would be to look at the npm cli because those problems are all also present there; they're just hidden behind the veneer of the cli.

I think we also need to clarify something from the discussion that happened over in the PR: when we say "HTTPS imports" do we mean that they are simply another way to reference and import modules into the node_modules, or are they implicitly dynamic imports?

[bmeck]

I'm not sure I understand. My understanding / expectation is it must be possible to do dynamic fetching of source texts over the network. I think trying to state it must be either always online or offline isn't really an easy topic since if we allow only one it invalidates some of the use cases of HTTPS imports.

[wperron]

that's how Deno does it and it's what makes more sense to me as well, I just wanted to clarify because a few of the comments on the PR seemed to assume it was only dynamic imports.

[bmeck]

Concern to Mitigate: Cross Origin Access

E.G. https: to file:, https: to node:

[bmeck]

WHATWG already only allows only same domain from http/https schemes. I think the presumption is that https: could load node:, but I actually think that isn't really a priority as many/most modules don't actually need direct access to things like fs:. I do think in general that it is a security concern, but not a new attack vector. Without really strong reasons I do think requiring https: to have those kinds of node: modules dynamically injected as params or something seems fine to me.

[bmeck]

node_modules lives on file: so it would be prevented by nature of being cross origin and not allowing https: to file:.

[wperron]

Browsers have a very clear origin to compare against, how would that work from the server? For reference this is also an ongoing conversation in Deno denoland/deno#4981

[bmeck]

@wperron same way as specced except we had to solve file: already for policies; for policies this was laid out with directory hierarchy as boundaries but same origin. I would note that due to how WHATWG's URL parser works file: resolution has an exploit if you don't realpath before performing checks.

[bmeck]

I would note that in general unlike the web we are not designing for a single base origin in node.

[mcollina]

HTTPS Agent support.

Given we are going to download many files, hopefully from the same domain it's critical for "first time" startup performance to keep the connections alive.

[mcollina]

I think it should have an Agent which is kept private for esm.

[bmeck]

we could, but it cannot reuse the implementation currently in core

[targos]

Agent can be made safer to protect against user manipulation, right?

[bmeck]

@targos it could, but it wouldn't be simple or might even be infeasible given how many things manipulate it for instrumentation reasons

[bmeck]

If we populate the cache manually / as a non-standard setup does this still need to apply?

[mcollina]

Visible caching on disk.

I think we should cache what is downloaded in the project folder and recommend this to be committed to the repo itself. I propose we use a web_modules folder. This is also a mitigato to availability attacks.

[bmeck]

This is incorrect. npm packages cannot be unpublished anymore since left-pad.

The npm team does unpublish things if you give requests with reasons such as security issues, licensing, etc. They also unpublish things without direct request if they find similar issues. The only thing that changes is that you cannot use the CLI to do so. This also is not true for most corporate npm proxies (which many/most companies do have for private package reasons).

[wperron]

We currently ship with a solution that is safe and secure.

well, not really: Node only ships with a local name resolution algorithm. The actual fetching logic is delegated to the npm cli or yarn. If those are compromised, so are the modules pulled into node_modules, and that has happened in the past. Adding an https:// module resolver to the core of Node would be a net addition to the current way of installing and and using dependencies.

[targos]

This is derailing a bit from the original discussion. To get back to the original topic: node_modules/ is used by the Node.js module resolver. Using it as a cache for https:// modules would change the behavior of the resolver.

[wperron]

@targos clearly the module resolver will need to be updated to account for https modules, making them first-class citizens just like modules identified by their npm identifier, so I don't think there's a conflict there, plus it makes the existing tooling and vendoring patterns just work out of the box, or at least makes it close enough that adapting it to https modules would be a relatively straight-forward upgrade

[bmeck]

@targos yes/no on changing the behavior, there is precedent on using it for other things like .cache, .bin, etc. Adding support for things that cannot be installed via a registry due to the naming conventions enforced by the registry like these wouldn't have any real effect.

[mcollina]

HTTP Proxy support for downloading modules.

A good way to mitigate many availability attack is to download the modules through an HTTP proxy. Currently Node.js does not have this functionality built-in.

[bmeck]

Currently policies allow rewriting URLs, would allowing wildcard URL rewriting like export templates be enough?

[bmeck]

Also, can this be done as a follow on during experimentation?

[bmeck]

I've made https://github.com/bmeck/local-fs-https-imports to play around with the purely ahead of time idea before trying to merge such a feature into core itself.

[wperron]

I like the node_modules/.https/ - seems like a nice way to make the old and the new play together nicely

[jimmywarting]

Concern To Mitigate: process

• process.exit()
• process.env

the hole process contains way to much information that can be leaked

[bmeck]

Unfortunately we can't put this in scope until we have some newer JS features; this likely has to wait for Realms or some such feature. We looked into policies and other mechanisms around limiting it for ESM in a few things in the past like: #26334. We need a way to introduce new global scopes w/o creating new intrinsics for things like Array which we don't have a way to do so currently. Users could replace globalThis.process with a getter that does a callsite check like in that and the other PR we attempted to get through, but the overall performance hit is not negligible. Other ways to limit it for now would be up to source transforms detecting usage of the globals and replacing it with a corresponding import/require.

[bmeck]

adding: fetch() is coming and is also a major concern

[bmeck]

bumped, updated to new link

[mcollina]

Filled

[bmeck]

Given the replies Thursday June 10th 1PM-2PM UTC seems to work and I will get a calendar entry added. Likely this one is just about phasing as mentioned above not about implementation details / shipping.

[MylesBorins]

I'd love to join!

[bmeck]

Thanks to @MylesBorins for making the calendar event on the Node.js public calendar. See you all in a day and a bit!

[guybedford]

Concern: npm packages using HTTPS imports.

Posting this to link discussion from @ljharb in #36328 (comment) re implications for supporting in node_modules.

Personally I would be concerned about npm packages being published that use HTTPS imports as that very much changes the security model / contract that npm provides to users in being able to comprehensively replicate a module graph via only a node_modules filesystem model.

This likely won't be a problem initially as no one would publish a package to npm that isn't supported in older Node.js versions, but once support reaches all versions, some company might think it's a good way to automatically manage updates / ensure registered code delivery.

While that is already possible today via network based processes and even install hooks, it's a pattern that we have luckily so far avoided from happening too much and I hope we can continue to work towards ensuring strong guarantees around the registry and package contracts that ensure end user agency.

I haven't watched the video so apologies if this is repeating talking points already said, feel free to fill in further here.

[bmeck]

@guybedford got it. split that specific concern to: #36430 (comment)

[guybedford]

@bmeck one thing that might make this easier is if Node.js were to have a "default policy file" check without the policy flag being necessary - so that the entire workflow can behave unflagged in the end, but like CORS or any other web security measure just requires an initial setup.

[bmeck]

@guybedford I think policy discussions could be done elsewhere.

[guybedford]

Certainly - my point is just that this doesn't stop there ultimately being an entirely "flagless" scenario for the use case.

[bmeck]

@guybedford does this actually expand to Buffer/process as well?

[bmeck]

meta concern to mitigate: accidental inclusion of transitive mutable state via node_modules.

A user should not have node_modules introduce transitive mutable state unless they opt-in to allow such behavior. This is important to ensure that node_modules can be seen as effectively immutable. While node_modules are not technically immutable today the general practice allows them to be treated as immutable; the introduction of network based transitive mutable state is actively more unstable and a concern around security and robustness that needs to be mitigated by other means as seen in this discussion.

[ljharb]

Also, any time a domain name's ownership changes, the new owner could serve whatever they want from the site.

[wperron]

That's also been a concern of mine, I wonder if it'd possible/advisable to add whois info in the lockfile for https deps 🤔

[bmeck]

@wperron Just about any out of band info can be added, time it is synchronized and checked is the main question. It is a security vector for example to do a WHOIS query.

[guybedford]

@bmeck do you have tooling for integrity lockdown that can work with remote URLs and policy?

[bmeck]

@guybedford node-policy needs to be updated, so no not at this time.

[bmeck]

Concern to mitigate: using other protocols to hop to local resources.

It is possible currently for data: to access node: and file: URLs (of which there is some usage in the wild). Since it is possible at least in this PR to load data: via http{s,}: we might want to ban loading these by default.

[bmeck]

I've banned everything except network dependencies in the PR for now.

[targos]

Note that depending on how the main application is started (notably via a CommonJS entry point), it's still possible to access the node: scheme using globalThis.process.mainModule.require().

[bmeck]

@targos yup, this is known and we tried to fix Buffer/process so they were not implicit globals in ESM but failed to do so due to a perf concern. So... ambient ways to mitigate it are left up to users and not covered by things like policies. A lot of mitigations are about limiting how things can be accessed so they can properly be mitigated, and unfortunately not mitigating them in core itself. Removing data: for example means that you have to grab them via globals, or be passed them somehow via dependency injection. It removes import from the problem areas.

[bmeck]

Concern to mitigate: preventing injection of arbitrary locations to import()

It should be possible to differentiate if a dependency is allowed to be loaded by dynamic import. Static import has hard coded strings and is less vulnerable to manipulation than import() that can take an arbitrary expression. Unfortunately due to JS spec we cannot prevent import() from resolving something that was already loaded by static import.

CC: @vdeturckheim per https://docs.google.com/document/d/1tS80LmzraV4yXEYIQ-fWaCm2MIpXfbtrSYjsI8kWKko/edit#heading=h.ifctpvwuev0a

[bmeck]

@targos it states that import() and import share the cache and deterministically share the result (this enforced by V8 source text module local cache, not the global cache which node controls). If one succeeds, the other must also succeed in perpetuity for the same source text and cache key (URL + import assertions these days [may grow]). So if you do import "fs"; dynamic import feeds through to FinishDynamicImport which feeds into the same mechanism as static imports (HostResolveImportedModule) which has the restriction: "Each time this operation is called with a specific referencingScriptOrModule, specifier pair as arguments it must return the same Module Record instance if it completes normally."

[targos]

Doesn't "if it completes normally" allow us to throw/reject the promise?

[bmeck]

@targos that just means after the first success it always succeeds, so you can only error out if it didn't succeed before.

[guybedford]

Reposting - Whether it is static or dynamic, could this not be handled via standard policy rules for allowed imports?

currently policy doesn't distinguish dynamic vs static. we could punt it to policies but even then spec doesn't let us completely prevent https: import from dynamic import if it was already loaded via static import.

@bmeck what I mean is that rather than specially handling dynamic imports as a policy, that the policy of just restricting which packages can import which modules can work equally well for static and dynamic imports.

[bmeck]

i think that is fine, just trying to be sure while i was reviewing the doc above

[sosoba]

IMHO this feature should be provided by --loader plugin according to the requirements of the situation. See https://nodejs.org/dist/latest-v16.x/docs/api/esm.html#esm_https_loader .

Therefore, it is more important to work on loaders API stability which will allow to loading modules from anywhere (e.g. from tgz file etc.) and however (on the fly, with file cache etc.).

[JakobJingleheimer]

There's quite a lot of rationale behind supporting this in core. It is possible to do with a loader, but that would likely result in divergent behaviour, security vulnerabilities, etc.