Merge pull request github#1229 from esben-semmle/js/whitelist-unwrappind

JS: whitelilist delimiter unwrapping for js/incomplete-sanitization

Author: Max Schaefer

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

@@ -101,6 +101,34 @@ predicate allBackslashesEscaped(DataFlow::Node nd) {
   allBackslashesEscaped(nd.getAPredecessor())
 }
 
+/**
+ * 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() = ""
+}
+
+/**
+ * Holds if `leftUnwrap` and `rightUnwrap` unwraps a string from a pair of surrounding delimiters.
+ */
+predicate isDelimiterUnwrapper(
+  DataFlow::MethodCallNode leftUnwrap, DataFlow::MethodCallNode rightUnwrap
+) {
+  exists(string left, string right |
+    left = "[" and right = "]"
+    or
+    left = "{" and right = "}"
+    or
+    left = "(" and right = ")"
+  |
+    removesFirstOccurence(leftUnwrap, left) and
+    removesFirstOccurence(rightUnwrap, right) and
+    leftUnwrap.getAMethodCall() = rightUnwrap
+  )
+}
+
 from MethodCallExpr repl, Expr old, string msg
 where
   repl.getMethodName() = "replace" and
@@ -122,7 +150,10 @@ where
       )
     ) and
     // don't flag replace operations in a loop
-    not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+()
+    not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+() and
+    // dont' flag unwrapper
+    not isDelimiterUnwrapper(repl.flow(), _) and
+    not isDelimiterUnwrapper(_, repl.flow())
     or
     exists(RegExpLiteral rel |
       isBackslashEscape(repl, rel) and

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

@@ -15,3 +15,13 @@
 | tst.js:61:10:61:18 | s.replace | This replaces only the first occurrence of "'" + "". |
 | tst.js:65:10:65:18 | s.replace | This replaces only the first occurrence of "'". |
 | tst.js:69:10:69:18 | s.replace | This replaces only the first occurrence of "'" + "". |
+| tst.js:133:2:133:10 | s.replace | This replaces only the first occurrence of '<'. |
+| tst.js:133:2:133:27 | s.repla ... replace | This replaces only the first occurrence of '>'. |
+| tst.js:135:2:135:10 | s.replace | This replaces only the first occurrence of '['. |
+| tst.js:135:2:135:30 | s.repla ... replace | This replaces only the first occurrence of ']'. |
+| tst.js:136:2:136:10 | s.replace | This replaces only the first occurrence of '{'. |
+| tst.js:136:2:136:30 | s.repla ... replace | This replaces only the first occurrence of '}'. |
+| tst.js:140:2:140:10 | s.replace | This replaces only the first occurrence of /{/. |
+| tst.js:140:2:140:27 | s.repla ... replace | This replaces only the first occurrence of /}/. |
+| tst.js:141:2:141:10 | s.replace | This replaces only the first occurrence of ']'. |
+| tst.js:141:2:141:27 | s.repla ... replace | This replaces only the first occurrence of '['. |

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

@@ -126,6 +126,21 @@ function good11(s) {
   return s.replace("%d", "42");
 }
 
+function good12(s) {
+	s.replace('[', '').replace(']', ''); // OK
+	s.replace('(', '').replace(')', ''); // OK
+	s.replace('{', '').replace('}', ''); // OK
+	s.replace('<', '').replace('>', ''); // NOT OK: too common as a bad HTML sanitizer
+
+	s.replace('[', '\\[').replace(']', '\\]'); // NOT OK
+	s.replace('{', '\\{').replace('}', '\\}'); // NOT OK
+
+	s = s.replace('[', ''); // OK
+	s = s.replace(']', ''); // OK
+	s.replace(/{/, '').replace(/}/, ''); // NOT OK: should have used a string literal if a single replacement was intended
+	s.replace(']', '').replace('[', ''); // probably OK, but still flagged
+}
+
 app.get('/some/path', function(req, res) {
   let untrusted = req.param("p");
 
@@ -162,4 +177,5 @@ app.get('/some/path', function(req, res) {
   good10(untrusted);
   flowifyComments(untrusted);
   good11(untrusted);
+  good12(untrusted);
 });