Skip to content

Commit

Permalink
fix(builtin): don't allow symlinks to escape or enter bazel managed n…
Browse files Browse the repository at this point in the history
…ode_module folders
  • Loading branch information
gregmagolan committed Apr 9, 2020
1 parent 5c2f6c1 commit 10bd951
Show file tree
Hide file tree
Showing 12 changed files with 105 additions and 32 deletions.
16 changes: 16 additions & 0 deletions internal/node/launcher.sh
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,22 @@ fi
# Bazel always sets the PWD to execroot/my_wksp so we go up one directory.
export BAZEL_PATCH_ROOT=$(dirname $PWD)

# Set all bazel managed node_modules directories as guarded so no symlinks may
# escape and no symlinks may enter
if [ "$(basename ${BAZEL_PATCH_ROOT})" == "execroot" ]; then
# We are in execroot, linker node_modules is in the PWD
export BAZEL_PATCH_GUARDS="${PWD}/node_modules"
else
# We in runfiles, linker node_modules is at the runfiles root
export BAZEL_PATCH_GUARDS="${BAZEL_PATCH_ROOT}/node_modules"
fi
if [[ -n "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then
# If BAZEL_NODE_MODULES_ROOT is set, add it to the list of bazel patch guards
# Also, add the external/${BAZEL_NODE_MODULES_ROOT} which is the correct path under execroot
# and under runfiles it is the legacy external runfiles path
export BAZEL_PATCH_GUARDS="${BAZEL_PATCH_GUARDS},${BAZEL_PATCH_ROOT}/${BAZEL_NODE_MODULES_ROOT},${PWD}/external/${BAZEL_NODE_MODULES_ROOT}"
fi

# The EXPECTED_EXIT_CODE lets us write bazel tests which assert that
# a binary fails to run. Otherwise any failure would make such a test
# fail before we could assert that we expected that failure.
Expand Down
15 changes: 11 additions & 4 deletions internal/node/node.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ def _compute_node_modules_root(ctx):
] if f])
return node_modules_root

def _write_require_patch_script(ctx):
def _write_require_patch_script(ctx, node_modules_root):
# Generates the JavaScript snippet of module roots mappings, with each entry
# in the form:
# {module_name: /^mod_name\b/, module_root: 'path/to/mod_name'}
Expand All @@ -91,8 +91,6 @@ def _write_require_patch_script(ctx):
mapping = "{module_name: /^%s\\b/, module_root: '%s'}" % (escaped, mr)
module_mappings.append(mapping)

node_modules_root = _compute_node_modules_root(ctx)

ctx.actions.expand_template(
template = ctx.file._require_patch_template,
output = ctx.outputs.require_patch_script,
Expand Down Expand Up @@ -175,7 +173,9 @@ def _nodejs_binary_impl(ctx):
sources_depsets.append(d.files)
sources = depset(transitive = sources_depsets)

_write_require_patch_script(ctx)
node_modules_root = _compute_node_modules_root(ctx)

_write_require_patch_script(ctx, node_modules_root)
_write_loader_script(ctx)

# Provide the target name as an environment variable avaiable to all actions for the
Expand All @@ -190,6 +190,13 @@ def _nodejs_binary_impl(ctx):
# runfiles helpers to use.
env_vars += "export BAZEL_WORKSPACE=%s\n" % ctx.workspace_name

# if BAZEL_NODE_MODULES_ROOT has not already been set by
# run_node, then set it to the computed value
env_vars += """if [[ -z "${BAZEL_NODE_MODULES_ROOT:-}" ]]; then
export BAZEL_NODE_MODULES_ROOT=%s
fi
""" % node_modules_root

for k in ctx.attr.configuration_env_vars + ctx.attr.default_env_vars:
# Check ctx.var first & if env var not in there then check
# ctx.configuration.default_shell_env. The former will contain values from --define=FOO=BAR
Expand Down
27 changes: 21 additions & 6 deletions internal/node/node_patches.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,10 @@ Object.defineProperty(exports, "__esModule", { value: true });
// es modules

// tslint:disable-next-line:no-any
exports.patcher = (fs = fs$1, root) => {
exports.patcher = (fs = fs$1, root, guards) => {
fs = fs || fs$1;
root = root || '';
guards = guards || [];
if (!root) {
if (process.env.VERBOSE_LOGS) {
console.error('fs patcher called without root path ' + __filename);
Expand All @@ -83,7 +84,7 @@ exports.patcher = (fs = fs$1, root) => {
const origReadlinkSync = fs.readlinkSync.bind(fs);
const origReaddir = fs.readdir.bind(fs);
const origReaddirSync = fs.readdirSync.bind(fs);
const { isEscape, isOutPath } = exports.escapeFunction(root);
const { isEscape, isOutPath } = exports.escapeFunction(root, guards);
// tslint:disable-next-line:no-any
fs.lstat = (...args) => {
let cb = args.length > 1 ? args[args.length - 1] : undefined;
Expand Down Expand Up @@ -483,23 +484,36 @@ exports.patcher = (fs = fs$1, root) => {
}
}
};
exports.escapeFunction = (root) => {
// ensure root is always absolute.
exports.escapeFunction = (root, guards) => {
// ensure root & guards are always absolute.
root = path.resolve(root);
guards = [...guards].map(g => path.resolve(g));
function isEscape(linkTarget, linkPath) {
if (!path.isAbsolute(linkPath)) {
linkPath = path.resolve(linkPath);
}
if (!path.isAbsolute(linkTarget)) {
linkTarget = path.resolve(linkTarget);
}
if (isGuardPath(linkPath) || isGuardPath(linkTarget)) {
// don't escape out of guard paths and don't symlink into guard paths
return true;
}
if (root) {
if (isOutPath(linkTarget) && !isOutPath(linkPath)) {
// don't escape out of the root
return true;
}
}
return false;
}
function isGuardPath(str) {
for (const g of guards) {
if (str === g || str.startsWith(g + path.sep))
return true;
}
return false;
}
function isOutPath(str) {
return !root || (!str.startsWith(root + path.sep) && str !== root);
}
Expand Down Expand Up @@ -644,12 +658,13 @@ var src_2 = src.subprocess;
*/

// todo auto detect bazel env vars instead of adding a new one.
const { BAZEL_PATCH_ROOT, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS } = process.env;
const { BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS } = process.env;
if (BAZEL_PATCH_ROOT) {
const guards = BAZEL_PATCH_GUARDS ? BAZEL_PATCH_GUARDS.split(',') : [];
if (VERBOSE_LOGS)
console.error(`bazel node patches enabled. root: ${BAZEL_PATCH_ROOT} symlinks in this directory will not escape`);
const fs = fs$1;
src.fs(fs, BAZEL_PATCH_ROOT);
src.fs(fs, BAZEL_PATCH_ROOT, guards);
}
else if (VERBOSE_LOGS) {
console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOT`);
Expand Down
19 changes: 19 additions & 0 deletions internal/providers/node_runtime_deps_info.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"""Custom provider that mimics the Runfiles, but doesn't incur the expense of creating the runfiles symlink tree"""

load("//internal/linker:link_node_modules.bzl", "add_arg", "write_node_modules_manifest")
load("//internal/providers:npm_package_info.bzl", "NpmPackageInfo")

NodeRuntimeDepsInfo = provider(
doc = """Stores runtime dependencies of a nodejs_binary or nodejs_test
Expand All @@ -38,6 +39,23 @@ do the same.
},
)

def _compute_node_modules_root(ctx):
"""Computes the node_modules root (if any) from data & deps targets."""
node_modules_root = ""
deps = []
if hasattr(ctx.attr, "data"):
deps += ctx.attr.data
if hasattr(ctx.attr, "deps"):
deps += ctx.attr.deps
for d in deps:
if NpmPackageInfo in d:
possible_root = "/".join([d[NpmPackageInfo].workspace, "node_modules"])
if not node_modules_root:
node_modules_root = possible_root
elif node_modules_root != possible_root:
fail("All npm dependencies need to come from a single workspace. Found '%s' and '%s'." % (node_modules_root, possible_root))
return node_modules_root

def run_node(ctx, inputs, arguments, executable, **kwargs):
"""Helper to replace ctx.actions.run
This calls node programs with a node_modules directory in place"""
Expand Down Expand Up @@ -77,6 +95,7 @@ def run_node(ctx, inputs, arguments, executable, **kwargs):
env[var] = ctx.var[var]
elif var in ctx.configuration.default_shell_env.keys():
env[var] = ctx.configuration.default_shell_env[var]
env["BAZEL_NODE_MODULES_ROOT"] = _compute_node_modules_root(ctx)

ctx.actions.run(
inputs = inputs + extra_inputs,
Expand Down
5 changes: 3 additions & 2 deletions packages/node-patches/register.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,15 @@
*/
const patcher = require('./src');
// todo auto detect bazel env vars instead of adding a new one.
const {BAZEL_PATCH_ROOT, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS} = process.env;
const {BAZEL_PATCH_ROOT, BAZEL_PATCH_GUARDS, NP_SUBPROCESS_BIN_DIR, VERBOSE_LOGS} = process.env;

if (BAZEL_PATCH_ROOT) {
const guards = BAZEL_PATCH_GUARDS ? BAZEL_PATCH_GUARDS.split(',') : [];
if (VERBOSE_LOGS)
console.error(`bazel node patches enabled. root: ${
BAZEL_PATCH_ROOT} symlinks in this directory will not escape`);
const fs = require('fs');
patcher.fs(fs, BAZEL_PATCH_ROOT);
patcher.fs(fs, BAZEL_PATCH_ROOT, guards);
} else if (VERBOSE_LOGS) {
console.error(`bazel node patches disabled. set environment BAZEL_PATCH_ROOT`);
}
Expand Down
23 changes: 19 additions & 4 deletions packages/node-patches/src/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,10 @@ type Dirent = any;
const _fs = require('fs');

// tslint:disable-next-line:no-any
export const patcher = (fs: any = _fs, root: string) => {
export const patcher = (fs: any = _fs, root: string, guards: string[]) => {
fs = fs || _fs;
root = root || '';
guards = guards || [];
if (!root) {
if (process.env.VERBOSE_LOGS) {
console.error('fs patcher called without root path ' + __filename);
Expand All @@ -54,7 +55,7 @@ export const patcher = (fs: any = _fs, root: string) => {
const origReaddir = fs.readdir.bind(fs);
const origReaddirSync = fs.readdirSync.bind(fs);

const {isEscape, isOutPath} = escapeFunction(root);
const {isEscape, isOutPath} = escapeFunction(root, guards);

const logged: {[k: string]: boolean} = {};

Expand Down Expand Up @@ -471,9 +472,10 @@ export const patcher = (fs: any = _fs, root: string) => {
}
};

export const escapeFunction = (root: string) => {
// ensure root is always absolute.
export const escapeFunction = (root: string, guards: string[]) => {
// ensure root & guards are always absolute.
root = path.resolve(root);
guards = [...guards].map(g => path.resolve(g));
function isEscape(linkTarget: string, linkPath: string) {
if (!path.isAbsolute(linkPath)) {
linkPath = path.resolve(linkPath);
Expand All @@ -483,14 +485,27 @@ export const escapeFunction = (root: string) => {
linkTarget = path.resolve(linkTarget);
}

if (isGuardPath(linkPath) || isGuardPath(linkTarget)) {
// don't escape out of guard paths and don't symlink into guard paths
return true;
}

if (root) {
if (isOutPath(linkTarget) && !isOutPath(linkPath)) {
// don't escape out of the root
return true;
}
}
return false;
}

function isGuardPath(str) {
for (const g of guards) {
if (str === g || str.startsWith(g + path.sep)) return true;
}
return false;
}

function isOutPath(str: string) {
return !root || (!str.startsWith(root + path.sep) && str !== root);
}
Expand Down
6 changes: 3 additions & 3 deletions packages/node-patches/test/fs/escape.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {escapeFunction} from '../../src/fs';
describe('escape function', () => {
it('isOutPath is correct', () => {
const root = '/a/b';
const {isOutPath} = escapeFunction(root);
const {isOutPath} = escapeFunction(root, []);

assert.ok(isOutPath('/a'));
assert.ok(isOutPath('/a/c/b'));
Expand All @@ -32,7 +32,7 @@ describe('escape function', () => {

it('isEscape is correct', () => {
const root = '/a/b';
const {isEscape} = escapeFunction(root);
const {isEscape} = escapeFunction(root, []);

assert.ok(isEscape('/a/c/boop', '/a/b/l'));
assert.ok(isEscape('/a/c/boop', '/a/b'));
Expand All @@ -43,7 +43,7 @@ describe('escape function', () => {

it('isEscape handles relative paths', () => {
const root = './a/b';
const {isEscape} = escapeFunction(root);
const {isEscape} = escapeFunction(root, []);

assert.ok(isEscape('./a/c/boop', './a/b/l'));
assert.ok(isEscape('./a/c/boop', './a/b'));
Expand Down
6 changes: 3 additions & 3 deletions packages/node-patches/test/fs/lstat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('testing lstat', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, path.join(fixturesDir));
patcher(patchedFs, path.join(fixturesDir), []);

const linkPath = path.join(fixturesDir, 'a', 'link');
assert.ok(
Expand Down Expand Up @@ -68,7 +68,7 @@ describe('testing lstat', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, path.join(fixturesDir, 'a'));
patcher(patchedFs, path.join(fixturesDir, 'a'), []);

const linkPath = path.join(fixturesDir, 'a', 'link');

Expand Down Expand Up @@ -114,7 +114,7 @@ describe('testing lstat', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, path.join(fixturesDir, 'a'));
patcher(patchedFs, path.join(fixturesDir, 'a'), []);

const linkPath = path.join(fixturesDir, 'b', 'link');

Expand Down
8 changes: 4 additions & 4 deletions packages/node-patches/test/fs/opendir.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ describe('testing opendir', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, fixturesDir);
patcher(patchedFs, fixturesDir, []);
(patchedFs as any).DEBUG = true;

let dir;
Expand Down Expand Up @@ -74,7 +74,7 @@ describe('testing opendir', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, path.join(fixturesDir, 'a'));
patcher(patchedFs, path.join(fixturesDir, 'a'), []);
(patchedFs as any).DEBUG = true;

let dir;
Expand Down Expand Up @@ -109,7 +109,7 @@ describe('testing opendir', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, path.join(fixturesDir));
patcher(patchedFs, path.join(fixturesDir), []);
(patchedFs as any).DEBUG = true;

const dir = await util.promisify(patchedFs.opendir)(path.join(fixturesDir, 'a'));
Expand Down Expand Up @@ -140,7 +140,7 @@ describe('testing opendir', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, path.join(fixturesDir, 'a'));
patcher(patchedFs, path.join(fixturesDir, 'a'), []);
(patchedFs as any).DEBUG = true;

const dir = await util.promisify(patchedFs.opendir)(path.join(fixturesDir, 'a'));
Expand Down
4 changes: 2 additions & 2 deletions packages/node-patches/test/fs/readdir.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('testing readdir', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, fixturesDir);
patcher(patchedFs, fixturesDir, []);

let dirents = patchedFs.readdirSync(path.join(fixturesDir, 'a'), {
withFileTypes: true,
Expand Down Expand Up @@ -77,7 +77,7 @@ describe('testing readdir', () => {

const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);
patcher(patchedFs, path.join(fixturesDir, 'a'));
patcher(patchedFs, path.join(fixturesDir, 'a'), []);

let dirents = patchedFs.readdirSync(path.join(fixturesDir, 'a'), {
withFileTypes: true,
Expand Down
4 changes: 2 additions & 2 deletions packages/node-patches/test/fs/readlink.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('testing readlink', () => {
const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);

patcher(patchedFs, path.join(fixturesDir));
patcher(patchedFs, path.join(fixturesDir), []);
const linkPath = path.join(fixturesDir, 'a', 'link');

assert.deepStrictEqual(
Expand Down Expand Up @@ -70,7 +70,7 @@ describe('testing readlink', () => {
const patchedFs = Object.assign({}, fs);
patchedFs.promises = Object.assign({}, fs.promises);

patcher(patchedFs, path.join(fixturesDir, 'a'));
patcher(patchedFs, path.join(fixturesDir, 'a'), []);
const linkPath = path.join(fs.realpathSync(fixturesDir), 'a', 'link');

assert.throws(() => {
Expand Down
Loading

0 comments on commit 10bd951

Please sign in to comment.