Merge pull request #7044 from asgerf/js/proto-pollution-fps

Approved by erik-krogh

Author: codeql-ci

javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll

@@ -1145,7 +1145,8 @@ private predicate reachableFromInput(
   DataFlow::Configuration cfg, PathSummary summary
 ) {
   callInputStep(f, invk, input, nd, cfg) and
-  summary = PathSummary::level()
+  summary = PathSummary::level() and
+  not cfg.isLabeledBarrier(nd, summary.getEndLabel())
   or
   exists(DataFlow::Node mid, PathSummary oldSummary |
     reachableFromInput(f, invk, input, mid, cfg, oldSummary) and

javascript/ql/lib/semmle/javascript/security/dataflow/PrototypePollutingAssignmentQuery.qll

@@ -32,6 +32,23 @@ class Configuration extends TaintTracking::Configuration {
     or
     // Concatenating with a string will in practice prevent the string `__proto__` from arising.
     node instanceof StringOps::ConcatenationRoot
+    or
+    node instanceof DataFlow::ThisNode
+    or
+    // Stop at .replace() calls that likely prevent __proto__ from arising
+    exists(StringReplaceCall replace |
+      node = replace and
+      replace.getAReplacedString() = ["_", "p", "r", "o", "t"] and
+      // Replacing with "_" is likely to be exploitable
+      not replace.getRawReplacement().getStringValue() = "_" and
+      (
+        replace.isGlobal()
+        or
+        // Non-global replace with a non-empty string can also prevent __proto__ by
+        // inserting a chunk of text that doesn't fit anywhere in __proto__
+        not replace.getRawReplacement().getStringValue() = ""
+      )
+    )
   }
 
   override predicate isAdditionalFlowStep(

javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/PrototypePollutingAssignment.expected

@@ -23,6 +23,16 @@ nodes
 | tst.js:45:9:45:11 | obj |
 | tst.js:48:9:48:11 | obj |
 | tst.js:48:9:48:11 | obj |
+| tst.js:78:5:78:37 | obj[req ... ', '')] |
+| tst.js:78:5:78:37 | obj[req ... ', '')] |
+| tst.js:78:9:78:19 | req.query.x |
+| tst.js:78:9:78:19 | req.query.x |
+| tst.js:78:9:78:36 | req.que ... _', '') |
+| tst.js:81:5:81:46 | obj[req ... g, '')] |
+| tst.js:81:5:81:46 | obj[req ... g, '')] |
+| tst.js:81:9:81:19 | req.query.x |
+| tst.js:81:9:81:19 | req.query.x |
+| tst.js:81:9:81:45 | req.que ... /g, '') |
 edges
 | tst.js:5:9:5:38 | taint | tst.js:8:12:8:16 | taint |
 | tst.js:5:9:5:38 | taint | tst.js:9:12:9:16 | taint |
@@ -47,6 +57,14 @@ edges
 | tst.js:33:23:33:25 | obj | tst.js:45:9:45:11 | obj |
 | tst.js:33:23:33:25 | obj | tst.js:48:9:48:11 | obj |
 | tst.js:33:23:33:25 | obj | tst.js:48:9:48:11 | obj |
+| tst.js:78:9:78:19 | req.query.x | tst.js:78:9:78:36 | req.que ... _', '') |
+| tst.js:78:9:78:19 | req.query.x | tst.js:78:9:78:36 | req.que ... _', '') |
+| tst.js:78:9:78:36 | req.que ... _', '') | tst.js:78:5:78:37 | obj[req ... ', '')] |
+| tst.js:78:9:78:36 | req.que ... _', '') | tst.js:78:5:78:37 | obj[req ... ', '')] |
+| tst.js:81:9:81:19 | req.query.x | tst.js:81:9:81:45 | req.que ... /g, '') |
+| tst.js:81:9:81:19 | req.query.x | tst.js:81:9:81:45 | req.que ... /g, '') |
+| tst.js:81:9:81:45 | req.que ... /g, '') | tst.js:81:5:81:46 | obj[req ... g, '')] |
+| tst.js:81:9:81:45 | req.que ... /g, '') | tst.js:81:5:81:46 | obj[req ... g, '')] |
 #select
 | tst.js:8:5:8:17 | object[taint] | tst.js:5:24:5:37 | req.query.data | tst.js:8:5:8:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
 | tst.js:9:5:9:17 | object[taint] | tst.js:5:24:5:37 | req.query.data | tst.js:9:5:9:17 | object[taint] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
@@ -55,3 +73,5 @@ edges
 | tst.js:39:9:39:11 | obj | tst.js:5:24:5:37 | req.query.data | tst.js:39:9:39:11 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
 | tst.js:45:9:45:11 | obj | tst.js:5:24:5:37 | req.query.data | tst.js:45:9:45:11 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
 | tst.js:48:9:48:11 | obj | tst.js:5:24:5:37 | req.query.data | tst.js:48:9:48:11 | obj | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:5:24:5:37 | req.query.data | here |
+| tst.js:78:5:78:37 | obj[req ... ', '')] | tst.js:78:9:78:19 | req.query.x | tst.js:78:5:78:37 | obj[req ... ', '')] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:78:9:78:19 | req.query.x | here |
+| tst.js:81:5:81:46 | obj[req ... g, '')] | tst.js:81:9:81:19 | req.query.x | tst.js:81:5:81:46 | obj[req ... g, '')] | This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@. | tst.js:81:9:81:19 | req.query.x | here |

javascript/ql/test/query-tests/Security/CWE-915/PrototypePollutingAssignment/tst.js

@@ -71,3 +71,13 @@ class Box {
         this.foo = 'bar'; // OK - 'this' won't refer to Object.prototype
     }
 }
+
+app.get('/foo', (req, res) => {
+    let obj = {};
+    obj[req.query.x.replace('_', '-')].x = 'foo'; // OK
+    obj[req.query.x.replace('_', '')].x = 'foo'; // NOT OK
+    obj[req.query.x.replace(/_/g, '')].x = 'foo'; // OK
+    obj[req.query.x.replace(/_/g, '-')].x = 'foo'; // OK
+    obj[req.query.x.replace(/__proto__/g, '')].x = 'foo'; // NOT OK - "__pr__proto__oto__"
+    obj[req.query.x.replace('o', '0')].x = 'foo'; // OK
+});