Merge pull request #5400 from erik-krogh/replaceCallbacks

Approved by asgerf

javascript/ql/src/semmle/javascript/StandardLibrary.qll

@@ -149,6 +149,25 @@ class StringReplaceCall extends DataFlow::MethodCallNode {
       pr.flowsTo(replacer.getAReturn()) and
       map.hasPropertyWrite(old, any(DataFlow::Node repl | repl.getStringValue() = new))
     )
+    or
+    // str.replace(regex, match => {
+    //   if (match === 'old') return 'new';
+    //   if (match === 'foo') return 'bar';
+    //   ...
+    // })
+    exists(
+      DataFlow::FunctionNode replacer, ConditionGuardNode guard, EqualityTest test,
+      DataFlow::Node ret
+    |
+      replacer = getCallback(1) and
+      guard.getOutcome() = test.getPolarity() and
+      guard.getTest() = test and
+      replacer.getParameter(0).flowsToExpr(test.getAnOperand()) and
+      test.getAnOperand().getStringValue() = old and
+      ret = replacer.getAReturn() and
+      guard.dominates(ret.getBasicBlock()) and
+      new = ret.getStringValue()
+    )
   }
 }
 

javascript/ql/src/semmle/javascript/security/IncompleteBlacklistSanitizer.qll

@@ -51,7 +51,9 @@ class StringReplaceCallSequence extends DataFlow::CallNode {
 
   /** Gets a string that is the replacement of this call. */
   string getAReplacementString() {
-    // this is more restrictive than `StringReplaceCall::replaces/2`, in the name of precision
+    getAMember().replaces(_, result)
+    or
+    // StringReplaceCall::replaces/2 can't always find the `old` string, so this is added as a fallback.
     getAMember().getRawReplacement().getStringValue() = result
   }
 

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

@@ -63,3 +63,5 @@
 | tst.js:303:10:303:34 | s().rep ... /g, '') | This HTML sanitizer does not sanitize single quotes |
 | tst.js:304:9:304:33 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize single quotes |
 | tst.js:305:10:305:34 | s().rep ... ]/g,'') | This HTML sanitizer does not sanitize double quotes |
+| tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) | This HTML sanitizer does not sanitize single quotes |
+| tst.js:320:9:329:3 | s().rep ... ;";\\n\\t}) | This HTML sanitizer does not sanitize single quotes |

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

@@ -38,6 +38,9 @@ nodes
 | tst.js:303:10:303:34 | s().rep ... /g, '') |
 | tst.js:303:10:303:34 | s().rep ... /g, '') |
 | tst.js:303:10:303:34 | s().rep ... /g, '') |
+| tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) |
+| tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) |
+| tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) |
 edges
 | tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') |
 | tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') |
@@ -55,6 +58,7 @@ edges
 | tst.js:301:10:301:32 | s().rep ... ]/g,'') | tst.js:301:10:301:32 | s().rep ... ]/g,'') |
 | tst.js:302:10:302:34 | s().rep ... ]/g,'') | tst.js:302:10:302:34 | s().rep ... ]/g,'') |
 | tst.js:303:10:303:34 | s().rep ... /g, '') | tst.js:303:10:303:34 | s().rep ... /g, '') |
+| tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) | tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) |
 #select
 | tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') | tst.js:243:9:243:31 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain double quotes when it reaches this attribute definition. | tst.js:243:9:243:31 | s().rep ... ]/g,'') | this final HTML sanitizer step |
 | tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') | tst.js:244:9:244:33 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain double quotes when it reaches this attribute definition. | tst.js:244:9:244:33 | s().rep ... /g, '') | this final HTML sanitizer step |
@@ -68,3 +72,4 @@ edges
 | tst.js:301:10:301:32 | s().rep ... ]/g,'') | tst.js:301:10:301:32 | s().rep ... ]/g,'') | tst.js:301:10:301:32 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain single quotes when it reaches this attribute definition. | tst.js:301:10:301:32 | s().rep ... ]/g,'') | this final HTML sanitizer step |
 | tst.js:302:10:302:34 | s().rep ... ]/g,'') | tst.js:302:10:302:34 | s().rep ... ]/g,'') | tst.js:302:10:302:34 | s().rep ... ]/g,'') | Cross-site scripting vulnerability as the output of $@ may contain single quotes when it reaches this attribute definition. | tst.js:302:10:302:34 | s().rep ... ]/g,'') | this final HTML sanitizer step |
 | tst.js:303:10:303:34 | s().rep ... /g, '') | tst.js:303:10:303:34 | s().rep ... /g, '') | tst.js:303:10:303:34 | s().rep ... /g, '') | Cross-site scripting vulnerability as the output of $@ may contain single quotes when it reaches this attribute definition. | tst.js:303:10:303:34 | s().rep ... /g, '') | this final HTML sanitizer step |
+| tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) | tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) | tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) | Cross-site scripting vulnerability as the output of $@ may contain single quotes when it reaches this attribute definition. | tst.js:309:10:318:3 | s().rep ... ;";\\n\\t}) | this final HTML sanitizer step |

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

@@ -304,3 +304,27 @@ function incompleteHtmlAttributeSanitization2() {
 	'="' + s().replace(/[<>&"]/g,'') + '"'; // OK
 	'=\'' + s().replace(/[<>&']/g,'') + '\''; // OK
 }
+
+function incompleteComplexSanitizers() {
+	'=\'' + s().replace(/[&<>"]/gm, function (str) { // NOT OK
+		if (str === "&")
+			return "&amp;";
+		if (str === "<")
+			return "&lt;";
+		if (str === ">")
+			return "&gt;";
+		if (str === "\"")
+			return "&quot;";
+	}) + '\'';
+
+	'="' + s().replace(/[&<>"]/gm, function (str) { // OK
+		if (str === "&")
+			return "&amp;";
+		if (str === "<")
+			return "&lt;";
+		if (str === ">")
+			return "&gt;";
+		if (str === "\"")
+			return "&quot;";
+	}) + '"';
+}