Merge pull request #1242 from esben-semmle/js/whitelist-trailing-newline-removal

Approved by xiemaisi

Author: semmle-qlci

change-notes/1.21/analysis-javascript.md

@@ -32,7 +32,7 @@
 | Double escaping or unescaping | More results | This rule now considers the flow of regular expressions literals. |
 | Expression has no effect       | Fewer false-positive results | This rule now treats uses of `Object.defineProperty` more conservatively. |
 | Incomplete regular expression for hostnames | More results | This rule now tracks regular expressions for host names further. |
-| Incomplete string escaping or encoding | More results | This rule now considers the flow of regular expressions literals. |
+| Incomplete string escaping or encoding | More results | This rule now considers the flow of regular expressions literals, and it no longer flags the removal of trailing newlines. |
 | Password in configuration file | Fewer false positive results | This query now excludes passwords that are inserted into the configuration file using a templating mechanism. |
 | Replacement of a substring with itself | More results | This rule now considers the flow of regular expressions literals. |
 | Server-side URL redirect       | Fewer false-positive results | This rule now treats URLs as safe in more cases where the hostname cannot be tampered with. |

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

@@ -129,6 +129,20 @@ predicate isDelimiterUnwrapper(
   )
 }
 
+/*
+ * Holds if `repl` is a standalone use of `String.prototype.replace` to remove a single newline.
+ */
+
+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
+  )
+}
+
 from MethodCallExpr repl, Expr old, string msg
 where
   repl.getMethodName() = "replace" and
@@ -153,7 +167,9 @@ where
     not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+() and
     // dont' flag unwrapper
     not isDelimiterUnwrapper(repl.flow(), _) and
-    not isDelimiterUnwrapper(_, repl.flow())
+    not isDelimiterUnwrapper(_, repl.flow()) and
+    // dont' flag the removal of trailing newlines
+    not removesTrailingNewLine(repl.flow())
     or
     exists(RegExpLiteral rel |
       isBackslashEscape(repl, rel) and

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

@@ -25,4 +25,6 @@
 | 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 '['. |
-| tst.js:185:9:185:17 | s.replace | This replaces only the first occurrence of /'/. |
+| tst.js:148:2:148:10 | x.replace | This replaces only the first occurrence of "\\n". |
+| tst.js:149:2:149:24 | x.repla ... replace | This replaces only the first occurrence of "\\n". |
+| tst.js:193:9:193:17 | s.replace | This replaces only the first occurrence of /'/. |

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

@@ -141,6 +141,14 @@ function good12(s) {
 	s.replace(']', '').replace('[', ''); // probably OK, but still flagged
 }
 
+function newlines(s) {
+	// motivation for whitelist
+	require("child_process").execSync("which emacs").toString().replace("\n", ""); // OK
+
+	x.replace("\n", "").replace(x, y); // NOT OK
+	x.replace(x, y).replace("\n", ""); // NOT OK
+}
+
 app.get('/some/path', function(req, res) {
   let untrusted = req.param("p");