Skip to content

Commit

Permalink
Merge pull request #301 from zwade/master
Browse files Browse the repository at this point in the history 
Fix prototype pollution in utilities.js
  • Loading branch information
@richardgirges
richardgirges authored Feb 2, 2022
2 parents 47bc50c + 5e83249 commit edd91ce
Show file tree
Hide file tree
Showing 3 changed files with 112 additions and 28 deletions.
20 changes: 7 additions & 13 deletions lib/processNested.js
View file
Original file line number Diff line number Diff line change
@@ -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++) {
Expand All @@ -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;
};
35 changes: 30 additions & 5 deletions lib/utilities.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,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.
Expand All @@ -88,13 +108,17 @@ 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);

if (!isSafeFromPollution(instance, field)) {
return instance;
}
// 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 {
Expand All @@ -117,7 +141,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);
};
Expand Down Expand Up @@ -219,7 +243,7 @@ const parseFileNameExtension = (preserveExtension, fileName) => {

const nameParts = fileName.split('.');
if (nameParts.length < 2) return result;

let extension = nameParts.pop();
if (
extension.length > maxExtLength &&
Expand Down Expand Up @@ -276,5 +300,6 @@ module.exports = {
promiseCallback,
checkAndMakeDir,
saveBufferToFile,
uriDecodeFileName
uriDecodeFileName,
isSafeFromPollution
};
85 changes: 75 additions & 10 deletions test/utilities.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ const {
copyFile,
saveBufferToFile,
parseFileName,
uriDecodeFileName
uriDecodeFileName,
isSafeFromPollution
} = require('../lib/utilities');

const mockFile = 'basketball.png';
Expand Down Expand Up @@ -135,7 +136,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.',
() => {
Expand Down Expand Up @@ -202,12 +203,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);
Expand Down Expand Up @@ -287,7 +288,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();
Expand Down Expand Up @@ -324,11 +325,11 @@ describe('Test of the utilities functions', function() {
});
});
});
});
});
});

});

describe('Test copyFile function', function(){
beforeEach(function() {
server.clearUploadsDir();
Expand All @@ -337,7 +338,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);
Expand All @@ -357,7 +358,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();
Expand All @@ -368,7 +369,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();
Expand Down Expand Up @@ -400,4 +401,68 @@ 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: {} }
];

const nonPrototypeFields = [
{ name: 'a', data: {} },
{ name: 'b', data: {} }
];

let fieldObject = undefined;
[...prototypeFields, ...nonPrototypeFields].forEach((field) => {
fieldObject = buildFields(fieldObject, field.name, field.data);
});

it(`Has ${nonPrototypeFields.length} keys`, () => {
assert.equal(Object.keys(fieldObject).length, nonPrototypeFields.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);
});
});
});

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);
});
});
});
});

0 comments on commit edd91ce

Please sign in to comment.