Skip to content

Commit

Permalink
src,loader,permission: throw on InternalWorker use
Browse files Browse the repository at this point in the history
Previously this PR it was expected that InternalWorker
usage doesn't require the --allow-worker when the permission
model is enabled. This, however, exposes a vulnerability
whenever the instance gets accessed by the user. For example
through diagnostics_channel.subscribe('worker_threads')

PR-URL: nodejs-private/node-private#629
Refs: https://hackerone.com/reports/2575105
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Robert Nagy <[email protected]>
CVE-ID: CVE-2025-23083
  • Loading branch information
RafaelGSS committed Jan 21, 2025
1 parent 8306457 commit bf59539
Show file tree
Hide file tree
Showing 5 changed files with 68 additions and 8 deletions.
2 changes: 2 additions & 0 deletions doc/api/cli.md
Original file line number Diff line number Diff line change
Expand Up @@ -1061,6 +1061,8 @@ added:
Enable module mocking in the test runner.

This feature requires `--allow-worker` if used with the [Permission Model][].

### `--experimental-transform-types`

<!-- YAML
Expand Down
6 changes: 2 additions & 4 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
Expand Up @@ -495,11 +495,9 @@ Worker::~Worker() {

void Worker::New(const FunctionCallbackInfo<Value>& args) {
Environment* env = Environment::GetCurrent(args);
THROW_IF_INSUFFICIENT_PERMISSIONS(
env, permission::PermissionScope::kWorkerThreads, "");
bool is_internal = args[5]->IsTrue();
if (!is_internal) {
THROW_IF_INSUFFICIENT_PERMISSIONS(
env, permission::PermissionScope::kWorkerThreads, "");
}
Isolate* isolate = args.GetIsolate();

CHECK(args.IsConstructCall());
Expand Down
8 changes: 4 additions & 4 deletions test/es-module/test-esm-loader-hooks.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => {
});
});

it('should work without worker permission', async () => {
it('should not work without worker permission', async () => {
const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
'--no-warnings',
'--permission',
Expand All @@ -190,9 +190,9 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => {
fixtures.path('es-modules/esm-top-level-await.mjs'),
]);

assert.strictEqual(stderr, '');
assert.match(stdout, /^1\r?\n2\r?\n$/);
assert.strictEqual(code, 0);
assert.match(stderr, /Error: Access to this API has been restricted/);
assert.strictEqual(stdout, '');
assert.strictEqual(code, 1);
assert.strictEqual(signal, null);
});

Expand Down
19 changes: 19 additions & 0 deletions test/parallel/test-permission-dc-worker-threads.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// Flags: --experimental-permission --allow-fs-read=* --experimental-test-module-mocks

Check failure on line 1 in test/parallel/test-permission-dc-worker-threads.js

View workflow job for this annotation

GitHub Actions / test-macOS (macos-14)

--- stderr --- out/Release/node: bad option: --experimental-permission Command: out/Release/node --experimental-permission --allow-fs-read=* --experimental-test-module-mocks --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/runner/work/node/node/node/test/parallel/test-permission-dc-worker-threads.js

Check failure on line 1 in test/parallel/test-permission-dc-worker-threads.js

View workflow job for this annotation

GitHub Actions / test-linux

--- stderr --- out/Release/node: bad option: --experimental-permission Command: out/Release/node --experimental-permission --allow-fs-read=* --experimental-test-module-mocks --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/runner/work/node/node/node/test/parallel/test-permission-dc-worker-threads.js

Check failure on line 1 in test/parallel/test-permission-dc-worker-threads.js

View workflow job for this annotation

GitHub Actions / test-macOS (macos-13)

--- stderr --- out/Release/node: bad option: --experimental-permission Command: out/Release/node --experimental-permission --allow-fs-read=* --experimental-test-module-mocks --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/runner/work/node/node/node/test/parallel/test-permission-dc-worker-threads.js
'use strict';

const common = require('../common');
const assert = require('node:assert');

{
const diagnostics_channel = require('node:diagnostics_channel');
diagnostics_channel.subscribe('worker_threads', common.mustNotCall());
const { mock } = require('node:test');

// Module mocking should throw instead of posting to worker_threads dc
assert.throws(() => {
mock.module('node:path');
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'WorkerThreads',
}));
}
41 changes: 41 additions & 0 deletions test/parallel/test-runner-module-mocking.js
Original file line number Diff line number Diff line change
Expand Up @@ -650,3 +650,44 @@ test('wrong import syntax should throw error after module mocking', async () =>
assert.match(stderr, /Error \[ERR_MODULE_NOT_FOUND\]: Cannot find module/);
assert.strictEqual(code, 1);
});

test('should throw ERR_ACCESS_DENIED when permission model is enabled', async (t) => {
const cwd = fixtures.path('test-runner');
const fixture = fixtures.path('test-runner', 'mock-nm.js');
const args = [
'--experimental-permission',
'--allow-fs-read=*',
'--experimental-test-module-mocks',
fixture,
];
const {
code,
stdout,
} = await common.spawnPromisified(process.execPath, args, { cwd });

assert.strictEqual(code, 1);

Check failure on line 668 in test/parallel/test-runner-module-mocking.js

View workflow job for this annotation

GitHub Actions / test-macOS (macos-14)

--- stderr --- (node:92738) ExperimentalWarning: Module mocking is an experimental feature and might change at any time (Use `node --trace-warnings ...` to show where the warning was created) --- stdout --- Test failure: 'should throw ERR_ACCESS_DENIED when permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:654:1 AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 9 !== 1 at TestContext.<anonymous> (/Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:668:10) at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: true, code: 'ERR_ASSERTION', actual: 9, expected: 1, operator: 'strictEqual' } Test failure: 'should throw ERR_ACCESS_DENIED when permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:654:1 AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 9 !== 1 at TestContext.<anonymous> (/Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:668:10) at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: true, code: 'ERR_ASSERTION', actual: 9, expected: 1, operator: 'strictEqual' } Test failure: 'should work when --allow-worker is passed and permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:673:1 AssertionError [ERR_ASSERTION]: /Users/runner/work/node/node/node/out/Release/node: bad option: --experimental-permission 9 !== 0 at TestContext.<anonymous> (/Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:690:10) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: false, code: 'ERR_ASSERTION', actual: 9, expected: 0, operator: 'strictEqual' } Test failure: 'should work when --allow-worker is passed and permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:673:1 AssertionError [ERR_ASSERTION]: /Users/runner/work/node/node/node/out/Release/node: bad option: --experimental-permission 9 !== 0 at TestContext.<anonymous> (/Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:690:10) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: false, code: 'ERR_ASSERTION', actual: 9, expected: 0, operator: 'strictEqual' } Command: out/Release/node --experimental-test-module-mocks --experimental-require-module --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js

Check failure on line 668 in test/parallel/test-runner-module-mocking.js

View workflow job for this annotation

GitHub Actions / test-linux

--- stderr --- (node:172512) ExperimentalWarning: Module mocking is an experimental feature and might change at any time (Use `node --trace-warnings ...` to show where the warning was created) --- stdout --- Test failure: 'should throw ERR_ACCESS_DENIED when permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:654:1 AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 9 !== 1 at TestContext.<anonymous> (/home/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:668:10) at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: true, code: 'ERR_ASSERTION', actual: 9, expected: 1, operator: 'strictEqual' } Test failure: 'should throw ERR_ACCESS_DENIED when permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:654:1 AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 9 !== 1 at TestContext.<anonymous> (/home/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:668:10) at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: true, code: 'ERR_ASSERTION', actual: 9, expected: 1, operator: 'strictEqual' } Test failure: 'should work when --allow-worker is passed and permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:673:1 AssertionError [ERR_ASSERTION]: /home/runner/work/node/node/node/out/Release/node: bad option: --experimental-permission 9 !== 0 at TestContext.<anonymous> (/home/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:690:10) at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: false, code: 'ERR_ASSERTION', actual: 9, expected: 0, operator: 'strictEqual' } Test failure: 'should work when --allow-worker is passed and permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:673:1 AssertionError [ERR_ASSERTION]: /home/runner/work/node/node/node/out/Release/node: bad option: --experimental-permission 9 !== 0 at TestContext.<anonymous> (/home/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:690:10) at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: false, code: 'ERR_ASSERTION', actual: 9, expected: 0, operator: 'strictEqual' } Command: out/Release/node --experimental-test-module-mocks --experimental-require-module --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js

Check failure on line 668 in test/parallel/test-runner-module-mocking.js

View workflow job for this annotation

GitHub Actions / test-macOS (macos-13)

--- stderr --- (node:76431) ExperimentalWarning: Module mocking is an experimental feature and might change at any time (Use `node --trace-warnings ...` to show where the warning was created) --- stdout --- Test failure: 'should throw ERR_ACCESS_DENIED when permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:654:1 AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 9 !== 1 at TestContext.<anonymous> (/Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:668:10) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: true, code: 'ERR_ASSERTION', actual: 9, expected: 1, operator: 'strictEqual' } Test failure: 'should throw ERR_ACCESS_DENIED when permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:654:1 AssertionError [ERR_ASSERTION]: Expected values to be strictly equal: 9 !== 1 at TestContext.<anonymous> (/Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:668:10) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: true, code: 'ERR_ASSERTION', actual: 9, expected: 1, operator: 'strictEqual' } Test failure: 'should work when --allow-worker is passed and permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:673:1 AssertionError [ERR_ASSERTION]: /Users/runner/work/node/node/node/out/Release/node: bad option: --experimental-permission 9 !== 0 at TestContext.<anonymous> (/Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:690:10) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: false, code: 'ERR_ASSERTION', actual: 9, expected: 0, operator: 'strictEqual' } Test failure: 'should work when --allow-worker is passed and permission model is enabled' Location: test/parallel/test-runner-module-mocking.js:673:1 AssertionError [ERR_ASSERTION]: /Users/runner/work/node/node/node/out/Release/node: bad option: --experimental-permission 9 !== 0 at TestContext.<anonymous> (/Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js:690:10) at async Test.run (node:internal/test_runner/test:981:9) at async Test.processPendingSubtests (node:internal/test_runner/test:678:7) { generatedMessage: false, code: 'ERR_ASSERTION', actual: 9, expected: 0, operator: 'strictEqual' } Command: out/Release/node --experimental-test-module-mocks --experimental-require-module --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /Users/runner/work/node/node/node/test/parallel/test-runner-module-mocking.js
assert.match(stdout, /Error: Access to this API has been restricted/);
assert.match(stdout, /permission: 'WorkerThreads'/);
});

test('should work when --allow-worker is passed and permission model is enabled', async (t) => {
const cwd = fixtures.path('test-runner');
const fixture = fixtures.path('test-runner', 'mock-nm.js');
const args = [
'--experimental-permission',
'--allow-fs-read=*',
'--allow-worker',
'--experimental-test-module-mocks',
fixture,
];
const {
code,
stdout,
stderr,
signal,
} = await common.spawnPromisified(process.execPath, args, { cwd });

assert.strictEqual(code, 0, stderr);
assert.strictEqual(signal, null);
assert.match(stdout, /pass 1/, stderr);
});

0 comments on commit bf59539

Please sign in to comment.