From 52f470e5671a9d15a31c2d67554a792aed96c8a0 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Mon, 29 Apr 2024 16:44:08 -0300 Subject: [PATCH 1/3] src,permission: resolve path on fs_permission Signed-off-by: RafaelGSS --- lib/internal/fs/utils.js | 29 +------- src/compile_cache.cc | 4 +- src/node_modules.cc | 1 + src/node_worker.cc | 2 +- src/permission/child_process_permission.cc | 4 +- src/permission/child_process_permission.h | 4 +- src/permission/fs_permission.cc | 17 +++-- src/permission/fs_permission.h | 4 +- src/permission/inspector_permission.cc | 4 +- src/permission/inspector_permission.h | 4 +- src/permission/permission.cc | 4 +- src/permission/permission.h | 15 ++-- src/permission/permission_base.h | 4 +- src/permission/worker_permission.cc | 4 +- src/permission/worker_permission.h | 4 +- test/fixtures/permission/fs-traversal.js | 68 ++++++++++++------- test/fixtures/permission/fs-write.js | 20 +++--- .../test-permission-model-path-resolution.js | 46 ------------- .../test-permission-fs-write-report.js | 3 +- test/parallel/test-permission-fs-write-v8.js | 2 +- 20 files changed, 107 insertions(+), 136 deletions(-) delete mode 100644 test/known_issues/test-permission-model-path-resolution.js diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 808c0e1fb0b5fa..18c6f8d419d47a 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -24,11 +24,8 @@ const { Symbol, TypedArrayPrototypeAt, TypedArrayPrototypeIncludes, - uncurryThis, } = primordials; -const permission = require('internal/process/permission'); - const { Buffer } = require('buffer'); const { UVException, @@ -68,8 +65,6 @@ const kType = Symbol('type'); const kStats = Symbol('stats'); const assert = require('internal/assert'); -const { encodeUtf8String } = internalBinding('encoding_binding'); - const { fs: { F_OK = 0, @@ -736,31 +731,10 @@ const validatePath = hideStackFrames((path, propName = 'path') => { ); }); -// TODO(rafaelgss): implement the path.resolve on C++ side -// See: https://github.com/nodejs/node/pull/44004#discussion_r930958420 -// The permission model needs the absolute path for the fs_permission -const resolvePath = pathModule.resolve; -const { isBuffer: BufferIsBuffer, from: BufferFrom } = Buffer; -const BufferToString = uncurryThis(Buffer.prototype.toString); -function possiblyTransformPath(path) { - if (permission.isEnabled()) { - if (typeof path === 'string') { - return resolvePath(path); - } - assert(isUint8Array(path)); - if (!BufferIsBuffer(path)) path = BufferFrom(path); - // Avoid Buffer.from() and use a C++ binding instead to encode the result - // of path.resolve() in order to prevent path traversal attacks that - // monkey-patch Buffer internals. - return encodeUtf8String(resolvePath(BufferToString(path))); - } - return path; -} - const getValidatedPath = hideStackFrames((fileURLOrPath, propName = 'path') => { const path = toPathIfFileURL(fileURLOrPath); validatePath(path, propName); - return possiblyTransformPath(path); + return path; }); const getValidatedFd = hideStackFrames((fd, propName = 'fd') => { @@ -994,7 +968,6 @@ module.exports = { getValidatedFd, getValidatedPath, handleErrorFromBinding, - possiblyTransformPath, preprocessSymlinkDestination, realpathCacheKey: Symbol('realpathCacheKey'), getStatFsFromBinding, diff --git a/src/compile_cache.cc b/src/compile_cache.cc index 07969ca754f5bc..5db3654673ca62 100644 --- a/src/compile_cache.cc +++ b/src/compile_cache.cc @@ -354,7 +354,7 @@ bool CompileCacheHandler::InitializeDirectory(Environment* env, cache_dir); if (UNLIKELY(!env->permission()->is_granted( - permission::PermissionScope::kFileSystemWrite, cache_dir))) { + env, permission::PermissionScope::kFileSystemWrite, cache_dir))) { Debug("[compile cache] skipping cache because write permission for %s " "is not granted\n", cache_dir); @@ -362,7 +362,7 @@ bool CompileCacheHandler::InitializeDirectory(Environment* env, } if (UNLIKELY(!env->permission()->is_granted( - permission::PermissionScope::kFileSystemRead, cache_dir))) { + env, permission::PermissionScope::kFileSystemRead, cache_dir))) { Debug("[compile cache] skipping cache because read permission for %s " "is not granted\n", cache_dir); diff --git a/src/node_modules.cc b/src/node_modules.cc index 0ac38eaf9b1859..dfd074648ebf74 100644 --- a/src/node_modules.cc +++ b/src/node_modules.cc @@ -308,6 +308,7 @@ const BindingData::PackageConfig* BindingData::TraverseParent( // to walk upwards if (UNLIKELY(is_permissions_enabled && !env->permission()->is_granted( + env, permission::PermissionScope::kFileSystemRead, std::string(check_path) + kPathSeparator))) { return nullptr; diff --git a/src/node_worker.cc b/src/node_worker.cc index 196eb3bccaee87..f744c6a1a97ff1 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -89,7 +89,7 @@ Worker::Worker(Environment* env, // Without this check, to use the permission model with // workers (--allow-worker) one would need to pass --allow-inspector as well if (env->permission()->is_granted( - node::permission::PermissionScope::kInspector)) { + env, node::permission::PermissionScope::kInspector)) { inspector_parent_handle_ = GetInspectorParentHandle(env, thread_id_, url.c_str(), name.c_str()); } diff --git a/src/permission/child_process_permission.cc b/src/permission/child_process_permission.cc index 43bf5b7cbbc04f..1eac86ff8e77c1 100644 --- a/src/permission/child_process_permission.cc +++ b/src/permission/child_process_permission.cc @@ -15,7 +15,9 @@ void ChildProcessPermission::Apply(Environment* env, deny_all_ = true; } -bool ChildProcessPermission::is_granted(PermissionScope perm, +bool ChildProcessPermission::is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param) const { return deny_all_ == false; } diff --git a/src/permission/child_process_permission.h b/src/permission/child_process_permission.h index 844e5ee07509f3..8851ae6f1d396a 100644 --- a/src/permission/child_process_permission.h +++ b/src/permission/child_process_permission.h @@ -15,7 +15,9 @@ class ChildProcessPermission final : public PermissionBase { void Apply(Environment* env, const std::vector& allow, PermissionScope scope) override; - bool is_granted(PermissionScope perm, + bool is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param = "") const override; private: diff --git a/src/permission/fs_permission.cc b/src/permission/fs_permission.cc index 0c881fa266d23d..f25637d7c02119 100644 --- a/src/permission/fs_permission.cc +++ b/src/permission/fs_permission.cc @@ -1,6 +1,7 @@ #include "fs_permission.h" #include "base_object-inl.h" #include "debug_utils-inl.h" +#include "env.h" #include "path.h" #include "v8.h" @@ -51,21 +52,23 @@ void FreeRecursivelyNode( } bool is_tree_granted( + node::Environment* env, const node::permission::FSPermission::RadixTree* granted_tree, const std::string_view& param) { + std::string resolved_param = node::PathResolve(env, {param}); #ifdef _WIN32 // is UNC file path - if (param.rfind("\\\\", 0) == 0) { + if (resolved_param.rfind("\\\\", 0) == 0) { // return lookup with normalized param size_t starting_pos = 4; // "\\?\" - if (param.rfind("\\\\?\\UNC\\") == 0) { + if (resolved_param.rfind("\\\\?\\UNC\\") == 0) { starting_pos += 4; // "UNC\" } auto normalized = param.substr(starting_pos); return granted_tree->Lookup(normalized, true); } #endif - return granted_tree->Lookup(param, true); + return granted_tree->Lookup(resolved_param, true); } void PrintTree(const node::permission::FSPermission::RadixTree::Node* node, @@ -146,7 +149,9 @@ void FSPermission::GrantAccess(PermissionScope perm, const std::string& res) { } } -bool FSPermission::is_granted(PermissionScope perm, +bool FSPermission::is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param = "") const { switch (perm) { case PermissionScope::kFileSystem: @@ -154,11 +159,11 @@ bool FSPermission::is_granted(PermissionScope perm, case PermissionScope::kFileSystemRead: return !deny_all_in_ && ((param.empty() && allow_all_in_) || allow_all_in_ || - is_tree_granted(&granted_in_fs_, param)); + is_tree_granted(env, &granted_in_fs_, param)); case PermissionScope::kFileSystemWrite: return !deny_all_out_ && ((param.empty() && allow_all_out_) || allow_all_out_ || - is_tree_granted(&granted_out_fs_, param)); + is_tree_granted(env, &granted_out_fs_, param)); default: return false; } diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h index 66f84f1b52e6e7..15e46732d96c8a 100644 --- a/src/permission/fs_permission.h +++ b/src/permission/fs_permission.h @@ -18,7 +18,9 @@ class FSPermission final : public PermissionBase { void Apply(Environment* env, const std::vector& allow, PermissionScope scope) override; - bool is_granted(PermissionScope perm, + bool is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param) const override; struct RadixTree { diff --git a/src/permission/inspector_permission.cc b/src/permission/inspector_permission.cc index 59d8547b375222..2db9eb0650418c 100644 --- a/src/permission/inspector_permission.cc +++ b/src/permission/inspector_permission.cc @@ -14,7 +14,9 @@ void InspectorPermission::Apply(Environment* env, deny_all_ = true; } -bool InspectorPermission::is_granted(PermissionScope perm, +bool InspectorPermission::is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param) const { return deny_all_ == false; } diff --git a/src/permission/inspector_permission.h b/src/permission/inspector_permission.h index 2b32ad0b078d5d..86841678d2b116 100644 --- a/src/permission/inspector_permission.h +++ b/src/permission/inspector_permission.h @@ -15,7 +15,9 @@ class InspectorPermission final : public PermissionBase { void Apply(Environment* env, const std::vector& allow, PermissionScope scope) override; - bool is_granted(PermissionScope perm, + bool is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param = "") const override; private: diff --git a/src/permission/permission.cc b/src/permission/permission.cc index 50c21734400481..c2f1bf0a5ddab9 100644 --- a/src/permission/permission.cc +++ b/src/permission/permission.cc @@ -50,10 +50,10 @@ static void Has(const FunctionCallbackInfo& args) { return; } return args.GetReturnValue().Set( - env->permission()->is_granted(scope, *utf8_arg)); + env->permission()->is_granted(env, scope, *utf8_arg)); } - return args.GetReturnValue().Set(env->permission()->is_granted(scope)); + return args.GetReturnValue().Set(env->permission()->is_granted(env, scope)); } } // namespace diff --git a/src/permission/permission.h b/src/permission/permission.h index 65c0ada72b63e0..698803710516e2 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -27,7 +27,7 @@ namespace permission { #define THROW_IF_INSUFFICIENT_PERMISSIONS(env, perm_, resource_, ...) \ do { \ - if (UNLIKELY(!(env)->permission()->is_granted(perm_, resource_))) { \ + if (UNLIKELY(!(env)->permission()->is_granted(env, perm_, resource_))) { \ node::permission::Permission::ThrowAccessDenied( \ (env), perm_, resource_); \ return __VA_ARGS__; \ @@ -37,7 +37,7 @@ namespace permission { #define ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS( \ env, wrap, perm_, resource_, ...) \ do { \ - if (UNLIKELY(!(env)->permission()->is_granted(perm_, resource_))) { \ + if (UNLIKELY(!(env)->permission()->is_granted(env, perm_, resource_))) { \ node::permission::Permission::AsyncThrowAccessDenied( \ (env), wrap, perm_, resource_); \ return __VA_ARGS__; \ @@ -48,10 +48,11 @@ class Permission { public: Permission(); - FORCE_INLINE bool is_granted(const PermissionScope permission, + FORCE_INLINE bool is_granted(Environment* env, + const PermissionScope permission, const std::string_view& res = "") const { if (LIKELY(!enabled_)) return true; - return is_scope_granted(permission, res); + return is_scope_granted(env, permission, res); } FORCE_INLINE bool enabled() const { return enabled_; } @@ -73,11 +74,13 @@ class Permission { void EnablePermissions(); private: - COLD_NOINLINE bool is_scope_granted(const PermissionScope permission, + COLD_NOINLINE bool is_scope_granted( + Environment* env, + const PermissionScope permission, const std::string_view& res = "") const { auto perm_node = nodes_.find(permission); if (perm_node != nodes_.end()) { - return perm_node->second->is_granted(permission, res); + return perm_node->second->is_granted(env, permission, res); } return false; } diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h index 429a1fa854e2f1..7ecb3297dc8457 100644 --- a/src/permission/permission_base.h +++ b/src/permission/permission_base.h @@ -44,7 +44,9 @@ class PermissionBase { virtual void Apply(Environment* env, const std::vector& allow, PermissionScope scope) = 0; - virtual bool is_granted(PermissionScope perm, + virtual bool is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param = "") const = 0; }; diff --git a/src/permission/worker_permission.cc b/src/permission/worker_permission.cc index 1eba2eab09bee1..8692cdda65b954 100644 --- a/src/permission/worker_permission.cc +++ b/src/permission/worker_permission.cc @@ -15,7 +15,9 @@ void WorkerPermission::Apply(Environment* env, deny_all_ = true; } -bool WorkerPermission::is_granted(PermissionScope perm, +bool WorkerPermission::is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param) const { return deny_all_ == false; } diff --git a/src/permission/worker_permission.h b/src/permission/worker_permission.h index 29b6a523122c6e..9bb5332306f08c 100644 --- a/src/permission/worker_permission.h +++ b/src/permission/worker_permission.h @@ -15,7 +15,9 @@ class WorkerPermission final : public PermissionBase { void Apply(Environment* env, const std::vector& allow, PermissionScope scope) override; - bool is_granted(PermissionScope perm, + bool is_granted( + Environment* env, + PermissionScope perm, const std::string_view& param = "") const override; private: diff --git a/test/fixtures/permission/fs-traversal.js b/test/fixtures/permission/fs-traversal.js index 6b35331d8f061f..f6f2f27c1b6ee7 100644 --- a/test/fixtures/permission/fs-traversal.js +++ b/test/fixtures/permission/fs-traversal.js @@ -28,52 +28,74 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); } { - fs.writeFile(traversalPath, 'test', common.expectsError({ + assert.throws(() => { + fs.writeFile(traversalPath, 'test', (error) => { + assert.ifError(error); + }); + }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(resolve(traversalPath)), + resource: path.toNamespacedPath(traversalPath), })); } { - fs.readFile(traversalPath, common.expectsError({ + assert.throws(() => { + fs.readFile(traversalPath, (error) => { + assert.ifError(error); + }); + }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', - resource: path.toNamespacedPath(resolve(traversalPath)), + resource: path.toNamespacedPath(traversalPath), })); } { assert.throws(() => { - fs.mkdtempSync(traversalFolderPath) - }, { + fs.mkdtempSync(traversalFolderPath, (error) => { + assert.ifError(error); + }); + }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: resolve(traversalFolderPath + 'XXXXXX'), - }); + resource: traversalFolderPath + 'XXXXXX', + })); } { - fs.mkdtemp(traversalFolderPath, common.expectsError({ - code: 'ERR_ACCESS_DENIED', - permission: 'FileSystemWrite', - resource: resolve(traversalFolderPath + 'XXXXXX'), - })); + assert.throws(() => { + fs.mkdtemp(traversalFolderPath, (error) => { + assert.ifError(error); + }); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemWrite', + resource: traversalFolderPath + 'XXXXXX', + })); } { - fs.readFile(bufferTraversalPath, common.expectsError({ + assert.throws(() => { + fs.readFile(bufferTraversalPath, (error) => { + assert.ifError(error); + }); + }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', - resource: resolve(traversalPath), + resource: traversalPath, })); } { - fs.readFile(uint8ArrayTraversalPath, common.expectsError({ + assert.throws(() => { + fs.readFile(uint8ArrayTraversalPath, (error) => { + assert.ifError(error); + }); + }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', - resource: resolve(traversalPath), + resource: traversalPath, })); } @@ -89,11 +111,11 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); } catch { } assert.throws(() => { - fs.readFileSync(cwd); + fs.readFile(cwd, common.mustNotCall()); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', - resource: resolve(cwd.toString()), + resource: cwd.toString(), })); } @@ -114,19 +136,19 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); assert.strictEqual(Buffer.from(resolve(traversalPathWithExtraChars)).toString(), traversalPath); assert.throws(() => { - fs.readFileSync(traversalPathWithExtraBytes); + fs.readFile(traversalPathWithExtraBytes, common.mustNotCall()); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', - resource: resolve(traversalPathWithExtraChars), + resource: traversalPathWithExtraChars, })); assert.throws(() => { - fs.readFileSync(new TextEncoder().encode(traversalPathWithExtraBytes.toString())); + fs.readFile(new TextEncoder().encode(traversalPathWithExtraBytes.toString()), common.mustNotCall()); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', - resource: resolve(traversalPathWithExtraChars), + resource: traversalPathWithExtraChars, })); } diff --git a/test/fixtures/permission/fs-write.js b/test/fixtures/permission/fs-write.js index 828f953ea2ce42..a1f26df2c892ab 100644 --- a/test/fixtures/permission/fs-write.js +++ b/test/fixtures/permission/fs-write.js @@ -14,8 +14,6 @@ const blockedFile = process.env.BLOCKEDFILE; const blockedFileURL = require('url').pathToFileURL(process.env.BLOCKEDFILE); const relativeProtectedFile = process.env.RELATIVEBLOCKEDFILE; const relativeProtectedFolder = process.env.RELATIVEBLOCKEDFOLDER; -const absoluteProtectedFile = path.resolve(relativeProtectedFile); -const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); { assert.ok(!process.permission.has('fs.write', blockedFolder)); @@ -48,7 +46,7 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); }, { code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(absoluteProtectedFile), + resource: path.toNamespacedPath(relativeProtectedFile), }); assert.throws(() => { @@ -80,7 +78,7 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); }, { code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(absoluteProtectedFile), + resource: path.toNamespacedPath(relativeProtectedFile), }).then(common.mustCall()); assert.rejects(() => { @@ -116,7 +114,7 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); }, { code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(absoluteProtectedFile), + resource: path.toNamespacedPath(relativeProtectedFile), }); assert.throws(() => { @@ -164,7 +162,7 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); },{ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(path.join(absoluteProtectedFolder, 'any-folder')), + resource: path.toNamespacedPath(path.join(relativeProtectedFolder, 'any-folder')), }); } @@ -207,7 +205,7 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); },{ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(absoluteProtectedFile), + resource: path.toNamespacedPath(relativeProtectedFile), }); assert.throws(() => { fs.renameSync(blockedFile, path.join(regularFolder, 'renamed')); @@ -240,12 +238,12 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); },{ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(path.join(absoluteProtectedFolder, 'any-file')), + resource: path.toNamespacedPath(path.join(relativeProtectedFolder, 'any-file')), }); fs.copyFile(regularFile, path.join(relativeProtectedFolder, 'any-file'), common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(path.join(absoluteProtectedFolder, 'any-file')), + resource: path.toNamespacedPath(path.join(relativeProtectedFolder, 'any-file')), })); } @@ -263,7 +261,7 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); },{ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(path.join(absoluteProtectedFolder, 'any-file')), + resource: path.toNamespacedPath(path.join(relativeProtectedFolder, 'any-file')), }); } @@ -281,7 +279,7 @@ const absoluteProtectedFolder = path.resolve(relativeProtectedFolder); },{ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(absoluteProtectedFolder), + resource: path.toNamespacedPath(relativeProtectedFolder), }); } diff --git a/test/known_issues/test-permission-model-path-resolution.js b/test/known_issues/test-permission-model-path-resolution.js deleted file mode 100644 index 909fc64da1f50a..00000000000000 --- a/test/known_issues/test-permission-model-path-resolution.js +++ /dev/null @@ -1,46 +0,0 @@ -'use strict'; - -// The permission model resolves paths to avoid path traversals, but in doing so -// it potentially interprets paths differently than the operating system would. -// This test demonstrates that merely enabling the permission model causes the -// application to potentially access a different file than it would without the -// permission model. - -const common = require('../common'); - -const assert = require('assert'); -const { execFileSync } = require('child_process'); -const { mkdirSync, symlinkSync, writeFileSync } = require('fs'); -const path = require('path'); - -if (common.isWindows) - assert.fail('not applicable to Windows'); - -const tmpdir = require('../common/tmpdir'); -tmpdir.refresh(); - -const a = path.join(tmpdir.path, 'a'); -const b = path.join(tmpdir.path, 'b'); -const c = path.join(tmpdir.path, 'c'); -const d = path.join(tmpdir.path, 'c/d'); - -writeFileSync(a, 'bad'); -symlinkSync('c/d', b); -mkdirSync(c); -mkdirSync(d); -writeFileSync(path.join(c, 'a'), 'good'); - -function run(...args) { - const interestingPath = `${tmpdir.path}/b/../a`; - args = [...args, '-p', `fs.readFileSync(${JSON.stringify(interestingPath)}, 'utf8')`]; - return execFileSync(process.execPath, args, { encoding: 'utf8' }).trim(); -} - -// Because this is a known_issues test, we cannot assert any assumptions besides -// the known issue itself. Instead, do a sanity check and report success if the -// sanity check fails. -if (run() !== 'good') { - process.exit(0); -} - -assert.strictEqual(run('--experimental-permission', `--allow-fs-read=${tmpdir.path}`), 'good'); diff --git a/test/parallel/test-permission-fs-write-report.js b/test/parallel/test-permission-fs-write-report.js index a66bd316d02439..c8f6673de03d83 100644 --- a/test/parallel/test-permission-fs-write-report.js +++ b/test/parallel/test-permission-fs-write-report.js @@ -7,7 +7,6 @@ if (!common.hasCrypto) common.skip('no crypto'); const assert = require('assert'); -const path = require('path'); { assert.throws(() => { @@ -15,7 +14,7 @@ const path = require('path'); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.resolve('./secret.txt'), + resource: './secret.txt', })); } diff --git a/test/parallel/test-permission-fs-write-v8.js b/test/parallel/test-permission-fs-write-v8.js index 201c03e067b575..bb33c307544a37 100644 --- a/test/parallel/test-permission-fs-write-v8.js +++ b/test/parallel/test-permission-fs-write-v8.js @@ -16,7 +16,7 @@ const path = require('path'); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', - resource: path.toNamespacedPath(path.resolve('./secret.txt')), + resource: path.toNamespacedPath('./secret.txt'), })); } From 5f430a119bcc01caafadb14a69f4339ebcd74fea Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Tue, 30 Apr 2024 12:49:25 -0300 Subject: [PATCH 2/3] fixup! remove known limitation mention --- doc/api/permissions.md | 2 -- 1 file changed, 2 deletions(-) diff --git a/doc/api/permissions.md b/doc/api/permissions.md index de6d84d45719a6..b849d98a54d54f 100644 --- a/doc/api/permissions.md +++ b/doc/api/permissions.md @@ -583,8 +583,6 @@ There are constraints you need to know before using this system: #### Limitations and Known Issues -* When the permission model is enabled, Node.js may resolve some paths - differently than when it is disabled. * Symbolic links will be followed even to locations outside of the set of paths that access has been granted to. Relative symbolic links may allow access to arbitrary files and directories. When starting applications with the From 8e3a422dfa6bd10e159eaeea5d004c9cfcc73ef9 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Tue, 30 Apr 2024 12:52:09 -0300 Subject: [PATCH 3/3] fixup! src,permission: resolve path on fs_permission --- src/permission/child_process_permission.cc | 5 ++- src/permission/child_process_permission.h | 5 ++- src/permission/fs_permission.cc | 5 ++- src/permission/fs_permission.h | 5 ++- src/permission/inspector_permission.cc | 5 ++- src/permission/inspector_permission.h | 5 ++- src/permission/permission.h | 9 +++-- src/permission/permission_base.h | 5 ++- src/permission/worker_permission.cc | 5 ++- src/permission/worker_permission.h | 5 ++- test/fixtures/permission/fs-traversal.js | 40 +++++----------------- 11 files changed, 31 insertions(+), 63 deletions(-) diff --git a/src/permission/child_process_permission.cc b/src/permission/child_process_permission.cc index 1eac86ff8e77c1..1dfbaecf1b11ed 100644 --- a/src/permission/child_process_permission.cc +++ b/src/permission/child_process_permission.cc @@ -15,9 +15,8 @@ void ChildProcessPermission::Apply(Environment* env, deny_all_ = true; } -bool ChildProcessPermission::is_granted( - Environment* env, - PermissionScope perm, +bool ChildProcessPermission::is_granted(Environment* env, + PermissionScope perm, const std::string_view& param) const { return deny_all_ == false; } diff --git a/src/permission/child_process_permission.h b/src/permission/child_process_permission.h index 8851ae6f1d396a..7c9078c5b30714 100644 --- a/src/permission/child_process_permission.h +++ b/src/permission/child_process_permission.h @@ -15,9 +15,8 @@ class ChildProcessPermission final : public PermissionBase { void Apply(Environment* env, const std::vector& allow, PermissionScope scope) override; - bool is_granted( - Environment* env, - PermissionScope perm, + bool is_granted(Environment* env, + PermissionScope perm, const std::string_view& param = "") const override; private: diff --git a/src/permission/fs_permission.cc b/src/permission/fs_permission.cc index f25637d7c02119..baf90a7d5ff6f8 100644 --- a/src/permission/fs_permission.cc +++ b/src/permission/fs_permission.cc @@ -149,9 +149,8 @@ void FSPermission::GrantAccess(PermissionScope perm, const std::string& res) { } } -bool FSPermission::is_granted( - Environment* env, - PermissionScope perm, +bool FSPermission::is_granted(Environment* env, + PermissionScope perm, const std::string_view& param = "") const { switch (perm) { case PermissionScope::kFileSystem: diff --git a/src/permission/fs_permission.h b/src/permission/fs_permission.h index 15e46732d96c8a..fea95369fc1bd2 100644 --- a/src/permission/fs_permission.h +++ b/src/permission/fs_permission.h @@ -18,9 +18,8 @@ class FSPermission final : public PermissionBase { void Apply(Environment* env, const std::vector& allow, PermissionScope scope) override; - bool is_granted( - Environment* env, - PermissionScope perm, + bool is_granted(Environment* env, + PermissionScope perm, const std::string_view& param) const override; struct RadixTree { diff --git a/src/permission/inspector_permission.cc b/src/permission/inspector_permission.cc index 2db9eb0650418c..95114e6634ab3f 100644 --- a/src/permission/inspector_permission.cc +++ b/src/permission/inspector_permission.cc @@ -14,9 +14,8 @@ void InspectorPermission::Apply(Environment* env, deny_all_ = true; } -bool InspectorPermission::is_granted( - Environment* env, - PermissionScope perm, +bool InspectorPermission::is_granted(Environment* env, + PermissionScope perm, const std::string_view& param) const { return deny_all_ == false; } diff --git a/src/permission/inspector_permission.h b/src/permission/inspector_permission.h index 86841678d2b116..9b214099095ee8 100644 --- a/src/permission/inspector_permission.h +++ b/src/permission/inspector_permission.h @@ -15,9 +15,8 @@ class InspectorPermission final : public PermissionBase { void Apply(Environment* env, const std::vector& allow, PermissionScope scope) override; - bool is_granted( - Environment* env, - PermissionScope perm, + bool is_granted(Environment* env, + PermissionScope perm, const std::string_view& param = "") const override; private: diff --git a/src/permission/permission.h b/src/permission/permission.h index 698803710516e2..2dfba1d35f0991 100644 --- a/src/permission/permission.h +++ b/src/permission/permission.h @@ -27,7 +27,7 @@ namespace permission { #define THROW_IF_INSUFFICIENT_PERMISSIONS(env, perm_, resource_, ...) \ do { \ - if (UNLIKELY(!(env)->permission()->is_granted(env, perm_, resource_))) { \ + if (UNLIKELY(!(env)->permission()->is_granted(env, perm_, resource_))) { \ node::permission::Permission::ThrowAccessDenied( \ (env), perm_, resource_); \ return __VA_ARGS__; \ @@ -37,7 +37,7 @@ namespace permission { #define ASYNC_THROW_IF_INSUFFICIENT_PERMISSIONS( \ env, wrap, perm_, resource_, ...) \ do { \ - if (UNLIKELY(!(env)->permission()->is_granted(env, perm_, resource_))) { \ + if (UNLIKELY(!(env)->permission()->is_granted(env, perm_, resource_))) { \ node::permission::Permission::AsyncThrowAccessDenied( \ (env), wrap, perm_, resource_); \ return __VA_ARGS__; \ @@ -74,9 +74,8 @@ class Permission { void EnablePermissions(); private: - COLD_NOINLINE bool is_scope_granted( - Environment* env, - const PermissionScope permission, + COLD_NOINLINE bool is_scope_granted(Environment* env, + const PermissionScope permission, const std::string_view& res = "") const { auto perm_node = nodes_.find(permission); if (perm_node != nodes_.end()) { diff --git a/src/permission/permission_base.h b/src/permission/permission_base.h index 7ecb3297dc8457..ea0226c73adc1b 100644 --- a/src/permission/permission_base.h +++ b/src/permission/permission_base.h @@ -44,9 +44,8 @@ class PermissionBase { virtual void Apply(Environment* env, const std::vector& allow, PermissionScope scope) = 0; - virtual bool is_granted( - Environment* env, - PermissionScope perm, + virtual bool is_granted(Environment* env, + PermissionScope perm, const std::string_view& param = "") const = 0; }; diff --git a/src/permission/worker_permission.cc b/src/permission/worker_permission.cc index 8692cdda65b954..3a51cf12e4ee85 100644 --- a/src/permission/worker_permission.cc +++ b/src/permission/worker_permission.cc @@ -15,9 +15,8 @@ void WorkerPermission::Apply(Environment* env, deny_all_ = true; } -bool WorkerPermission::is_granted( - Environment* env, - PermissionScope perm, +bool WorkerPermission::is_granted(Environment* env, + PermissionScope perm, const std::string_view& param) const { return deny_all_ == false; } diff --git a/src/permission/worker_permission.h b/src/permission/worker_permission.h index 9bb5332306f08c..9ec40a3d1c66ca 100644 --- a/src/permission/worker_permission.h +++ b/src/permission/worker_permission.h @@ -15,9 +15,8 @@ class WorkerPermission final : public PermissionBase { void Apply(Environment* env, const std::vector& allow, PermissionScope scope) override; - bool is_granted( - Environment* env, - PermissionScope perm, + bool is_granted(Environment* env, + PermissionScope perm, const std::string_view& param = "") const override; private: diff --git a/test/fixtures/permission/fs-traversal.js b/test/fixtures/permission/fs-traversal.js index f6f2f27c1b6ee7..b568ede5b874a3 100644 --- a/test/fixtures/permission/fs-traversal.js +++ b/test/fixtures/permission/fs-traversal.js @@ -28,11 +28,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); } { - assert.throws(() => { - fs.writeFile(traversalPath, 'test', (error) => { - assert.ifError(error); - }); - }, common.expectsError({ + fs.writeFile(traversalPath, 'test', common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', resource: path.toNamespacedPath(traversalPath), @@ -40,11 +36,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); } { - assert.throws(() => { - fs.readFile(traversalPath, (error) => { - assert.ifError(error); - }); - }, common.expectsError({ + fs.readFile(traversalPath, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', resource: path.toNamespacedPath(traversalPath), @@ -53,9 +45,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); { assert.throws(() => { - fs.mkdtempSync(traversalFolderPath, (error) => { - assert.ifError(error); - }); + fs.mkdtempSync(traversalFolderPath); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', @@ -64,11 +54,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); } { - assert.throws(() => { - fs.mkdtemp(traversalFolderPath, (error) => { - assert.ifError(error); - }); - }, common.expectsError({ + fs.mkdtemp(traversalFolderPath, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemWrite', resource: traversalFolderPath + 'XXXXXX', @@ -76,11 +62,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); } { - assert.throws(() => { - fs.readFile(bufferTraversalPath, (error) => { - assert.ifError(error); - }); - }, common.expectsError({ + fs.readFile(bufferTraversalPath, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', resource: traversalPath, @@ -88,11 +70,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); } { - assert.throws(() => { - fs.readFile(uint8ArrayTraversalPath, (error) => { - assert.ifError(error); - }); - }, common.expectsError({ + fs.readFile(uint8ArrayTraversalPath, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', resource: traversalPath, @@ -111,7 +89,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); } catch { } assert.throws(() => { - fs.readFile(cwd, common.mustNotCall()); + fs.readFileSync(cwd); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', @@ -136,7 +114,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); assert.strictEqual(Buffer.from(resolve(traversalPathWithExtraChars)).toString(), traversalPath); assert.throws(() => { - fs.readFile(traversalPathWithExtraBytes, common.mustNotCall()); + fs.readFileSync(traversalPathWithExtraBytes); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead', @@ -144,7 +122,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); })); assert.throws(() => { - fs.readFile(new TextEncoder().encode(traversalPathWithExtraBytes.toString()), common.mustNotCall()); + fs.readFileSync(new TextEncoder().encode(traversalPathWithExtraBytes.toString())); }, common.expectsError({ code: 'ERR_ACCESS_DENIED', permission: 'FileSystemRead',