Skip to content

Commit cd35275

Browse files
tniessenRafaelGSS
authored andcommittedOct 13, 2023
permission: improve path traversal protection
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

File tree

2 files changed

+12
-7
lines changed

2 files changed

+12
-7
lines changed
 

Diff for: ‎lib/internal/fs/utils.js

+3-2
Original file line numberDiff line numberDiff line change
@@ -708,12 +708,13 @@ const validatePath = hideStackFrames((path, propName = 'path') => {
708708
// TODO(rafaelgss): implement the path.resolve on C++ side
709709
// See: https://github.com/nodejs/node/pull/44004#discussion_r930958420
710710
// The permission model needs the absolute path for the fs_permission
711+
const resolvePath = pathModule.resolve;
711712
function possiblyTransformPath(path) {
712713
if (permission.isEnabled()) {
713714
if (typeof path === 'string') {
714-
return pathModule.resolve(path);
715+
return resolvePath(path);
715716
} else if (Buffer.isBuffer(path)) {
716-
return Buffer.from(pathModule.resolve(path.toString()));
717+
return Buffer.from(resolvePath(path.toString()));
717718
}
718719
}
719720
return path;

Diff for: ‎test/fixtures/permission/fs-traversal.js

+9-5
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,10 @@ const assert = require('assert');
66
const fs = require('fs');
77
const path = require('path');
88

9+
// This should not affect how the permission model resolves paths.
10+
const { resolve } = path;
11+
path.resolve = (s) => s;
12+
913
const blockedFolder = process.env.BLOCKEDFOLDER;
1014
const allowedFolder = process.env.ALLOWEDFOLDER;
1115
const traversalPath = allowedFolder + '../file.md';
@@ -27,7 +31,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
2731
}, common.expectsError({
2832
code: 'ERR_ACCESS_DENIED',
2933
permission: 'FileSystemWrite',
30-
resource: path.toNamespacedPath(path.resolve(traversalPath)),
34+
resource: path.toNamespacedPath(resolve(traversalPath)),
3135
}));
3236
}
3337

@@ -39,7 +43,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
3943
}, common.expectsError({
4044
code: 'ERR_ACCESS_DENIED',
4145
permission: 'FileSystemRead',
42-
resource: path.toNamespacedPath(path.resolve(traversalPath)),
46+
resource: path.toNamespacedPath(resolve(traversalPath)),
4347
}));
4448
}
4549

@@ -51,7 +55,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
5155
}, common.expectsError({
5256
code: 'ERR_ACCESS_DENIED',
5357
permission: 'FileSystemWrite',
54-
resource: path.resolve(traversalFolderPath + 'XXXXXX'),
58+
resource: resolve(traversalFolderPath + 'XXXXXX'),
5559
}));
5660
}
5761

@@ -63,7 +67,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
6367
}, common.expectsError({
6468
code: 'ERR_ACCESS_DENIED',
6569
permission: 'FileSystemWrite',
66-
resource: path.resolve(traversalFolderPath + 'XXXXXX'),
70+
resource: resolve(traversalFolderPath + 'XXXXXX'),
6771
}));
6872
}
6973

@@ -75,7 +79,7 @@ const bufferTraversalPath = Buffer.from(allowedFolder + '../file.md');
7579
}, common.expectsError({
7680
code: 'ERR_ACCESS_DENIED',
7781
permission: 'FileSystemRead',
78-
resource: path.resolve(traversalPath),
82+
resource: resolve(traversalPath),
7983
}));
8084
}
8185

0 commit comments

Comments
 (0)
Please sign in to comment.