Merge pull request #2247 from max-schaefer/odasa-8149

Approved by asger-semmle, esbena

change-notes/1.23/analysis-javascript.md

@@ -32,6 +32,7 @@
 
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Double escaping or unescaping (`js/double-escaping`) | More results | This rule now detects additional escaping and unescaping functions. |
 | Incomplete string escaping or encoding (`js/incomplete-sanitization`) | Fewer false-positive results | This rule now recognizes additional ways delimiters can be stripped away. |
 | Client-side cross-site scripting (`js/xss`) | More results, fewer false-positive results | More potential vulnerabilities involving functions that manipulate DOM attributes are now recognized, and more sanitizers are detected. |
 | Code injection (`js/code-injection`) | More results | More potential vulnerabilities involving functions that manipulate DOM event handler attributes are now recognized. |

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

@@ -10,8 +10,8 @@ attacks such as cross-site scripting. One particular example of this is HTML ent
 where HTML special characters are replaced by HTML character entities to prevent them from being
 interpreted as HTML markup. For example, the less-than character is encoded as <code>&amp;lt;</code>
 and the double-quote character as <code>&amp;quot;</code>.
-Other examples include backslash-escaping for including untrusted data in string literals and
-percent-encoding for URI components.
+Other examples include backslash escaping or JSON encoding for including untrusted data in string
+literals, and percent-encoding for URI components.
 </p>
 <p>
 The reverse process of replacing escape sequences with the characters they represent is known as

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

@@ -46,7 +46,12 @@ string getStringValue(RegExpLiteral rl) {
  */
 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
+  )
 }
 
 /**
@@ -54,38 +59,31 @@ DataFlow::Node getASimplePredecessor(DataFlow::Node nd) {
  * 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.
+ * A method call that performs string replacement.
  */
-class Replacement extends DataFlow::Node {
-  RegExpLiteral pattern;
-
-  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()
-    )
-  }
-
+abstract class Replacement extends DataFlow::Node {
   /**
    * Holds if this replacement replaces the string `input` with `output`.
    */
-  predicate replaces(string input, string output) {
-    exists(DataFlow::MethodCallNode mcn |
-      mcn = this and
-      input = getStringValue(pattern) and
-      output = mcn.getArgument(1).getStringValue()
-    )
-  }
+  abstract predicate replaces(string input, string output);
+
+  /**
+   * Gets the input of this replacement.
+   */
+  abstract DataFlow::Node getInput();
+
+  /**
+   * Gets the output of this replacement.
+   */
+  abstract DataFlow::SourceNode getOutput();
 
   /**
    * Holds if this replacement escapes `char` using `metachar`.
@@ -118,9 +116,12 @@ class Replacement extends DataFlow::Node {
   /**
    * Gets the previous replacement in this chain of replacements.
    */
-  Replacement getPreviousReplacement() {
-    result = getASimplePredecessor*(this.(DataFlow::MethodCallNode).getReceiver())
-  }
+  Replacement getPreviousReplacement() { result.getOutput() = getASimplePredecessor*(getInput()) }
+
+  /**
+   * Gets the next replacement in this chain of replacements.
+   */
+  Replacement getNextReplacement() { this = result.getPreviousReplacement() }
 
   /**
    * Gets an earlier replacement in this chain of replacements that
@@ -130,7 +131,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,11 +145,100 @@ 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)
+      )
     )
   }
 }
 
+/**
+ * A call to `String.prototype.replace` that replaces all instances of a pattern.
+ */
+class GlobalStringReplacement extends Replacement, DataFlow::MethodCallNode {
+  RegExpLiteral pattern;
+
+  GlobalStringReplacement() {
+    this.getMethodName() = "replace" and
+    pattern.flow().(DataFlow::SourceNode).flowsTo(this.getArgument(0)) and
+    this.getNumArgument() = 2 and
+    pattern.isGlobal()
+  }
+
+  override predicate replaces(string input, string output) {
+    input = getStringValue(pattern) and
+    output = this.getArgument(1).getStringValue()
+    or
+    exists(DataFlow::FunctionNode replacer, DataFlow::PropRead pr, DataFlow::ObjectLiteralNode map |
+      replacer = getCallback(1) and
+      replacer.getParameter(0).flowsToExpr(pr.getPropertyNameExpr()) and
+      pr = map.getAPropertyRead() and
+      pr.flowsTo(replacer.getAReturn()) and
+      map.asExpr().(ObjectExpr).getPropertyByName(input).getInit().getStringValue() = output
+    )
+  }
+
+  override DataFlow::Node getInput() { result = this.getReceiver() }
+
+  override DataFlow::SourceNode getOutput() { result = this }
+}
+
+/**
+ * A call to `JSON.stringify`, viewed as a string replacement.
+ */
+class JsonStringifyReplacement extends Replacement, DataFlow::CallNode {
+  JsonStringifyReplacement() { this = DataFlow::globalVarRef("JSON").getAMemberCall("stringify") }
+
+  override predicate replaces(string input, string output) {
+    input = "\\" and output = "\\\\"
+    // the other replacements are not relevant for this query
+  }
+
+  override DataFlow::Node getInput() { result = this.getArgument(0) }
+
+  override DataFlow::SourceNode getOutput() { result = this }
+}
+
+/**
+ * A call to `JSON.parse`, viewed as a string replacement.
+ */
+class JsonParseReplacement extends Replacement {
+  JsonParserCall self;
+
+  JsonParseReplacement() { this = self }
+
+  override predicate replaces(string input, string output) {
+    input = "\\\\" and output = "\\"
+    // the other replacements are not relevant for this query
+  }
+
+  override DataFlow::Node getInput() { result = self.getInput() }
+
+  override DataFlow::SourceNode getOutput() { result = self.getOutput() }
+}
+
+/**
+ * A string replacement wrapped in a utility function.
+ */
+class WrappedReplacement extends Replacement, DataFlow::CallNode {
+  int i;
+
+  Replacement inner;
+
+  WrappedReplacement() {
+    exists(DataFlow::FunctionNode wrapped | wrapped.getFunction() = getACallee() |
+      wrapped.getParameter(i).flowsTo(inner.getPreviousReplacement*().getInput()) and
+      inner.getNextReplacement*().getOutput().flowsTo(wrapped.getAReturn())
+    )
+  }
+
+  override predicate replaces(string input, string output) { inner.replaces(input, output) }
+
+  override DataFlow::Node getInput() { result = getArgument(i) }
+
+  override DataFlow::SourceNode getOutput() { result = this }
+}
+
 from Replacement primary, Replacement supplementary, string message, string metachar
 where
   primary.escapes(metachar, _) and

javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/DoubleEscaping.expected

@@ -5,3 +5,8 @@
 | tst.js:53:10:53:33 | s.repla ... , '\\\\') | This replacement may produce '\\' characters that are double-unescaped $@. | tst.js:53:10:54:33 | s.repla ... , '\\'') | here |
 | tst.js:60:7:60:28 | s.repla ...  '%25') | This replacement may double-escape '%' characters from $@. | tst.js:59:7:59:28 | s.repla ...  '%26') | here |
 | tst.js:68:10:70:38 | s.repla ... &") | This replacement may double-escape '&' characters from $@. | tst.js:68:10:69:39 | s.repla ... apos;") | here |
+| tst.js:74:10:77:10 | JSON.st ...       ) | This replacement may double-escape '\\' characters from $@. | tst.js:75:12:76:37 | s.repla ... u003E") | here |
+| tst.js:86:10:86:22 | JSON.parse(s) | This replacement may produce '\\' characters that are double-unescaped $@. | tst.js:86:10:86:47 | JSON.pa ... g, "<") | here |
+| tst.js:99:10:99:66 | s.repla ... &amp;") | This replacement may double-escape '&' characters from $@. | tst.js:99:10:99:43 | s.repla ... epl[c]) | here |
+| tst.js:107:10:107:53 | encodeD ... &amp;") | This replacement may double-escape '&' characters from $@. | tst.js:107:10:107:30 | encodeD ... otes(s) | here |
+| tst.js:115:10:115:47 | encodeQ ... &amp;") | This replacement may double-escape '&' characters from $@. | tst.js:115:10:115:24 | encodeQuotes(s) | here |

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

@@ -69,3 +69,68 @@ function badEncode(s) {
           .replace(indirect2, "'")
           .replace(indirect3, "&");
 }
+
+function badEscape1(s) {
+  return JSON.stringify(
+           s.replace(/</g, "\\u003C")
+            .replace(/>/g, "\\u003E")
+         );
+}
+
+function goodEscape1(s) {
+  return JSON.stringify(s)
+             .replace(/</g, "\\u003C").replace(/>/g, "\\u003E");
+}
+
+function badUnescape2(s) {
+  return JSON.parse(s).replace(/\\u003C/g, "<").replace(/\\u003E/g, ">");
+}
+
+function goodUnescape2(s) {
+  return JSON.parse(s.replace(/\\u003C/g, "<").replace(/\\u003E/g, ">"));
+}
+
+function badEncodeWithReplacer(s) {
+  var repl = {
+    '"': "&quot;",
+    "'": "&apos;",
+    "&": "&amp;"
+  };
+  return s.replace(/["']/g, (c) => repl[c]).replace(/&/g, "&amp;");
+}
+
+function encodeDoubleQuotes(s) {
+  return s.replace(/"/g, "&quot;");
+}
+
+function badWrappedEncode(s) {
+  return encodeDoubleQuotes(s).replace(/&/g, "&amp;");
+}
+
+function encodeQuotes(s) {
+  return s.replace(/"/g, "&quot;").replace(/'/g, "&apos;");
+}
+
+function badWrappedEncode2(s) {
+  return encodeQuotes(s).replace(/&/g, "&amp;");
+}
+
+function roundtrip(s) {
+  return JSON.parse(JSON.stringify(s));
+}
+
+// dubious, but out of scope for this query
+function badRoundtrip(s) {
+  return s.replace(/\\\\/g, "\\").replace(/\\/g, "\\\\");
+}
+
+function testWithCapturedVar(x) {
+  var captured = x;
+  (function() {
+    captured = captured.replace(/\\/g, "\\\\");
+  })();
+}
+
+function cloneAndStringify(s) {
+  return JSON.stringify(JSON.parse(JSON.stringify(s)));
+}