More more complete fix for prototype pollution

vulnerability first addressed in #384
mozilla · May 7, 2022 · 1ea0ab1 · 1ea0ab1
1 parent c7acb02
commit 1ea0ab1
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,14 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
 
+## [6.2.3] - 2022-05-07
+
+### Fixed
+
+- More more complete fix for prototype pollution vulnerability first addressed
+  in #384 (Marc-Aurèle Darche @madarche, Snyk Security team)
+
+
 ## [6.2.2] - 2022-03-27
 
 ### Fixed

diff --git a/packages/convict/src/main.js b/packages/convict/src/main.js
@@ -11,8 +11,8 @@ const cloneDeep = require('lodash.clonedeep')
 
 // Forbidden key paths, for protection against prototype pollution
 const FORBIDDEN_KEY_PATHS = [
-  '__proto__',
-  'this.constructor.prototype',
+  '__proto__.',
+  'this.constructor.prototype.',
 ]
 
 const ALLOWED_OPTION_STRICT = 'strict'
@@ -567,8 +567,9 @@ const convict = function convict(def, opts) {
      * exist, they will be initialized to empty objects
      */
     set: function(k, v) {
-      for (const path of FORBIDDEN_KEY_PATHS) {
-        if (k.startsWith(`${path}.`)) {
+      for (const forbidden_key_path of FORBIDDEN_KEY_PATHS) {
+        if (k.startsWith(forbidden_key_path) ||
+            k.includes(`.${forbidden_key_path}`)) {
           return this
         }
       }

diff --git a/packages/convict/test/prototype_pollution.test.js b/packages/convict/test/prototype_pollution.test.js
@@ -14,6 +14,20 @@ describe('Convict prototype pollution resistance', function() {
     config.set('__proto__.nested.polluted_proto_nested', 'Polluted!')
     expect({}).not.toHaveProperty('nested')
     expect({}).not.toHaveProperty('nested.polluted_proto_nested')
+
+    config.set('this.__proto__.polluted_proto_root', 'Polluted!')
+    expect({}).not.toHaveProperty('polluted_proto_root')
+
+    config.set('this.__proto__.nested.polluted_proto_nested', 'Polluted!')
+    expect({}).not.toHaveProperty('nested')
+    expect({}).not.toHaveProperty('nested.polluted_proto_nested')
+
+    config.set('foo.__proto__.polluted_proto_root', 'Polluted!')
+    expect({}).not.toHaveProperty('polluted_proto_root')
+
+    config.set('foo.__proto__.nested.polluted_proto_nested', 'Polluted!')
+    expect({}).not.toHaveProperty('nested')
+    expect({}).not.toHaveProperty('nested.polluted_proto_nested')
   })
 
   test('against this.constructor.prototype', function() {
@@ -26,6 +40,20 @@ describe('Convict prototype pollution resistance', function() {
     config.set('this.constructor.prototype.nested.polluted_constructor_prototype_nested', 'Polluted!')
     expect({}).not.toHaveProperty('nested')
     expect({}).not.toHaveProperty('nested.polluted_constructor_prototype_nested')
+
+    config.set('this.this.constructor.prototype.polluted_constructor_prototype_root', 'Polluted!')
+    expect({}).not.toHaveProperty('polluted_constructor_prototype_root')
+
+    config.set('this.this.constructor.prototype.nested.polluted_constructor_prototype_nested', 'Polluted!')
+    expect({}).not.toHaveProperty('nested')
+    expect({}).not.toHaveProperty('nested.polluted_constructor_prototype_nested')
+
+    config.set('foo.this.constructor.prototype.polluted_constructor_prototype_root', 'Polluted!')
+    expect({}).not.toHaveProperty('polluted_constructor_prototype_root')
+
+    config.set('foo.this.constructor.prototype.nested.polluted_constructor_prototype_nested', 'Polluted!')
+    expect({}).not.toHaveProperty('nested')
+    expect({}).not.toHaveProperty('nested.polluted_constructor_prototype_nested')
   })
 
 })