Skip to content

Commit 46ce278

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 54cd268 commit 46ce278

File tree

2 files changed

+39
-1
lines changed

2 files changed

+39
-1
lines changed

lib/internal/fs/utils.js

+6-1
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,8 @@ const kType = Symbol('type');
6767
const kStats = Symbol('stats');
6868
const assert = require('internal/assert');
6969

70+
const { encodeUtf8String } = internalBinding('encoding_binding');
71+
7072
const {
7173
fs: {
7274
F_OK = 0,
@@ -754,7 +756,10 @@ function possiblyTransformPath(path) {
754756
}
755757
assert(isUint8Array(path));
756758
if (!BufferIsBuffer(path)) path = BufferFrom(path);
757-
return BufferFrom(resolvePath(BufferToString(path)));
759+
// Avoid Buffer.from() and use a C++ binding instead to encode the result
760+
// of path.resolve() in order to prevent path traversal attacks that
761+
// monkey-patch Buffer internals.
762+
return encodeUtf8String(resolvePath(BufferToString(path)));
758763
}
759764
return path;
760765
}

test/fixtures/permission/fs-traversal.js

+33
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)