Merge pull request #665 from asger-semmle/js-property-concat-sanitizer

Approved by esben-semmle, xiemaisi

change-notes/1.20/analysis-javascript.md

@@ -33,6 +33,8 @@
 | Unused variable, import, function or class | Fewer false-positive results | This rule now flags fewer variables that are implictly used by JSX elements, and no longer flags variables with leading underscore. |
 | Uncontrolled data used in path expression | Fewer false-positive results | This rule now recognizes the Express `root` option, which prevents path traversal. |
 | Useless assignment to property. | Fewer false-positive results | This rule now treats assignments with complex right-hand sides correctly. |
+| Unsafe dynamic method access              | Fewer false-positive results | This rule no longer flags concatenated strings as unsafe method names. |
+| Unvalidated dynamic method call           | More true-positive results | This rule now flags concatenated strings as unvalidated method names in more cases. |
 
 ## Changes to QL libraries
 

javascript/ql/src/semmle/javascript/StringConcatenation.qll

@@ -75,4 +75,35 @@ module StringConcatenation {
    * Holds if there is a taint step from `src` to `dst` through string concatenation.
    */
   predicate taintStep(DataFlow::Node src, DataFlow::Node dst) { taintStep(src, dst, _, _) }
+
+  /**
+   * Holds if `node` is the root of a concatenation tree, that is,
+   * it is a concatenation operator that is not itself the immediate operand to
+   * another concatenation operator.
+   */
+  predicate isRoot(DataFlow::Node node) {
+    exists(getAnOperand(node)) and
+    not node = getAnOperand(_)
+  }
+
+  /**
+   * Gets the root of the concatenation tree in which `node` is an operand or operator.
+   */
+  DataFlow::Node getRoot(DataFlow::Node node) {
+    isRoot(node) and
+    result = node
+    or
+    exists(DataFlow::Node operator |
+      node = getAnOperand(operator) and
+      result = getRoot(operator)
+    )
+  }
+
+  /**
+   * Holds if `node` is a string concatenation that only acts as a string coercion.
+   */
+  predicate isCoercion(DataFlow::Node node) {
+    getNumOperand(node) = 2 and
+    getOperand(node, _).asExpr().getStringValue() = ""
+  }
 }

javascript/ql/src/semmle/javascript/security/dataflow/PropertyInjectionShared.qll

@@ -6,18 +6,6 @@
 import javascript
 
 module PropertyInjection {
-  /**
-   * A data-flow node that sanitizes user-controlled property names that flow through it.
-   */
-  abstract class Sanitizer extends DataFlow::Node { }
-
-  /**
-   * Concatenation with a constant, acting as a sanitizer.
-   */
-  private class ConcatSanitizer extends Sanitizer {
-    ConcatSanitizer() { StringConcatenation::getAnOperand(this).asExpr() instanceof ConstantString }
-  }
-
   /**
    * Holds if the methods of the given value are unsafe, such as `eval`.
    */

javascript/ql/src/semmle/javascript/security/dataflow/RemotePropertyInjection.qll

@@ -42,7 +42,7 @@ module RemotePropertyInjection {
     override predicate isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node) or
       node instanceof Sanitizer or
-      node instanceof PropertyInjection::Sanitizer
+      node = StringConcatenation::getRoot(any(ConstantString str).flow())
     }
   }
 

javascript/ql/src/semmle/javascript/security/dataflow/UnsafeDynamicMethodAccess.qll

@@ -60,9 +60,12 @@ module UnsafeDynamicMethodAccess {
     }
 
     override predicate isSanitizer(DataFlow::Node node) {
-      super.isSanitizer(node) or
+      super.isSanitizer(node)
-      node instanceof Sanitizer or
+      or
-      node instanceof PropertyInjection::Sanitizer
+      node instanceof Sanitizer
+      or
+      exists(StringConcatenation::getOperand(node, _)) and
+      not StringConcatenation::isCoercion(node)
     }
 
     /**

javascript/ql/src/semmle/javascript/security/dataflow/UnvalidatedDynamicMethodCall.qll

@@ -68,10 +68,7 @@ module UnvalidatedDynamicMethodCall {
       sink.(Sink).getFlowLabel() = label
     }
 
-    override predicate isSanitizer(DataFlow::Node nd) {
+    override predicate isSanitizer(DataFlow::Node nd) { super.isSanitizer(nd) }
-      super.isSanitizer(nd) or
-      nd instanceof PropertyInjection::Sanitizer
-    }
 
     override predicate isAdditionalFlowStep(
       DataFlow::Node src, DataFlow::Node dst, DataFlow::FlowLabel srclabel,

javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/UnsafeDynamicMethodAccess.expected

@@ -23,6 +23,10 @@ nodes
 | tst.js:11:7:11:18 | message.name |
 | tst.js:15:5:15:14 | window[ev] |
 | tst.js:15:12:15:13 | ev |
+| tst.js:21:5:21:29 | window[ ... e.name] |
+| tst.js:21:12:21:28 | '' + message.name |
+| tst.js:21:17:21:23 | message |
+| tst.js:21:17:21:28 | message.name |
 edges
 | example.js:9:37:9:38 | ev | example.js:10:30:10:31 | ev |
 | example.js:10:9:10:37 | message | example.js:13:12:13:18 | message |
@@ -36,6 +40,7 @@ edges
 | tst.js:4:9:4:37 | message | tst.js:5:12:5:18 | message |
 | tst.js:4:9:4:37 | message | tst.js:6:16:6:22 | message |
 | tst.js:4:9:4:37 | message | tst.js:11:7:11:13 | message |
+| tst.js:4:9:4:37 | message | tst.js:21:17:21:23 | message |
 | tst.js:4:19:4:37 | JSON.parse(ev.data) | tst.js:4:9:4:37 | message |
 | tst.js:4:30:4:31 | ev | tst.js:4:30:4:36 | ev.data |
 | tst.js:4:30:4:36 | ev.data | tst.js:4:19:4:37 | JSON.parse(ev.data) |
@@ -46,9 +51,13 @@ edges
 | tst.js:11:7:11:13 | message | tst.js:11:7:11:18 | message.name |
 | tst.js:11:7:11:18 | message.name | tst.js:11:5:11:19 | f[message.name] |
 | tst.js:15:12:15:13 | ev | tst.js:15:5:15:14 | window[ev] |
+| tst.js:21:12:21:28 | '' + message.name | tst.js:21:5:21:29 | window[ ... e.name] |
+| tst.js:21:17:21:23 | message | tst.js:21:17:21:28 | message.name |
+| tst.js:21:17:21:28 | message.name | tst.js:21:12:21:28 | '' + message.name |
 #select
 | example.js:13:5:13:24 | window[message.name] | example.js:9:37:9:38 | ev | example.js:13:5:13:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | example.js:9:37:9:38 | ev | user-controlled value |
 | tst.js:5:5:5:24 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:5:5:5:24 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |
 | tst.js:6:9:6:28 | window[message.name] | tst.js:3:37:3:38 | ev | tst.js:6:9:6:28 | window[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |
 | tst.js:11:5:11:19 | f[message.name] | tst.js:3:37:3:38 | ev | tst.js:11:5:11:19 | f[message.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |
 | tst.js:15:5:15:14 | window[ev] | tst.js:3:37:3:38 | ev | tst.js:15:5:15:14 | window[ev] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |
+| tst.js:21:5:21:29 | window[ ... e.name] | tst.js:3:37:3:38 | ev | tst.js:21:5:21:29 | window[ ... e.name] | Invocation of method derived from $@ may lead to remote code execution. | tst.js:3:37:3:38 | ev | user-controlled value |

javascript/ql/test/query-tests/Security/CWE-094/UnsafeDynamicMethodAccess/tst.js

@@ -13,4 +13,10 @@ window.addEventListener('message', (ev) => {
     obj[message.name](message.payload); // OK - may crash, but no code execution involved
 
     window[ev](ev); // NOT OK
+
+    window[configData() + ' ' + message.name](message.payload); // OK - concatenation restricts choice of methods
+
+    window[configData() + message.name](message.payload); // OK - concatenation restricts choice of methods
+
+    window['' + message.name](message.payload); // NOT OK - coercion does not restrict choice of methods
 });

javascript/ql/test/query-tests/Security/CWE-754/UnvalidatedDynamicMethodCall.expected

@@ -46,6 +46,14 @@ nodes
 | tst.js:26:11:26:14 | name |
 | tst.js:28:7:28:15 | obj[name] |
 | tst.js:28:11:28:14 | name |
+| tst.js:34:9:34:24 | key |
+| tst.js:34:15:34:24 | "$" + name |
+| tst.js:34:21:34:24 | name |
+| tst.js:35:5:35:12 | obj[key] |
+| tst.js:35:5:35:12 | obj[key] |
+| tst.js:35:9:35:11 | key |
+| tst.js:37:7:37:14 | obj[key] |
+| tst.js:37:11:37:13 | key |
 | tst.js:47:39:47:40 | ev |
 | tst.js:48:9:48:39 | name |
 | tst.js:48:16:48:34 | JSON.parse(ev.data) |
@@ -78,6 +86,7 @@ edges
 | tst.js:7:9:7:39 | name | tst.js:21:11:21:14 | name |
 | tst.js:7:9:7:39 | name | tst.js:26:11:26:14 | name |
 | tst.js:7:9:7:39 | name | tst.js:28:11:28:14 | name |
+| tst.js:7:9:7:39 | name | tst.js:34:21:34:24 | name |
 | tst.js:7:16:7:34 | JSON.parse(ev.data) | tst.js:7:16:7:39 | JSON.pa ... a).name |
 | tst.js:7:16:7:39 | JSON.pa ... a).name | tst.js:7:9:7:39 | name |
 | tst.js:7:27:7:28 | ev | tst.js:7:27:7:33 | ev.data |
@@ -101,6 +110,13 @@ edges
 | tst.js:26:11:26:14 | name | tst.js:26:7:26:15 | obj[name] |
 | tst.js:26:11:26:14 | name | tst.js:26:7:26:15 | obj[name] |
 | tst.js:28:11:28:14 | name | tst.js:28:7:28:15 | obj[name] |
+| tst.js:34:9:34:24 | key | tst.js:35:9:35:11 | key |
+| tst.js:34:9:34:24 | key | tst.js:37:11:37:13 | key |
+| tst.js:34:15:34:24 | "$" + name | tst.js:34:9:34:24 | key |
+| tst.js:34:21:34:24 | name | tst.js:34:15:34:24 | "$" + name |
+| tst.js:35:9:35:11 | key | tst.js:35:5:35:12 | obj[key] |
+| tst.js:35:9:35:11 | key | tst.js:35:5:35:12 | obj[key] |
+| tst.js:37:11:37:13 | key | tst.js:37:7:37:14 | obj[key] |
 | tst.js:47:39:47:40 | ev | tst.js:48:27:48:28 | ev |
 | tst.js:48:9:48:39 | name | tst.js:49:19:49:22 | name |
 | tst.js:48:16:48:34 | JSON.parse(ev.data) | tst.js:48:16:48:39 | JSON.pa ... a).name |
@@ -128,4 +144,7 @@ edges
 | tst.js:26:7:26:15 | obj[name] | tst.js:6:39:6:40 | ev | tst.js:26:7:26:15 | obj[name] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
 | tst.js:26:7:26:15 | obj[name] | tst.js:6:39:6:40 | ev | tst.js:26:7:26:15 | obj[name] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
 | tst.js:28:7:28:15 | obj[name] | tst.js:6:39:6:40 | ev | tst.js:28:7:28:15 | obj[name] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
+| tst.js:35:5:35:12 | obj[key] | tst.js:6:39:6:40 | ev | tst.js:35:5:35:12 | obj[key] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
+| tst.js:35:5:35:12 | obj[key] | tst.js:6:39:6:40 | ev | tst.js:35:5:35:12 | obj[key] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
+| tst.js:37:7:37:14 | obj[key] | tst.js:6:39:6:40 | ev | tst.js:37:7:37:14 | obj[key] | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:6:39:6:40 | ev | user-controlled |
 | tst.js:50:5:50:6 | fn | tst.js:47:39:47:40 | ev | tst.js:50:5:50:6 | fn | Invocation of method with $@ name may dispatch to unexpected target and cause an exception. | tst.js:47:39:47:40 | ev | user-controlled |

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

@@ -32,9 +32,9 @@
     }
 
     let key = "$" + name;
-    obj[key]();     // NOT OK, but not flagged
+    obj[key]();     // NOT OK
     if (typeof obj[key] === 'function')
-      obj[key]();   // OK
+      obj[key]();   // OK - but still flagged
 
     if (typeof fn === 'function') {
       fn.apply(obj); // OK