Merge branch 'main' into incomplete-url-string-sanitization

2026-05-03 04:39:29 +02:00 · 2022-03-24 11:27:30 +01:00
parent bf888f0f0b 496aab78a7
commit 65f8f56095
180 changed files with 9978 additions and 1026 deletions
--- a/javascript/ql/lib/change-notes/2022-03-23-api-graph-receiver-label.md
+++ b/javascript/ql/lib/change-notes/2022-03-23-api-graph-receiver-label.md
@@ -0,0 +1,7 @@
+---
+category: fix
+---
+* The following predicates on `API::Node` have been changed so as not to include the receiver. The receiver should now only be accessed via `getReceiver()`.
+  - `getParameter(int i)` previously included the receiver when `i = -1`
+  - `getAParameter()` previously included the receiver
+  - `getLastParameter()` previously included the receiver for calls with no arguments
--- a/javascript/ql/lib/semmle/javascript/ApiGraphs.qll
+++ b/javascript/ql/lib/semmle/javascript/ApiGraphs.qll
@@ -187,8 +187,7 @@ module API {
    }

    /**
-     * Gets a node representing a parameter or the receiver of the function represented by this
-     * node.
+     * Gets a node representing a parameter of the function represented by this node.
     *
     * This predicate may result in a mix of parameters from different call sites in cases where
     * there are multiple invocations of this API component.
@@ -198,8 +197,6 @@ module API {
    Node getAParameter() {
      Stages::ApiStage::ref() and
      result = this.getParameter(_)
-      or
-      result = this.getReceiver()
    }

    /**
@@ -561,9 +558,10 @@ module API {
          rhs = f.getExceptionalReturn()
        )
        or
-        exists(int i |
-          lbl = Label::parameter(i) and
-          argumentPassing(base, i, rhs)
+        exists(int i | argumentPassing(base, i, rhs) |
+          lbl = Label::parameter(i)
+          or
+          i = -1 and lbl = Label::receiver()
        )
        or
        exists(DataFlow::SourceNode src, DataFlow::PropWrite pw |
@@ -1096,8 +1094,8 @@ module API {
     */
    LabelParameter parameter(int i) { result.getIndex() = i }

-    /** Gets the `parameter` edge label for the receiver. */
-    LabelParameter receiver() { result = parameter(-1) }
+    /** Gets the edge label for the receiver. */
+    LabelReceiver receiver() { any() }

    /** Gets the `return` edge label. */
    LabelReturn return() { any() }
@@ -1132,12 +1130,13 @@ module API {
        MkLabelUnknownMember() or
        MkLabelParameter(int i) {
          i =
-            [-1 .. max(int args |
+            [0 .. max(int args |
                args = any(InvokeExpr invk).getNumArgument() or
                args = any(Function f).getNumParameter()
              )] or
          i = [0 .. 10]
        } or
+        MkLabelReceiver() or
        MkLabelReturn() or
        MkLabelPromised() or
        MkLabelPromisedError() or
@@ -1225,6 +1224,11 @@ module API {
        /** Gets the index of the parameter for this label. */
        int getIndex() { result = i }
      }
+
+      /** A label for the receiver of call, that is, the value passed as `this`. */
+      class LabelReceiver extends ApiLabel, MkLabelReceiver {
+        override string toString() { result = "receiver" }
+      }
    }
  }
 }
--- a/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
+++ b/javascript/ql/lib/semmle/javascript/dataflow/Configuration.qll
@@ -1365,27 +1365,31 @@ private predicate loadStep(

 /**
 * Holds if there is flow to `base.startProp`, and `base.startProp` flows to `nd.endProp` under `cfg/summary`.
+ *
+ * If `onlyRelevantInCall` is true, the `base` object will not be propagated out of return edges, because
+ * the flow that originally reached `base.startProp` used a call edge.
 */
 pragma[nomagic]
 private predicate reachableFromStoreBase(
  string startProp, string endProp, DataFlow::Node base, DataFlow::Node nd,
-  DataFlow::Configuration cfg, PathSummary summary
+  DataFlow::Configuration cfg, PathSummary summary, boolean onlyRelevantInCall
 ) {
  exists(PathSummary s1, PathSummary s2, DataFlow::Node rhs |
-    reachableFromSource(rhs, cfg, s1)
+    reachableFromSource(rhs, cfg, s1) and
+    onlyRelevantInCall = s1.hasCall()
    or
-    reachableFromStoreBase(_, _, _, rhs, cfg, s1)
+    reachableFromStoreBase(_, _, _, rhs, cfg, s1, onlyRelevantInCall)
  |
    storeStep(rhs, nd, startProp, cfg, s2) and
    endProp = startProp and
    base = nd and
    summary =
-      MkPathSummary(false, s1.hasCall().booleanOr(s2.hasCall()), DataFlow::FlowLabel::data(),
-        DataFlow::FlowLabel::data())
+      MkPathSummary(false, s2.hasCall(), DataFlow::FlowLabel::data(), DataFlow::FlowLabel::data())
  )
  or
  exists(PathSummary newSummary, PathSummary oldSummary |
-    reachableFromStoreBaseStep(startProp, endProp, base, nd, cfg, oldSummary, newSummary) and
+    reachableFromStoreBaseStep(startProp, endProp, base, nd, cfg, oldSummary, newSummary,
+      onlyRelevantInCall) and
    summary = oldSummary.appendValuePreserving(newSummary)
  )
 }
@@ -1399,14 +1403,16 @@ private predicate reachableFromStoreBase(
 pragma[noinline]
 private predicate reachableFromStoreBaseStep(
  string startProp, string endProp, DataFlow::Node base, DataFlow::Node nd,
-  DataFlow::Configuration cfg, PathSummary oldSummary, PathSummary newSummary
+  DataFlow::Configuration cfg, PathSummary oldSummary, PathSummary newSummary,
+  boolean onlyRelevantInCall
 ) {
  exists(DataFlow::Node mid |
-    reachableFromStoreBase(startProp, endProp, base, mid, cfg, oldSummary) and
-    flowStep(mid, cfg, nd, newSummary)
+    reachableFromStoreBase(startProp, endProp, base, mid, cfg, oldSummary, onlyRelevantInCall) and
+    flowStep(mid, cfg, nd, newSummary) and
+    onlyRelevantInCall.booleanAnd(newSummary.hasReturn()) = false
    or
    exists(string midProp |
-      reachableFromStoreBase(startProp, midProp, base, mid, cfg, oldSummary) and
+      reachableFromStoreBase(startProp, midProp, base, mid, cfg, oldSummary, onlyRelevantInCall) and
      isAdditionalLoadStoreStep(mid, nd, midProp, endProp, cfg) and
      newSummary = PathSummary::level()
    )
@@ -1446,7 +1452,7 @@ private predicate storeToLoad(
    PathSummary s1, PathSummary s2
  |
    storeStep(pred, storeBase, storeProp, cfg, s1) and
-    reachableFromStoreBase(storeProp, loadProp, storeBase, loadBase, cfg, s2) and
+    reachableFromStoreBase(storeProp, loadProp, storeBase, loadBase, cfg, s2, _) and
    oldSummary = s1.appendValuePreserving(s2) and
    loadStep(loadBase, succ, loadProp, cfg, newSummary)
  )
--- a/javascript/ql/lib/semmle/javascript/internal/ConceptsImports.qll
+++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsImports.qll
@@ -0,0 +1,6 @@
+/**
+ * This file contains imports required for the JavaScript version of `ConceptsShared.qll`.
+ * Since they are language-specific, they can't be placed directly in that file, as it is shared between languages.
+ */
+
+import semmle.javascript.dataflow.DataFlow::DataFlow as DataFlow
--- a/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll
+++ b/javascript/ql/lib/semmle/javascript/internal/ConceptsShared.qll
@@ -0,0 +1,13 @@
+/**
+ * Provides Concepts which are shared across languages.
+ *
+ * Each language has a language specific `Concepts.qll` file that can import the
+ * shared concepts from this file. A language can either re-export the concept directly,
+ * or can add additional member-predicates that are needed for that language.
+ *
+ * Moving forward, `Concepts.qll` will be the staging ground for brand new concepts from
+ * each language, but we will maintain a discipline of moving those concepts to
+ * `ConceptsShared.qll` ASAP.
+ */
+
+private import ConceptsImports
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/ExternalAPIUsedWithUntrustedDataCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/ExternalAPIUsedWithUntrustedDataCustomizations.qll
@@ -219,7 +219,6 @@ module ExternalApiUsedWithUntrustedData {
        or
        exists(string callbackName, int index |
          node = getNamedParameter(base.getParameter(index).getMember(callbackName), paramName) and
-          index != -1 and // ignore receiver
          result =
            basename + ".[callback " + index + " '" + callbackName + "'].[param '" + paramName +
              "']"
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/HttpToFileAccessCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/HttpToFileAccessCustomizations.qll
@@ -4,10 +4,14 @@
 * for adding your own.
 */

-import javascript
-import semmle.javascript.security.dataflow.RemoteFlowSources
-
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * writing user-controlled data to files, as well as extension points
+ * for adding your own.
+ */
 module HttpToFileAccess {
+  import HttpToFileAccessSpecific
+
  /**
   * A data flow source for writing user-controlled data to files.
   */
@@ -23,18 +27,6 @@ module HttpToFileAccess {
   */
  abstract class Sanitizer extends DataFlow::Node { }

-  /**
-   * An access to a user-controlled HTTP request input, considered as a flow source for writing user-controlled data to files
-   */
-  private class RequestInputAccessAsSource extends Source {
-    RequestInputAccessAsSource() { this instanceof HTTP::RequestInputAccess }
-  }
-
-  /** A response from a server, considered as a flow source for writing user-controlled data to files. */
-  private class ServerResponseAsSource extends Source {
-    ServerResponseAsSource() { this = any(ClientRequest r).getAResponseDataNode() }
-  }
-
  /** A sink that represents file access method (write, append) argument */
  class FileAccessAsSink extends Sink {
    FileAccessAsSink() { exists(FileSystemWriteAccess src | this = src.getADataNode()) }
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/HttpToFileAccessQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/HttpToFileAccessQuery.qll
@@ -6,8 +6,7 @@
 * `HttpToFileAccessCustomizations` should be imported instead.
 */

-import javascript
-import HttpToFileAccessCustomizations::HttpToFileAccess
+private import HttpToFileAccessCustomizations::HttpToFileAccess

 /**
 * A taint tracking configuration for writing user-controlled data to files.
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/HttpToFileAccessSpecific.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/HttpToFileAccessSpecific.qll
@@ -0,0 +1,19 @@
+/**
+ * Provides imports and classes needed for `HttpToFileAccessQuery` and `HttpToFileAccessCustomizations`.
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.RemoteFlowSources
+private import HttpToFileAccessCustomizations::HttpToFileAccess
+
+/**
+ * An access to a user-controlled HTTP request input, considered as a flow source for writing user-controlled data to files
+ */
+private class RequestInputAccessAsSource extends Source {
+  RequestInputAccessAsSource() { this instanceof HTTP::RequestInputAccess }
+}
+
+/** A response from a server, considered as a flow source for writing user-controlled data to files. */
+private class ServerResponseAsSource extends Source {
+  ServerResponseAsSource() { this = any(ClientRequest r).getAResponseDataNode() }
+}
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringCustomizations.qll
@@ -3,10 +3,13 @@
 * format injections, as well as extension points for adding your own.
 */

-import javascript
-import semmle.javascript.security.dataflow.DOM
-
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * format injections, as well as extension points for adding your own.
+ */
 module TaintedFormatString {
+  import TaintedFormatStringSpecific
+
  /**
   * A data flow source for format injections.
   */
@@ -23,9 +26,7 @@ module TaintedFormatString {
  abstract class Sanitizer extends DataFlow::Node { }

  /** A source of remote user input, considered as a flow source for format injection. */
-  class RemoteSource extends Source {
-    RemoteSource() { this instanceof RemoteFlowSource }
-  }
+  class RemoteSource extends Source instanceof RemoteFlowSource { }

  /**
   * A format argument to a printf-like function, considered as a flow sink for format injection.
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringQuery.qll
@@ -8,9 +8,7 @@
 * `TaintedFormatStringCustomizations` should be imported instead.
 */

-import javascript
-import semmle.javascript.security.dataflow.DOM
-import TaintedFormatStringCustomizations::TaintedFormatString
+private import TaintedFormatStringCustomizations::TaintedFormatString

 /**
 * A taint-tracking configuration for format injections.
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringSpecific.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/TaintedFormatStringSpecific.qll
@@ -0,0 +1,6 @@
+/**
+ * Provides JS-specific imports needed for `TaintedFormatStringQuery` and `TaintedFormatStringCustomizations`.
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.DOM
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomCustomizations.qll
@@ -32,36 +32,58 @@ module XssThroughDom {
   */
  string unsafeDomPropertyName() { result = ["innerText", "textContent", "value", "name", "src"] }

-  /**
-   * A source for text from the DOM from a JQuery method call.
-   */
-  class JQueryTextSource extends Source, JQuery::MethodCall {
+  /** A read of a DOM property seen as a source for cross-site scripting vulnerabilities through the DOM. */
+  abstract class DomPropertySource extends Source {
+    /**
+     * Gets the name of the DOM property that the source originated from.
+     */
+    abstract string getPropertyName();
+  }
+
+  /* Gets a jQuery method where the receiver looks like `$("<p>" + ... )`, which is benign for this query. */
+  private JQuery::MethodCall benignJQueryMethod() {
+    exists(DataFlow::Node prefix |
+      DomBasedXss::isPrefixOfJQueryHtmlString(result
+            .getReceiver()
+            .(DataFlow::CallNode)
+            .getAnArgument(), prefix)
+    |
+      prefix.getStringValue().regexpMatch("\\s*<.*")
+    )
+  }
+
+  /** A source for text from the DOM from a JQuery method call. */
+  class JQueryTextSource extends Source instanceof JQuery::MethodCall {
    JQueryTextSource() {
-      (
-        this.getMethodName() = ["text", "val"] and this.getNumArgument() = 0
-        or
-        exists(string methodName, string value |
-          this.getMethodName() = methodName and
-          this.getNumArgument() = 1 and
-          forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
-          this.getArgument(0).mayHaveStringValue(value)
-        |
-          methodName = "attr" and value = unsafeAttributeName()
-          or
-          methodName = "prop" and value = unsafeDomPropertyName()
-        )
-      ) and
-      // looks like a $("<p>" + ... ) source, which is benign for this query.
-      not exists(DataFlow::Node prefix |
-        DomBasedXss::isPrefixOfJQueryHtmlString(this.getReceiver()
-              .(DataFlow::CallNode)
-              .getAnArgument(), prefix)
-      |
-        prefix.getStringValue().regexpMatch("\\s*<.*")
-      )
+      this.getMethodName() = ["text", "val"] and
+      this.getNumArgument() = 0 and
+      not this = benignJQueryMethod()
    }
  }

+  /**
+   * A source for text from a DOM property read by jQuery.
+   */
+  class JQueryDOMPropertySource extends DomPropertySource instanceof JQuery::MethodCall {
+    string prop;
+
+    JQueryDOMPropertySource() {
+      exists(string methodName |
+        this.getMethodName() = methodName and
+        this.getNumArgument() = 1 and
+        forex(InferredType t | t = this.getArgument(0).analyze().getAType() | t = TTString()) and
+        this.getArgument(0).mayHaveStringValue(prop)
+      |
+        methodName = "attr" and prop = unsafeAttributeName()
+        or
+        methodName = "prop" and prop = unsafeDomPropertyName()
+      ) and
+      not this = benignJQueryMethod()
+    }
+
+    override string getPropertyName() { result = prop }
+  }
+
  /**
   * A source for text from the DOM from a `d3` method call.
   */
@@ -88,19 +110,25 @@ module XssThroughDom {
  /**
   * A source for text from the DOM from a DOM property read or call to `getAttribute()`.
   */
-  class DomTextSource extends Source {
+  class DomTextSource extends DomPropertySource {
+    string prop;
+
    DomTextSource() {
      exists(DataFlow::PropRead read | read = this |
        read.getBase().getALocalSource() = DOM::domValueRef() and
-        read.mayHavePropertyName(unsafeDomPropertyName())
+        prop = unsafeDomPropertyName() and
+        read.mayHavePropertyName(prop)
      )
      or
      exists(DataFlow::MethodCallNode mcn | mcn = this |
        mcn.getReceiver().getALocalSource() = DOM::domValueRef() and
        mcn.getMethodName() = "getAttribute" and
-        mcn.getArgument(0).mayHaveStringValue(unsafeAttributeName())
+        prop = unsafeAttributeName() and
+        mcn.getArgument(0).mayHaveStringValue(prop)
      )
    }
+
+    override string getPropertyName() { result = prop }
  }

  /** DEPRECATED: Alias for DomTextSource */
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/XssThroughDomQuery.qll
@@ -35,4 +35,13 @@ class Configuration extends TaintTracking::Configuration {
  override predicate isSanitizerEdge(DataFlow::Node pred, DataFlow::Node succ) {
    DomBasedXss::isOptionallySanitizedEdge(pred, succ)
  }
+
+  override predicate hasFlowPath(DataFlow::SourcePathNode src, DataFlow::SinkPathNode sink) {
+    super.hasFlowPath(src, sink) and
+    // filtering away readings of `src` that end in a URL sink.
+    not (
+      sink.getNode() instanceof DomBasedXss::WriteURLSink and
+      src.getNode().(DomPropertySource).getPropertyName() = "src"
+    )
+  }
 }
--- a/javascript/ql/lib/semmle/javascript/security/internal/SensitiveDataHeuristics.qll
+++ b/javascript/ql/lib/semmle/javascript/security/internal/SensitiveDataHeuristics.qll
@@ -98,7 +98,8 @@ module HeuristicNames {
   * suggesting nouns within the string do not represent the meaning of the whole string (e.g. a URL or a SQL query).
   */
  string notSensitiveRegexp() {
-    result = "(?is).*([^\\w$.-]|redact|censor|obfuscate|hash|md5|sha|((?<!un)(en))?(crypt|code)).*"
+    result =
+      "(?is).*([^\\w$.-]|redact|censor|obfuscate|hash|md5|sha|random|((?<!un)(en))?(crypt|code)).*"
  }

  /**