JavaScript: Teach IncompleteSanitization to recognize incomplete URL {en,de}coding.

change-notes/1.18/analysis-javascript.md

@@ -39,6 +39,7 @@
 | Arguments redefined | Fewer results | This rule previously also flagged redefinitions of `eval`. This was an oversight that is now fixed. |
 | CORS misconfiguration for credentials transfer | More true-positive results | This rule now treats header names case-insensitively. |
 | Hard-coded credentials | More true-positive results | This rule now recognizes secret cryptographic keys. |
+| Incomplete sanitization | More true-positive results | This rule now recognizes incomplete URL encoding and decoding. |
 | Insecure randomness | More true-positive results | This rule now recognizes secret cryptographic keys. |
 | Missing rate limiting | More true-positive results, fewer false-positive results | This rule now recognizes additional rate limiters and expensive route handlers. | 
 | Missing X-Frame-Options HTTP header | Fewer false-positive results | This rule now treats header names case-insensitively. |

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

@@ -106,8 +106,19 @@ where repl.getMethodName() = "replace" and
       (
        not old.(RegExpLiteral).isGlobal() and
        msg = "This replaces only the first occurrence of " + old + "." and
-       // only flag if this is likely to be a sanitizer
+       // only flag if this is likely to be a sanitizer or URL encoder or decoder
-       getAMatchedString(old) = metachar() and
+       exists (string m | m = getAMatchedString(old) |
+         // sanitizer
+         m = metachar()
+         or
+         exists (string urlEscapePattern | urlEscapePattern = "(%[0-9A-Fa-f]{2})+" |
+           // URL decoder
+           m.regexpMatch(urlEscapePattern)
+           or
+           // URL encoder
+           repl.getArgument(1).getStringValue().regexpMatch(urlEscapePattern)
+         )
+       ) and
        // don't flag replace operations in a loop
        not DataFlow::valueNode(repl.getReceiver()) = DataFlow::valueNode(repl).getASuccessor+()
        or

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

@@ -7,3 +7,5 @@
 | tst.js:29:20:29:27 | /('\|")/g | This does not backslash-escape the backslash character. |
 | tst.js:33:20:33:22 | '\|' | This replaces only the first occurrence of '\|'. |
 | tst.js:37:20:37:23 | /"/g | This does not backslash-escape the backslash character. |
+| tst.js:41:20:41:22 | "/" | This replaces only the first occurrence of "/". |
+| tst.js:45:20:45:24 | "%25" | This replaces only the first occurrence of "%25". |

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

@@ -37,6 +37,14 @@ function bad9(s) {
   return s.replace(/"/g, "\\\"");  // NOT OK
 }
 
+function bad10(s) {
+  return s.replace("/", "%2F"); // NOT OK
+}
+
+function bad11(s) {
+  return s.replace("%25", "%"); // NOT OK
+}
+
 
 function good1(s) {
   while (s.indexOf("'") > 0)
@@ -91,6 +99,10 @@ function flowifyComments(s) {
   return s.replace(/#/g, '💩'); // OK
 }
 
+function good11(s) {
+  return s.replace("%d", "42");
+}
+
 app.get('/some/path', function(req, res) {
   let untrusted = req.param("p");
 
@@ -106,6 +118,8 @@ app.get('/some/path', function(req, res) {
   bad7(untrusted);
   bad8(untrusted);
   bad9(untrusted);
+  bad10(untrusted);
+  bad11(untrusted);
 
   good1(untrusted);
   good2(untrusted);
@@ -118,4 +132,5 @@ app.get('/some/path', function(req, res) {
   good9(untrusted);
   good10(untrusted);
   flowifyComments(untrusted);
+  good11(untrusted);
 });