From f447a4611a49d1843c17bf9ffbc86a835f1f1b9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tobias=20Nie=C3=9Fen?= Date: Tue, 10 Oct 2023 16:30:07 +0000 Subject: [PATCH] permission: fix Uint8Array path traversal Previous security patches addressed path traversal vulnerabilities for string and Buffer inputs, but ignored Uint8Array inputs. This commit fixes the existing logic to account for the latter. The previous implementation would silently ignore unexpected inputs, whereas this commit introduces an explicit assertion to prevent that unsafe behavior. PR-URL: https://github.com/nodejs-private/node-private/pull/456 Reviewed-By: Rafael Gonzaga CVE-ID: CVE-2023-39332 --- lib/internal/fs/utils.js | 8 ++++++-- test/fixtures/permission/fs-traversal.js | 13 +++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index 0fc9bd9b98781f..611b6c242047b9 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -22,6 +22,7 @@ const { Symbol, TypedArrayPrototypeAt, TypedArrayPrototypeIncludes, + uncurryThis, } = primordials; const permission = require('internal/process/permission'); @@ -709,13 +710,16 @@ const validatePath = hideStackFrames((path, propName = 'path') => { // 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); - } else if (Buffer.isBuffer(path)) { - return Buffer.from(resolvePath(path.toString())); } + assert(isUint8Array(path)); + if (!BufferIsBuffer(path)) path = BufferFrom(path); + return BufferFrom(resolvePath(BufferToString(path))); } return path; } diff --git a/test/fixtures/permission/fs-traversal.js b/test/fixtures/permission/fs-traversal.js index e45600fff88af7..18243edfeadf7e 100644 --- a/test/fixtures/permission/fs-traversal.js +++ b/test/fixtures/permission/fs-traversal.js @@ -15,6 +15,7 @@ const allowedFolder = process.env.ALLOWEDFOLDER; const traversalPath = allowedFolder + '../file.md'; const traversalFolderPath = allowedFolder + '../folder'; const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md'); +const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath); { assert.ok(process.permission.has('fs.read', allowedFolder)); @@ -83,6 +84,18 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md'); })); } +{ + assert.throws(() => { + fs.readFile(uint8ArrayTraversalPath, (error) => { + assert.ifError(error); + }); + }, common.expectsError({ + code: 'ERR_ACCESS_DENIED', + permission: 'FileSystemRead', + resource: resolve(traversalPath), + })); +} + { assert.ok(!process.permission.has('fs.read', traversalPath)); assert.ok(!process.permission.has('fs.write', traversalPath));