Skip to content

Commit a4edd22

Browse files
tniessenRafaelGSS
authored andcommitted
fs: protect against modified Buffer internals in possiblyTransformPath
Use encodeUtf8String from the encoding_binding internal binding to convert the result of path.resolve() to a Uint8Array instead of using Buffer.from(), whose result can be manipulated by the user by monkey-patching internals such as Buffer.prototype.utf8Write. HackerOne report: https://hackerone.com/reports/2218653 PR-URL: nodejs-private/node-private#497 Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> CVE-ID: CVE-2024-21896
1 parent b4fb4ba commit a4edd22

File tree

2 files changed

+39
-1
lines changed

2 files changed

+39
-1
lines changed

lib/internal/fs/utils.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ const kType = Symbol('type');
6565
const kStats = Symbol('stats');
6666
const assert = require('internal/assert');
6767

68+
const { encodeUtf8String } = internalBinding('encoding_binding');
69+
6870
const {
6971
fs: {
7072
F_OK = 0,
@@ -702,7 +704,10 @@ function possiblyTransformPath(path) {
702704
}
703705
assert(isUint8Array(path));
704706
if (!BufferIsBuffer(path)) path = BufferFrom(path);
705-
return BufferFrom(resolvePath(BufferToString(path)));
707+
// Avoid Buffer.from() and use a C++ binding instead to encode the result
708+
// of path.resolve() in order to prevent path traversal attacks that
709+
// monkey-patch Buffer internals.
710+
return encodeUtf8String(resolvePath(BufferToString(path)));
706711
}
707712
return path;
708713
}

test/fixtures/permission/fs-traversal.js

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,39 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
9696
}));
9797
}
9898

99+
// Monkey-patching Buffer internals should also not allow path traversal.
100+
{
101+
const extraChars = '.'.repeat(40);
102+
const traversalPathWithExtraChars = traversalPath + extraChars;
103+
const traversalPathWithExtraBytes = Buffer.from(traversalPathWithExtraChars);
104+
105+
Buffer.prototype.utf8Write = ((w) => function(str, ...args) {
106+
assert.strictEqual(str, resolve(traversalPath) + extraChars);
107+
return w.apply(this, [traversalPath, ...args]);
108+
})(Buffer.prototype.utf8Write);
109+
110+
// Sanity check (remove if the internals of Buffer.from change):
111+
// The custom implementation of utf8Write should cause Buffer.from() to encode
112+
// traversalPath instead of the sanitized output of resolve().
113+
assert.strictEqual(Buffer.from(resolve(traversalPathWithExtraChars)).toString(), traversalPath);
114+
115+
assert.throws(() => {
116+
fs.readFile(traversalPathWithExtraBytes, common.mustNotCall());
117+
}, common.expectsError({
118+
code: 'ERR_ACCESS_DENIED',
119+
permission: 'FileSystemRead',
120+
resource: resolve(traversalPathWithExtraChars),
121+
}));
122+
123+
assert.throws(() => {
124+
fs.readFile(new TextEncoder().encode(traversalPathWithExtraBytes.toString()), common.mustNotCall());
125+
}, common.expectsError({
126+
code: 'ERR_ACCESS_DENIED',
127+
permission: 'FileSystemRead',
128+
resource: resolve(traversalPathWithExtraChars),
129+
}));
130+
}
131+
99132
{
100133
assert.ok(!process.permission.has('fs.read', traversalPath));
101134
assert.ok(!process.permission.has('fs.write', traversalPath));

0 commit comments

Comments
 (0)