Merge branch 'main' into maikypedia/ldap-injection

ruby/ql/lib/CHANGELOG.md

@@ -1,3 +1,28 @@
+## 0.7.1
+
+### New Features
+
+* The `DataFlow::StateConfigSig` signature module has gained default implementations for `isBarrier/2` and `isAdditionalFlowStep/4`. 
+  Hence it is no longer needed to provide `none()` implementations of these predicates if they are not needed.
+
+### Major Analysis Improvements
+
+* The API graph library (`codeql.ruby.ApiGraphs`) has been significantly improved, with better support for inheritance,
+  and data-flow nodes can now be converted to API nodes by calling `.track()` or `.backtrack()` on the node.
+  API graphs allow for efficient modelling of how a given value is used by the code base, or how values produced by the code base
+  are consumed by a library. See the documentation for `API::Node` for details and examples.
+
+### Minor Analysis Improvements
+
+* Data flow configurations can now include a predicate `neverSkip(Node node)`
+  in order to ensure inclusion of certain nodes in the path explanations. The
+  predicate defaults to the end-points of the additional flow steps provided in
+  the configuration, which means that such steps now always are visible by
+  default in path explanations.
+* The `'QUERY_STRING'` field of a Rack `env` parameter is now recognized as a source of remote user input.
+* Query parameters and cookies from `Rack::Response` objects are recognized as potential sources of remote flow input.
+* Calls to `Rack::Utils.parse_query` now propagate taint.
+
 ## 0.7.0
 
 ### Deprecated APIs

ruby/ql/lib/change-notes/2023-06-28-tracking-on-demand.md

@@ -1,7 +0,0 @@
----
-category: majorAnalysis
----
-* The API graph library (`codeql.ruby.ApiGraphs`) has been significantly improved, with better support for inheritance,
-  and data-flow nodes can now be converted to API nodes by calling `.track()` or `.backtrack()` on the node.
-  API graphs allow for efficient modelling of how a given value is used by the code base, or how values produced by the code base
-  are consumed by a library. See the documentation for `API::Node` for details and examples.

ruby/ql/lib/change-notes/2023-07-12-default-stateconfigsig-predicates.md

@@ -1,6 +0,0 @@
----
-category: feature
----
-* The `DataFlow::StateConfigSig` signature module has gained default implementations for `isBarrier/2` and `isAdditionalFlowStep/4`. 
-  Hence it is no longer needed to provide `none()` implementations of these predicates if they are not needed.
-  

ruby/ql/lib/change-notes/released/0.7.1.md

@@ -0,0 +1,24 @@
+## 0.7.1
+
+### New Features
+
+* The `DataFlow::StateConfigSig` signature module has gained default implementations for `isBarrier/2` and `isAdditionalFlowStep/4`. 
+  Hence it is no longer needed to provide `none()` implementations of these predicates if they are not needed.
+
+### Major Analysis Improvements
+
+* The API graph library (`codeql.ruby.ApiGraphs`) has been significantly improved, with better support for inheritance,
+  and data-flow nodes can now be converted to API nodes by calling `.track()` or `.backtrack()` on the node.
+  API graphs allow for efficient modelling of how a given value is used by the code base, or how values produced by the code base
+  are consumed by a library. See the documentation for `API::Node` for details and examples.
+
+### Minor Analysis Improvements
+
+* Data flow configurations can now include a predicate `neverSkip(Node node)`
+  in order to ensure inclusion of certain nodes in the path explanations. The
+  predicate defaults to the end-points of the additional flow steps provided in
+  the configuration, which means that such steps now always are visible by
+  default in path explanations.
+* The `'QUERY_STRING'` field of a Rack `env` parameter is now recognized as a source of remote user input.
+* Query parameters and cookies from `Rack::Response` objects are recognized as potential sources of remote flow input.
+* Calls to `Rack::Utils.parse_query` now propagate taint.

ruby/ql/lib/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.7.0
+lastReleaseVersion: 0.7.1

ruby/ql/lib/codeql/ruby/Concepts.qll

@@ -843,6 +843,58 @@ module XmlParserCall {
   }
 }
 
+/**
+ * A data-flow node that constructs an XPath expression.
+ *
+ * If it is important that the XPath expression is indeed executed, then use `XPathExecution`.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `XPathConstruction::Range` instead.
+ */
+class XPathConstruction extends DataFlow::Node instanceof XPathConstruction::Range {
+  /** Gets the argument that specifies the XPath expressions to be constructed. */
+  DataFlow::Node getXPath() { result = super.getXPath() }
+}
+
+/** Provides a class for modeling new XPath construction APIs. */
+module XPathConstruction {
+  /**
+   * A data-flow node that constructs an XPath expression.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `XPathConstruction` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the argument that specifies the XPath expressions to be constructed. */
+    abstract DataFlow::Node getXPath();
+  }
+}
+
+/**
+ * A data-flow node that executes an XPath expression.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `XPathExecution::Range` instead.
+ */
+class XPathExecution extends DataFlow::Node instanceof XPathExecution::Range {
+  /** Gets the argument that specifies the XPath expressions to be executed. */
+  DataFlow::Node getXPath() { result = super.getXPath() }
+}
+
+/** Provides a class for modeling new XPath execution APIs. */
+module XPathExecution {
+  /**
+   * A data-flow node that executes an XPath expression.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `XPathExecution` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the argument that specifies the XPath expressions to be executed. */
+    abstract DataFlow::Node getXPath();
+  }
+}
+
 /**
  * A data-flow node that may represent a database object in an ORM system.
  *

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlow.qll

@@ -46,6 +46,14 @@ signature module ConfigSig {
    */
   default predicate allowImplicitRead(Node node, ContentSet c) { none() }
 
+  /**
+   * Holds if `node` should never be skipped over in the `PathGraph` and in path
+   * explanations.
+   */
+  default predicate neverSkip(Node node) {
+    isAdditionalFlowStep(node, _) or isAdditionalFlowStep(_, node)
+  }
+
   /**
    * Gets the virtual dispatch branching limit when calculating field flow.
    * This can be overridden to a smaller value to improve performance (a
@@ -141,6 +149,17 @@ signature module StateConfigSig {
    */
   default predicate allowImplicitRead(Node node, ContentSet c) { none() }
 
+  /**
+   * Holds if `node` should never be skipped over in the `PathGraph` and in path
+   * explanations.
+   */
+  default predicate neverSkip(Node node) {
+    isAdditionalFlowStep(node, _) or
+    isAdditionalFlowStep(_, node) or
+    isAdditionalFlowStep(node, _, _, _) or
+    isAdditionalFlowStep(_, _, node, _)
+  }
+
   /**
    * Gets the virtual dispatch branching limit when calculating field flow.
    * This can be overridden to a smaller value to improve performance (a
@@ -410,5 +429,22 @@ module MergePathGraph3<
   /**
    * Provides the query predicates needed to include a graph in a path-problem query.
    */
-  module PathGraph = Merged::PathGraph;
+  module PathGraph implements PathGraphSig<PathNode> {
+    /** Holds if `(a,b)` is an edge in the graph of data flow path explanations. */
+    query predicate edges(PathNode a, PathNode b) { Merged::PathGraph::edges(a, b) }
+
+    /** Holds if `n` is a node in the graph of data flow path explanations. */
+    query predicate nodes(PathNode n, string key, string val) {
+      Merged::PathGraph::nodes(n, key, val)
+    }
+
+    /**
+     * Holds if `(arg, par, ret, out)` forms a subpath-tuple, that is, flow through
+     * a subpath between `par` and `ret` with the connecting edges `arg -> par` and
+     * `ret -> out` is summarized as the edge `arg -> out`.
+     */
+    query predicate subpaths(PathNode arg, PathNode par, PathNode ret, PathNode out) {
+      Merged::PathGraph::subpaths(arg, par, ret, out)
+    }
+  }
 }

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowImpl.qll

@@ -66,6 +66,12 @@ signature module FullStateConfigSig {
    */
   predicate allowImplicitRead(Node node, ContentSet c);
 
+  /**
+   * Holds if `node` should never be skipped over in the `PathGraph` and in path
+   * explanations.
+   */
+  predicate neverSkip(Node node);
+
   /**
    * Gets the virtual dispatch branching limit when calculating field flow.
    * This can be overridden to a smaller value to improve performance (a
@@ -2024,7 +2030,8 @@ module Impl<FullStateConfigSig Config> {
         castNode(this.asNode()) or
         clearsContentCached(this.asNode(), _) or
         expectsContentCached(this.asNode(), _) or
-        neverSkipInPathGraph(this.asNode())
+        neverSkipInPathGraph(this.asNode()) or
+        Config::neverSkip(this.asNode())
       }
     }
 

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowImpl1.qll

@@ -313,6 +313,8 @@ private module Config implements FullStateConfigSig {
     any(Configuration config).allowImplicitRead(node, c)
   }
 
+  predicate neverSkip(Node node) { none() }
+
   int fieldFlowBranchLimit() { result = min(any(Configuration config).fieldFlowBranchLimit()) }
 
   FlowFeature getAFeature() { result = any(Configuration config).getAFeature() }

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowImpl2.qll

@@ -313,6 +313,8 @@ private module Config implements FullStateConfigSig {
     any(Configuration config).allowImplicitRead(node, c)
   }
 
+  predicate neverSkip(Node node) { none() }
+
   int fieldFlowBranchLimit() { result = min(any(Configuration config).fieldFlowBranchLimit()) }
 
   FlowFeature getAFeature() { result = any(Configuration config).getAFeature() }

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowImplForHttpClientLibraries.qll

@@ -313,6 +313,8 @@ private module Config implements FullStateConfigSig {
     any(Configuration config).allowImplicitRead(node, c)
   }
 
+  predicate neverSkip(Node node) { none() }
+
   int fieldFlowBranchLimit() { result = min(any(Configuration config).fieldFlowBranchLimit()) }
 
   FlowFeature getAFeature() { result = any(Configuration config).getAFeature() }

ruby/ql/lib/codeql/ruby/dataflow/internal/DataFlowImplForPathname.qll

@@ -313,6 +313,8 @@ private module Config implements FullStateConfigSig {
     any(Configuration config).allowImplicitRead(node, c)
   }
 
+  predicate neverSkip(Node node) { none() }
+
   int fieldFlowBranchLimit() { result = min(any(Configuration config).fieldFlowBranchLimit()) }
 
   FlowFeature getAFeature() { result = any(Configuration config).getAFeature() }

ruby/ql/lib/codeql/ruby/frameworks/Rack.qll

@@ -7,7 +7,9 @@
  */
 module Rack {
   import rack.internal.App
+  import rack.internal.Request
   import rack.internal.Response::Public as Response
+  import rack.internal.Utils
 
   /** DEPRECATED: Alias for App::AppCandidate */
   deprecated class AppCandidate = App::AppCandidate;

ruby/ql/lib/codeql/ruby/frameworks/XmlParsing.qll

@@ -45,6 +45,17 @@ private class NokogiriXmlParserCall extends XmlParserCall::Range, DataFlow::Call
   }
 }
 
+/** Execution of a XPath statement. */
+private class NokogiriXPathExecution extends XPathExecution::Range, DataFlow::CallNode {
+  NokogiriXPathExecution() {
+    exists(NokogiriXmlParserCall parserCall |
+      this = parserCall.getAMethodCall(["xpath", "at_xpath", "search", "at"])
+    )
+  }
+
+  override DataFlow::Node getXPath() { result = this.getArgument(0) }
+}
+
 /**
  * Holds if `assign` enables the `default_substitute_entities` option in
  * libxml-ruby.
@@ -123,6 +134,40 @@ private predicate xmlMiniEntitySubstitutionEnabled() {
   enablesLibXmlDefaultEntitySubstitution(_)
 }
 
+/** Execution of a XPath statement. */
+private class LibXmlXPathExecution extends XPathExecution::Range, DataFlow::CallNode {
+  LibXmlXPathExecution() {
+    exists(LibXmlRubyXmlParserCall parserCall |
+      this = parserCall.getAMethodCall(["find", "find_first"])
+    )
+  }
+
+  override DataFlow::Node getXPath() { result = this.getArgument(0) }
+}
+
+/** A call to `REXML::Document.new`, considered as a XML parsing. */
+private class RexmlParserCall extends XmlParserCall::Range, DataFlow::CallNode {
+  RexmlParserCall() {
+    this = API::getTopLevelMember("REXML").getMember("Document").getAnInstantiation()
+  }
+
+  override DataFlow::Node getInput() { result = this.getArgument(0) }
+
+  /** No option for parsing */
+  override predicate externalEntitiesEnabled() { none() }
+}
+
+/** Execution of a XPath statement. */
+private class RexmlXPathExecution extends XPathExecution::Range, DataFlow::CallNode {
+  RexmlXPathExecution() {
+    this =
+      [API::getTopLevelMember("REXML").getMember("XPath"), API::getTopLevelMember("XPath")]
+          .getAMethodCall(["each", "first", "match"])
+  }
+
+  override DataFlow::Node getXPath() { result = this.getArgument(1) }
+}
+
 /**
  * A call to `ActiveSupport::XmlMini.parse` considered as an `XmlParserCall`.
  */

ruby/ql/lib/codeql/ruby/frameworks/rack/internal/App.qll

@@ -4,6 +4,7 @@
 
 private import codeql.ruby.AST
 private import codeql.ruby.ApiGraphs
+private import codeql.ruby.Concepts
 private import codeql.ruby.DataFlow
 private import codeql.ruby.typetracking.TypeTracker
 private import Response::Private as RP
@@ -18,16 +19,7 @@ private class PotentialRequestHandler extends DataFlow::CallableNode {
     (
       this.(DataFlow::MethodNode).getMethodName() = "call"
       or
-      not this instanceof DataFlow::MethodNode and
-      exists(DataFlow::CallNode cn | cn.getMethodName() = "run" |
-        this.(DataFlow::LocalSourceNode).flowsTo(cn.getArgument(0))
-        or
-        // TODO: `Proc.new` should automatically propagate flow from its block argument
-        any(DataFlow::CallNode proc |
-          proc = API::getTopLevelMember("Proc").getAnInstantiation() and
-          proc.getBlock() = this
-        ).(DataFlow::LocalSourceNode).flowsTo(cn.getArgument(0))
-      )
+      this = API::getTopLevelCall("run").getArgument(0).asCallable()
     )
   }
 }
@@ -86,4 +78,20 @@ module App {
     /** Gets a response returned from this request handler. */
     RP::PotentialResponseNode getAResponse() { result = resp }
   }
+
+  /** A read of the query string via `env['QUERY_STRING']`. */
+  private class EnvQueryStringRead extends Http::Server::RequestInputAccess::Range {
+    EnvQueryStringRead() {
+      this =
+        any(RequestHandler h)
+            .getEnv()
+            .getAnElementRead(ConstantValue::fromStringlikeValue("QUERY_STRING"))
+    }
+
+    override string getSourceType() { result = "Rack env" }
+
+    override Http::Server::RequestInputKind getKind() {
+      result = Http::Server::parameterInputKind()
+    }
+  }
 }

ruby/ql/lib/codeql/ruby/frameworks/rack/internal/Request.qll

@@ -0,0 +1,39 @@
+/**
+ * Provides modeling for the `Request` component of the `Rack` library.
+ */
+
+private import codeql.ruby.AST
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+
+/**
+ * Provides modeling for the `Request` component of the `Rack` library.
+ */
+module Request {
+  private class RackRequest extends API::Node {
+    RackRequest() { this = API::getTopLevelMember("Rack").getMember("Request").getInstance() }
+  }
+
+  /** An access to the parameters of a request to a rack application via a `Rack::Request` instance. */
+  private class RackRequestParamsAccess extends Http::Server::RequestInputAccess::Range {
+    RackRequestParamsAccess() {
+      this = any(RackRequest req).getAMethodCall(["params", "query_string", "[]", "fullpath"])
+    }
+
+    override string getSourceType() { result = "Rack::Request#params" }
+
+    override Http::Server::RequestInputKind getKind() {
+      result = Http::Server::parameterInputKind()
+    }
+  }
+
+  /** An access to the cookies of a request to a rack application via a `Rack::Request` instance. */
+  private class RackRequestCookiesAccess extends Http::Server::RequestInputAccess::Range {
+    RackRequestCookiesAccess() { this = any(RackRequest req).getAMethodCall("cookies") }
+
+    override string getSourceType() { result = "Rack::Request#cookies" }
+
+    override Http::Server::RequestInputKind getKind() { result = Http::Server::cookieInputKind() }
+  }
+}

ruby/ql/lib/codeql/ruby/frameworks/rack/internal/Utils.qll

@@ -0,0 +1,29 @@
+/**
+ * Provides modeling for the `Utils` component of the `Rack` library.
+ */
+
+private import codeql.ruby.ApiGraphs
+private import codeql.ruby.dataflow.FlowSummary
+
+/**
+ * Provides modeling for the `Utils` component of the `Rack` library.
+ */
+module Utils {
+  /** Flow summary for `Rack::Utils.parse_query`, which parses a query string. */
+  private class ParseQuerySummary extends SummarizedCallable {
+    ParseQuerySummary() { this = "Rack::Utils.parse_query" }
+
+    override MethodCall getACall() {
+      result =
+        API::getTopLevelMember("Rack")
+            .getMember("Utils")
+            .getAMethodCall("parse_query")
+            .asExpr()
+            .getExpr()
+    }
+
+    override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+      input = "Argument[0]" and output = "ReturnValue" and preservesValue = false
+    }
+  }
+}

ruby/ql/lib/codeql/ruby/security/XpathInjectionCustomizations.qll

@@ -0,0 +1,55 @@
+/**
+ * Provides class and predicates to track external data that
+ * may represent malicious xpath query objects.
+ *
+ * This module is intended to be imported into a taint-tracking query.
+ */
+
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+private import codeql.ruby.dataflow.BarrierGuards
+private import codeql.ruby.dataflow.RemoteFlowSources
+
+/** Models Xpath Injection related classes and functions */
+module XpathInjection {
+  /** A data flow source for "XPath injection" vulnerabilities. */
+  abstract class Source extends DataFlow::Node { }
+
+  /** A data flow sink for "XPath injection" vulnerabilities */
+  abstract class Sink extends DataFlow::Node { }
+
+  /** A sanitizer for "XPath injection" vulnerabilities. */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A source of remote user input, considered as a flow source.
+   */
+  private class RemoteFlowSourceAsSource extends Source, RemoteFlowSource { }
+
+  /**
+   * An execution of an XPath expression, considered as a sink.
+   */
+  private class XPathExecutionAsSink extends Sink {
+    XPathExecutionAsSink() { this = any(XPathExecution e).getXPath() }
+  }
+
+  /**
+   * A construction of an XPath expression, considered as a sink.
+   */
+  private class XPathConstructionAsSink extends Sink {
+    XPathConstructionAsSink() { this = any(XPathConstruction c).getXPath() }
+  }
+
+  /**
+   * A comparison with a constant string, considered as a sanitizer-guard.
+   */
+  private class StringConstCompareAsSanitizerGuard extends Sanitizer, StringConstCompareBarrier { }
+
+  /**
+   * An inclusion check against an array of constant strings, considered as a
+   * sanitizer-guard.
+   */
+  private class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
+    StringConstArrayInclusionCallBarrier
+  { }
+}

ruby/ql/lib/codeql/ruby/security/XpathInjectionQuery.qll

@@ -0,0 +1,27 @@
+/**
+ * Provides a taint-tracking configuration for detecting "Xpath Injection" vulnerabilities.
+ *
+ * Note, for performance reasons: only import this file if
+ * `XpathInjection::Configuration` is needed, otherwise
+ * `XpathInjectionCustomizations` should be imported instead.
+ */
+
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+import XpathInjectionCustomizations::XpathInjection
+
+/** Provides a taint-tracking configuration for detecting "Xpath Injection" vulnerabilities. */
+module XpathInjection {
+  /**
+   * A taint-tracking configuration for detecting "Xpath Injection" vulnerabilities.
+   */
+  private module Config implements DataFlow::ConfigSig {
+    predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+
+    predicate isBarrier(DataFlow::Node node) { node instanceof Sanitizer }
+  }
+
+  import TaintTracking::Global<Config>
+}

ruby/ql/lib/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/ruby-all
-version: 0.7.1-dev
+version: 0.7.2-dev
 groups: ruby
 extractor: ruby
 dbscheme: ruby.dbscheme