JS: Add StartsWith sanitizer

2020-07-03 14:34:59 +01:00 · 2020-07-03 14:34:59 +01:00 · b5104ae42d
--- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll
@ -50,5 +50,9 @@ module ClientSideUrlRedirect {
      g instanceof DocumentUrl and
      succ.(DataFlow::PropRead).accesses(pred, "href")
    }
+
+    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
+      guard instanceof HostnameSanitizerGuard
+    }
  }
 }
--- a/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll
@ -6,7 +6,7 @@

 import javascript
 import semmle.javascript.security.dataflow.RemoteFlowSources
-import UrlConcatenation
+private import UrlConcatenation

 module ClientSideUrlRedirect {
  private import Xss::DomBasedXss as DomBasedXss
--- a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirect.qll
@ -34,7 +34,8 @@ module ServerSideUrlRedirect {
    }

    override predicate isSanitizerGuard(TaintTracking::SanitizerGuardNode guard) {
-      guard instanceof LocalUrlSanitizingGuard
+      guard instanceof LocalUrlSanitizingGuard or
+      guard instanceof HostnameSanitizerGuard
    }
  }
 }
--- a/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirectCustomizations.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/ServerSideUrlRedirectCustomizations.qll
@ -6,7 +6,7 @@

 import javascript
 import RemoteFlowSources
-import UrlConcatenation
+private import UrlConcatenation

 module ServerSideUrlRedirect {
  /**
--- a/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll
+++ b/javascript/ql/src/semmle/javascript/security/dataflow/UrlConcatenation.qll
@ -96,3 +96,17 @@ predicate hostnameSanitizingPrefixEdge(DataFlow::Node source, DataFlow::Node sin
    hasHostnameSanitizingSubstring(StringConcatenation::getOperand(operator, [0 .. n - 1]))
  )
 }
+
+/**
+ * A check that sanitizes the hostname of a URL.
+ */
+class HostnameSanitizerGuard extends TaintTracking::SanitizerGuardNode, StringOps::StartsWith {
+  HostnameSanitizerGuard() {
+    hasHostnameSanitizingSubstring(getSubstring())
+  }
+
+  override predicate sanitizes(boolean outcome, Expr e) {
+    outcome = getPolarity() and
+    e = getBaseString().asExpr()
+  }
+}
--- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected
@ -4,12 +4,6 @@ nodes
 | sanitizer.js:2:15:2:25 | window.name |
 | sanitizer.js:4:27:4:29 | url |
 | sanitizer.js:4:27:4:29 | url |
-| sanitizer.js:7:27:7:29 | url |
-| sanitizer.js:7:27:7:29 | url |
-| sanitizer.js:10:27:10:29 | url |
-| sanitizer.js:10:27:10:29 | url |
-| sanitizer.js:13:27:13:29 | url |
-| sanitizer.js:13:27:13:29 | url |
 | sanitizer.js:16:27:16:29 | url |
 | sanitizer.js:16:27:16:29 | url |
 | sanitizer.js:19:27:19:29 | url |
@ -22,8 +16,6 @@ nodes
 | sanitizer.js:28:27:28:29 | url |
 | sanitizer.js:31:27:31:29 | url |
 | sanitizer.js:31:27:31:29 | url |
-| sanitizer.js:34:27:34:29 | url |
-| sanitizer.js:34:27:34:29 | url |
 | sanitizer.js:37:27:37:29 | url |
 | sanitizer.js:37:27:37:29 | url |
 | tst2.js:2:7:2:33 | href |
@ -109,12 +101,6 @@ nodes
 edges
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url |
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:4:27:4:29 | url |
-| sanitizer.js:2:9:2:25 | url | sanitizer.js:7:27:7:29 | url |
-| sanitizer.js:2:9:2:25 | url | sanitizer.js:7:27:7:29 | url |
-| sanitizer.js:2:9:2:25 | url | sanitizer.js:10:27:10:29 | url |
-| sanitizer.js:2:9:2:25 | url | sanitizer.js:10:27:10:29 | url |
-| sanitizer.js:2:9:2:25 | url | sanitizer.js:13:27:13:29 | url |
-| sanitizer.js:2:9:2:25 | url | sanitizer.js:13:27:13:29 | url |
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url |
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:16:27:16:29 | url |
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:19:27:19:29 | url |
@ -127,8 +113,6 @@ edges
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:28:27:28:29 | url |
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:31:27:31:29 | url |
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:31:27:31:29 | url |
-| sanitizer.js:2:9:2:25 | url | sanitizer.js:34:27:34:29 | url |
-| sanitizer.js:2:9:2:25 | url | sanitizer.js:34:27:34:29 | url |
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:37:27:37:29 | url |
 | sanitizer.js:2:9:2:25 | url | sanitizer.js:37:27:37:29 | url |
 | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:2:9:2:25 | url |
@ -209,16 +193,12 @@ edges
 | tst.js:6:34:6:55 | documen ... on.href | tst.js:6:20:6:56 | indirec ... n.href) |
 #select
 | sanitizer.js:4:27:4:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:4:27:4:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
-| sanitizer.js:7:27:7:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:7:27:7:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
-| sanitizer.js:10:27:10:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:10:27:10:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
-| sanitizer.js:13:27:13:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:13:27:13:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
 | sanitizer.js:16:27:16:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:16:27:16:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
 | sanitizer.js:19:27:19:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:19:27:19:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
 | sanitizer.js:22:27:22:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:22:27:22:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
 | sanitizer.js:25:27:25:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:25:27:25:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
 | sanitizer.js:28:27:28:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:28:27:28:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
 | sanitizer.js:31:27:31:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:31:27:31:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
-| sanitizer.js:34:27:34:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:34:27:34:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
 | sanitizer.js:37:27:37:29 | url | sanitizer.js:2:15:2:25 | window.name | sanitizer.js:37:27:37:29 | url | Untrusted URL redirection due to $@. | sanitizer.js:2:15:2:25 | window.name | user-provided value |
 | tst2.js:4:21:4:55 | href.su ... '?')+1) | tst2.js:2:14:2:28 | window.location | tst2.js:4:21:4:55 | href.su ... '?')+1) | Untrusted URL redirection due to $@. | tst2.js:2:14:2:28 | window.location | user-provided value |
 | tst6.js:4:21:4:28 | redirect | tst6.js:2:18:2:45 | $locati ... irect') | tst6.js:4:21:4:28 | redirect | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |
--- a/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js
+++ b/javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/sanitizer.js
@ -4,13 +4,13 @@ function f() {
        window.location = url; // NOT OK - can be example.com.evil.com
    }
    if (url.startsWith('https://example.com/')) {
-        window.location = url; // OK - but flagged anyway
+        window.location = url; // OK
    }
    if (url.startsWith('https://example.com//')) {
-        window.location = url; // OK - but flagged anyway
+        window.location = url; // OK
    }
    if (url.startsWith('https://example.com/foo')) {
-        window.location = url; // OK - but flagged anyway 
+        window.location = url; // OK 
    }
    if (url.startsWith('https://')) {
        window.location = url; // NOT OK - does not restrict hostname
@ -31,9 +31,13 @@ function f() {
        window.location = url; // NOT OK - can be //example.com.evil.com
    }
    if (url.startsWith('//example.com/')) {
-        window.location = url; // OK - but flagged anyway
+        window.location = url; // OK
    }
    if (url.endsWith('https://example.com/')) {
        window.location = url; // NOT OK - could be evil.com?x=https://example.com/
    }
+    let basedir = whatever() ? 'foo' : 'bar';
+    if (url.startsWith('https://example.com/' + basedir)) {
+        window.location = url; // OK - the whole prefix is not known, but enough to restrict hostname
+    }
 }