From fb4428049655d47df61dcb5b443abd64fe51f386 Mon Sep 17 00:00:00 2001 From: Rafael Gonzaga Date: Mon, 11 Nov 2024 14:31:44 -0300 Subject: [PATCH] permission: ignore internalModuleStat on module loading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This improves Permission Model usage when allowing read access to specifi modules. To achieve that, the permission model check on internalModuleStat has been removed meaning that on module loading, uv_fs_stat is performed on files and folders even when the permission model is enabled. Although a uv_fs_stat is performed, reading/executing the module will still pass by the permission model check. Without this PR when an app tries to --allow-fs-read=./a.js --allow-fs-read=./b.js where `a` attempt to load b, it will fails as it reads $pwd and no permission has been given to this path. PR-URL: https://github.com/nodejs/node/pull/55797 Reviewed-By: Yagiz Nizipli Reviewed-By: Ulises Gascón --- doc/api/cli.md | 27 ------- doc/api/permissions.md | 7 -- lib/internal/modules/cjs/loader.js | 6 +- src/node_file.cc | 6 +- test/fixtures/permission/fs-read.js | 7 ++ test/fixtures/permission/main-module.js | 1 + test/fixtures/permission/main-module.mjs | 1 + test/fixtures/permission/required-module.js | 1 + test/fixtures/permission/required-module.mjs | 1 + ...test-permission-fs-internal-module-stat.js | 12 +-- test/parallel/test-permission-fs-require.js | 76 +++++++++++++++++++ 11 files changed, 93 insertions(+), 52 deletions(-) create mode 100644 test/fixtures/permission/main-module.js create mode 100644 test/fixtures/permission/main-module.mjs create mode 100644 test/fixtures/permission/required-module.js create mode 100644 test/fixtures/permission/required-module.mjs create mode 100644 test/parallel/test-permission-fs-require.js diff --git a/doc/api/cli.md b/doc/api/cli.md index e166252d80e45c..2e2578e883e903 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -219,15 +219,8 @@ The initializer module also needs to be allowed. Consider the following example: ```console $ node --experimental-permission t.js -node:internal/modules/cjs/loader:162 - const result = internalModuleStat(receiver, filename); - ^ Error: Access to this API has been restricted - at stat (node:internal/modules/cjs/loader:162:18) - at Module._findPath (node:internal/modules/cjs/loader:640:16) - at resolveMainPath (node:internal/modules/run_main:15:25) - at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:53:24) at node:internal/main/run_main_module:23:47 { code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', @@ -300,18 +293,8 @@ new WASI({ ```console $ node --experimental-permission --allow-fs-read=* index.js -node:wasi:99 - const wrap = new _WASI(args, env, preopens, stdio); - ^ Error: Access to this API has been restricted - at new WASI (node:wasi:99:18) - at Object. (/home/index.js:3:1) - at Module._compile (node:internal/modules/cjs/loader:1476:14) - at Module._extensions..js (node:internal/modules/cjs/loader:1555:10) - at Module.load (node:internal/modules/cjs/loader:1288:32) - at Module._load (node:internal/modules/cjs/loader:1104:12) - at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:191:14) at node:internal/main/run_main_module:30:49 { code: 'ERR_ACCESS_DENIED', permission: 'WASI', @@ -341,18 +324,8 @@ new Worker(__filename); ```console $ node --experimental-permission --allow-fs-read=* index.js -node:internal/worker:188 - this[kHandle] = new WorkerImpl(url, - ^ Error: Access to this API has been restricted - at new Worker (node:internal/worker:188:21) - at Object. (/home/index.js.js:3:1) - at Module._compile (node:internal/modules/cjs/loader:1120:14) - at Module._extensions..js (node:internal/modules/cjs/loader:1174:10) - at Module.load (node:internal/modules/cjs/loader:998:32) - at Module._load (node:internal/modules/cjs/loader:839:12) - at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12) at node:internal/main/run_main_module:17:47 { code: 'ERR_ACCESS_DENIED', permission: 'WorkerThreads' diff --git a/doc/api/permissions.md b/doc/api/permissions.md index e37c2982bc146a..a03285e28641e8 100644 --- a/doc/api/permissions.md +++ b/doc/api/permissions.md @@ -47,15 +47,8 @@ will be restricted. ```console $ node --experimental-permission index.js -node:internal/modules/cjs/loader:171 - const result = internalModuleStat(receiver, filename); - ^ Error: Access to this API has been restricted - at stat (node:internal/modules/cjs/loader:171:18) - at Module._findPath (node:internal/modules/cjs/loader:627:16) - at resolveMainPath (node:internal/modules/run_main:19:25) - at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:76:24) at node:internal/main/run_main_module:23:47 { code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 3f161daa7f3c55..95763de0701581 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -160,7 +160,6 @@ const packageJsonReader = require('internal/modules/package_json_reader'); const { getOptionValue, getEmbedderOptions } = require('internal/options'); const shouldReportRequiredModules = getLazy(() => process.env.WATCH_REPORT_DEPENDENCIES); -const permission = require('internal/process/permission'); const { vm_dynamic_import_default_internal, } = internalBinding('symbols'); @@ -729,11 +728,8 @@ Module._findPath = function(request, paths, isMain) { // For each path for (let i = 0; i < paths.length; i++) { // Don't search further if path doesn't exist - // or doesn't have permission to it const curPath = paths[i]; - if (insidePath && curPath && - ((permission.isEnabled() && !permission.has('fs.read', curPath)) || _stat(curPath) < 1) - ) { + if (insidePath && curPath && _stat(curPath) < 1) { continue; } diff --git a/src/node_file.cc b/src/node_file.cc index dfdc25fd1d8986..5a50aacb1b939d 100644 --- a/src/node_file.cc +++ b/src/node_file.cc @@ -1037,6 +1037,8 @@ static void ExistsSync(const FunctionCallbackInfo& args) { // Used to speed up module loading. Returns 0 if the path refers to // a file, 1 when it's a directory or < 0 on error (usually -ENOENT.) // The speedup comes from not creating thousands of Stat and Error objects. +// Do not expose this function through public API as it doesn't hold +// Permission Model checks. static void InternalModuleStat(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); @@ -1045,8 +1047,6 @@ static void InternalModuleStat(const FunctionCallbackInfo& args) { BufferValue path(env->isolate(), args[1]); CHECK_NOT_NULL(*path); ToNamespacedPath(env, &path); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, path.ToStringView()); uv_fs_t req; int rc = uv_fs_stat(env->event_loop(), &req, *path, nullptr); @@ -1069,8 +1069,6 @@ static int32_t FastInternalModuleStat( HandleScope scope(env->isolate()); auto path = std::filesystem::path(input.data, input.data + input.length); - THROW_IF_INSUFFICIENT_PERMISSIONS( - env, permission::PermissionScope::kFileSystemRead, path.string(), -1); switch (std::filesystem::status(path).type()) { case std::filesystem::file_type::directory: diff --git a/test/fixtures/permission/fs-read.js b/test/fixtures/permission/fs-read.js index b189af295793e6..186117a6b768dd 100644 --- a/test/fixtures/permission/fs-read.js +++ b/test/fixtures/permission/fs-read.js @@ -282,6 +282,13 @@ const regularFile = __filename; permission: 'FileSystemRead', resource: path.toNamespacedPath(blockedFolder), })); + assert.throws(() => { + fs.readdirSync(blockedFolder, { recursive: true }); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemRead', + resource: path.toNamespacedPath(blockedFolder), + })); assert.throws(() => { fs.readdirSync(blockedFolder); }, common.expectsError({ diff --git a/test/fixtures/permission/main-module.js b/test/fixtures/permission/main-module.js new file mode 100644 index 00000000000000..cac52e04dddd24 --- /dev/null +++ b/test/fixtures/permission/main-module.js @@ -0,0 +1 @@ +require('./required-module'); \ No newline at end of file diff --git a/test/fixtures/permission/main-module.mjs b/test/fixtures/permission/main-module.mjs new file mode 100644 index 00000000000000..e7c28f7f6cab19 --- /dev/null +++ b/test/fixtures/permission/main-module.mjs @@ -0,0 +1 @@ +import './required-module.mjs'; \ No newline at end of file diff --git a/test/fixtures/permission/required-module.js b/test/fixtures/permission/required-module.js new file mode 100644 index 00000000000000..e8dbf442c5b1a2 --- /dev/null +++ b/test/fixtures/permission/required-module.js @@ -0,0 +1 @@ +console.log('ok'); \ No newline at end of file diff --git a/test/fixtures/permission/required-module.mjs b/test/fixtures/permission/required-module.mjs new file mode 100644 index 00000000000000..e8dbf442c5b1a2 --- /dev/null +++ b/test/fixtures/permission/required-module.mjs @@ -0,0 +1 @@ +console.log('ok'); \ No newline at end of file diff --git a/test/parallel/test-permission-fs-internal-module-stat.js b/test/parallel/test-permission-fs-internal-module-stat.js index 2f000f4814a0ca..f0b9d86f0809a8 100644 --- a/test/parallel/test-permission-fs-internal-module-stat.js +++ b/test/parallel/test-permission-fs-internal-module-stat.js @@ -9,8 +9,6 @@ if (!common.hasCrypto) { } const { internalBinding } = require('internal/test/binding'); -const assert = require('node:assert'); -const path = require('node:path'); const fixtures = require('../common/fixtures'); const blockedFile = fixtures.path('permission', 'deny', 'protected-file.md'); @@ -18,11 +16,7 @@ const internalFsBinding = internalBinding('fs'); // Run this inside a for loop to trigger the fast API for (let i = 0; i < 10_000; i++) { - assert.throws(() => { - internalFsBinding.internalModuleStat(internalFsBinding, blockedFile); - }, { - code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemRead', - resource: path.toNamespacedPath(blockedFile), - }); + // internalModuleStat does not use permission model. + // doesNotThrow + internalFsBinding.internalModuleStat(internalFsBinding, blockedFile); } diff --git a/test/parallel/test-permission-fs-require.js b/test/parallel/test-permission-fs-require.js new file mode 100644 index 00000000000000..6a2e9201dac7b4 --- /dev/null +++ b/test/parallel/test-permission-fs-require.js @@ -0,0 +1,76 @@ +// Flags: --experimental-permission --allow-fs-read=* --allow-child-process +'use strict'; + +const common = require('../common'); +common.skipIfWorker(); +const fixtures = require('../common/fixtures'); + +const assert = require('node:assert'); +const { spawnSync } = require('node:child_process'); + +{ + const mainModule = fixtures.path('permission', 'main-module.js'); + const requiredModule = fixtures.path('permission', 'required-module.js'); + const { status, stdout, stderr } = spawnSync( + process.execPath, + [ + '--experimental-permission', + '--allow-fs-read', mainModule, + '--allow-fs-read', requiredModule, + mainModule, + ] + ); + + assert.strictEqual(status, 0, stderr.toString()); + assert.strictEqual(stdout.toString(), 'ok\n'); +} + +{ + // When required module is not passed as allowed path + const mainModule = fixtures.path('permission', 'main-module.js'); + const { status, stderr } = spawnSync( + process.execPath, + [ + '--experimental-permission', + '--allow-fs-read', mainModule, + mainModule, + ] + ); + + assert.strictEqual(status, 1, stderr.toString()); + assert.match(stderr.toString(), /Error: Access to this API has been restricted/); +} + +{ + // ESM loader test + const mainModule = fixtures.path('permission', 'main-module.mjs'); + const requiredModule = fixtures.path('permission', 'required-module.mjs'); + const { status, stdout, stderr } = spawnSync( + process.execPath, + [ + '--experimental-permission', + '--allow-fs-read', mainModule, + '--allow-fs-read', requiredModule, + mainModule, + ] + ); + + assert.strictEqual(status, 0, stderr.toString()); + assert.strictEqual(stdout.toString(), 'ok\n'); +} + +{ + // When required module is not passed as allowed path (ESM) + const mainModule = fixtures.path('permission', 'main-module.mjs'); + const { status, stderr } = spawnSync( + process.execPath, + [ + '--experimental-permission', + '--allow-fs-read', mainModule, + mainModule, + ] + ); + + assert.strictEqual(status, 1, stderr.toString()); + assert.match(stderr.toString(), /Error: Access to this API has been restricted/); +}