Merge pull request github#3415 from esbena/js/membershiptest

Approved by asgerf

Author: semmle-qlci

change-notes/1.25/analysis-javascript.md

@@ -23,6 +23,9 @@
 
 * TypeScript 3.9 is now supported.
 
+* The analysis of sanitizers has improved, leading to more accurate
+  results from the security queries.
+
 ## New queries
 
 | **Query**                                                                       | **Tags**                                                          | **Purpose**                                                                                                                                                                            |

javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql

@@ -61,18 +61,14 @@ DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
     sw.getSubstring().mayHaveStringValue(scheme)
   )
   or
-  // check of the form `array.includes(getScheme(nd))`
-  exists(InclusionTest test, DataFlow::ArrayCreationNode array | test = result |
-    schemeOf(nd).flowsTo(test.getContainedNode()) and
-    array.flowsTo(test.getContainerNode()) and
-    array.getAnElement().mayHaveStringValue(scheme.getWithOrWithoutColon())
-  )
-  or
-  // check of the form `getScheme(nd) === scheme`
-  exists(EqualityTest test, Expr op1, Expr op2 | test.flow() = result |
-    test.hasOperands(op1, op2) and
-    schemeOf(nd).flowsToExpr(op1) and
-    op2.mayHaveStringValue(scheme.getWithOrWithoutColon())
+  exists(MembershipCandidate candidate |
+    result = candidate.getTest()
+    or
+    // fall back to the candidate if the test itself is implicit
+    not exists(candidate.getTest()) and result = candidate
+  |
+    candidate.getAMemberString() = scheme.getWithOrWithoutColon() and
+    schemeOf(nd).flowsTo(candidate)
   )
   or
   // propagate through trimming, case conversion, and regexp replace

javascript/ql/src/Security/CWE-400/PrototypePollutionUtility.ql

@@ -449,8 +449,10 @@ class BlacklistInclusionGuard extends DataFlow::LabeledBarrierGuardNode, Inclusi
  */
 class WhitelistInclusionGuard extends DataFlow::LabeledBarrierGuardNode {
   WhitelistInclusionGuard() {
-    this instanceof TaintTracking::PositiveIndexOfSanitizer or
-    this instanceof TaintTracking::InclusionSanitizer
+    this instanceof TaintTracking::PositiveIndexOfSanitizer
+    or
+    this instanceof TaintTracking::MembershipTestSanitizer and
+    not this = any(MembershipCandidate::ObjectPropertyNameMembershipCandidate c).getTest() // handled with more precision in `HasOwnPropertyGuard`
   }
 
   override predicate blocks(boolean outcome, Expr e, DataFlow::FlowLabel lbl) {

javascript/ql/src/javascript.qll

@@ -37,6 +37,7 @@ import semmle.javascript.JsonParsers
 import semmle.javascript.JSX
 import semmle.javascript.Lines
 import semmle.javascript.Locations
+import semmle.javascript.MembershipCandidates
 import semmle.javascript.Modules
 import semmle.javascript.NodeJS
 import semmle.javascript.NPM

javascript/ql/src/semmle/javascript/InclusionTests.qll

@@ -5,7 +5,7 @@
 private import javascript
 
 /**
- * A expression that checks if an element is contained in an array
+ * An expression that checks if an element is contained in an array
  * or is a substring of another string.
  *
  * Examples:

javascript/ql/src/semmle/javascript/MembershipCandidates.qll

@@ -0,0 +1,261 @@
+/**
+ * Provides classes for recognizing membership tests.
+ */
+
+import javascript
+
+/**
+ * An expression that is tested for membership of a collection.
+ *
+ * Additional candidates can be added by subclassing `MembershipCandidate::Range`
+ */
+class MembershipCandidate extends DataFlow::Node {
+  MembershipCandidate::Range range;
+
+  MembershipCandidate() { this = range }
+
+  /**
+   * Gets the expression that performs the membership test, if any.
+   */
+  DataFlow::Node getTest() { result = range.getTest() }
+
+  /**
+   * Gets a string that this candidate is tested against, if
+   * it can be determined.
+   */
+  string getAMemberString() { result = range.getAMemberString() }
+
+  /**
+   * Gets a node that this candidate is tested against, if
+   * it can be determined.
+   */
+  DataFlow::Node getAMemberNode() { result = range.getAMemberNode() }
+
+  /**
+   * Gets the polarity of the test.
+   *
+   * If the polarity is `false` the test returns `true` if the
+   * collection does not contain this candidate.
+   */
+  boolean getTestPolarity() { result = range.getTestPolarity() }
+}
+
+/**
+ * Provides classes for recognizing membership candidates.
+ */
+module MembershipCandidate {
+  /**
+   * An expression that is tested for membership of a collection.
+   */
+  abstract class Range extends DataFlow::Node {
+    /**
+     * Gets the expression that performs the membership test, if any.
+     */
+    abstract DataFlow::Node getTest();
+
+    /**
+     * Gets a string that this candidate is tested against, if
+     * it can be determined.
+     */
+    string getAMemberString() { this.getAMemberNode().mayHaveStringValue(result) }
+
+    /**
+     * Gets a node that this candidate is tested against, if
+     * it can be determined.
+     */
+    DataFlow::Node getAMemberNode() { none() }
+
+    /**
+     * Gets the polarity of the test.
+     *
+     * If the polarity is `false` the test returns `true` if the
+     * collection does not contain this candidate.
+     */
+    boolean getTestPolarity() { result = true }
+  }
+
+  /**
+   * An `InclusionTest` candidate viewed as a `MembershipCandidate`.
+   */
+  private class OrdinaryInclusionTestCandidate extends MembershipCandidate::Range {
+    InclusionTest test;
+
+    OrdinaryInclusionTestCandidate() { this = test.getContainedNode() }
+
+    override DataFlow::Node getTest() { result = test }
+
+    override boolean getTestPolarity() { result = test.getPolarity() }
+  }
+
+  /**
+   * A candidate that may be a member of an array.
+   */
+  class ArrayMembershipCandidate extends OrdinaryInclusionTestCandidate {
+    DataFlow::ArrayCreationNode array;
+
+    ArrayMembershipCandidate() { array.flowsTo(test.getContainerNode()) }
+
+    /**
+     * Gets the array of this test.
+     */
+    DataFlow::ArrayCreationNode getArray() { result = array }
+
+    override DataFlow::Node getAMemberNode() { result = array.getAnElement() }
+  }
+
+  /**
+   * A candidate that may be a member of an array constructed
+   * from a call to `String.prototype.split`.
+   */
+  private class ShorthandArrayMembershipCandidate extends OrdinaryInclusionTestCandidate {
+    string toSplit;
+    string separator;
+
+    ShorthandArrayMembershipCandidate() {
+      exists(StringSplitCall split |
+        split.flowsTo(test.getContainerNode()) and
+        toSplit = split.getBaseString().getStringValue() and
+        separator = split.getSeparator()
+      )
+    }
+
+    override string getAMemberString() { result = toSplit.splitAt(separator) }
+  }
+
+  /**
+   * An `EqualityTest` operand viewed as a `MembershipCandidate`.
+   */
+  private class EqualityTestOperand extends MembershipCandidate::Range, DataFlow::ValueNode {
+    EqualityTest test;
+
+    EqualityTestOperand() { this = test.getAnOperand().flow() }
+
+    override DataFlow::Node getTest() { result = test.flow() }
+
+    override DataFlow::Node getAMemberNode() { test.hasOperands(this.asExpr(), result.asExpr()) }
+
+    override boolean getTestPolarity() { result = test.getPolarity() }
+  }
+
+  /**
+   * A regular expression that enumerates all of its matched strings.
+   */
+  private class EnumerationRegExp extends RegExpTerm {
+    EnumerationRegExp() {
+      this.isRootTerm() and
+      RegExp::isFullyAnchoredTerm(this) and
+      exists(RegExpTerm child | this.getAChild*() = child |
+        child instanceof RegExpSequence or
+        child instanceof RegExpCaret or
+        child instanceof RegExpDollar or
+        child instanceof RegExpConstant or
+        child instanceof RegExpAlt or
+        child instanceof RegExpGroup
+      )
+    }
+
+    /**
+     * Gets a string matched by this regular expression.
+     */
+    string getAMember() { result = this.getAChild*().getAMatchedString() }
+  }
+
+  /**
+   * A candidate that may be matched by a regular expression that
+   * enumerates all of its matched strings.
+   */
+  private class RegExpEnumerationCandidate extends MembershipCandidate::Range, DataFlow::Node {
+    DataFlow::Node test;
+    EnumerationRegExp enumeration;
+    boolean polarity;
+
+    RegExpEnumerationCandidate() {
+      exists(DataFlow::MethodCallNode mcn, DataFlow::Node base, string m, DataFlow::Node firstArg |
+        (
+          test = any(ConditionGuardNode g).getTest().flow() and
+          mcn.flowsTo(test) and
+          polarity = true
+          or
+          exists(EqualityTest eq, Expr null, Expr op |
+            test = eq.flow() and
+            polarity = eq.getPolarity().booleanNot() and
+            mcn.flowsToExpr(op) and
+            eq.hasOperands(op, null) and
+            SyntacticConstants::isNull(null)
+          )
+        ) and
+        mcn.calls(base, m) and
+        firstArg = mcn.getArgument(0)
+      |
+        // /re/.test(u) or /re/.exec(u)
+        enumeration = RegExp::getRegExpObjectFromNode(base) and
+        (m = "test" or m = "exec") and
+        firstArg = this
+        or
+        // u.match(/re/) or u.match("re")
+        base = this and
+        m = "match" and
+        enumeration = RegExp::getRegExpFromNode(firstArg)
+      )
+    }
+
+    override DataFlow::Node getTest() { result = test }
+
+    override string getAMemberString() { result = enumeration.getAMember() }
+
+    override boolean getTestPolarity() { result = polarity }
+  }
+
+  /**
+   * A candidate that may be a member of a collection class, such as a map or set.
+   */
+  class CollectionMembershipCandidate extends MembershipCandidate::Range {
+    DataFlow::MethodCallNode test;
+
+    CollectionMembershipCandidate() { test.getMethodName() = "has" and this = test.getArgument(0) }
+
+    override DataFlow::Node getTest() { result = test }
+  }
+
+  /**
+   * A candidate that may be a property name of an object.
+   */
+  class ObjectPropertyNameMembershipCandidate extends MembershipCandidate::Range,
+    DataFlow::ValueNode {
+    DataFlow::ValueNode test;
+    DataFlow::ValueNode membersNode;
+
+    ObjectPropertyNameMembershipCandidate() {
+      exists(InExpr inExpr |
+        this = inExpr.getLeftOperand().flow() and
+        test = inExpr.flow() and
+        membersNode = inExpr.getRightOperand().flow()
+      )
+      or
+      exists(DataFlow::MethodCallNode hasOwn |
+        this = hasOwn.getArgument(0) and
+        test = hasOwn and
+        hasOwn.calls(membersNode, "hasOwnProperty")
+      )
+    }
+
+    override DataFlow::Node getTest() { result = test }
+
+    override string getAMemberString() {
+      exists(membersNode.getALocalSource().getAPropertyWrite(result))
+    }
+  }
+
+  /**
+   * A candidate that may match a switch case.
+   */
+  class SwitchCaseCandidate extends MembershipCandidate::Range {
+    SwitchStmt switch;
+
+    SwitchCaseCandidate() { this = switch.getExpr().flow() }
+
+    override DataFlow::Node getTest() { none() }
+
+    override DataFlow::Node getAMemberNode() { result = switch.getACase().getExpr().flow() }
+  }
+}

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -827,6 +827,9 @@ module TaintTracking {
     override predicate appliesTo(Configuration cfg) { any() }
   }
 
+  /** DEPRECATED. This class has been renamed to `MembershipTestSanitizer`. */
+  deprecated class StringInclusionSanitizer = MembershipTestSanitizer;
+
   /**
    * A test of form `x.length === "0"`, preventing `x` from being tainted.
    */
@@ -849,18 +852,19 @@ module TaintTracking {
     override predicate appliesTo(Configuration cfg) { any() }
   }
 
-  /** DEPRECATED. This class has been renamed to `InclusionSanitizer`. */
-  deprecated class StringInclusionSanitizer = InclusionSanitizer;
+  /** DEPRECATED. This class has been renamed to `MembershipTestSanitizer`. */
+  deprecated class InclusionSanitizer = MembershipTestSanitizer;
 
-  /** A check of the form `whitelist.includes(x)` or equivalent, which sanitizes `x` in its "then" branch. */
-  class InclusionSanitizer extends AdditionalSanitizerGuardNode {
-    InclusionTest inclusion;
+  /**
+   * A check of the form `whitelist.includes(x)` or equivalent, which sanitizes `x` in its "then" branch.
+   */
+  class MembershipTestSanitizer extends AdditionalSanitizerGuardNode {
+    MembershipCandidate candidate;
 
-    InclusionSanitizer() { this = inclusion }
+    MembershipTestSanitizer() { this = candidate.getTest() }
 
     override predicate sanitizes(boolean outcome, Expr e) {
-      outcome = inclusion.getPolarity() and
-      e = inclusion.getContainedNode().asExpr()
+      candidate = e.flow() and candidate.getTestPolarity() = outcome
     }
 
     override predicate appliesTo(Configuration cfg) { any() }
@@ -895,8 +899,12 @@ module TaintTracking {
   /** Gets a variable that is defined exactly once. */
   private Variable singleDef() { strictcount(result.getADefinition()) = 1 }
 
-  /** A check of the form `if(x == 'some-constant')`, which sanitizes `x` in its "then" branch. */
-  class ConstantComparison extends AdditionalSanitizerGuardNode, DataFlow::ValueNode {
+  /**
+   * A check of the form `if(x == 'some-constant')`, which sanitizes `x` in its "then" branch.
+   *
+   * DEPRECATED: use `MembershipTestSanitizer` instead.
+   */
+  deprecated class ConstantComparison extends SanitizerGuardNode, DataFlow::ValueNode {
     Expr x;
     override EqualityTest astNode;
 
@@ -914,7 +922,10 @@ module TaintTracking {
       outcome = astNode.getPolarity() and x = e
     }
 
-    override predicate appliesTo(Configuration cfg) { any() }
+    /**
+     * Holds if this guard applies to the flow in `cfg`.
+     */
+    predicate appliesTo(Configuration cfg) { any() }
   }
 
   /**