diff --git a/change-notes/1.18/analysis-javascript.md b/change-notes/1.18/analysis-javascript.md index fb20a0e6ec4..b8cfe807015 100644 --- a/change-notes/1.18/analysis-javascript.md +++ b/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. | diff --git a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql b/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql index a56fe440426..8191f868e91 100644 --- a/javascript/ql/src/Security/CWE-116/IncompleteSanitization.ql +++ b/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 - getAMatchedString(old) = metachar() and + // only flag if this is likely to be a sanitizer or URL encoder or decoder + 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 diff --git a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization.expected b/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization.expected index ea4d3f8c63f..4bfee136d97 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/IncompleteSanitization.expected +++ b/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". | diff --git a/javascript/ql/test/query-tests/Security/CWE-116/tst.js b/javascript/ql/test/query-tests/Security/CWE-116/tst.js index cb9a3ede3c8..a07e98665e2 100644 --- a/javascript/ql/test/query-tests/Security/CWE-116/tst.js +++ b/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); });