src,permission: resolve path on fs_permission

Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com>
PR-URL: https://github.com/nodejs/node/pull/52761
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>

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

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,

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);

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;

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());
   }

src/permission/child_process_permission.cc

@@ -15,7 +15,8 @@ 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;
 }

src/permission/child_process_permission.h

@@ -15,7 +15,8 @@ class ChildProcessPermission final : public PermissionBase {
   void Apply(Environment* env,
              const std::vector<std::string>& allow,
              PermissionScope scope) override;
-  bool is_granted(PermissionScope perm,
+  bool is_granted(Environment* env,
+                  PermissionScope perm,
                   const std::string_view& param = "") const override;
 
  private:

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,8 @@ 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 +158,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;
   }

src/permission/fs_permission.h

@@ -18,7 +18,8 @@ class FSPermission final : public PermissionBase {
   void Apply(Environment* env,
              const std::vector<std::string>& 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 {

src/permission/inspector_permission.cc

@@ -14,7 +14,8 @@ 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;
 }

src/permission/inspector_permission.h

@@ -15,7 +15,8 @@ class InspectorPermission final : public PermissionBase {
   void Apply(Environment* env,
              const std::vector<std::string>& allow,
              PermissionScope scope) override;
-  bool is_granted(PermissionScope perm,
+  bool is_granted(Environment* env,
+                  PermissionScope perm,
                   const std::string_view& param = "") const override;
 
  private:

src/permission/permission.cc

@@ -50,10 +50,10 @@ static void Has(const FunctionCallbackInfo<Value>& 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

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,12 @@ 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;
   }

src/permission/permission_base.h

@@ -44,7 +44,8 @@ class PermissionBase {
   virtual void Apply(Environment* env,
                      const std::vector<std::string>& 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;
 };
 

src/permission/worker_permission.cc

@@ -15,7 +15,8 @@ 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;
 }

src/permission/worker_permission.h

@@ -15,7 +15,8 @@ class WorkerPermission final : public PermissionBase {
   void Apply(Environment* env,
              const std::vector<std::string>& allow,
              PermissionScope scope) override;
-  bool is_granted(PermissionScope perm,
+  bool is_granted(Environment* env,
+                  PermissionScope perm,
                   const std::string_view& param = "") const override;
 
  private:

test/fixtures/permission/fs-traversal.js

@@ -31,7 +31,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
   fs.writeFile(traversalPath, 'test', common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'FileSystemWrite',
-    resource: path.toNamespacedPath(resolve(traversalPath)),
+    resource: path.toNamespacedPath(traversalPath),
   }));
 }
 
@@ -39,33 +39,33 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
   fs.readFile(traversalPath, 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);
+  }, 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'),
-    }));
+  fs.mkdtemp(traversalFolderPath, common.expectsError({
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemWrite',
+    resource: traversalFolderPath + 'XXXXXX',
+  }));
 }
 
 {
   fs.readFile(bufferTraversalPath, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'FileSystemRead',
-    resource: resolve(traversalPath),
+    resource: traversalPath,
   }));
 }
 
@@ -73,7 +73,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
   fs.readFile(uint8ArrayTraversalPath, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'FileSystemRead',
-    resource: resolve(traversalPath),
+    resource: traversalPath,
   }));
 }
 
@@ -93,7 +93,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
   }, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'FileSystemRead',
-    resource: resolve(cwd.toString()),
+    resource: cwd.toString(),
   }));
 }
 
@@ -118,7 +118,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
   }, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'FileSystemRead',
-    resource: resolve(traversalPathWithExtraChars),
+    resource: traversalPathWithExtraChars,
   }));
 
   assert.throws(() => {
@@ -126,7 +126,7 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
   }, common.expectsError({
     code: 'ERR_ACCESS_DENIED',
     permission: 'FileSystemRead',
-    resource: resolve(traversalPathWithExtraChars),
+    resource: traversalPathWithExtraChars,
   }));
 }
 

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),
   });
 }
 

test/known_issues/test-permission-model-path-resolution.js

@@ -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');

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',
   }));
 }
 

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'),
   }));
 }