Fix prototype pollution in utilities.js

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

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.
@@ -79,13 +99,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 {
@@ -108,7 +132,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 +233,7 @@ const parseFileNameExtension = (preserveExtension, fileName) => {
 
   const nameParts = fileName.split('.');
   if (nameParts.length < 2) return result;
-  
+
   let extension = nameParts.pop();
   if (
     extension.length > maxExtLength &&
@@ -266,5 +290,6 @@ module.exports = {
   promiseCallback,
   checkAndMakeDir,
   saveBufferToFile,
-  uriDecodeFileName
+  uriDecodeFileName,
+  isSafeFromPollution
 };

test/utilities.spec.js

@@ -19,7 +19,8 @@ const {
   copyFile,
   saveBufferToFile,
   parseFileName,
-  uriDecodeFileName
+  uriDecodeFileName,
+  isSafeFromPollution
 } = require('../lib/utilities');
 
 const mockFile = 'basketball.png';
@@ -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.',
       () => {
@@ -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);
@@ -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();
@@ -324,11 +325,11 @@ describe('Test of the utilities functions', function() {
             });
           });
         });
-      });      
+      });
     });
 
   });
-  
+
   describe('Test copyFile function', function(){
     beforeEach(function() {
       server.clearUploadsDir();
@@ -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);
@@ -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();
@@ -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();
@@ -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);
+      });
+    });
+  });
 });