JavaScript: Use custom flow labels in ClientSideUrlRedirect.

javascript/ql/src/semmle/javascript/security/dataflow/ClientSideUrlRedirect.qll

@@ -23,6 +23,14 @@ module ClientSideUrlRedirect {
    */
   abstract class Sanitizer extends DataFlow::Node { }
 
+  /**
+   * A flow label for values that represent the URL of the current document, and
+   * hence are only partially user-controlled.
+   */
+  class DocumentUrl extends DataFlow::FlowLabel {
+    DocumentUrl() { this = "document.url" }
+  }
+
   /**
    * A taint-tracking configuration for reasoning about unvalidated URL redirections.
    */
@@ -35,24 +43,37 @@ module ClientSideUrlRedirect {
       source instanceof Source
     }
 
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel lbl) {
+      source instanceof RemoteFlowSource and
+      lbl = DataFlow::FlowLabel::taint()
+      or
+      isDocumentURL(source.asExpr()) and
+      lbl instanceof DocumentUrl
+    }
+
     override predicate isSink(DataFlow::Node sink) {
       sink instanceof Sink
     }
 
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel f) {
+      sink instanceof UrlSink and
+      f = DataFlow::FlowLabel::taint()
+    }
+
     override predicate isSanitizer(DataFlow::Node node) {
       super.isSanitizer(node) or
-      isSafeLocationProperty(node.asExpr()) or
       node instanceof Sanitizer
     }
 
     override predicate isSanitizer(DataFlow::Node source, DataFlow::Node sink) {
       sanitizingPrefixEdge(source, sink)
     }
-  }
 
-  /** A source of remote user input, considered as a flow source for unvalidated URL redirects. */
-  class RemoteFlowSourceAsSource extends Source {
-    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+    override predicate isAdditionalFlowStep(DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel f, DataFlow::FlowLabel g) {
+      queryAccess(pred, succ) and
+      f instanceof DocumentUrl and
+      g = DataFlow::FlowLabel::taint()
+    }
   }
 
   /**
@@ -84,39 +105,13 @@ module ClientSideUrlRedirect {
     )
   }
 
-  /**
-   * A taint tracking configuration for identifying accesses of the query string of the current URL.
-   */
-  private class LocationHrefDataFlowConfiguration extends TaintTracking::Configuration {
-    LocationHrefDataFlowConfiguration() {
-      this = "LocationHrefDataFlowConfiguration"
-    }
-
-    override predicate isSource(DataFlow::Node source) {
-      isDocumentURL(source.asExpr())
-    }
-
-    override predicate isSink(DataFlow::Node sink) {
-      queryAccess(sink, _)
-    }
-  }
-
-  /**
-   * An access of the query string of the current URL.
-   */
-  class LocationSearchSource extends Source {
-    LocationSearchSource() {
-      exists(LocationHrefDataFlowConfiguration cfg, DataFlow::Node nd |
-        cfg.hasFlow(_, nd) and
-        queryAccess(nd, this)
-      )
-    }
+  abstract class UrlSink extends DataFlow::Node {
   }
 
   /**
    * A sink which is used to set the window location.
    */
-  class LocationSink extends Sink, DataFlow::ValueNode {
+  class LocationSink extends UrlSink, DataFlow::ValueNode {
     LocationSink() {
       // A call to a `window.navigate` or `window.open`
       exists (string name |
@@ -157,7 +152,7 @@ module ClientSideUrlRedirect {
   /**
    * An expression that may be interpreted as the URL of a script.
    */
-  abstract class ScriptUrlSink extends Sink {
+  abstract class ScriptUrlSink extends UrlSink {
   }
 
   /**

javascript/ql/test/query-tests/Security/CWE-601/ClientSideUrlRedirect/ClientSideUrlRedirect.expected

@@ -1,9 +1,8 @@
-| tst2.js:4:21:4:55 | href.su ... '?')+1) | Untrusted URL redirection due to $@. | tst2.js:4:21:4:55 | href.su ... '?')+1) | user-provided value |
+| 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 | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |
 | tst6.js:6:17:6:24 | redirect | Untrusted URL redirection due to $@. | tst6.js:2:18:2:45 | $locati ... irect') | user-provided value |
 | tst6.js:8:21:8:56 | $locati ... + "foo" | Untrusted URL redirection due to $@. | tst6.js:8:21:8:48 | $locati ... irect') | user-provided value |
-| tst7.js:2:12:2:35 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:2:12:2:35 | documen ... .search | user-provided value |
-| tst7.js:5:27:5:50 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:5:27:5:50 | documen ... .search | user-provided value |
-| tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:42 | documen ... on.hash | user-provided value |
-| tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:55 | documen ... ring(1) | user-provided value |
-| tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:19:2:69 | /.*redi ... n.href) | user-provided value |
+| tst7.js:2:12:2:35 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:2:12:2:28 | document.location | user-provided value |
+| tst7.js:5:27:5:50 | documen ... .search | Untrusted URL redirection due to $@. | tst7.js:5:27:5:43 | document.location | user-provided value |
+| tst9.js:2:21:2:55 | documen ... ring(1) | Untrusted URL redirection due to $@. | tst9.js:2:21:2:37 | document.location | user-provided value |
+| tst.js:2:19:2:72 | /.*redi ... ref)[1] | Untrusted URL redirection due to $@. | tst.js:2:47:2:63 | document.location | user-provided value |