|
| 1 | +From 389f239a282de04651cebdc99bc0af5d19aa955d Mon Sep 17 00:00:00 2001 |
| 2 | +From: RafaelGSS < [email protected]> |
| 3 | +Date: Tue, 27 Aug 2024 18:00:12 -0300 |
| 4 | +Subject: [PATCH] src,loader,permission: throw on InternalWorker use |
| 5 | + |
| 6 | +Previously this PR it was expected that InternalWorker |
| 7 | +usage doesn't require the --allow-worker when the permission |
| 8 | +model is enabled. This, however, exposes a vulnerability |
| 9 | +whenever the instance gets accessed by the user. For example |
| 10 | +through diagnostics_channel.subscribe('worker_threads') |
| 11 | + |
| 12 | +PR-URL: https://github.com/nodejs-private/node-private/pull/652 |
| 13 | +Refs: https://hackerone.com/reports/2575105 |
| 14 | +CVE-ID: CVE-2025-23083 |
| 15 | +--- |
| 16 | + src/node_worker.cc | 6 ++---- |
| 17 | + test/es-module/test-esm-loader-hooks.mjs | 8 ++++---- |
| 18 | + .../test-permission-dc-worker-threads.js | 19 +++++++++++++++++++ |
| 19 | + 3 files changed, 25 insertions(+), 8 deletions(-) |
| 20 | + create mode 100644 test/parallel/test-permission-dc-worker-threads.js |
| 21 | + |
| 22 | +diff --git a/src/node_worker.cc b/src/node_worker.cc |
| 23 | +index 196eb3bc..31268115 100644 |
| 24 | +--- a/src/node_worker.cc |
| 25 | ++++ b/src/node_worker.cc |
| 26 | +@@ -484,12 +484,10 @@ Worker::~Worker() { |
| 27 | + |
| 28 | + void Worker::New(const FunctionCallbackInfo<Value>& args) { |
| 29 | + Environment* env = Environment::GetCurrent(args); |
| 30 | ++ THROW_IF_INSUFFICIENT_PERMISSIONS( |
| 31 | ++ env, permission::PermissionScope::kWorkerThreads, ""); |
| 32 | + auto is_internal = args[5]; |
| 33 | + CHECK(is_internal->IsBoolean()); |
| 34 | +- if (is_internal->IsFalse()) { |
| 35 | +- THROW_IF_INSUFFICIENT_PERMISSIONS( |
| 36 | +- env, permission::PermissionScope::kWorkerThreads, ""); |
| 37 | +- } |
| 38 | + Isolate* isolate = args.GetIsolate(); |
| 39 | + |
| 40 | + CHECK(args.IsConstructCall()); |
| 41 | +diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs |
| 42 | +index 8e616c0d..225ab26a 100644 |
| 43 | +--- a/test/es-module/test-esm-loader-hooks.mjs |
| 44 | ++++ b/test/es-module/test-esm-loader-hooks.mjs |
| 45 | +@@ -154,7 +154,7 @@ describe('Loader hooks', { concurrency: true }, () => { |
| 46 | + }); |
| 47 | + }); |
| 48 | + |
| 49 | +- it('should work without worker permission', async () => { |
| 50 | ++ it('should not work without worker permission', async () => { |
| 51 | + const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [ |
| 52 | + '--no-warnings', |
| 53 | + '--experimental-permission', |
| 54 | +@@ -165,9 +165,9 @@ describe('Loader hooks', { concurrency: true }, () => { |
| 55 | + fixtures.path('es-modules/esm-top-level-await.mjs'), |
| 56 | + ]); |
| 57 | + |
| 58 | +- assert.strictEqual(stderr, ''); |
| 59 | +- assert.match(stdout, /^1\r?\n2\r?\n$/); |
| 60 | +- assert.strictEqual(code, 0); |
| 61 | ++ assert.match(stderr, /Error: Access to this API has been restricted/); |
| 62 | ++ assert.strictEqual(stdout, ''); |
| 63 | ++ assert.strictEqual(code, 1); |
| 64 | + assert.strictEqual(signal, null); |
| 65 | + }); |
| 66 | + |
| 67 | +diff --git a/test/parallel/test-permission-dc-worker-threads.js b/test/parallel/test-permission-dc-worker-threads.js |
| 68 | +new file mode 100644 |
| 69 | +index 00000000..73cbf029 |
| 70 | +--- /dev/null |
| 71 | ++++ b/test/parallel/test-permission-dc-worker-threads.js |
| 72 | +@@ -0,0 +1,19 @@ |
| 73 | ++// Flags: --experimental-permission --allow-fs-read=* --experimental-test-module-mocks |
| 74 | ++'use strict'; |
| 75 | ++ |
| 76 | ++const common = require('../common'); |
| 77 | ++const assert = require('node:assert'); |
| 78 | ++ |
| 79 | ++{ |
| 80 | ++ const diagnostics_channel = require('node:diagnostics_channel'); |
| 81 | ++ diagnostics_channel.subscribe('worker_threads', common.mustNotCall()); |
| 82 | ++ const { mock } = require('node:test'); |
| 83 | ++ |
| 84 | ++ // Module mocking should throw instead of posting to worker_threads dc |
| 85 | ++ assert.throws(() => { |
| 86 | ++ mock.module('node:path'); |
| 87 | ++ }, common.expectsError({ |
| 88 | ++ code: 'ERR_ACCESS_DENIED', |
| 89 | ++ permission: 'WorkerThreads', |
| 90 | ++ })); |
| 91 | ++} |
| 92 | +-- |
| 93 | +2.25.1 |
| 94 | + |
0 commit comments