aspect-build/rules_js

ESM imports escape the sandbox & runfiles

gregmagolan opened this issue · 16 comments

gregmagolan commented

The relevent code in node is in resolve.js where there is a realPathSync call

https://github.com/nodejs/node/blob/4c5b96b376aa778cbe651362c29a26e0d4c32ccd/lib/internal/modules/esm/resolve.js#L310

This realPathSycn call does not get the fs monkey paches while the cjs require loader's realPathSync call does. It is unclear why.

This is the underlying reason why we current need --preserve-symlinks-main on by default in js_library so the .mjs entry points don't escape their runfiles.

It is also the reason mocha was observed to escape the sandbox in the repro #353 (comment) and likely related to #347.

This affects .mjs entry points and programs that use esm imports. For example, mocha uses the import() built-in that is affected.

gregmagolan commented

We suspect the underlying reason why the fs patches apply to the commonjs path and not the esm path in node is the fs import style in https://github.com/nodejs/node/blob/4c5b96b376aa778cbe651362c29a26e0d4c32ccd/lib/internal/modules/esm/resolve.js

const {
  realpathSync,
  statSync,
  Stats,
} = require('fs');

while the commonjs import is

const fs = require('fs');

Presumably both modules are evaluated before our fs patches, which are done in a --require from the command line. Our patches would apply only to the commonjs path since it has a reference to the fs module which we monkey patch while the esm path has references to the fs functions it needs directly which are not monkey patched.

  • 👍1
gregmagolan commented

I tested the changed needed in Node.js to resolve this and it works as expected. The change is just aspect-forks/node@6616ddd.

gregmagolan commented

Best known solution seems like maintaining a fork of node and updating the toolchain to use it.

mkg20001 commented

you can use experimental-loaders to insert the monkey patch as a global prelude, etc https://nodejs.org/api/esm.html#globalpreload

this would run before anything else (note that experimental loaders is in another thread, you need globalPrelude to run stuff on the main thread)

you could also see if you can just use experimental-loaders custom resolve function instead of patching fs (it works starting from node 12, with just slight api name changes)

edit: there's an option like --require but for esm, but it's wip/not yet added

gregmagolan commented

https://nodejs.org/api/module.html#modulesyncbuiltinesmexports

Does this fix the issue?

It doesn't. This is being called already after the monkey patches are made,

rules_js/js/private/node-patches/register.js

Line 50 in c95dda4

module.syncBuiltinESMExports()
. I don't remember the context anymore however. @jbedard was working on that one.

mkg20001 commented

I think that patching fs prob won't affect ESM loading as it happens in a seperate thread and you should use experimental-loader resolve in that case (or try the fs patch in a file loaded via --experimental-loader)

gregmagolan commented

Thanks for the suggestion @mkg20001 . I'll spend a bit of time today looking into that.

gregmagolan commented

@alexeagle For reference, there were a few attempts to make the ESM loader's fs calls monkey patchable but they were rejected by the node maintainers:

nodejs/node#39711: module: allow monkey-patching internal fs binding
nodejs/node#39513: module: use monkey-patchable readFileSync

Other (possibly) related PRs & issues:

nodejs/node#41076: Research the feasibility to providing a VFS extension mechanism to fs, require(), child_process, etc

gregmagolan commented

@mkg20001 I tested monkey patching the fs module in a globalPreload hook via --experimental-loaders with no luck. The require('fs') called in https://github.com/nodejs/node/blob/4c5b96b376aa778cbe651362c29a26e0d4c32ccd/lib/internal/modules/esm/resolve.js#L30 appears to still happen before the globalPreload so the patch of realPathSync doesn't take effect there.

mkg20001 commented
Aghassi commented

The custom resolve function on a loader sounds promising if it works and prevents nodejs forking which would be not ideal if it can be avoided.

matthewjh commented

@gregmagolan did you have any further luck with the loader approach?

I am currently debugging some sources of sandbox escape with Node.js v18, and this is with the patches to the ESM resolver.js. As you well know, it is tricky business. There are some places that use internalModuleStat rather than fs.statSync, which is not patchable. I noticed in Node v21 this API is now used instead of fs.statSync in the esm loader files, so it appears this problem is likely to get worse rather than better:

https://github.com/nodejs/node/blob/v21.0.0/lib/internal/modules/esm/resolve.js

Luckily, within the context of build and test actions, --experimental_use_hermetic_linux_sandbox seems to be extremely effective at ensuring hermetic JS execution thanks to using hardlinks (so no more symlinks to follow) and chroot (so no walking parent dirs up to the root looking for competing node_modules dirs). Reflecting on this, I do wonder whether aspects of this approach could be emulated to make bazel run of js_binary targets more robust. My main concern here is developers running tools and binaries locally - not deployed binaries, where we should be able to rely on the FS not being polluted, e.g. because they run in docker containers via js_image_layer. Obviously, it is annoying when a developer misses a dep and, rather than manifesting in a "module not found" error, there are subtle bugs due to node loading separate instances of the same module because one was resolved inside runfiles, and one under the bindir (etc.).

I wonder whether this "developer running a js_binary through bazel run" case could be handled specifically by a wrapper script crawling the runfiles dir for all the node_module and application files pertinent to the binary (as rules_js would own this code, we can be sure to stay within the runfiles dir), and then creating a mirroring hardlink tree under /tmp with the node_modules and application files, and then launching node within this /tmp hardlink tree. This, like I say, would apply the benefits seen with the hardlink sandbox to bazel run for js_binary. There is clearly some overlap here with js_devserver.

What do you think?

matthewjh commented

Other things to look at:

  1. Whether the fs guards could better handle Node.js "misbehaviour" here and redirect sandbox escapes back within the sandbox. I think I noticed realPathSync(path) returning path without modification if path is already outside of runfiles. In this case, returning the path inside runfiles that links onto path might be better, as then subsequent fs calls that base themselves on path will fall into the expected pathways.

  2. The CJS loader uses this Module._nodeModulePaths function to get all possible node_modules directories from the current location up to the root...
    e.g.

/my/bazel/path/execroot/bin/mybin.runfiles/app/node_modules
/my/bazel/path/execroot/bin/mybin.runfiles/node_modules
/my/bazel/path/execroot/bin/node_modules
/my/bazel/path/execroot/node_modules
/my/bazel/path/node_modules
/my/bazel/node_modules
/my/node_modules
/node_modules

These then feed into downstream operations, some of which use internalModuleStat. This is not good and I'm pretty sure accounts for some of the escapes I'm seeing.

Luckily, it looks like Module._nodeModulePaths is patchable, meaning rules_js could conceivably drop all roots outside runfiles and hide these from Node.js:

https://github.com/ilearnio/module-alias/blob/dev/index.js#L17

gregmagolan commented

I haven't looked further into the loader approach since the spike a while ago.

I came across this proposal however that looks interesting and could open a path for a solution: nodejs/node#52219. Haven't had time yet to look in detail but at face value it sounds promising.