Merge branch 'main' into polyQhelp

javascript/ql/src/Security/CWE-915/PrototypePollutingAssignment.inc.qhelp

@@ -35,8 +35,8 @@
   <p>
     In the example below, the untrusted value <code>req.params.id</code> is used as the property name
     <code>req.session.todos[id]</code>. If a malicious user passes in the ID value <code>__proto__</code>,
-    the variable <code>todo</code> will then refer to <code>Object.prototype</code>.
-    Finally, the modification of <code>todo</code> then allows the attacker to inject arbitrary properties
+    the variable <code>items</code> will then refer to <code>Object.prototype</code>.
+    Finally, the modification of <code>items</code> then allows the attacker to inject arbitrary properties
     onto <code>Object.prototype</code>.
   </p>
 
@@ -48,6 +48,12 @@
   </p>
 
   <sample src="examples/PrototypePollutingAssignmentFixed.js"/>
+
+  <p>
+    Another way to fix it is to prevent the <code>__proto__</code> property from being used as a key, as shown below:
+  </p>
+
+  <sample src="examples/PrototypePollutingAssignmentFixed2.js"/>
   
 </example>
 

javascript/ql/src/Security/CWE-915/examples/PrototypePollutingAssignmentFixed2.js

@@ -0,0 +1,16 @@
+let express = require('express');
+let app = express()
+
+app.put('/todos/:id', (req, res) => {
+    let id = req.params.id;
+    if (id === '__proto__' || id === 'constructor' || id === 'prototype') {
+        res.end(403);
+        return;
+    }
+    let items = req.session.todos[id];
+    if (!items) {
+        items = req.session.todos[id] = {};
+    }
+    items[req.query.name] = req.query.text;
+    res.end(200);
+});

javascript/ql/test/tutorials/Introducing the JavaScript libraries/query17.qll

@@ -11,11 +11,8 @@ class PasswordTracker extends DataFlow::Configuration {
   override predicate isSink(DataFlow::Node nd) { this.passwordVarAssign(_, nd) }
 
   predicate passwordVarAssign(Variable v, DataFlow::Node nd) {
-    exists(SsaExplicitDefinition def |
-      nd = DataFlow::ssaDefinitionNode(def) and
-      def.getSourceVariable() = v and
-      v.getName().toLowerCase() = "password"
-    )
+    v.getAnAssignedExpr() = nd.asExpr() and
+    v.getName().toLowerCase() = "password"
   }
 }
 

javascript/ql/test/tutorials/Introducing the JavaScript libraries/tests.expected

@@ -9,6 +9,7 @@ test_query4
 | tst.js:29:1:29:5 | 1 + 2 | This expression should be bracketed to clarify precedence rules. |
 test_query19
 test_query17
+| tst.js:38:18:38:23 | "blah" | Password variable password is assigned a constant string. |
 test_query18
 | m.js:1:1:3:0 | <toplevel> | 0 |
 test_query8
@@ -18,6 +19,7 @@ test_query11
 | tst.js:31:12:31:12 | x | Dead store of local variable. |
 | tst.js:31:15:31:15 | y | Dead store of local variable. |
 | tst.js:31:18:31:18 | x | Dead store of local variable. |
+| tst.js:38:7:38:23 | password = "blah" | Dead store of local variable. |
 test_query12
 test_query20
 test_query3

javascript/ql/test/tutorials/Introducing the JavaScript libraries/tst.js

@@ -32,4 +32,8 @@ function l(x, y, x) {
   for (i=0;i<10;++i);
 }
 
-var j, j;
+var j, j;
+
+function foo() {
+  var password = "blah";
+}