Skip to content

Commit

Permalink
permission: improve path traversal protection
Browse files Browse the repository at this point in the history
Always use the original implementation of pathModule.resolve. If the
application overwrites the value of pathModule.resolve with a custom
implementation, it should not have any effect on the permission model.

PR-URL: nodejs-private/node-private#456
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
CVE-ID: CVE-2023-39331
  • Loading branch information
tniessen authored and RafaelGSS committed Oct 13, 2023
1 parent 3b23b2e commit 32bcf4c
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 7 deletions.
5 changes: 3 additions & 2 deletions lib/internal/fs/utils.js
Expand Up @@ -708,12 +708,13 @@ 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;
function possiblyTransformPath(path) {
if (permission.isEnabled()) {
if (typeof path === 'string') {
return pathModule.resolve(path);
return resolvePath(path);
} else if (Buffer.isBuffer(path)) {
return Buffer.from(pathModule.resolve(path.toString()));
return Buffer.from(resolvePath(path.toString()));
}
}
return path;
Expand Down
14 changes: 9 additions & 5 deletions test/fixtures/permission/fs-traversal.js
Expand Up @@ -6,6 +6,10 @@ const assert = require('assert');
const fs = require('fs');
const path = require('path');

// This should not affect how the permission model resolves paths.
const { resolve } = path;
path.resolve = (s) => s;

const blockedFolder = process.env.BLOCKEDFOLDER;
const allowedFolder = process.env.ALLOWEDFOLDER;
const traversalPath = allowedFolder + '../file.md';
Expand All @@ -27,7 +31,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemWrite',
resource: path.toNamespacedPath(path.resolve(traversalPath)),
resource: path.toNamespacedPath(resolve(traversalPath)),
}));
}

Expand All @@ -39,7 +43,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemRead',
resource: path.toNamespacedPath(path.resolve(traversalPath)),
resource: path.toNamespacedPath(resolve(traversalPath)),
}));
}

Expand All @@ -51,7 +55,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemWrite',
resource: path.resolve(traversalFolderPath + 'XXXXXX'),
resource: resolve(traversalFolderPath + 'XXXXXX'),
}));
}

Expand All @@ -63,7 +67,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemWrite',
resource: path.resolve(traversalFolderPath + 'XXXXXX'),
resource: resolve(traversalFolderPath + 'XXXXXX'),
}));
}

Expand All @@ -75,7 +79,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemRead',
resource: path.resolve(traversalPath),
resource: resolve(traversalPath),
}));
}

Expand Down

0 comments on commit 32bcf4c

Please sign in to comment.