Merge pull request #4778 from asgerf/js/more-prototype-pollution

Approved by erik-krogh, mchammer01

Author: codeql-ci

javascript/change-notes/2020-11-25-prototype-pollution.md

@@ -0,0 +1,11 @@
+lgtm,codescanning
+* We've improved the detection of prototype pollution, and the queries involved have been reorganized:
+  * A new query "Prototype-polluting assignment" (`js/prototype-polluting-assignment`) has been added. This query
+  highlights direct modifications of an object obtained via a user-controlled property name, which may accidentally alter `Object.prototype`.
+  * The query previously named "Prototype pollution" (`js/prototype-pollution`) has been renamed to "Prototype-polluting merge call".
+  This highlights indirect modification of `Object.prototype` via an unsafe `merge` call taking a user-controlled object as argument.
+  * The query previously named "Prototype pollution in utility function" (`js/prototype-pollution-utility`) has been renamed to "Prototype-polluting function".
+  This query highlights the implementation of an unsafe `merge` function, to ensure a robust API is exposed downstream.
+  * The above queries have been moved to the Security/CWE-915 folder, and assigned the following tags: CWE-078, CWE-079, CWE-094, CWE-400, and CWE-915.
+  * The query "Type confusion through parameter tampering" (`js/type-confusion-through-parameter-tampering`) now highlights
+  ineffective prototype pollution checks that can be bypassed by type confusion.

javascript/config/suites/javascript/security

@@ -37,8 +37,9 @@
 + semmlecode-javascript-queries/Security/CWE-338/InsecureRandomness.ql: /Security/CWE/CWE-338
 + semmlecode-javascript-queries/Security/CWE-346/CorsMisconfigurationForCredentials.ql: /Security/CWE/CWE-346
 + semmlecode-javascript-queries/Security/CWE-352/MissingCsrfMiddleware.ql: /Security/CWE/CWE-352
-+ semmlecode-javascript-queries/Security/CWE-400/PrototypePollution.ql: /Security/CWE/CWE-400
-+ semmlecode-javascript-queries/Security/CWE-400/PrototypePollutionUtility.ql: /Security/CWE/CWE-400
++ semmlecode-javascript-queries/Security/CWE-915/PrototypePollutingAssignment.ql: /Security/CWE/CWE-915
++ semmlecode-javascript-queries/Security/CWE-915/PrototypePollutingFunction.ql: /Security/CWE/CWE-915
++ semmlecode-javascript-queries/Security/CWE-915/PrototypePollutingMergeCall.ql: /Security/CWE/CWE-915
 + semmlecode-javascript-queries/Security/CWE-400/RemotePropertyInjection.ql: /Security/CWE/CWE-400
 + semmlecode-javascript-queries/Security/CWE-502/UnsafeDeserialization.ql: /Security/CWE/CWE-502
 + semmlecode-javascript-queries/Security/CWE-506/HardcodedDataInterpretedAsCode.ql: /Security/CWE/CWE-506

javascript/ql/src/Security/CWE-843/TypeConfusionThroughParameterTampering.ql

@@ -15,5 +15,6 @@ import DataFlow::PathGraph
 
 from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
 where cfg.hasFlowPath(source, sink)
-select sink.getNode(), source, sink, "Potential type confusion for $@.", source.getNode(),
-  "HTTP request parameter"
+select sink.getNode(), source, sink,
+  "Potential type confusion as $@ may be either an array or a string.", source.getNode(),
+  "this HTTP request parameter"

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

@@ -0,0 +1,62 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+
+<overview>
+  <p>
+    Most JavaScript objects inherit the properties of the built-in <code>Object.prototype</code> object.
+    Prototype pollution is a type of vulnerability in which an attacker is able to modify <code>Object.prototype</code>.
+    Since most objects inherit from the compromised <code>Object.prototype</code> object, the attacker can use this
+    to tamper with the application logic, and often escalate to remote code execution or cross-site scripting.
+  </p>
+
+  <p>
+    One way to cause prototype pollution is by modifying an object obtained via a user-controlled property name.
+    Most objects have a special <code>__proto__</code> property that refers to <code>Object.prototype</code>.
+    An attacker can abuse this special property to trick the application into performing unintended modifications
+    of <code>Object.prototype</code>.
+  </p>
+</overview>
+
+<recommendation>
+  <p>
+    Use an associative data structure that is resilient to untrusted key values, such as a <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map">Map</a>.
+    In some cases, a prototype-less object created with <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/create">Object.create(null)</a>
+    may be preferable.
+  </p>
+  <p>
+    Alternatively, restrict the computed property name so it can't clash with a built-in property, either by
+    prefixing it with a constant string, or by rejecting inputs that don't conform to the expected format.
+  </p>
+</recommendation>
+
+<example>
+  <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
+    onto <code>Object.prototype</code>.
+  </p>
+
+  <sample src="examples/PrototypePollutingAssignment.js"/>
+
+  <p>
+    One way to fix this is to use <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map">Map</a> objects to associate key/value pairs
+    instead of regular objects, as shown below:
+  </p>
+
+  <sample src="examples/PrototypePollutingAssignmentFixed.js"/>
+  
+</example>
+
+<references>
+  <li>MDN:
+    <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto">Object.prototype.__proto__</a>
+  </li>
+  <li>MDN:
+    <a href="https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map">Map</a>
+  </li>
+</references>
+</qhelp>

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

@@ -0,0 +1,26 @@
+/**
+ * @name Prototype-polluting assignment
+ * @description Modifying an object obtained via a user-controlled property name may
+ *              lead to accidental mutation of the built-in Object prototype,
+ *              and possibly escalate to remote code execution or cross-site scripting.
+ * @kind path-problem
+ * @problem.severity warning
+ * @precision high
+ * @id js/prototype-polluting-assignment
+ * @tags security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-094
+ *       external/cwe/cwe-400
+ *       external/cwe/cwe-915
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.PrototypePollutingAssignment::PrototypePollutingAssignment
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink, source, sink,
+  "This assignment may alter Object.prototype if a malicious '__proto__' string is injected from $@.",
+  source.getNode(), "here"

…/CWE-400/PrototypePollutionUtility.qhelp …CWE-915/PrototypePollutingFunction.qhelpjavascript/ql/src/Security/CWE-400/PrototypePollutionUtility.qhelp renamed to javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.qhelp javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.qhelp renamed to javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.qhelp

@@ -29,7 +29,7 @@
 
   <p>
     Only merge or assign a property recursively when it is an own property of the <em>destination</em> object.
-    Alternatively, blacklist the property names <code>__proto__</code> and <code>constructor</code>
+    Alternatively, block the property names <code>__proto__</code> and <code>constructor</code>
     from being merged or assigned to.
   </p>
 </recommendation>
@@ -39,7 +39,7 @@
     This function recursively copies properties from <code>src</code> to <code>dst</code>:
   </p>
 
-  <sample src="examples/PrototypePollutionUtility.js"/>
+  <sample src="examples/PrototypePollutingFunction.js"/>
 
   <p>
     However, if <code>src</code> is the object <code>{"__proto__": {"isAdmin": true}}</code>,
@@ -51,13 +51,13 @@
     are merged recursively:
   </p>
 
-  <sample src="examples/PrototypePollutionUtility_fixed.js"/>
+  <sample src="examples/PrototypePollutingFunction_fixed.js"/>
 
   <p>
-    Alternatively, blacklist the <code>__proto__</code> and <code>constructor</code> properties:
+    Alternatively, block the <code>__proto__</code> and <code>constructor</code> properties:
   </p>
 
-  <sample src="examples/PrototypePollutionUtility_fixed2.js"/>
+  <sample src="examples/PrototypePollutingFunction_fixed2.js"/>
 </example>
 
 <references>

…ity/CWE-400/PrototypePollutionUtility.ql …ty/CWE-915/PrototypePollutingFunction.qljavascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql renamed to javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.ql javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql renamed to javascript/ql/src/Security/CWE-915/PrototypePollutingFunction.ql

@@ -1,14 +1,17 @@
 /**
- * @name Prototype pollution in utility function
- * @description Recursively assigning properties on objects may cause
- *              accidental modification of a built-in prototype object.
+ * @name Prototype-polluting function
+ * @description Functions recursively assigning properties on objects may be
+ *              the cause of accidental modification of a built-in prototype object.
  * @kind path-problem
  * @problem.severity warning
  * @precision high
  * @id js/prototype-pollution-utility
  * @tags security
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-094
  *       external/cwe/cwe-400
- *       external/cwe/cwe-471
+ *       external/cwe/cwe-915
  */
 
 import javascript
@@ -276,26 +279,26 @@ class PropNameTracking extends DataFlow::Configuration {
   }
 
   override predicate isBarrierGuard(DataFlow::BarrierGuardNode node) {
-    node instanceof BlacklistEqualityGuard or
-    node instanceof WhitelistEqualityGuard or
+    node instanceof DenyListEqualityGuard or
+    node instanceof AllowListEqualityGuard or
     node instanceof HasOwnPropertyGuard or
     node instanceof InExprGuard or
     node instanceof InstanceOfGuard or
     node instanceof TypeofGuard or
-    node instanceof BlacklistInclusionGuard or
-    node instanceof WhitelistInclusionGuard or
+    node instanceof DenyListInclusionGuard or
+    node instanceof AllowListInclusionGuard or
     node instanceof IsPlainObjectGuard
   }
 }
 
 /**
  * Sanitizer guard of form `x === "__proto__"` or `x === "constructor"`.
  */
-class BlacklistEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNode {
+class DenyListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNode {
   override EqualityTest astNode;
   string propName;
 
-  BlacklistEqualityGuard() {
+  DenyListEqualityGuard() {
     astNode.getAnOperand().getStringValue() = propName and
     propName = unsafePropName()
   }
@@ -310,10 +313,10 @@ class BlacklistEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNod
 /**
  * An equality test with something other than `__proto__` or `constructor`.
  */
-class WhitelistEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNode {
+class AllowListEqualityGuard extends DataFlow::LabeledBarrierGuardNode, ValueNode {
   override EqualityTest astNode;
 
-  WhitelistEqualityGuard() {
+  AllowListEqualityGuard() {
     not astNode.getAnOperand().getStringValue() = unsafePropName() and
     astNode.getAnOperand() instanceof Literal
   }
@@ -427,10 +430,10 @@ class TypeofGuard extends DataFlow::LabeledBarrierGuardNode, DataFlow::ValueNode
 /**
  * A check of form `["__proto__"].includes(x)` or similar.
  */
-class BlacklistInclusionGuard extends DataFlow::LabeledBarrierGuardNode, InclusionTest {
+class DenyListInclusionGuard extends DataFlow::LabeledBarrierGuardNode, InclusionTest {
   UnsafePropLabel label;
 
-  BlacklistInclusionGuard() {
+  DenyListInclusionGuard() {
     exists(DataFlow::ArrayCreationNode array |
       array.getAnElement().getStringValue() = label and
       array.flowsTo(getContainerNode())
@@ -447,8 +450,8 @@ class BlacklistInclusionGuard extends DataFlow::LabeledBarrierGuardNode, Inclusi
 /**
  * A check of form `xs.includes(x)` or similar, which sanitizes `x` in the true case.
  */
-class WhitelistInclusionGuard extends DataFlow::LabeledBarrierGuardNode {
-  WhitelistInclusionGuard() {
+class AllowListInclusionGuard extends DataFlow::LabeledBarrierGuardNode {
+  AllowListInclusionGuard() {
     this instanceof TaintTracking::PositiveIndexOfSanitizer
     or
     this instanceof TaintTracking::MembershipTestSanitizer and

…ecurity/CWE-400/PrototypePollution.qhelp …WE-915/PrototypePollutingMergeCall.qhelpjavascript/ql/src/Security/CWE-400/PrototypePollution.qhelp renamed to javascript/ql/src/Security/CWE-915/PrototypePollutingMergeCall.qhelp javascript/ql/src/Security/CWE-400/PrototypePollution.qhelp renamed to javascript/ql/src/Security/CWE-915/PrototypePollutingMergeCall.qhelp

@@ -34,7 +34,7 @@
     and then copied into a new object:
   </p>
 
-  <sample src="examples/PrototypePollution1.js"/>
+  <sample src="examples/PrototypePollutingMergeCall1.js"/>
 
   <p>
     Prior to lodash 4.17.11 this would be vulnerable to prototype pollution. An attacker could send the following GET request:
@@ -47,7 +47,7 @@
     Fix this by updating the lodash version:
   </p>
 
-  <sample src="examples/PrototypePollution_fixed.json"/>
+  <sample src="examples/PrototypePollutingMergeCall_fixed.json"/>
 
   <p>
     Note that some web frameworks, such as Express, parse query parameters using extended URL-encoding
@@ -56,7 +56,7 @@
     The example below would also be susceptible to prototype pollution:
   </p>
 
-  <sample src="examples/PrototypePollution2.js"/>
+  <sample src="examples/PrototypePollutingMergeCall2.js"/>
 
   <p>
     In the above example, an attacker can cause prototype pollution by sending the following GET request:

…c/Security/CWE-400/PrototypePollution.ql …y/CWE-915/PrototypePollutingMergeCall.qljavascript/ql/src/Security/CWE-400/PrototypePollution.ql renamed to javascript/ql/src/Security/CWE-915/PrototypePollutingMergeCall.ql javascript/ql/src/Security/CWE-400/PrototypePollution.ql renamed to javascript/ql/src/Security/CWE-915/PrototypePollutingMergeCall.ql

@@ -1,14 +1,18 @@
 /**
- * @name Prototype pollution
+ * @name Prototype-polluting merge call
  * @description Recursively merging a user-controlled object into another object
- *              can allow an attacker to modify the built-in Object prototype.
+ *              can allow an attacker to modify the built-in Object prototype,
+ *              and possibly escalate to remote code execution or cross-site scripting.
  * @kind path-problem
  * @problem.severity error
  * @precision high
  * @id js/prototype-pollution
  * @tags security
- *       external/cwe/cwe-250
+ *       external/cwe/cwe-078
+ *       external/cwe/cwe-079
+ *       external/cwe/cwe-094
  *       external/cwe/cwe-400
+ *       external/cwe/cwe-915
  */
 
 import javascript

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

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