From 205f1e643e25648173239b2de885fec430268492 Mon Sep 17 00:00:00 2001 From: RafaelGSS Date: Sat, 6 May 2023 13:23:32 -0300 Subject: [PATCH] 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 CVE-ID: CVE-2023-30584 --- lib/internal/fs/utils.js | 2 +- lib/internal/process/permission.js | 6 +-- test/fixtures/permission/fs-traversal.js | 47 +++++++++++++++++ test/parallel/test-cli-permission-deny-fs.js | 3 +- .../test-permission-fs-traversal-path.js | 51 +++++++++++++++++++ 5 files changed, 103 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/permission/fs-traversal.js create mode 100644 test/parallel/test-permission-fs-traversal-path.js diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 03fd6c7aee861b..763f39af62bbbb 100644 --- a/lib/internal/fs/utils.js +++ b/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); } } diff --git a/lib/internal/process/permission.js b/lib/internal/process/permission.js index e400dcae9f934e..f17bfade519bfb 100644 --- a/lib/internal/process/permission.js +++ b/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); } } diff --git a/test/fixtures/permission/fs-traversal.js b/test/fixtures/permission/fs-traversal.js new file mode 100644 index 00000000000000..7bf1d523dad674 --- /dev/null +++ b/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)); +} \ No newline at end of file diff --git a/test/parallel/test-cli-permission-deny-fs.js b/test/parallel/test-cli-permission-deny-fs.js index 927d582094cd41..964a0ad0a0e0c2 100644 --- a/test/parallel/test-cli-permission-deny-fs.js +++ b/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")); diff --git a/test/parallel/test-permission-fs-traversal-path.js b/test/parallel/test-permission-fs-traversal-path.js new file mode 100644 index 00000000000000..2b294a4574e2f3 --- /dev/null +++ b/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(); +}