Diff-informed queries via primary/secondary abstractions

java/ql/lib/semmle/code/java/security/XSS.qll

@@ -44,10 +44,10 @@ private class DefaultXssSink extends XssSink {
   DefaultXssSink() {
     sinkNode(this, ["html-injection", "js-injection"])
     or
-    exists(MethodCall ma |
-      ma.getMethod() instanceof WritingMethod and
-      XssVulnerableWriterSourceToWritingMethodFlow::flowToExpr(ma.getQualifier()) and
-      this.asExpr() = ma.getArgument(_)
+    exists(DataFlow::Node n |
+      XssVulnerableWriterSourceToWritingMethodFlow::flowTo(n) and
+      XssVulnerableWriterSourceToWritingMethodFlowSecondaryConfig::getPrimaryOfSecondaryNode(_, n) =
+        this
     )
   }
 }
@@ -73,8 +73,24 @@ private module XssVulnerableWriterSourceToWritingMethodFlowConfig implements Dat
   }
 }
 
+private module XssVulnerableWriterSourceToWritingMethodFlowSecondaryConfig implements
+  DataFlow::SecondaryConfig
+{
+  DataFlow::Node getPrimaryOfSecondaryNode(
+    DataFlow::IsSourceOrSink sourceOrSink, DataFlow::Node sink
+  ) {
+    sourceOrSink instanceof DataFlow::IsSink and
+    exists(MethodCall ma |
+      XssVulnerableWriterSourceToWritingMethodFlowConfig::isSink(sink) and
+      sink.asExpr() = ma.getQualifier() and
+      result.asExpr() = ma.getAnArgument()
+    )
+  }
+}
+
 private module XssVulnerableWriterSourceToWritingMethodFlow =
-  TaintTracking::Global<XssVulnerableWriterSourceToWritingMethodFlowConfig>;
+  TaintTracking::FindSinks<XssVulnerableWriterSourceToWritingMethodFlowConfig,
+    XssVulnerableWriterSourceToWritingMethodFlowSecondaryConfig>;
 
 /** A method that can be used to output data to an output stream or writer. */
 private class WritingMethod extends Method {

java/ql/lib/semmle/code/java/security/XssQuery.qll

@@ -20,9 +20,7 @@ module XssConfig implements DataFlow::ConfigSig {
   predicate isAdditionalFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
     any(XssAdditionalTaintStep s).step(node1, node2)
   }
-
-  predicate observeDiffInformedIncrementalMode() { any() }
 }
 
 /** Tracks flow from remote sources to cross site scripting vulnerabilities. */
-module XssFlow = TaintTracking::Global<XssConfig>;
+module XssFlow = TaintTracking::Primary<XssConfig>;

java/ql/src/Security/CWE/CWE-079/XSS.ql

@@ -15,6 +15,16 @@
 import semmle.code.java.security.XssQuery
 import XssFlow::PathGraph
 
+class IsDiffInformed extends DataFlow::DiffInformedQuery {
+  // This predicate is overridden to be more precise than the default
+  // implementation in order to support secondary secondary data-flow
+  // configurations that find sinks.
+  override Location getASelectedSourceLocation(DataFlow::Node source) {
+    XssConfig::isSource(source) and
+    result = source.getLocation()
+  }
+}
+
 from XssFlow::PathNode source, XssFlow::PathNode sink
 where XssFlow::flowPath(source, sink)
 select sink.getNode(), source, sink, "Cross-site scripting vulnerability due to a $@.",

shared/dataflow/codeql/dataflow/DataFlow.qll

@@ -689,6 +689,10 @@
     predicate flowToExpr(DataFlowExpr sink);
   }
 
+  class DiffInformedQuery = DiffInformedQueryImpl;
+
+  import SecondaryConfigHelpers
+
   /**
    * Constructs a global data flow computation.
    */
@@ -742,6 +746,33 @@
     import Flow
   }
 
+  module Primary<ConfigSig Config> implements GlobalFlowSig {
+    private module Config0 implements FullStateConfigSig {
+      import DefaultState<Config>
+      import Config
+
+      predicate accessPathLimit = Config::accessPathLimit/0;
+
+      predicate isAdditionalFlowStep(Node node1, Node node2, string model) {
+        Config::isAdditionalFlowStep(node1, node2) and model = "Config"
+      }
+    }
+
+    private module C implements FullStateConfigSig {
+      import MakePrimaryDiffInformed<Config0>
+
+      predicate accessPathLimit = Config0::accessPathLimit/0;
+    }
+
+    private module Stage1 = ImplStage1<C>;
+
+    import Stage1::PartialFlow
+
+    private module Flow = Impl<C, Stage1::Stage1NoState>;
+
+    import Flow
+  }
+
   signature class PathNodeSig {
     /** Gets a textual representation of this element. */
     string toString();

shared/dataflow/codeql/dataflow/TaintTracking.qll

@@ -143,6 +143,61 @@
     import Flow
   }
 
+  /**
+   * Constructs a global taint tracking computation.
+   */
+  module Primary<DataFlow::ConfigSig Config> implements DataFlow::GlobalFlowSig {
+    private module Config0 implements DataFlowInternal::FullStateConfigSig {
+      import DataFlowInternal::DefaultState<Config>
+      import Config
+
+      predicate isAdditionalFlowStep(
+        DataFlowLang::Node node1, DataFlowLang::Node node2, string model
+      ) {
+        Config::isAdditionalFlowStep(node1, node2) and model = "Config"
+      }
+    }
+
+    private module C implements DataFlowInternal::FullStateConfigSig {
+      import DataFlowInternal::MakePrimaryDiffInformed<AddTaintDefaults<Config0>>
+    }
+
+    private module Stage1 = DataFlowInternalStage1::ImplStage1<C>;
+
+    import Stage1::PartialFlow
+
+    private module Flow = DataFlowInternal::Impl<C, Stage1::Stage1NoState>;
+
+    import Flow
+  }
+
+  module FindSinks<DataFlow::ConfigSig Config, DataFlow::SecondaryConfig SC> implements
+    DataFlow::GlobalFlowSig
+  {
+    private module Config0 implements DataFlowInternal::FullStateConfigSig {
+      import DataFlowInternal::DefaultState<Config>
+      import Config
+
+      predicate isAdditionalFlowStep(
+        DataFlowLang::Node node1, DataFlowLang::Node node2, string model
+      ) {
+        Config::isAdditionalFlowStep(node1, node2) and model = "Config"
+      }
+    }
+
+    private module C implements DataFlowInternal::FullStateConfigSig {
+      import DataFlowInternal::MakeSinkFinder<AddTaintDefaults<Config0>, SC>
+    }
+
+    private module Stage1 = DataFlowInternalStage1::ImplStage1<C>;
+
+    import Stage1::PartialFlow
+
+    private module Flow = DataFlowInternal::Impl<C, Stage1::Stage1NoState>;
+
+    import Flow
+  }
+
   signature int speculationLimitSig();
 
   private module AddSpeculativeTaintSteps<
@@ -166,7 +221,7 @@
 
     class FlowState extends TFlowState {
       private Config::FlowState state;
       private int spec;
 
       FlowState() { this = TMkFlowState(state, spec) }
 

shared/dataflow/codeql/dataflow/internal/DataFlowImpl.qll

@@ -18,6 +18,9 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
   private import DataFlowImplCommon::MakeImplCommon<Location, Lang>
   private import DataFlowImplCommonPublic
   private import Aliases
+  private import codeql.util.AlertFiltering
+
+  private module AlertFiltering = AlertFilteringImpl<Location>;
 
   /**
    * An input configuration for data flow using flow state. This signature equals
@@ -169,6 +172,117 @@ module MakeImpl<LocationSig Location, InputSig<Location> Lang> {
     }
   }
 
+  /**
+   * This private type is used instead of `Unit` to ensure that users of this
+   * library don't take an accidental dependency on convertability to `Unit`,
+   * allowing us to change the implementation in the future.
+   */
+  private newtype TDiffInformedBase = MkDiffInformedBase()
+
+  abstract class DiffInformedQueryImpl extends TDiffInformedBase {
+    DiffInformedQueryImpl() { any() }
+
+    string toString() { result = "DataFlow::DiffInformedQuery" }
+
+    Location getASelectedSourceLocation(Node source) { result = source.getLocation() }
+
+    Location getASelectedSinkLocation(Node sink) { result = sink.getLocation() }
+
+    pragma[nomagic]
+    predicate hasSourceInDiffRange() {
+      AlertFiltering::filterByLocation(this.getASelectedSourceLocation(_))
+    }
+
+    pragma[nomagic]
+    predicate hasSinkInDiffRange() {
+      AlertFiltering::filterByLocation(this.getASelectedSinkLocation(_))
+    }
+  }
+
+  module MakePrimaryDiffInformed<FullStateConfigSig Config> implements FullStateConfigSig {
+    // Workaround for name clash
+    predicate accessPathLimit = Config::accessPathLimit/0;
+
+    import Config
+
+    predicate observeDiffInformedIncrementalMode() {
+      // Add to existing configuration to support composition of config transformers
+      Config::observeDiffInformedIncrementalMode()
+      or
+      exists(DiffInformedQueryImpl q)
+    }
+
+    Location getASelectedSourceLocation(Node source) {
+      result = Config::getASelectedSourceLocation(source)
+      or
+      exists(DiffInformedQueryImpl q | result = q.getASelectedSourceLocation(source))
+    }
+
+    Location getASelectedSinkLocation(Node sink) {
+      result = Config::getASelectedSinkLocation(sink)
+      or
+      exists(DiffInformedQueryImpl q | result = q.getASelectedSinkLocation(sink))
+    }
+  }
+
+  module SecondaryConfigHelpers {
+    newtype IsSourceOrSink =
+      IsSource() or
+      IsSink()
+
+    signature module SecondaryConfig {
+      /**
+       * Gets the source/sink node from the primary configuration that is
+       * informed by a given source/sink node from the secondary configuration.
+       * Whether the secondary node is a source or a sink is determined by
+       * `sourceOrSink`.
+       */
+      bindingset[sourceOrSink, secondaryNode]
+      Node getPrimaryOfSecondaryNode(IsSourceOrSink sourceOrSink, Node secondaryNode);
+    }
+  }
+
+  module MakeSinkFinder<FullStateConfigSig Config, SecondaryConfig SC> implements FullStateConfigSig
+  {
+    // Workaround for name clash
+    predicate accessPathLimit = Config::accessPathLimit/0;
+
+    import Config
+
+    predicate observeDiffInformedIncrementalMode() {
+      // Add to existing configuration to support composition of config transformers
+      Config::observeDiffInformedIncrementalMode()
+      or
+      // Because this configuration is for finding sinks to be used in a main
+      // data-flow configuration, this configuration should only restrict the
+      // sinks to be found if there are no main-configuration sources in the
+      // diff range. That's because if there is such a source, we need to
+      // report query results for it even with sinks outside the diff range.
+      //
+      // The `MakeSinkFinder` and `MakeSourceFinder` modules are separate
+      // because each can only call one of `hasSourceInDiffRange` or
+      // `hasSinkInDiffRange`. Otherwise it would look like a non-monotonic
+      // recursion.
+      exists(DiffInformedQuery q | not q.hasSourceInDiffRange())
+    }
+
+    Location getASelectedSourceLocation(Node source) {
+      result = Config::getASelectedSourceLocation(source)
+      or
+      exists(DiffInformedQueryImpl q, IsSource s |
+        result = q.getASelectedSourceLocation(SC::getPrimaryOfSecondaryNode(s, source))
+      )
+    }
+
+    Location getASelectedSinkLocation(Node sink) {
+      result = Config::getASelectedSinkLocation(sink)
+      or
+      exists(DiffInformedQueryImpl q, IsSink s |
+        result = q.getASelectedSinkLocation(SC::getPrimaryOfSecondaryNode(s, sink))
+      )
+    }
+  }
+
   /**
    * Constructs a data flow computation given a full input configuration, and
    * an initial stage 1 pruning.