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: https://github.com/nodejs-private/node-private/pull/497
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
CVE-ID: CVE-2024-21896
This commit is contained in:
Tobias Nießen authored and RafaelGSS committed 2024-02-14 14:27:34 -03:00
1 parent 54cd268059
commit 46ce278079
2 files changed
+39 -1

No files matched your search

+6 -1
View File
@@ -67,6 +67,8 @@ const kType = Symbol('type');
const kStats = Symbol('stats');
const assert = require('internal/assert');
const { encodeUtf8String } = internalBinding('encoding_binding');
const {
fs: {
F_OK = 0,
@@ -754,7 +756,10 @@ function possiblyTransformPath(path) {
}
assert(isUint8Array(path));
if (!BufferIsBuffer(path)) path = BufferFrom(path);
return BufferFrom(resolvePath(BufferToString(path)));
// Avoid Buffer.from() and use a C++ binding instead to encode the result
// of path.resolve() in order to prevent path traversal attacks that
// monkey-patch Buffer internals.
return encodeUtf8String(resolvePath(BufferToString(path)));
}
return path;
}
+33
View File
@@ -96,6 +96,39 @@ const uint8ArrayTraversalPath = new TextEncoder().encode(traversalPath);
}));
}
// Monkey-patching Buffer internals should also not allow path traversal.
{
const extraChars = '.'.repeat(40);
const traversalPathWithExtraChars = traversalPath + extraChars;
const traversalPathWithExtraBytes = Buffer.from(traversalPathWithExtraChars);
Buffer.prototype.utf8Write = ((w) => function(str, ...args) {
assert.strictEqual(str, resolve(traversalPath) + extraChars);
return w.apply(this, [traversalPath, ...args]);
})(Buffer.prototype.utf8Write);
// Sanity check (remove if the internals of Buffer.from change):
// The custom implementation of utf8Write should cause Buffer.from() to encode
// traversalPath instead of the sanitized output of resolve().
assert.strictEqual(Buffer.from(resolve(traversalPathWithExtraChars)).toString(), traversalPath);
assert.throws(() => {
fs.readFile(traversalPathWithExtraBytes, common.mustNotCall());
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemRead',
resource: resolve(traversalPathWithExtraChars),
}));
assert.throws(() => {
fs.readFile(new TextEncoder().encode(traversalPathWithExtraBytes.toString()), common.mustNotCall());
}, common.expectsError({
code: 'ERR_ACCESS_DENIED',
permission: 'FileSystemRead',
resource: resolve(traversalPathWithExtraChars),
}));
}
{
assert.ok(!process.permission.has('fs.read', traversalPath));
assert.ok(!process.permission.has('fs.write', traversalPath));