Support for file-system based persistent code cache in user-land module loaders

[joyeecheung]

This stemmed from a Twitter thread. Specifically I am wondering if there are any concerns over having something similar to what https://github.com/zertosh/v8-compile-cache does in core, the general idea is:

1. If the user enables this feature (probably should be off by default) e.g. via an environment variable, whenever we compile a module, we produce the code cache for the module, and on process exit, we store any new cache produced in a cache directory on the file system.
2. The next time the process is launched (with this feature enabled again), whenever we are loading a module, we attempt to load the cache from that directory and use it when compiling the module, in order to speed up the start up (where most of the time is usually spent on compilation).

This is also similar to what Chrome does with the V8 code cache.

The motivation for implementing this in core is that, for a user-land module to do this for CJS, it has to monkey patch the CJS loader, and this increases the compatibility burden (v8-compile-cache has 17M weekly downloads, and it needs to monkey-patch Module.prototype._compile to work. From a glance of its issue tracker it seems some of the issues are not really fixable in the user land either, like piping into the internal source maps cache). For ESM currently the user land can only use --loader to customize the compilation, which has a cost of its own (especially when we move it to a separate thread), creating a disparity from CJS, and also even with --loader I doubt if user-land code can integrate into e.g. the source map cache without asking us to expose too much internals.

The most risky part of this feature might be the growth of the cache, but it seems manageable if:

1. The feature is opt-in (via an Environment variable, for example, or a method that can be called to enable/disable from user land).
2. We do some checks for the size of the cache directory when this feature is used, and set a default cache size limit to prevent unbound growth.

This doesn't seem too radical, for example we already persist something like the repl history by default, and we also have features like NODE_V8_COVERAGE that does a similar "writing a lot of data to a directory when enabled" thing. I don't think this would increase the code complexity much either (we might also want a read-only version of this for SEA in the future too). So opening this issue to see if there are any concerns about having this in core before implementing it.

[joyeecheung]

cc @nodejs/startup @nodejs/loaders

[jakebailey]

For completeness, there's also https://www.npmjs.com/package/v8-compile-cache-lib with another 9 million weekly downloads; this one is used by ts-node and others. @cspotcode

Given how many short lived node processes there are out there, having this sort of thing enabled globally feels like it could be a good idea (depending on the downsides).

[bnoordhuis]

Code cache corruption is an issue though. V8 only performs the lightest of sanity checks. Bad inputs will crash the process, or worse. It opens up new attack vectors.

[bmeck]

A singular DB instead of a file per cache entry would likely be better I would imagine. Avoids things like .pyc mismatches and can be used to add extra checks as a container format. Attack vector introduction is real but should likely be discussed since it might be simple to find an adequate mitigation even if it requires opt-in.

…

On Sat, Apr 8, 2023, 4:50 AM Ben Noordhuis ***@***.***> wrote: Code cache corruption is an issue though. V8 only performs the lightest of sanity checks. Bad inputs will crash the process, or worse. It opens up new attack vectors. — Reply to this email directly, view it on GitHub <#47472 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI5CVZNMONQDGCR32O3XAEYHFANCNFSM6AAAAAAWW757XE> . You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

[joyeecheung]

It opens up new attack vectors.

I can't think of anything new that's out of the scope in our threat model - to feed bad input to the module loader, the attacker needs to have access to the cache directory and corrupt the cache on-disk. But if the attacker has that level of access to the file system, the integrity about the actual source files already can't be trusted - unless policy is enabled, but in that case we could take policy into account in the implementation, whereas it'd be harder for any user-land solutions to do this, no matter how popular they already are. And this seems to be an even better motivation to provide this in core because the existing popular user-land solutions with ~25M weekly downloads already monkey patch Module.prototype._compile in a way that completely drops the policy assertions. We have already explicitly stated that we trust the file system when loading a module in our threat model, anyway. In addition we have features like NODE_EXTRA_CA_CERTS / SSL_CERT_DIR / NODE_REPL_EXTERNAL_MODULE / NODE_ICU_DATA etc., and they are probably much easier/straight-forward to exploit compared to code caches (even in those cases, exploits that depend on altering the inputs to those environment variables are already out of our security scope, because again we simply trust the file system).

If the feature is opt-in, the mitigation against any new-found venerability that actually is in our security scope (even though I think that'd be unlikely given the reasons stated above) would also be simple - the user can just stop using it (e.g. unset the environment variable), and we can quickly make a security release by making it a noop until the vulnerability is addressed, and it shouldn't result in behavioral regression - the regression would only be on the module loading performance.

[bnoordhuis]

One obvious angle is running node as setuid root, or otherwise running with elevated privileges (ex. capabilities on Linux.)

[joyeecheung]

@bnoordhuis Where is the threat coming from in those cases? How would this be different compared to e.g. NODE_EXTRA_CA_CERTS / SSL_CERT_DIR / NODE_REPL_EXTERNAL_MODULE / NODE_ICU_DATA?

[bnoordhuis]

We're being careful to ignore those environment variables when running as setuid root or a host of other things. That same caution should be applied when reading files from disk.

[joyeecheung]

We're being careful to ignore those environment variables when running as setuid root or a host of other things. That same caution should be applied when reading files from disk.

Yes I agree though I don't see this as a new threat - I think whatever we need is probably already covered by SafeGetEnv and if there's something missing, we should fix SafeGetEnv for all these variables, and I doubt environment variables for on-disk code cache should be treated any differently compared to these sensitive variables in this regard.

P.S.: we don't use SafeGetEnv for all the variables I mentioned above...maybe we should...

P.P.S.: On the other hand we have this popular package with ~25M weekly downloads in the ecosystem that does the something similar without taking elevated privileges into account...I would say implementing it in core could probably help improving the situation with the potential threat in the ecosystem

P.P.P.S.: we should probably consider exposing safeGetEnv to user-land for this purpose.

[bnoordhuis]

I don't see this as a new threat

The difference is that right now we only look at environment variables that are (from an external attacker's perspective) effectively immutable. Once you start loading files as a privileged process, you have to start worrying about things like symlink attacks.

I'm not saying it's impossible to make it work but it increases the attack surface considerably and it's subtle enough that it's easier to get wrong than right.

edit: and setuid root is just an example. I'm sure I can come up with half a dozen more attack vectors if I put my mind to it and security isn't even my primary line of work. A dedicated attacker should be able to come up with at least half a dozen more.

[github-actions]

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

[anonrig]

@joyeecheung Can we continue on this issue? I think it is extremely beneficial

[GeoffreyBooth]

It could maybe integrate with SQLite if we add that in #50169

[joyeecheung]

I wonder what’s the path forward. Perhaps we need to make a decision about whether the security implications are big enough a concern to implement this (I’d say however since the v8-compile-cache package is effectively used a lot in the wild, the security implications are already there in the ecosystem and implementing something like that in core would ideally make it easier to manage). Tagging it TSC-agenda.

[GeoffreyBooth]

@joyeecheung Could maybe a baby step be providing an API for userland to implement this without needing to monkey-patch the CommonJS loader? Either something along the lines of the module customization hooks (like register a file, or register a function to run as the hook) or a new API specifically for this. Like if we don’t yet know how we would implement the persist-to-disk part of this just yet, provide some kind of API where the user can provide functions for the “save to disk” and “load from disk” parts, and core handles the “load into VM” and “export from VM” parts. Even if we eventually figure out a solution for persistence within core, like #50169, having a way to customize it might still be useful so that users can implement things like a code cache shared across processes or even across servers.

[joyeecheung]

To me it seems like providing an user land API is actually a bigger chunk of design work, considering how complicated the loaders are, and also that a user land solution using the compilation hooks still need to work with other components that we do not expose (policy, permissions, etc.). The feature proposed in the OP is more of a black box, all that's exposed to users is just one magical environment variable, all the implementation details would be up to us so it involves less API design work.

Note that providing a hook does not waive the security concerns by the way, it just transfer the concerns to user-land packages, which does not even have access to our internal permission model, policy manifest, and environment variable safe guards, and therefore it would be harder for them to provide a more robust solution.

[GeoffreyBooth]

all that’s exposed to users is just one magical environment variable

Could it be as simple as --compilation-cache=./cache.json where the user provides a path to a file to use for saving and retrieving the cache? Then we can use traditional file read/write methods like v8-compile-cache.

[joyeecheung]

What would that json file look like though? If it's still a script path -> cache path mapping, it still doesn't remove the security concerns, just transfers all that to user land. This still requires an API to pass the cache out of Node.js core for the package to persist them (and there's no guarantee about the integrity of this cache). It doesn't sound like less work, and if our eventual goal is to provide this as a built-in feature so that it can be implemented robustly with e.g. the environment variable guards we have and in a more performant way, it's extra work that eventually lead to leaked internal steps that we have to maintain.

[GeoffreyBooth]

What would that json file look like though?

Or maybe it should be a path to a folder. I don’t know, it’s just a reference to wherever the data should be saved, in whatever format we would want to save it in. What I’m suggesting here is that it is a built-in feature; the user tells Node where to save the data, but everything else is handled by Node and not customizable. It would be like we put v8-compile-cache into core. And within core we would handle the security implications, like before loading a cached compilation into V8 we would check to see if that module was permitted via the policies, etc.

I’m not sure what you’re referring to re environment variables.

[joyeecheung]

Currently blocked on https://chromium-review.googlesource.com/c/v8/v8/+/5401780 to fix import() support. It has landed, need to wait for a couple of days to confirm it's not regressing anyone before backporting.

[joyeecheung]

Opened #52535

[GeoffreyBooth]

Fixed by #52535