From aaeca325193deedc7ac8ea163cbe574514d44b94 Mon Sep 17 00:00:00 2001
From: Max Schaefer <max@semmle.com>
Date: Wed, 30 Oct 2019 09:55:28 +0000
Subject: [PATCH] JavaScript: Recognize string escaping using `.replace` with a
 callback.

---
 javascript/ql/src/Security/CWE-116/DoubleEscaping.ql     | 8 ++++++++
 .../CWE-116/DoubleEscaping/DoubleEscaping.expected       | 1 +
 .../query-tests/Security/CWE-116/DoubleEscaping/tst.js   | 9 +++++++++
 3 files changed, 18 insertions(+)

diff --git a/javascript/ql/src/Security/CWE-116/DoubleEscaping.ql b/javascript/ql/src/Security/CWE-116/DoubleEscaping.ql
index 589a935d5fe..4e6969e54c3 100644
--- a/javascript/ql/src/Security/CWE-116/DoubleEscaping.ql
+++ b/javascript/ql/src/Security/CWE-116/DoubleEscaping.ql
@@ -156,6 +156,14 @@ class GlobalStringReplacement extends Replacement, DataFlow::MethodCallNode {
   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() {
diff --git a/javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/DoubleEscaping.expected b/javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/DoubleEscaping.expected
index e3d5a9a4252..d7e2b558a06 100644
--- a/javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/DoubleEscaping.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/DoubleEscaping.expected
@@ -7,3 +7,4 @@
 | tst.js:68:10:70:38 | s.repla ... &amp;") | 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 |
diff --git a/javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/tst.js b/javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/tst.js
index ee9cd930b6f..78c73ce9584 100644
--- a/javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/tst.js
+++ b/javascript/ql/test/query-tests/Security/CWE-116/DoubleEscaping/tst.js
@@ -89,3 +89,12 @@ function badUnescape2(s) {
 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;");
+}