Merge pull request #2369 from max-schaefer/js/odasa-8179

Approved by esbena

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -16,6 +16,7 @@
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
 | Clear-text logging of sensitive information (`js/clear-text-logging`) | More results | More results involving `process.env` and indirect calls to logging methods are recognized. |
+| Incomplete string escaping or encoding (`js/incomplete-sanitization`) | Fewer false positive results | This query now recognizes additional cases where a single replacement is likely to be intentional. |
 | Unbound event handler receiver (`js/unbound-event-handler-receiver`) | Fewer false positive results | This query now recognizes additional ways event handler receivers can be bound. | 
 
 ## Changes to libraries

javascript/ql/src/Security/CWE-116/DoubleEscaping.ql

@@ -15,76 +15,51 @@
 
 import javascript
 
-/**
- * Holds if `rl` is a simple constant, which is bound to the result of the predicate.
- *
- * For example, `/a/g` has string value `"a"` and `/abc/` has string value `"abc"`,
- * while `/ab?/` and `/a(?=b)/` do not have a string value.
- *
- * Flags are ignored, so `/a/i` is still considered to have string value `"a"`,
- * even though it also matches `"A"`.
- *
- * Note the somewhat subtle use of monotonic aggregate semantics, which makes the
- * `strictconcat` fail if one of the children of the root is not a constant (legacy
- * semantics would simply skip such children).
- */
-language[monotonicAggregates]
-string getStringValue(RegExpLiteral rl) {
-  exists(RegExpTerm root | root = rl.getRoot() |
-    result = root.(RegExpConstant).getValue()
-    or
-    result = strictconcat(RegExpTerm ch, int i |
-        ch = root.(RegExpSequence).getChild(i)
-      |
-        ch.(RegExpConstant).getValue() order by i
-      )
-  )
-}
-
 /**
  * Gets a predecessor of `nd` that is not an SSA phi node.
  */
 DataFlow::Node getASimplePredecessor(DataFlow::Node nd) {
   result = nd.getAPredecessor() and
-  not nd.(DataFlow::SsaDefinitionNode).getSsaVariable().getDefinition() instanceof SsaPhiNode
+  not exists(SsaDefinition ssa |
+    ssa = nd.(DataFlow::SsaDefinitionNode).getSsaVariable().getDefinition()
+  |
+    ssa instanceof SsaPhiNode or
+    ssa instanceof SsaVariableCapture
+  )
 }
 
 /**
  * Holds if `metachar` is a meta-character that is used to escape special characters
  * into a form described by regular expression `regex`.
  */
 predicate escapingScheme(string metachar, string regex) {
-  metachar = "&" and regex = "&.*;"
+  metachar = "&" and regex = "&.+;"
   or
-  metachar = "%" and regex = "%.*"
+  metachar = "%" and regex = "%.+"
   or
-  metachar = "\\" and regex = "\\\\.*"
+  metachar = "\\" and regex = "\\\\.+"
 }
 
 /**
  * A call to `String.prototype.replace` that replaces all instances of a pattern.
  */
-class Replacement extends DataFlow::Node {
-  RegExpLiteral pattern;
-
+class Replacement extends StringReplaceCall {
   Replacement() {
-    exists(DataFlow::MethodCallNode mcn | this = mcn |
-      mcn.getMethodName() = "replace" and
-      pattern.flow().(DataFlow::SourceNode).flowsTo(mcn.getArgument(0)) and
-      mcn.getNumArgument() = 2 and
-      pattern.isGlobal()
-    )
+    isGlobal()
   }
 
   /**
-   * Holds if this replacement replaces the string `input` with `output`.
+   * Gets the input of this replacement.
    */
-  predicate replaces(string input, string output) {
-    exists(DataFlow::MethodCallNode mcn |
-      mcn = this and
-      input = getStringValue(pattern) and
-      output = mcn.getArgument(1).getStringValue()
-    )
+  DataFlow::Node getInput() {
+    result = this.getReceiver()
+  }
+
+  /**
+   * Gets the output of this replacement.
+   */
+  DataFlow::SourceNode getOutput() {
+    result = this
   }
 
   /**
@@ -119,7 +94,7 @@ class Replacement extends DataFlow::Node {
    * Gets the previous replacement in this chain of replacements.
    */
   Replacement getPreviousReplacement() {
-    result = getASimplePredecessor*(this.(DataFlow::MethodCallNode).getReceiver())
+    result.getOutput() = getASimplePredecessor*(getInput())
   }
 
   /**
@@ -130,7 +105,9 @@ class Replacement extends DataFlow::Node {
     exists(Replacement pred | pred = this.getPreviousReplacement() |
       if pred.escapes(_, metachar)
       then result = pred
-      else result = pred.getAnEarlierEscaping(metachar)
+      else (
+        not pred.unescapes(metachar, _) and result = pred.getAnEarlierEscaping(metachar)
+      )
     )
   }
 
@@ -142,7 +119,9 @@ class Replacement extends DataFlow::Node {
     exists(Replacement succ | this = succ.getPreviousReplacement() |
       if succ.unescapes(metachar, _)
       then result = succ
-      else result = succ.getALaterUnescaping(metachar)
+      else (
+        not succ.escapes(_, metachar) and result = succ.getALaterUnescaping(metachar)
+      )
     )
   }
 }

javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql

@@ -20,53 +20,47 @@ import javascript
 string metachar() { result = "'\"\\&<>\n\r\t*|{}[]%$".charAt(_) }
 
 /** Gets a string matched by `e` in a `replace` call. */
-string getAMatchedString(Expr e) {
-  result = getAMatchedConstant(e.(RegExpLiteral).getRoot()).getValue()
+string getAMatchedString(DataFlow::Node e) {
+  result = e.(DataFlow::RegExpLiteralNode).getRoot().getAMatchedString()
   or
   result = e.getStringValue()
 }
 
-/** Gets a constant matched by `t`. */
-RegExpConstant getAMatchedConstant(RegExpTerm t) {
-  result = t
-  or
-  result = getAMatchedConstant(t.(RegExpAlt).getAlternative())
-  or
-  result = getAMatchedConstant(t.(RegExpGroup).getAChild())
-  or
-  exists(RegExpCharacterClass recc | recc = t and not recc.isInverted() |
-    result = getAMatchedConstant(recc.getAChild())
-  )
-}
-
 /** Holds if `t` is simple, that is, a union of constants. */
 predicate isSimple(RegExpTerm t) {
   t instanceof RegExpConstant
   or
   isSimple(t.(RegExpGroup).getAChild())
   or
-  (
-    t instanceof RegExpAlt
-    or
-    t instanceof RegExpCharacterClass and not t.(RegExpCharacterClass).isInverted()
-  ) and
+  isSimpleCharacterClass(t)
+  or
+  isSimpleAlt(t)
+}
+
+/** Holds if `t` is a non-inverted character class that contains no ranges. */
+predicate isSimpleCharacterClass(RegExpCharacterClass t) {
+  not t.isInverted() and
+  forall(RegExpTerm ch | ch = t.getAChild() | isSimple(ch))
+}
+
+/** Holds if `t` is an alternation of simple terms. */
+predicate isSimpleAlt(RegExpAlt t) {
   forall(RegExpTerm ch | ch = t.getAChild() | isSimple(ch))
 }
 
 /**
  * Holds if `mce` is of the form `x.replace(re, new)`, where `re` is a global
  * regular expression and `new` prefixes the matched string with a backslash.
  */
-predicate isBackslashEscape(MethodCallExpr mce, RegExpLiteral re) {
-  mce.getMethodName() = "replace" and
-  re.flow().(DataFlow::SourceNode).flowsToExpr(mce.getArgument(0)) and
-  re.isGlobal() and
-  exists(string new | new = mce.getArgument(1).getStringValue() |
-    // `new` is `\$&`, `\$1` or similar
-    new.regexpMatch("\\\\\\$(&|\\d)")
+predicate isBackslashEscape(StringReplaceCall mce, DataFlow::RegExpLiteralNode re) {
+  mce.isGlobal() and
+  re = mce.getRegExp() and
+  (
+    // replacement with `\$&`, `\$1` or similar
+    mce.getRawReplacement().getStringValue().regexpMatch("\\\\\\$(&|\\d)")
     or
-    // `new` is `\c`, where `c` is a constant matched by `re`
-    new.regexpMatch("\\\\\\Q" + getAMatchedString(re) + "\\E")
+    // replacement of `c` with `\c`
+    exists(string c | mce.replaces(c, "\\" + c))
   )
 }
 
@@ -78,7 +72,7 @@ predicate allBackslashesEscaped(DataFlow::Node nd) {
   nd = DataFlow::globalVarRef("JSON").getAMemberCall("stringify")
   or
   // check whether `nd` itself escapes backslashes
-  exists(RegExpLiteral rel | isBackslashEscape(nd.asExpr(), rel) |
+  exists(DataFlow::RegExpLiteralNode rel | isBackslashEscape(nd, rel) |
     // if it's a complex regexp, we conservatively assume that it probably escapes backslashes
     not isSimple(rel.getRoot()) or
     getAMatchedString(rel) = "\\"
@@ -104,10 +98,8 @@ predicate allBackslashesEscaped(DataFlow::Node nd) {
 /**
  * Holds if `repl` looks like a call to "String.prototype.replace" that deliberately removes the first occurrence of `str`.
  */
-predicate removesFirstOccurence(DataFlow::MethodCallNode repl, string str) {
-  repl.getMethodName() = "replace" and
-  repl.getArgument(0).getStringValue() = str and
-  repl.getArgument(1).getStringValue() = ""
+predicate removesFirstOccurence(StringReplaceCall repl, string str) {
+  not exists(repl.getRegExp()) and repl.replaces(str, "")
 }
 
 /**
@@ -134,25 +126,30 @@ predicate isDelimiterUnwrapper(
 }
 
 /*
- * Holds if `repl` is a standalone use of `String.prototype.replace` to remove a single newline.
+ * Holds if `repl` is a standalone use of `String.prototype.replace` to remove a single newline,
+ * dollar or percent character.
+ *
+ * This is often done on inputs that are known to only contain a single instance of the character,
+ * such as output from a shell command that is known to end with a single newline, or strings
+ * like "$1.20" or "50%".
  */
 
-predicate removesTrailingNewLine(DataFlow::MethodCallNode repl) {
-  repl.getMethodName() = "replace" and
-  repl.getArgument(0).mayHaveStringValue("\n") and
-  repl.getArgument(1).mayHaveStringValue("") and
-  not exists(DataFlow::MethodCallNode other | other.getMethodName() = "replace" |
-    repl.getAMethodCall() = other or
-    other.getAMethodCall() = repl
+predicate whitelistedRemoval(StringReplaceCall repl) {
+  not repl.isGlobal() and
+  exists(string s | s = "\n" or s = "%" or s = "$" |
+    repl.replaces(s, "") and
+    not exists(StringReplaceCall other |
+      repl.getAMethodCall() = other or
+      other.getAMethodCall() = repl
+    )
   )
 }
 
-from MethodCallExpr repl, Expr old, string msg
+from StringReplaceCall repl, DataFlow::Node old, string msg
 where
-  repl.getMethodName() = "replace" and
-  (old = repl.getArgument(0) or old.flow().(DataFlow::SourceNode).flowsToExpr(repl.getArgument(0))) and
+  (old = repl.getArgument(0) or old = repl.getRegExp()) and
   (
-    not old.(RegExpLiteral).isGlobal() and
+    not repl.isGlobal() and
     msg = "This replaces only the first occurrence of " + old + "." and
     // only flag if this is likely to be a sanitizer or URL encoder or decoder
     exists(string m | m = getAMatchedString(old) |
@@ -171,17 +168,17 @@ where
       (m = ".." or m = "/.." or m = "../" or m = "/../")
     ) and
     // don't flag replace operations in a loop
-    not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+() and
+    not repl.getReceiver() = repl.getASuccessor+() and
     // dont' flag unwrapper
-    not isDelimiterUnwrapper(repl.flow(), _) and
-    not isDelimiterUnwrapper(_, repl.flow()) and
-    // dont' flag the removal of trailing newlines
-    not removesTrailingNewLine(repl.flow())
+    not isDelimiterUnwrapper(repl, _) and
+    not isDelimiterUnwrapper(_, repl) and
+    // don't flag replacements of certain characters with whitespace
+    not whitelistedRemoval(repl)
     or
-    exists(RegExpLiteral rel |
+    exists(DataFlow::RegExpLiteralNode rel |
       isBackslashEscape(repl, rel) and
-      not allBackslashesEscaped(DataFlow::valueNode(repl)) and
+      not allBackslashesEscaped(repl) and
       msg = "This does not escape backslash characters in the input."
     )
   )
-select repl.getCallee(), msg
+select repl.getCalleeNode(), msg

javascript/ql/src/semmle/javascript/Regexp.qll

@@ -173,6 +173,23 @@ class RegExpTerm extends Locatable, @regexpterm {
       parent.(StringLiteral).flow() instanceof RegExpPatternSource
     )
   }
+
+  /**
+   * Gets the single string this regular-expression term matches.
+   *
+   * This predicate is only defined for (sequences/groups of) constant regular expressions.
+   * In particular, terms involving zero-width assertions like `^` or `\b` are not considered
+   * to have a constant value.
+   *
+   * Note that this predicate does not take flags of the enclosing regular-expression literal
+   * into account.
+   */
+  string getConstantValue() { none() }
+
+  /**
+   * Gets a string that is matched by this regular-expression term.
+   */
+  string getAMatchedString() { result = getConstantValue() }
 }
 
 /**
@@ -223,6 +240,8 @@ class RegExpConstant extends RegExpTerm, @regexp_constant {
   predicate isCharacter() { any() }
 
   override predicate isNullable() { none() }
+
+  override string getConstantValue() { result = getValue() }
 }
 
 /**
@@ -266,6 +285,8 @@ class RegExpAlt extends RegExpTerm, @regexp_alt {
   int getNumAlternative() { result = getNumChild() }
 
   override predicate isNullable() { getAlternative().isNullable() }
+
+  override string getAMatchedString() { result = getAlternative().getAMatchedString() }
 }
 
 /**
@@ -289,6 +310,21 @@ class RegExpSequence extends RegExpTerm, @regexp_seq {
   override predicate isNullable() {
     forall(RegExpTerm child | child = getAChild() | child.isNullable())
   }
+
+  override string getConstantValue() {
+    result = getConstantValue(0)
+  }
+
+  /**
+   * Gets the single string matched by the `i`th child and all following children of
+   * this sequence, if any.
+   */
+  private string getConstantValue(int i) {
+    i = getNumChild() and
+    result = ""
+    or
+    result = getChild(i).getConstantValue() + getConstantValue(i+1)
+  }
 }
 
 /**
@@ -549,6 +585,10 @@ class RegExpGroup extends RegExpTerm, @regexp_group {
   string getName() { isNamedCapture(this, result) }
 
   override predicate isNullable() { getAChild().isNullable() }
+
+  override string getConstantValue() { result = getAChild().getConstantValue() }
+
+  override string getAMatchedString() { result = getAChild().getAMatchedString() }
 }
 
 /**
@@ -734,6 +774,10 @@ class RegExpCharacterClass extends RegExpTerm, @regexp_char_class {
   predicate isInverted() { isInverted(this) }
 
   override predicate isNullable() { none() }
+
+  override string getAMatchedString() {
+    not isInverted() and result = getAChild().getAMatchedString()
+  }
 }
 
 /**