Merge pull request #8481 from erik-krogh/schemeChain

JS: recognize string replacement chains as scheme checks in js/incomplete-url-scheme-check

javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql

@@ -14,6 +14,7 @@
  */
 
 import javascript
+import semmle.javascript.security.IncompleteBlacklistSanitizer as IncompleteBlacklistSanitizer
 
 /** A URL scheme that can be used to represent executable code. */
 class DangerousScheme extends string {
@@ -55,6 +56,21 @@ DataFlow::SourceNode schemeOf(DataFlow::Node url) {
   )
 }
 
+/**
+ * A chain of replace calls that replaces one or more dangerous schemes.
+ */
+class SchemeReplacementChain extends IncompleteBlacklistSanitizer::StringReplaceCallSequence {
+  SchemeReplacementChain() { this.getAMember().getAReplacedString() instanceof DangerousScheme }
+
+  /**
+   * Gets the source node that the replacement happens on.
+   * The result is the receiver of the first call in the chain.
+   */
+  DataFlow::Node getReplacementSource() {
+    result = this.getReceiver+() and not result instanceof DataFlow::MethodCallNode
+  }
+}
+
 /** Gets a data-flow node that checks `nd` against the given `scheme`. */
 DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
   // check of the form `nd.startsWith(scheme)`
@@ -73,6 +89,11 @@ DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
     schemeOf(nd).flowsTo(candidate)
   )
   or
+  exists(SchemeReplacementChain chain | result = chain |
+    scheme = chain.getAMember().getAReplacedString() and
+    nd = chain.getReplacementSource()
+  )
+  or
   // propagate through trimming, case conversion, and regexp replace
   exists(DataFlow::MethodCallNode stringop |
     stringop.getMethodName().matches("trim%") or

javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected

@@ -11,3 +11,5 @@
 | IncompleteUrlSchemeCheck.js:87:7:87:40 | /^(java ... scheme) | This check does not consider vbscript:. |
 | IncompleteUrlSchemeCheck.js:94:10:94:15 | scheme | This check does not consider vbscript:. |
 | IncompleteUrlSchemeCheck.js:104:6:104:39 | /^(java ... scheme) | This check does not consider vbscript:. |
+| IncompleteUrlSchemeCheck.js:110:12:112:29 | url  // ... :/, "") | This check does not consider vbscript:. |
+| IncompleteUrlSchemeCheck.js:124:11:124:34 | url.rep ... :/, "") | This check does not consider vbscript:. |

javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js

@@ -105,3 +105,26 @@ function test14(url) {
         return "about:blank";
     return url;
 }
+
+function chain1(url) {
+    return url  // NOT OK
+        .replace(/javascript:/, "")
+        .replace(/data:/, "");
+}
+
+function chain2(url) {
+    return url  // OK
+        .replace(/javascript:/, "")
+        .replace(/data:/, "")
+        .replace(/vbscript:/, "");
+}
+
+function chain3(url) {
+    url = url.replace(/javascript:/, "")
+    url = url.replace(/data:/, ""); // NOT OK
+    return url;
+}
+
+function chain4(url) {
+    return url.replace(/(javascript|data):/, ""); // NOT OK - but not flagged [INCONSISTENCY]
+}