JS: Replace DocumentUrl with TaintedUrlSuffix

Author: asgerf

javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectCustomizations.qll

@@ -5,6 +5,7 @@
  */
 
 import javascript
+private import semmle.javascript.security.TaintedUrlSuffix
 
 module ClientSideUrlRedirect {
   /**
@@ -31,12 +32,12 @@ module ClientSideUrlRedirect {
   abstract class Sanitizer extends DataFlow::Node { }
 
   /**
+   * DEPRECATED. Replaced by functionality from the `TaintedUrlSuffix` library.
+   *
    * A flow label for values that represent the URL of the current document, and
    * hence are only partially user-controlled.
    */
-  abstract class DocumentUrl extends DataFlow::FlowLabel {
-    DocumentUrl() { this = "document.url" } // TODO: replace with TaintedUrlSuffix
-  }
+  deprecated class DocumentUrl = TaintedUrlSuffix::TaintedUrlSuffixLabel;
 
   /**
    * DEPRECATED: Use `ActiveThreatModelSource` from Concepts instead!
@@ -50,8 +51,8 @@ module ClientSideUrlRedirect {
     ActiveThreatModelSourceAsSource() { not this.(ClientSideRemoteFlowSource).getKind().isPath() }
 
     override DataFlow::FlowLabel getAFlowLabel() {
-      if this.(ClientSideRemoteFlowSource).getKind().isUrl()
-      then result instanceof DocumentUrl
+      if this = TaintedUrlSuffix::source()
+      then result = TaintedUrlSuffix::label()
       else result.isTaint()
     }
   }
@@ -60,7 +61,7 @@ module ClientSideUrlRedirect {
    * Holds if `node` extracts a part of a URL that does not contain the suffix.
    */
   pragma[inline]
-  predicate isPrefixExtraction(DataFlow::MethodCallNode node) {
+  deprecated predicate isPrefixExtraction(DataFlow::MethodCallNode node) {
     // Block flow through prefix-extraction `substring(0, ...)` and `split("#")[0]`
     node.getMethodName() = [StringOps::substringMethodName(), "split"] and
     not untrustedUrlSubstring(_, node)
@@ -70,7 +71,7 @@ module ClientSideUrlRedirect {
    * Holds if `substring` refers to a substring of `base` which is considered untrusted
    * when `base` is the current URL.
    */
-  predicate untrustedUrlSubstring(DataFlow::Node base, DataFlow::Node substring) {
+  deprecated predicate untrustedUrlSubstring(DataFlow::Node base, DataFlow::Node substring) {
     exists(DataFlow::MethodCallNode mcn, string methodName |
       mcn = substring and mcn.calls(base, methodName)
     |

javascript/ql/lib/semmle/javascript/security/dataflow/ClientSideUrlRedirectQuery.qll

@@ -10,9 +10,10 @@
 import javascript
 import UrlConcatenation
 import ClientSideUrlRedirectCustomizations::ClientSideUrlRedirect
+import semmle.javascript.security.TaintedUrlSuffix
 
 // Materialize flow labels
-private class ConcreteDocumentUrl extends DocumentUrl {
+deprecated private class ConcreteDocumentUrl extends DocumentUrl {
   ConcreteDocumentUrl() { this = this }
 }
 
@@ -35,8 +36,7 @@ module ClientSideUrlRedirectConfig implements DataFlow::StateConfigSig {
   }
 
   predicate isBarrier(DataFlow::Node node, DataFlow::FlowLabel state) {
-    isPrefixExtraction(node) and
-    state instanceof DocumentUrl
+    TaintedUrlSuffix::isBarrier(node, state)
   }
 
   predicate isBarrierOut(DataFlow::Node node) { hostnameSanitizingPrefixEdge(node, _) }
@@ -47,9 +47,7 @@ module ClientSideUrlRedirectConfig implements DataFlow::StateConfigSig {
     DataFlow::Node node1, DataFlow::FlowLabel state1, DataFlow::Node node2,
     DataFlow::FlowLabel state2
   ) {
-    untrustedUrlSubstring(node1, node2) and
-    state1 instanceof DocumentUrl and
-    state2.isTaint()
+    TaintedUrlSuffix::step(node1, node2, state1, state2)
     or
     exists(HtmlSanitizerCall call |
       node1 = call.getInput() and

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

@@ -1,13 +1,13 @@
 import React from "react";
 import {Helmet} from "react-helmet";
- 
+
 class Application extends React.Component {
   render () {
     return (
         <div className="application">
             <Helmet>
                 <title>My unsafe app</title>
-                <script type="application/javascript" src={document.location.hash}/>
+                <script type="application/javascript" src={document.location.hash.substr(1)}/> {/* NOT OK */}
             </Helmet>
         </div>
     );
@@ -18,28 +18,31 @@ export default Application
 
 import Link from 'next/link'
 export function NextLink() {
-    return <Link href={document.location.hash}><a>this page!</a></Link>;
+    return <>
+      <Link href={document.location.hash}><a>safe</a></Link> {/* OK */}
+      <Link href={document.location.hash.substr(1)}><a>unsafe</a></Link> {/* NOT OK */}
+    </>;
 }
 
 import { useRouter } from 'next/router'
 
 export function nextRouter() {
   const router = useRouter();
-  return <span onClick={() => router.push(document.location.hash.substr(1))}>Click to XSS 1</span>
+  return <span onClick={() => router.push(document.location.hash.substr(1))}>Click to XSS 1</span> // NOT OK
 }
 
 import { withRouter } from 'next/router'
 
 function Page({ router }) {
-  return <span onClick={() => router.push(document.location.hash.substr(1))}>Click to XSS 2</span>
+  return <span onClick={() => router.push(document.location.hash.substr(1))}>Click to XSS 2</span> // NOT OK
 }
 
 export const pageWithRouter = withRouter(Page);
 
 export function plainLink() {
-  return <a href={document.location.hash.substr(1)}>my plain link!</a>;
+  return <a href={document.location.hash.substr(1)}>my plain link!</a>; // NOT OK
 }
 
 export function someUnknown() {
-  return <FOO data={document.location.hash.substr(1)}>is safe.</FOO>;
-}
+  return <FOO data={document.location.hash.substr(1)}>is safe.</FOO>; // OK
+}

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

@@ -1,14 +1,14 @@
 // OK - cannot affect hostname
-location.href = '/foo' + document.location.search;
+location.href = '/foo' + document.location.search.substring(1);
 
 // NOT OK
-location.href = '/' + document.location.search;
+location.href = '/' + document.location.search.substring(1);
 
 // NOT OK
-location.href = '//' + document.location.search;
+location.href = '//' + document.location.search.substring(1);
 
 // NOT OK
-location.href = '//foo' + document.location.search;
+location.href = '//foo' + document.location.search.substring(1);
 
 // NOT OK
-location.href = 'https://foo' + document.location.search;
+location.href = 'https://foo' + document.location.search.substring(1);

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

@@ -1,5 +1,5 @@
 function foo() {
     var urlParts = window.location.hash.split('?');
     var loc = urlParts[0] + "?" + boxes.value;
-    window.location = loc; // OK [INCONSISTENCY] - always starts with '#'
+    window.location = loc; // OK - always starts with '#'
 }

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

@@ -1,8 +1,18 @@
 function foo() {
     var url = document.location.toString();
-    window.location = url.substring(0).substring(1); // OK
-    window.location = url.substring(0, 10).substring(1); // OK
-    window.location = url.substring(0, url.indexOf('/', 10)).substring(1); // OK
+    window.location = url.substring(0).substring(1); // OK [INCONSISTENCY] - but not important
+    window.location = url.substring(0, 10).substring(1); // OK [INCONSISTENCY]
+    window.location = url.substring(0, url.indexOf('/', 10)).substring(1); // OK [INCONSISTENCY]
+
+    var url2 = document.location.toString();
+    window.location = url2.substring(0).substring(unknown()); // NOT OK
+    window.location = url2.substring(0, 10).substring(unknown()); // NOT OK
+    window.location = url2.substring(0, url2.indexOf('/', 10)).substring(unknown()); // NOT OK
+
+    var search = document.location.search.toString();
+    window.location = search.substring(0).substring(1); // NOT OK
+    window.location = search.substring(0, 10).substring(1); // NOT OK
+    window.location = search.substring(0, search.indexOf('/', 10)).substring(1); // NOT OK
 }
 
 function bar() {

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

@@ -1,5 +1,5 @@
 // NOT OK
-new Worker(document.location.search);
+new Worker(document.location.search.substring(1));
 
 // NOT OK
-$("<script>").attr("src", document.location.search);
+$("<script>").attr("src", document.location.search.substring(1));