From c951dadf857f3c4e81e8f012f9d827b1bf7a26a6 Mon Sep 17 00:00:00 2001 From: Zachary Wade Date: Tue, 1 Feb 2022 17:08:02 -0500 Subject: [PATCH 1/2] Fix prototype pollution in utilities.js --- lib/utilities.js | 8 ++++---- test/utilities.spec.js | 46 +++++++++++++++++++++++++++++++++--------- 2 files changed, 41 insertions(+), 13 deletions(-) diff --git a/lib/utilities.js b/lib/utilities.js index b9bd3e7..f2b80de 100644 --- a/lib/utilities.js +++ b/lib/utilities.js @@ -79,13 +79,13 @@ const buildOptions = function() { const buildFields = (instance, field, value) => { // Do nothing if value is not set. if (value === null || value === undefined) return instance; - instance = instance || {}; + instance = instance || Object.create(null); // Non-array fields if (!instance[field]) { instance[field] = value; return instance; } - // Array fields + // Array fields if (instance[field] instanceof Array) { instance[field].push(value); } else { @@ -108,7 +108,7 @@ const checkAndMakeDir = (fileUploadOptions, filePath) => { if (!filePath) return false; const parentPath = path.dirname(filePath); // Create folder if it doesn't exist. - if (!fs.existsSync(parentPath)) fs.mkdirSync(parentPath, { recursive: true }); + if (!fs.existsSync(parentPath)) fs.mkdirSync(parentPath, { recursive: true }); // Checks folder again and return a results. return fs.existsSync(parentPath); }; @@ -209,7 +209,7 @@ const parseFileNameExtension = (preserveExtension, fileName) => { const nameParts = fileName.split('.'); if (nameParts.length < 2) return result; - + let extension = nameParts.pop(); if ( extension.length > maxExtLength && diff --git a/test/utilities.spec.js b/test/utilities.spec.js index 338b782..cfbf460 100644 --- a/test/utilities.spec.js +++ b/test/utilities.spec.js @@ -135,7 +135,7 @@ describe('Test of the utilities functions', function() { const result = parseFileName({}, name); assert.equal(result.length, 255); }); - + it( 'Strips away all non-alphanumeric characters (excluding hyphens/underscores) when enabled.', () => { @@ -202,12 +202,12 @@ describe('Test of the utilities functions', function() { it('buildOptions adds value to the result from the several source argumets', () => { let result = buildOptions(source, sourceAddon); assert.deepStrictEqual(result, expectedAddon); - }); + }); }); //buildFields tests describe('Test buildOptions function', () => { - + it('buildFields does nothing if null value has been passed', () => { let fields = null; fields = buildFields(fields, 'test', null); @@ -287,7 +287,7 @@ describe('Test of the utilities functions', function() { it('Failed if nonexistent file passed', function(done){ let filePath = path.join(uploadDir, getTempFilename()); - + deleteFile(filePath, function(err){ if (err) { return done(); @@ -324,11 +324,11 @@ describe('Test of the utilities functions', function() { }); }); }); - }); + }); }); }); - + describe('Test copyFile function', function(){ beforeEach(function() { server.clearUploadsDir(); @@ -337,7 +337,7 @@ describe('Test of the utilities functions', function() { it('Copy a file and check a hash', function(done) { let srcPath = path.join(fileDir, mockFile); let dstPath = path.join(uploadDir, mockFile); - + copyFile(srcPath, dstPath, function(err){ if (err) { return done(err); @@ -357,7 +357,7 @@ describe('Test of the utilities functions', function() { it('Failed if wrong source file path passed', function(done){ let srcPath = path.join(fileDir, 'unknown'); let dstPath = path.join(uploadDir, mockFile); - + copyFile(srcPath, dstPath, function(err){ if (err) { return done(); @@ -368,7 +368,7 @@ describe('Test of the utilities functions', function() { it('Failed if wrong destination file path passed', function(done){ let srcPath = path.join(fileDir, 'unknown'); let dstPath = path.join('unknown', 'unknown'); - + copyFile(srcPath, dstPath, function(err){ if (err) { return done(); @@ -400,4 +400,32 @@ describe('Test of the utilities functions', function() { }); }); }); + + describe('Test for no prototype pollution in buildFields', function() { + const prototypeFields = [ + { name: '__proto__', data: {} }, + { name: 'constructor', data: {} }, + { name: 'toString', data: {} } + ]; + + let fieldObject = undefined; + prototypeFields.forEach((field) => { + fieldObject = buildFields(fieldObject, field.name, field.data); + }); + + it(`Has ${prototypeFields.length} keys`, () => { + assert.equal(Object.keys(fieldObject).length, prototypeFields.length); + }); + + it(`Has null as its prototype`, () => { + assert.equal(Object.getPrototypeOf(fieldObject), null); + }); + + prototypeFields.forEach((field) => { + it(`${field.name} property is not an array`, () => { + // Note, Array.isArray is an insufficient test due to it returning false for Objects with an array prototype. + assert.equal(fieldObject[field.name] instanceof Array, false); + }); + }); + }) }); From 5e832497cdb75742205ee3b43189e7397e4d85a2 Mon Sep 17 00:00:00 2001 From: Zachary Wade Date: Tue, 1 Feb 2022 20:23:54 -0500 Subject: [PATCH 2/2] Refactor prototype pollution check to be more comprehensive --- lib/processNested.js | 20 ++++++----------- lib/utilities.js | 27 ++++++++++++++++++++++- test/utilities.spec.js | 49 ++++++++++++++++++++++++++++++++++++------ 3 files changed, 76 insertions(+), 20 deletions(-) diff --git a/lib/processNested.js b/lib/processNested.js index f6a8ff0..9c8b648 100644 --- a/lib/processNested.js +++ b/lib/processNested.js @@ -1,11 +1,9 @@ -const OBJECT_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Object.prototype); -const ARRAY_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Array.prototype); - +const { isSafeFromPollution } = require("./utilities"); module.exports = function(data){ - if (!data || data.length < 1) return {}; - - let d = {}, + if (!data || data.length < 1) return Object.create(null); + + let d = Object.create(null), keys = Object.keys(data); for (let i = 0; i < keys.length; i++) { @@ -15,27 +13,23 @@ module.exports = function(data){ keyParts = key .replace(new RegExp(/\[/g), '.') .replace(new RegExp(/\]/g), '') - .split('.'); + .split('.'); for (let index = 0; index < keyParts.length; index++){ let k = keyParts[index]; // Ensure we don't allow prototype pollution - const IN_ARRAY_PROTOTYPE = ARRAY_PROTOTYPE_KEYS.includes(k) && Array.isArray(current); - if (OBJECT_PROTOTYPE_KEYS.includes(k) || IN_ARRAY_PROTOTYPE) { + if (!isSafeFromPollution(current, k)) { continue; } if (index >= keyParts.length - 1){ current[k] = value; } else { - if (!current[k]) current[k] = !isNaN(keyParts[index + 1]) ? [] : {}; + if (!current[k]) current[k] = !isNaN(keyParts[index + 1]) ? [] : Object.create(null); current = current[k]; } } } - - - return d; }; diff --git a/lib/utilities.js b/lib/utilities.js index f2b80de..80c0811 100644 --- a/lib/utilities.js +++ b/lib/utilities.js @@ -69,6 +69,26 @@ const buildOptions = function() { return result; }; +// The default prototypes for both objects and arrays. +// Used by isSafeFromPollution +const OBJECT_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Object.prototype); +const ARRAY_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Array.prototype); + +/** + * Determines whether a key insertion into an object could result in a prototype pollution + * @param {Object} base - The object whose insertion we are checking + * @param {string} key - The key that will be inserted + */ +const isSafeFromPollution = (base, key) => { + // We perform an instanceof check instead of Array.isArray as the former is more + // permissive for cases in which the object as an Array prototype but was not constructed + // via an Array constructor or literal. + const TOUCHES_ARRAY_PROTOTYPE = (base instanceof Array) && ARRAY_PROTOTYPE_KEYS.includes(key); + const TOUCHES_OBJECT_PROTOTYPE = OBJECT_PROTOTYPE_KEYS.includes(key); + + return !TOUCHES_ARRAY_PROTOTYPE && !TOUCHES_OBJECT_PROTOTYPE; +}; + /** * Builds request fields (using to build req.body and req.files) * @param {Object} instance - request object. @@ -80,6 +100,10 @@ const buildFields = (instance, field, value) => { // Do nothing if value is not set. if (value === null || value === undefined) return instance; instance = instance || Object.create(null); + + if (!isSafeFromPollution(instance, field)) { + return instance; + } // Non-array fields if (!instance[field]) { instance[field] = value; @@ -266,5 +290,6 @@ module.exports = { promiseCallback, checkAndMakeDir, saveBufferToFile, - uriDecodeFileName + uriDecodeFileName, + isSafeFromPollution }; diff --git a/test/utilities.spec.js b/test/utilities.spec.js index cfbf460..5a8d1b5 100644 --- a/test/utilities.spec.js +++ b/test/utilities.spec.js @@ -19,7 +19,8 @@ const { copyFile, saveBufferToFile, parseFileName, - uriDecodeFileName + uriDecodeFileName, + isSafeFromPollution } = require('../lib/utilities'); const mockFile = 'basketball.png'; @@ -408,13 +409,18 @@ describe('Test of the utilities functions', function() { { name: 'toString', data: {} } ]; + const nonPrototypeFields = [ + { name: 'a', data: {} }, + { name: 'b', data: {} } + ]; + let fieldObject = undefined; - prototypeFields.forEach((field) => { + [...prototypeFields, ...nonPrototypeFields].forEach((field) => { fieldObject = buildFields(fieldObject, field.name, field.data); }); - it(`Has ${prototypeFields.length} keys`, () => { - assert.equal(Object.keys(fieldObject).length, prototypeFields.length); + it(`Has ${nonPrototypeFields.length} keys`, () => { + assert.equal(Object.keys(fieldObject).length, nonPrototypeFields.length); }); it(`Has null as its prototype`, () => { @@ -423,9 +429,40 @@ describe('Test of the utilities functions', function() { prototypeFields.forEach((field) => { it(`${field.name} property is not an array`, () => { - // Note, Array.isArray is an insufficient test due to it returning false for Objects with an array prototype. + // Note, Array.isArray is an insufficient test due to it returning false + // for Objects with an array prototype. assert.equal(fieldObject[field.name] instanceof Array, false); }); }); - }) + }); + + describe('Test for correct detection of prototype pollution', function() { + const validInsertions = [ + { base: {}, key: 'a' }, + { base: { a: 1 }, key: 'a' }, + { base: { __proto__: { a: 1 } }, key: 'a' }, + { base: [1], key: 0 }, + { base: { __proto__: [1] }, key: 0 } + ]; + + const invalidInsertions = [ + { base: {}, key: '__proto__' }, + { base: {}, key: 'constructor' }, + { base: [1], key: '__proto__' }, + { base: [1], key: 'length' }, + { base: { __proto__: [1] }, key: 'length' } + ]; + + validInsertions.forEach((insertion) => { + it(`Key ${insertion.key} should be valid for ${JSON.stringify(insertion.base)}`, () => { + assert.equal(isSafeFromPollution(insertion.base, insertion.key), true); + }); + }); + + invalidInsertions.forEach((insertion) => { + it(`Key ${insertion.key} should not be valid for ${JSON.stringify(insertion.base)}`, () => { + assert.equal(isSafeFromPollution(insertion.base, insertion.key), false); + }); + }); + }); });