Merge pull request #201 from asger-semmle/string-concatenation-squashed

Approved by esben-semmle

javascript/ql/src/javascript.qll

@@ -38,6 +38,7 @@ import semmle.javascript.Regexp
 import semmle.javascript.SSA
 import semmle.javascript.StandardLibrary
 import semmle.javascript.Stmt
+import semmle.javascript.StringConcatenation
 import semmle.javascript.Templates
 import semmle.javascript.Tokens
 import semmle.javascript.TypeScript

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

@@ -0,0 +1,80 @@
+/**
+ * Provides predicates for analyzing string concatenations and their operands.
+ */
+import javascript
+
+module StringConcatenation {
+  /** Gets a data flow node referring to the result of the given concatenation. */
+  private DataFlow::Node getAssignAddResult(AssignAddExpr expr) {
+    result = expr.flow()
+    or
+    exists (SsaExplicitDefinition def | def.getDef() = expr |
+      result = DataFlow::valueNode(def.getVariable().getAUse()))
+  }
+
+  /** Gets the `n`th operand to the string concatenation defining `node`. */
+  DataFlow::Node getOperand(DataFlow::Node node, int n) {
+    exists (AddExpr add | node = add.flow() |
+      n = 0 and result = add.getLeftOperand().flow()
+      or
+      n = 1 and result = add.getRightOperand().flow())
+    or
+    exists (TemplateLiteral template | node = template.flow() |
+      result = template.getElement(n).flow() and
+      not exists (TaggedTemplateExpr tag | template = tag.getTemplate()))
+    or
+    exists (AssignAddExpr assign | node = getAssignAddResult(assign) |
+      n = 0 and result = assign.getLhs().flow()
+      or
+      n = 1 and result = assign.getRhs().flow())
+    or
+    exists (DataFlow::ArrayCreationNode array, DataFlow::MethodCallNode call |
+      call = array.getAMethodCall("join") and
+      call.getArgument(0).mayHaveStringValue("") and
+      (
+        // step from array element to array
+        result = array.getElement(n) and
+        node = array
+        or
+        // step from array to join call
+        node = call and
+        result = array and
+        n = 0
+      ))
+  }
+
+  /** Gets an operand to the string concatenation defining `node`. */
+  DataFlow::Node getAnOperand(DataFlow::Node node) {
+    result = getOperand(node, _)
+  }
+
+  /** Gets the number of operands to the given concatenation. */
+  int getNumOperand(DataFlow::Node node) {
+    result = strictcount(getAnOperand(node))
+  }
+
+  /** Gets the first operand to the string concatenation defining `node`. */
+  DataFlow::Node getFirstOperand(DataFlow::Node node) {
+    result = getOperand(node, 0)
+  }
+
+  /** Gets the last operand to the string concatenation defining `node`. */
+  DataFlow::Node getLastOperand(DataFlow::Node node) {
+    result = getOperand(node, getNumOperand(node) - 1)
+  }
+  
+  /**
+   * Holds if `src` flows to `dst` through the `n`th operand of the given concatenation operator.
+   */
+  predicate taintStep(DataFlow::Node src, DataFlow::Node dst, DataFlow::Node operator, int n) {
+    src = getOperand(dst, n) and
+    operator = dst
+  }
+
+  /**
+   * 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, _, _)
+  }
+}

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

@@ -304,6 +304,47 @@ class ArrayLiteralNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode
 
 }
 
+/** A data flow node corresponding to a `new Array()` or `Array()` invocation. */
+class ArrayConstructorInvokeNode extends DataFlow::InvokeNode {
+  ArrayConstructorInvokeNode() {
+    getCallee() = DataFlow::globalVarRef("Array")
+  }
+
+  /** Gets the `i`th initial element of this array, if one is provided. */
+  DataFlow::ValueNode getElement(int i) {
+    getNumArgument() > 1 and // A single-argument invocation specifies the array length, not an element.
+    result = getArgument(i)
+  }
+
+  /** Gets an initial element of this array, if one is provided. */
+  DataFlow::ValueNode getAnElement() {
+    getNumArgument() > 1 and
+    result = getAnArgument()
+  }
+}
+
+/**
+ * A data flow node corresponding to the creation or a new array, either through an array literal
+ * or an invocation of the `Array` constructor.
+ */
+class ArrayCreationNode extends DataFlow::ValueNode, DataFlow::DefaultSourceNode {
+  ArrayCreationNode() {
+    this instanceof ArrayLiteralNode or
+    this instanceof ArrayConstructorInvokeNode
+  }
+
+  /** Gets the `i`th initial element of this array, if one is provided. */
+  DataFlow::ValueNode getElement(int i) {
+    result = this.(ArrayLiteralNode).getElement(i) or
+    result = this.(ArrayConstructorInvokeNode).getElement(i)
+  }
+
+  /** Gets an initial element of this array, if one if provided. */
+  DataFlow::ValueNode getAnElement() {
+    result = getElement(_)
+  }
+}
+
 /**
  * A data flow node corresponding to a `default` import from a module, or a
  * (AMD or CommonJS) `require` of a module.

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

@@ -358,19 +358,8 @@ module TaintTracking {
    */
   class StringConcatenationTaintStep extends AdditionalTaintStep, DataFlow::ValueNode {
     override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
-      succ = this and
-      (
-        // addition propagates taint
-        astNode.(AddExpr).getAnOperand() = pred.asExpr() or
-        astNode.(AssignAddExpr).getAChildExpr() = pred.asExpr() or
-        exists (SsaExplicitDefinition ssa |
-          astNode = ssa.getVariable().getAUse() and
-          pred.asExpr().(AssignAddExpr) = ssa.getDef()
-        )
-        or
-        // templating propagates taint
-        astNode.(TemplateLiteral).getAnElement() = pred.asExpr()
-      )
+        succ = this and
+        StringConcatenation::taintStep(pred, succ)
     }
   }
   

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

@@ -77,9 +77,9 @@ module DomBasedXss {
         or
         // or it doesn't start with something other than `<`, and so at least
         // _may_ be interpreted as HTML
-        not exists (Expr prefix, string strval |
+        not exists (DataFlow::Node prefix, string strval |
           isPrefixOfJQueryHtmlString(astNode, prefix) and
-          strval = prefix.getStringValue() and
+          strval = prefix.asExpr().getStringValue() and
           not strval.regexpMatch("\\s*<.*")
         )
       )
@@ -93,13 +93,14 @@ module DomBasedXss {
    * Holds if `prefix` is a prefix of `htmlString`, which may be intepreted as
    * HTML by a jQuery method.
    */
-  private predicate isPrefixOfJQueryHtmlString(Expr htmlString, Expr prefix) {
+  private predicate isPrefixOfJQueryHtmlString(Expr htmlString, DataFlow::Node prefix) {
     any(JQueryMethodCall call).interpretsArgumentAsHtml(htmlString) and
-    prefix = htmlString
+    prefix = htmlString.flow()
     or
-    exists (Expr pred | isPrefixOfJQueryHtmlString(htmlString, pred) |
-      prefix = pred.(AddExpr).getLeftOperand() or
-      prefix = pred.(ParExpr).getExpression()
+    exists (DataFlow::Node pred | isPrefixOfJQueryHtmlString(htmlString, pred) |
+      prefix = StringConcatenation::getFirstOperand(pred)
+      or
+      prefix = pred.getAPredecessor()
     )
   }
 

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

@@ -34,33 +34,23 @@ module ServerSideUrlRedirect {
       )
     }
   }
-
-  /**
-   * Gets the left operand of `nd` if it is a concatenation.
-   */
-  private DataFlow::Node getPrefixOperand(DataFlow::Node nd) {
-    exists (Expr e | e instanceof AddExpr or e instanceof AssignAddExpr |
-      nd = DataFlow::valueNode(e) and
-      result = DataFlow::valueNode(e.getChildExpr(0))
-    )
-  }
   
   /**
    * Gets a node that is transitively reachable from `nd` along prefix predecessor edges.
    */
   private DataFlow::Node prefixCandidate(Sink sink) {
     result = sink or
-    result = getPrefixOperand(prefixCandidate(sink)) or
-    result = prefixCandidate(sink).getAPredecessor()
+    result = prefixCandidate(sink).getAPredecessor() or
+    result = StringConcatenation::getFirstOperand(prefixCandidate(sink))
   }
-  
+
   /**
    * Gets an expression that may end up being a prefix of the string concatenation `nd`.
    */
   private Expr getAPrefix(Sink sink) {
     exists (DataFlow::Node prefix |
       prefix = prefixCandidate(sink) and
-      not exists(getPrefixOperand(prefix)) and
+      not exists(StringConcatenation::getFirstOperand(prefix)) and
       not exists(prefix.getAPredecessor()) and
       result = prefix.asExpr()
     )

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

@@ -11,16 +11,13 @@ import javascript
  * `nd` or one of its operands, assuming that it is a concatenation.
  */
 private predicate hasSanitizingSubstring(DataFlow::Node nd) {
-  exists (Expr e | e = nd.asExpr() |
-    (e instanceof AddExpr or e instanceof AssignAddExpr) and
-    hasSanitizingSubstring(DataFlow::valueNode(e.getAChildExpr()))
-    or
-    e.getStringValue().regexpMatch(".*[?#].*")
-  )
+  nd.asExpr().getStringValue().regexpMatch(".*[?#].*")
   or
-  nd.isIncomplete(_)
+  hasSanitizingSubstring(StringConcatenation::getAnOperand(nd))
   or
   hasSanitizingSubstring(nd.getAPredecessor())
+  or
+  nd.isIncomplete(_)
 }
 
 /**
@@ -30,17 +27,7 @@ private predicate hasSanitizingSubstring(DataFlow::Node nd) {
  * This is considered as a sanitizing edge for the URL redirection queries.
  */
 predicate sanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sink) {
-  exists (AddExpr add, DataFlow::Node left |
-    source.asExpr() = add.getRightOperand() and
-    sink.asExpr() = add and
-    left.asExpr() = add.getLeftOperand() and
-    hasSanitizingSubstring(left)
-  )
-  or
-  exists (TemplateLiteral tl, int i, DataFlow::Node elt |
-    source.asExpr() = tl.getElement(i) and
-    sink.asExpr() = tl and
-    elt.asExpr() = tl.getElement([0..i-1]) and
-    hasSanitizingSubstring(elt)
-  )
+  exists (DataFlow::Node operator, int n |
+    StringConcatenation::taintStep(source, sink, operator, n) and
+    hasSanitizingSubstring(StringConcatenation::getOperand(operator, [0..n-1])))
 }