permission: handle fs path traversal

PR-URL: https://github.com/nodejs-private/node-private/pull/403
Refs: https://hackerone.com/bugs?subject=nodejs&report_id=1952978
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
CVE-ID: CVE-2023-30584

lib/internal/fs/utils.js

@@ -709,7 +709,7 @@ const validatePath = hideStackFrames((path, propName = 'path') => {
 // The permission model needs the absolute path for the fs_permission
 function possiblyTransformPath(path) {
   if (permission.isEnabled()) {
-    if (typeof path === 'string' && !pathModule.isAbsolute(path)) {
+    if (typeof path === 'string') {
       return pathModule.resolve(path);
     }
   }

lib/internal/process/permission.js

@@ -7,7 +7,7 @@ const {
 
 const permission = internalBinding('permission');
 const { validateString } = require('internal/validators');
-const { isAbsolute, resolve } = require('path');
+const { resolve } = require('path');
 
 let experimentalPermission;
 
@@ -26,9 +26,7 @@ module.exports = ObjectFreeze({
       // TODO: add support for WHATWG URLs and Uint8Arrays.
       validateString(reference, 'reference');
       if (StringPrototypeStartsWith(scope, 'fs')) {
-        if (!isAbsolute(reference)) {
-          reference = resolve(reference);
-        }
+        reference = resolve(reference);
       }
     }
 

test/fixtures/permission/fs-traversal.js

@@ -0,0 +1,47 @@
+'use strict'
+
+const common = require('../../common');
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+
+const blockedFolder = process.env.BLOCKEDFOLDER;
+const allowedFolder = process.env.ALLOWEDFOLDER;
+const traversalPath = allowedFolder + '../file.md'
+
+{
+  assert.ok(process.permission.has('fs.read', allowedFolder));
+  assert.ok(process.permission.has('fs.write', allowedFolder));
+  assert.ok(!process.permission.has('fs.read', blockedFolder));
+  assert.ok(!process.permission.has('fs.write', blockedFolder));
+}
+
+{
+  assert.throws(() => {
+    fs.writeFile(traversalPath, 'test', (error) => {
+      assert.ifError(error);
+    });
+  }, common.expectsError({
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemWrite',
+    resource: path.toNamespacedPath(path.resolve(traversalPath)),
+  }));
+}
+
+{
+  assert.throws(() => {
+    fs.readFile(traversalPath, (error) => {
+      assert.ifError(error);
+    });
+  }, common.expectsError({
+    code: 'ERR_ACCESS_DENIED',
+    permission: 'FileSystemRead',
+    resource: path.toNamespacedPath(path.resolve(traversalPath)),
+  }));
+}
+
+{
+  assert.ok(!process.permission.has('fs.read', traversalPath));
+  assert.ok(!process.permission.has('fs.write', traversalPath));
+}

test/parallel/test-cli-permission-deny-fs.js

@@ -27,11 +27,12 @@ const path = require('path');
 }
 
 {
+  const tmpPath = path.resolve('/tmp/');
   const { status, stdout } = spawnSync(
     process.execPath,
     [
       '--experimental-permission',
-      '--allow-fs-write', '/tmp/', '-e',
+      '--allow-fs-write', tmpPath, '-e',
       `console.log(process.permission.has("fs"));
       console.log(process.permission.has("fs.read"));
       console.log(process.permission.has("fs.write"));

test/parallel/test-permission-fs-traversal-path.js

@@ -0,0 +1,51 @@
+// Flags: --experimental-permission --allow-fs-read=* --allow-fs-write=* --allow-child-process
+'use strict';
+
+const common = require('../common');
+common.skipIfWorker();
+
+const fixtures = require('../common/fixtures');
+if (!common.canCreateSymLink())
+  common.skip('insufficient privileges');
+if (!common.hasCrypto)
+  common.skip('no crypto');
+
+const assert = require('assert');
+const fs = require('fs');
+const { spawnSync } = require('child_process');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+const file = fixtures.path('permission', 'fs-traversal.js');
+const blockedFolder = tmpdir.path;
+const allowedFolder = path.join(tmpdir.path, 'subdirectory/');
+const commonPathWildcard = path.join(__filename, '../../common*');
+
+{
+  tmpdir.refresh();
+  fs.mkdirSync(allowedFolder);
+}
+
+{
+  const { status, stderr } = spawnSync(
+    process.execPath,
+    [
+      '--experimental-permission',
+      `--allow-fs-read=${file},${commonPathWildcard},${allowedFolder}`,
+      `--allow-fs-write=${allowedFolder}`,
+      file,
+    ],
+    {
+      env: {
+        ...process.env,
+        BLOCKEDFOLDER: blockedFolder,
+        ALLOWEDFOLDER: allowedFolder,
+      },
+    }
+  );
+  assert.strictEqual(status, 0, stderr.toString());
+}
+
+{
+  tmpdir.refresh();
+}