Skip to content

Commit c7d385d

Browse files
committed
permission: clamp worker grants to parent when execArgv is set
Address review feedback on the previous JS approach: - Remove any userland execArgv copying (incomplete: NODE_OPTIONS, repeated allow flags, false sense of inheritance). - After Worker option parse in node_worker.cc, if the parent has the Permission Model enabled, intersect permission-related grants so the worker cannot exceed the parent (boolean allows AND path allowlists). - Non-permission execArgv differences remain possible. This may be semver-major relative to treating worker permission non-inheritance as permanent API surface; flagged for reviewer judgment. Signed-off-by: yunshingng <yunshingng25@gmail.com>
1 parent b2b2b41 commit c7d385d

2 files changed

Lines changed: 150 additions & 0 deletions

File tree

src/node_worker.cc

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -504,6 +504,71 @@ Worker::~Worker() {
504504
Debug(this, "Worker %llu destroyed", thread_id_.id);
505505
}
506506

507+
508+
// When the parent runs with the Permission Model enabled, an explicit Worker
509+
// execArgv (including []) must not yield a wider grant set than the parent.
510+
// Non-permission CLI differences remain allowed; only permission-related
511+
// options are intersected with the parent after parse.
512+
static bool PathAllowedByParent(const std::vector<std::string>& parent_paths,
513+
const std::string& child) {
514+
if (parent_paths.empty()) {
515+
return false;
516+
}
517+
for (const std::string& p : parent_paths) {
518+
if (p == "*" || p == child) {
519+
return true;
520+
}
521+
if (!p.empty() && child.size() >= p.size() &&
522+
child.compare(0, p.size(), p) == 0) {
523+
return true;
524+
}
525+
}
526+
return false;
527+
}
528+
529+
static void IntersectPathList(std::vector<std::string>* worker,
530+
const std::vector<std::string>& parent) {
531+
if (worker->empty()) {
532+
return;
533+
}
534+
std::vector<std::string> out;
535+
for (const std::string& w : *worker) {
536+
if (PathAllowedByParent(parent, w)) {
537+
out.push_back(w);
538+
}
539+
}
540+
*worker = std::move(out);
541+
}
542+
543+
static void ClampWorkerPermissionToParent(Environment* env,
544+
PerIsolateOptions* worker_opts) {
545+
if (!env->permission()->enabled() || worker_opts == nullptr) {
546+
return;
547+
}
548+
549+
EnvironmentOptions* parent =
550+
env->isolate_data()->options()->get_per_env_options();
551+
EnvironmentOptions* w = worker_opts->get_per_env_options();
552+
553+
// Parent is under the Permission Model → worker stays under it.
554+
w->permission = true;
555+
556+
w->allow_addons = w->allow_addons && parent->allow_addons;
557+
w->allow_inspector = w->allow_inspector && parent->allow_inspector;
558+
w->allow_child_process =
559+
w->allow_child_process && parent->allow_child_process;
560+
w->allow_net = w->allow_net && parent->allow_net;
561+
w->allow_wasi = w->allow_wasi && parent->allow_wasi;
562+
w->allow_ffi = w->allow_ffi && parent->allow_ffi;
563+
w->allow_openssl_store =
564+
w->allow_openssl_store && parent->allow_openssl_store;
565+
w->allow_worker_threads =
566+
w->allow_worker_threads && parent->allow_worker_threads;
567+
568+
IntersectPathList(&w->allow_fs_read, parent->allow_fs_read);
569+
IntersectPathList(&w->allow_fs_write, parent->allow_fs_write);
570+
}
571+
507572
void Worker::New(const FunctionCallbackInfo<Value>& args) {
508573
Environment* env = Environment::GetCurrent(args);
509574
THROW_IF_INSUFFICIENT_PERMISSIONS(
@@ -688,6 +753,11 @@ void Worker::New(const FunctionCallbackInfo<Value>& args) {
688753
// essential to load user codes and must not be blocked by the inspector
689754
// for internal scripts.
690755
// Still, `--inspect-node` can break on the first line of internal scripts.
756+
757+
if (env->permission()->enabled() && per_isolate_opts) {
758+
ClampWorkerPermissionToParent(env, per_isolate_opts.get());
759+
}
760+
691761
if (is_internal) {
692762
per_isolate_opts->per_env->get_debug_options()
693763
->DisableWaitOrBreakFirstLine();
Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
'use strict';
2+
3+
// Under --permission, explicit Worker execArgv (including []) must not produce
4+
// a wider grant set than the parent (C++ clamp / intersection).
5+
6+
const common = require('../common');
7+
const { isMainThread } = require('worker_threads');
8+
9+
if (!isMainThread) {
10+
common.skip('This test only works on a main thread');
11+
}
12+
13+
const assert = require('assert');
14+
const fs = require('fs');
15+
const path = require('path');
16+
const { spawnSync } = require('child_process');
17+
const tmpdir = require('../common/tmpdir');
18+
19+
tmpdir.refresh();
20+
21+
const allowed = tmpdir.path;
22+
const deniedFile = path.join(tmpdir.path, '..', 'permission-worker-denied-file');
23+
fs.writeFileSync(deniedFile, 'secret\n');
24+
25+
const workerSource = `
26+
const { parentPort } = require('worker_threads');
27+
const fs = require('fs');
28+
const denied = ${JSON.stringify(deniedFile)};
29+
let result;
30+
try {
31+
result = { ok: true, data: fs.readFileSync(denied, 'utf8') };
32+
} catch (err) {
33+
result = { ok: false, code: err.code, message: err.message };
34+
}
35+
parentPort.postMessage(result);
36+
`;
37+
38+
function runCase(label, useEmptyExecArgv) {
39+
const execArgvLine = useEmptyExecArgv ? 'execArgv: [],' : '';
40+
const code = `
41+
const { Worker } = require('worker_threads');
42+
const w = new Worker(${JSON.stringify(workerSource)}, {
43+
eval: true,
44+
${execArgvLine}
45+
});
46+
w.on('message', (msg) => {
47+
process.stdout.write(JSON.stringify({ label: ${JSON.stringify(label)}, msg }) + '\\n');
48+
process.exit(0);
49+
});
50+
w.on('error', (err) => {
51+
console.error(err);
52+
process.exit(1);
53+
});
54+
`;
55+
return spawnSync(
56+
process.execPath,
57+
[
58+
'--permission',
59+
`--allow-fs-read=${allowed}`,
60+
'--allow-worker',
61+
'-e',
62+
code,
63+
],
64+
{ encoding: 'utf8', timeout: 15000 },
65+
);
66+
}
67+
68+
const defaultWorker = runCase('default', false);
69+
const emptyExecArgv = runCase('empty-execArgv', true);
70+
71+
assert.strictEqual(defaultWorker.status, 0, defaultWorker.stderr);
72+
assert.strictEqual(emptyExecArgv.status, 0, emptyExecArgv.stderr);
73+
74+
const defaultMsg = JSON.parse(defaultWorker.stdout.trim().split('\n').pop());
75+
const emptyMsg = JSON.parse(emptyExecArgv.stdout.trim().split('\n').pop());
76+
77+
assert.strictEqual(defaultMsg.msg.ok, false, JSON.stringify(defaultMsg));
78+
assert.strictEqual(defaultMsg.msg.code, 'ERR_ACCESS_DENIED');
79+
assert.strictEqual(emptyMsg.msg.ok, false, JSON.stringify(emptyMsg));
80+
assert.strictEqual(emptyMsg.msg.code, 'ERR_ACCESS_DENIED');

0 commit comments

Comments
 (0)