From c81c8738c4eb69ad947d4276975b98886ea9c175 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Mon, 18 Apr 2022 19:05:51 +0800 Subject: [PATCH 1/3] fs: harden filehandle.read(params) typecheck Make sure that first argument is a nullable object Co-authored-by: Antoine du Hamel --- lib/internal/fs/promises.js | 4 ++++ test/parallel/test-fs-promises.js | 14 ++++++++++++-- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 34fd0f586766dd..f93ecd0cf2f85b 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -77,6 +77,7 @@ const { validateBuffer, validateEncoding, validateInteger, + validateObject, validateString, } = require('internal/validators'); const pathModule = require('path'); @@ -517,6 +518,9 @@ async function read(handle, bufferOrParams, offset, length, position) { let buffer = bufferOrParams; if (!isArrayBufferView(buffer)) { // This is fh.read(params) + if (bufferOrParams !== undefined) { + validateObject(bufferOrParams, 'options', { nullable: true }); + } ({ buffer = Buffer.alloc(16384), offset = 0, diff --git a/test/parallel/test-fs-promises.js b/test/parallel/test-fs-promises.js index 03f71069897345..c0cfb468a2aed9 100644 --- a/test/parallel/test-fs-promises.js +++ b/test/parallel/test-fs-promises.js @@ -153,14 +153,24 @@ async function executeOnHandle(dest, func) { }); } - // Use fallback buffer allocation when input not buffer + // Use fallback buffer allocation when first argument is null { await executeOnHandle(dest, async (handle) => { - const ret = await handle.read(0, 0, 0, 0); + const ret = await handle.read(null, 0, 0, 0); assert.strictEqual(ret.buffer.length, 16384); }); } + // TypeError if buffer is not ArrayBufferView or nullable object + { + await executeOnHandle(dest, async (handle) => { + await assert.rejects( + async () => handle.read(0, 0, 0, 0), + { code: 'ERR_INVALID_ARG_TYPE' } + ); + }); + } + // Bytes written to file match buffer { await executeOnHandle(dest, async (handle) => { From fc34407546ccd101b6bb820dcca17d4628ae58c8 Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Mon, 18 Apr 2022 19:54:40 +0800 Subject: [PATCH 2/3] fs: harden fs.read(params, callback) typecheck --- lib/fs.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/fs.js b/lib/fs.js index ffa216f35388e0..bcfa8aa6b2b611 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -639,6 +639,9 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) { buffer = Buffer.alloc(16384); } + if (params !== undefined) { + validateObject(params, 'options', { nullable: true }); + } ({ offset = 0, length = buffer.byteLength - offset, From f56c68358e449211dec5173104c09dafc0ee767f Mon Sep 17 00:00:00 2001 From: LiviaMedeiros Date: Tue, 19 Apr 2022 14:19:55 +0800 Subject: [PATCH 3/3] fs: harden fs.readSync(buffer, options) typecheck Co-authored-by: Antoine du Hamel --- lib/fs.js | 16 +++++---- .../test-fs-readSync-optional-params.js | 34 ++++++++++++++----- 2 files changed, 34 insertions(+), 16 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index bcfa8aa6b2b611..06aa44b303e9ca 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -701,26 +701,28 @@ ObjectDefineProperty(read, kCustomPromisifyArgsSymbol, * offset?: number; * length?: number; * position?: number | bigint | null; - * }} [offset] + * }} [offsetOrOptions] * @returns {number} */ -function readSync(fd, buffer, offset, length, position) { +function readSync(fd, buffer, offsetOrOptions, length, position) { fd = getValidatedFd(fd); validateBuffer(buffer); - if (arguments.length <= 3) { - // Assume fs.readSync(fd, buffer, options) - const options = offset || kEmptyObject; + let offset = offsetOrOptions; + if (arguments.length <= 3 || typeof offsetOrOptions === 'object') { + if (offsetOrOptions !== undefined) { + validateObject(offsetOrOptions, 'options', { nullable: true }); + } ({ offset = 0, length = buffer.byteLength - offset, position = null, - } = options); + } = offsetOrOptions ?? kEmptyObject); } - if (offset == null) { + if (offset === undefined) { offset = 0; } else { validateInteger(offset, 'offset', 0); diff --git a/test/parallel/test-fs-readSync-optional-params.js b/test/parallel/test-fs-readSync-optional-params.js index 5388e8037765fd..7fc1abfd91a57c 100644 --- a/test/parallel/test-fs-readSync-optional-params.js +++ b/test/parallel/test-fs-readSync-optional-params.js @@ -8,13 +8,20 @@ const filepath = fixtures.path('x.txt'); const expected = Buffer.from('xyz\n'); -function runTest(defaultBuffer, options) { +function runTest(defaultBuffer, options, errorCode = false) { let fd; try { fd = fs.openSync(filepath, 'r'); - const result = fs.readSync(fd, defaultBuffer, options); - assert.strictEqual(result, expected.length); - assert.deepStrictEqual(defaultBuffer, expected); + if (errorCode) { + assert.throws( + () => fs.readSync(fd, defaultBuffer, options), + { code: errorCode } + ); + } else { + const result = fs.readSync(fd, defaultBuffer, options); + assert.strictEqual(result, expected.length); + assert.deepStrictEqual(defaultBuffer, expected); + } } finally { if (fd != null) fs.closeSync(fd); } @@ -31,7 +38,6 @@ for (const options of [ { length: expected.length, position: 0 }, { offset: 0, length: expected.length, position: 0 }, - { offset: null }, { position: null }, { position: -1 }, { position: 0n }, @@ -41,17 +47,27 @@ for (const options of [ null, undefined, - // Test if bad params are interpreted as default (not mandatory) + // Test malicious corner case: it works as {length: 4} but not intentionally + new String('4444'), +]) { + runTest(Buffer.allocUnsafe(expected.length), options); +} + +for (const options of [ + + // Test various invalid options false, true, Infinity, 42n, Symbol(), + 'amString', + [], + () => {}, - // Test even more malicious corner cases + // Test if arbitrary entity with expected .length is not mistaken for options '4'.repeat(expected.length), - new String('4444'), [4, 4, 4, 4], ]) { - runTest(Buffer.allocUnsafe(expected.length), mustNotMutateObjectDeep(options)); + runTest(Buffer.allocUnsafe(expected.length), mustNotMutateObjectDeep(options), 'ERR_INVALID_ARG_TYPE'); }