Skip to content

Commit

Permalink
fs: protect against modified Buffer internals in possiblyTransformPath
Browse files Browse the repository at this point in the history
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
  • Loading branch information
tniessen authored and RafaelGSS committed Feb 13, 2024
1 parent 186a6e1 commit 480fc16
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 1 deletion.
7 changes: 6 additions & 1 deletion lib/internal/fs/utils.js
Expand Up @@ -65,6 +65,8 @@ const kType = Symbol('type');
const kStats = Symbol('stats');
const assert = require('internal/assert');

const { encodeUtf8String } = internalBinding('encoding_binding');

const {
fs: {
F_OK = 0,
Expand Down Expand Up @@ -719,7 +721,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;
}
Expand Down
33 changes: 33 additions & 0 deletions test/fixtures/permission/fs-traversal.js
Expand Up @@ -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));
Expand Down

0 comments on commit 480fc16

Please sign in to comment.