Merge pull request github#5033 from asgerf/js/generalized-remote-flow-source

Approved by erik-krogh

Author: codeql-ci

javascript/change-notes/2021-03-15-client-side-remote-flow-sources.md

@@ -0,0 +1,6 @@
+lgtm,codescanning
+* The security queries now distinguish more clearly between different parts of `window.location`.
+  When the taint source of an alert is based on `window.location`, the source will usually
+  occur closer to where user-controlled data is obtained, such as at `location.hash`.
+* `js/request-forgery` no longer considers client-side path parameters to be a source due to
+  the restricted character set usable in a path, resulting in fewer false-positive results.

javascript/ql/src/semmle/javascript/Closure.qll

@@ -129,6 +129,9 @@ module Closure {
     container = result.getContainer()
   }
 
+  pragma[noinline]
+  private ClosureRequireCall getARequireInTopLevel(ClosureModule m) { result.getTopLevel() = m }
+
   /**
    * A module using the Closure module system, declared using `goog.module()` or `goog.declareModuleId()`.
    */
@@ -146,10 +149,8 @@ module Closure {
     string getClosureNamespace() { result = getModuleDeclaration().getClosureNamespace() }
 
     override Module getAnImportedModule() {
-      exists(ClosureRequireCall imprt |
-        imprt.getTopLevel() = this and
-        result.(ClosureModule).getClosureNamespace() = imprt.getClosureNamespace()
-      )
+      result.(ClosureModule).getClosureNamespace() =
+        getARequireInTopLevel(this).getClosureNamespace()
     }
 
     /**

javascript/ql/src/semmle/javascript/ES2015Modules.qll

@@ -1,6 +1,7 @@
 /** Provides classes for working with ECMAScript 2015 modules. */
 
 import javascript
+private import semmle.javascript.internal.CachedStages
 
 /**
  * An ECMAScript 2015 module.
@@ -654,7 +655,9 @@ abstract class ReExportDeclaration extends ExportDeclaration {
   ES2015Module getReExportedES2015Module() { result = getReExportedModule() }
 
   /** Gets the module from which this declaration re-exports. */
+  cached
   Module getReExportedModule() {
+    Stages::Imports::ref() and
     result.getFile() = getEnclosingModule().resolve(getImportedPath().(PathExpr))
     or
     result = resolveFromTypeRoot()

javascript/ql/src/semmle/javascript/dataflow/TaintTracking.qll

@@ -239,6 +239,35 @@ module TaintTracking {
     }
   }
 
+  /** Gets a data flow node referring to the client side URL. */
+  private DataFlow::SourceNode clientSideUrlRef(DataFlow::TypeTracker t) {
+    t.start() and
+    result.(ClientSideRemoteFlowSource).getKind().isUrl()
+    or
+    exists(DataFlow::TypeTracker t2 | result = clientSideUrlRef(t2).track(t2, t))
+  }
+
+  /** Gets a data flow node referring to the client side URL. */
+  private DataFlow::SourceNode clientSideUrlRef() {
+    result = clientSideUrlRef(DataFlow::TypeTracker::end())
+  }
+
+  /**
+   * Holds if `read` reads a property of the client-side URL, which is not tainted.
+   * In this case, the read is excluded from the default set of taint steps.
+   */
+  private predicate isSafeClientSideUrlProperty(DataFlow::PropRead read) {
+    // Block all properties of client-side URLs, as .hash and .search are considered sources of their own
+    read = clientSideUrlRef().getAPropertyRead()
+    or
+    exists(StringSplitCall c |
+      c.getBaseString().getALocalSource() =
+        [DOM::locationRef(), DOM::locationRef().getAPropertyRead("href")] and
+      c.getSeparator() = "?" and
+      read = c.getAPropertyRead("0")
+    )
+  }
+
   /**
    * Holds if there is taint propagation through the heap from `pred` to `succ`.
    */
@@ -264,7 +293,8 @@ module TaintTracking {
     or
     // reading from a tainted object yields a tainted result
     succ.(DataFlow::PropRead).getBase() = pred and
-    not AccessPath::DominatingPaths::hasDominatingWrite(succ)
+    not AccessPath::DominatingPaths::hasDominatingWrite(succ) and
+    not isSafeClientSideUrlProperty(succ)
     or
     // iterating over a tainted iterator taints the loop variable
     exists(ForOfStmt fos |

javascript/ql/src/semmle/javascript/frameworks/Angular2.qll

@@ -77,46 +77,61 @@ module Angular2 {
   }
 
   /** Gets a reference to a `ParamMap` object, usually containing values from the URL. */
-  DataFlow::SourceNode paramMap() {
-    result.hasUnderlyingType("@angular/router", "ParamMap")
+  private DataFlow::SourceNode paramMap(ClientSideRemoteFlowKind kind) {
+    result.hasUnderlyingType("@angular/router", "ParamMap") and kind.isPath()
     or
-    result = activatedRouteProp(["paramMap", "queryParamMap"])
+    result = activatedRouteProp("paramMap") and kind.isPath()
     or
-    result = urlSegment().getAPropertyRead("parameterMap")
+    result = activatedRouteProp("queryParamMap") and kind.isQuery()
+    or
+    result = urlSegment().getAPropertyRead("parameterMap") and kind.isPath()
   }
 
+  /** Gets a reference to a `ParamMap` object, usually containing values from the URL. */
+  DataFlow::SourceNode paramMap() { result = paramMap(_) }
+
   /** Gets a reference to a `Params` object, usually containing values from the URL. */
-  DataFlow::SourceNode paramDictionaryObject() {
+  private DataFlow::SourceNode paramDictionaryObject(ClientSideRemoteFlowKind kind) {
     result.hasUnderlyingType("@angular/router", "Params") and
+    kind.isPath() and
     not result instanceof DataFlow::ObjectLiteralNode // ignore object literals found by contextual typing
     or
-    result = activatedRouteProp(["params", "queryParams"])
+    result = activatedRouteProp("params") and kind.isPath()
+    or
+    result = activatedRouteProp("queryParams") and kind.isQuery()
     or
-    result = paramMap().getAPropertyRead("params")
+    result = paramMap(kind).getAPropertyRead("params")
     or
-    result = urlSegment().getAPropertyRead("parameters")
+    result = urlSegment().getAPropertyRead("parameters") and kind.isPath()
   }
 
+  /** Gets a reference to a `Params` object, usually containing values from the URL. */
+  DataFlow::SourceNode paramDictionaryObject() { result = paramDictionaryObject(_) }
+
   /**
    * A value from `@angular/router` derived from the URL.
    */
-  class AngularSource extends RemoteFlowSource {
+  class AngularSource extends ClientSideRemoteFlowSource {
+    ClientSideRemoteFlowKind kind;
+
     AngularSource() {
-      this = paramMap().getAMethodCall(["get", "getAll"])
+      this = paramMap(kind).getAMethodCall(["get", "getAll"])
       or
-      this = paramDictionaryObject()
+      this = paramDictionaryObject(kind)
       or
-      this = activatedRouteProp("fragment")
+      this = activatedRouteProp("fragment") and kind.isFragment()
       or
-      this = urlSegment().getAPropertyRead("path")
+      this = urlSegment().getAPropertyRead("path") and kind.isPath()
       or
       // Note that Router.url and RouterStateSnapshot.url are strings, not UrlSegment[]
-      this = router().getAPropertyRead("url")
+      this = router().getAPropertyRead("url") and kind.isUrl()
       or
-      this = routerStateSnapshot().getAPropertyRead("url")
+      this = routerStateSnapshot().getAPropertyRead("url") and kind.isUrl()
     }
 
     override string getSourceType() { result = "Angular route parameter" }
+
+    override ClientSideRemoteFlowKind getKind() { result = kind }
   }
 
   /** Gets a reference to a `DomSanitizer` object. */

javascript/ql/src/semmle/javascript/frameworks/AngularJS/AngularJSCore.qll

@@ -636,7 +636,7 @@ private class LocationFlowSource extends RemoteFlowSource {
  *
  * See https://docs.angularjs.org/api/ngRoute/service/$routeParams for more details.
  */
-private class RouteParamSource extends RemoteFlowSource {
+private class RouteParamSource extends ClientSideRemoteFlowSource {
   RouteParamSource() {
     exists(ServiceReference service |
       service.getName() = "$routeParams" and
@@ -645,6 +645,8 @@ private class RouteParamSource extends RemoteFlowSource {
   }
 
   override string getSourceType() { result = "$routeParams" }
+
+  override ClientSideRemoteFlowKind getKind() { result.isPath() }
 }
 
 /**

javascript/ql/src/semmle/javascript/frameworks/React.qll

@@ -686,14 +686,24 @@ private DataFlow::SourceNode reactRouterDom() {
   result = DataFlow::moduleImport("react-router-dom")
 }
 
-private class ReactRouterSource extends RemoteFlowSource {
+private class ReactRouterSource extends ClientSideRemoteFlowSource {
+  ClientSideRemoteFlowKind kind;
+
   ReactRouterSource() {
-    this = reactRouterDom().getAMemberCall("useParams")
+    this = reactRouterDom().getAMemberCall("useParams") and kind.isPath()
     or
-    this = reactRouterDom().getAMemberCall("useRouteMatch").getAPropertyRead(["params", "url"])
+    exists(string prop |
+      this = reactRouterDom().getAMemberCall("useRouteMatch").getAPropertyRead(prop)
+    |
+      prop = "params" and kind.isPath()
+      or
+      prop = "url" and kind.isUrl()
+    )
   }
 
   override string getSourceType() { result = "react-router path parameters" }
+
+  override ClientSideRemoteFlowKind getKind() { result = kind }
 }
 
 /**

javascript/ql/src/semmle/javascript/internal/CachedStages.qll

@@ -162,6 +162,8 @@ module Stages {
       exists(any(Import i).getImportedModule())
       or
       exists(DataFlow::moduleImport(_))
+      or
+      exists(any(ReExportDeclaration d).getReExportedModule())
     }
   }
 

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

@@ -25,11 +25,8 @@ module ClientSideUrlRedirect {
   class Configuration extends TaintTracking::Configuration {
     Configuration() { this = "ClientSideUrlRedirect" }
 
-    override predicate isSource(DataFlow::Node source) { source instanceof Source }
-
     override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel lbl) {
-      source = DOM::locationSource() and
-      lbl instanceof DocumentUrl
+      source.(Source).getAFlowLabel() = lbl
     }
 
     override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
@@ -46,7 +43,7 @@ module ClientSideUrlRedirect {
     override predicate isAdditionalFlowStep(
       DataFlow::Node pred, DataFlow::Node succ, DataFlow::FlowLabel f, DataFlow::FlowLabel g
     ) {
-      queryAccess(pred, succ) and
+      untrustedUrlSubstring(pred, succ) and
       f instanceof DocumentUrl and
       g.isTaint()
       or

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

@@ -13,7 +13,10 @@ module ClientSideUrlRedirect {
   /**
    * A data flow source for unvalidated URL redirect vulnerabilities.
    */
-  abstract class Source extends DataFlow::Node { }
+  abstract class Source extends DataFlow::Node {
+    /** Gets a flow label to associate with this source. */
+    DataFlow::FlowLabel getAFlowLabel() { result.isTaint() }
+  }
 
   /**
    * A data flow sink for unvalidated URL redirect vulnerabilities.
@@ -35,22 +38,32 @@ module ClientSideUrlRedirect {
 
   /** A source of remote user input, considered as a flow source for unvalidated URL redirects. */
   class RemoteFlowSourceAsSource extends Source {
-    RemoteFlowSourceAsSource() { this instanceof RemoteFlowSource }
+    RemoteFlowSourceAsSource() {
+      this instanceof RemoteFlowSource and
+      not this.(ClientSideRemoteFlowSource).getKind().isPath()
+    }
+
+    override DataFlow::FlowLabel getAFlowLabel() {
+      if this.(ClientSideRemoteFlowSource).getKind().isUrl()
+      then result instanceof DocumentUrl
+      else result.isTaint()
+    }
   }
 
   /**
-   * Holds if `queryAccess` is an expression that may access the query string
-   * of a URL that flows into `nd` (that is, the part after the `?`).
+   * DEPRECATED. Can usually be replaced with `untrustedUrlSubstring`.
+   * Query accesses via `location.hash` or `location.search` are now independent
+   * `RemoteFlowSource` instances, and only substrings of `location` need to be handled via steps.
    */
-  predicate queryAccess(DataFlow::Node nd, DataFlow::Node queryAccess) {
-    exists(string propertyName |
-      queryAccess.asExpr().(PropAccess).accesses(nd.asExpr(), propertyName)
-    |
-      propertyName = "search" or propertyName = "hash"
-    )
-    or
+  deprecated predicate queryAccess = untrustedUrlSubstring/2;
+
+  /**
+   * 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) {
     exists(MethodCallExpr mce, string methodName |
-      mce = queryAccess.asExpr() and mce.calls(nd.asExpr(), methodName)
+      mce = substring.asExpr() and mce.calls(base.asExpr(), methodName)
     |
       methodName = "split" and
       // exclude all splits where only the prefix is accessed, which is safe for url-redirects.
@@ -63,17 +76,12 @@ module ClientSideUrlRedirect {
     )
     or
     exists(MethodCallExpr mce |
-      queryAccess.asExpr() = mce and
+      substring.asExpr() = mce and
       mce = any(DataFlow::RegExpCreationNode re).getAMethodCall("exec").asExpr() and
-      nd.asExpr() = mce.getArgument(0)
+      base.asExpr() = mce.getArgument(0)
     )
   }
 
-  /**
-   * A sanitizer that reads the first part a location split by "?", e.g. `location.href.split('?')[0]`.
-   */
-  class QueryPrefixSanitizer extends Sanitizer, DomBasedXss::QueryPrefixSanitizer { }
-
   /**
    * A sink which is used to set the window location.
    */