Merge pull request #14124 from alexrford/rb/dataflow-query-refactor

Ruby: Use the new dataflow API for checked in queries

Author: alexrford

ruby/ql/lib/codeql/ruby/experimental/UnicodeBypassValidationQuery.qll

@@ -19,22 +19,64 @@ class PostValidation extends DataFlow::FlowState {
   PostValidation() { this = "PostValidation" }
 }
 
+/**
+ * A state signifying if a logical validation has been performed or not.
+ */
+private newtype ValidationState =
+  // A state signifying that a logical validation has not been performed.
+  PreValidationState() or
+  // A state signifying that a logical validation has been performed.
+  PostValidationState()
+
 /**
  * A taint-tracking configuration for detecting "Unicode transformation mishandling" vulnerabilities.
  *
  * This configuration uses two flow states, `PreValidation` and `PostValidation`,
  * to track the requirement that a logical validation has been performed before the Unicode Transformation.
+ * DEPRECATED: Use `UnicodeBypassValidationFlow`
  */
-class Configuration extends TaintTracking::Configuration {
+deprecated class Configuration extends TaintTracking::Configuration {
   Configuration() { this = "UnicodeBypassValidation" }
 
+  private ValidationState convertState(DataFlow::FlowState state) {
+    state instanceof PreValidation and result = PreValidationState()
+    or
+    state instanceof PostValidation and result = PostValidationState()
+  }
+
   override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
-    source instanceof RemoteFlowSource and state instanceof PreValidation
+    UnicodeBypassValidationConfig::isSource(source, this.convertState(state))
   }
 
   override predicate isAdditionalTaintStep(
     DataFlow::Node nodeFrom, DataFlow::FlowState stateFrom, DataFlow::Node nodeTo,
     DataFlow::FlowState stateTo
+  ) {
+    UnicodeBypassValidationConfig::isAdditionalFlowStep(nodeFrom, this.convertState(stateFrom),
+      nodeTo, this.convertState(stateTo))
+  }
+
+  /* A Unicode Tranformation (Unicode tranformation) is considered a sink when the algorithm used is either NFC or NFKC.  */
+  override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+    UnicodeBypassValidationConfig::isSink(sink, this.convertState(state))
+  }
+}
+
+/**
+ * A taint-tracking configuration for detecting "Unicode transformation mishandling" vulnerabilities.
+ *
+ * This configuration uses two flow states, `PreValidation` and `PostValidation`,
+ * to track the requirement that a logical validation has been performed before the Unicode Transformation.
+ */
+private module UnicodeBypassValidationConfig implements DataFlow::StateConfigSig {
+  class FlowState = ValidationState;
+
+  predicate isSource(DataFlow::Node source, FlowState state) {
+    source instanceof RemoteFlowSource and state = PreValidationState()
+  }
+
+  predicate isAdditionalFlowStep(
+    DataFlow::Node nodeFrom, FlowState stateFrom, DataFlow::Node nodeTo, FlowState stateTo
   ) {
     (
       exists(Escaping escaping | nodeFrom = escaping.getAnInput() and nodeTo = escaping.getOutput())
@@ -75,12 +117,12 @@ class Configuration extends TaintTracking::Configuration {
         nodeTo = cn
       )
     ) and
-    stateFrom instanceof PreValidation and
-    stateTo instanceof PostValidation
+    stateFrom = PreValidationState() and
+    stateTo = PostValidationState()
   }
 
   /* A Unicode Tranformation (Unicode tranformation) is considered a sink when the algorithm used is either NFC or NFKC.  */
-  override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+  predicate isSink(DataFlow::Node sink, FlowState state) {
     (
       exists(DataFlow::CallNode cn |
         cn.getMethodName() = "unicode_normalize" and
@@ -118,6 +160,11 @@ class Configuration extends TaintTracking::Configuration {
         sink = cn.getArgument(0)
       )
     ) and
-    state instanceof PostValidation
+    state = PostValidationState()
   }
 }
+
+/**
+ * Taint-tracking configuration for detecting "Unicode transformation mishandling" vulnerabilities.
+ */
+module UnicodeBypassValidationFlow = TaintTracking::GlobalWithState<UnicodeBypassValidationConfig>;

ruby/ql/lib/codeql/ruby/experimental/ZipSlipQuery.qll

@@ -12,8 +12,9 @@ private import codeql.ruby.ApiGraphs
 /**
  * A taint-tracking configuration for reasoning about zip slip
  * vulnerabilities.
+ * DEPRECATED: Use `ZipSlipFlow`
  */
-class Configuration extends TaintTracking::Configuration {
+deprecated class Configuration extends TaintTracking::Configuration {
   Configuration() { this = "ZipSlip" }
 
   override predicate isSource(DataFlow::Node source) { source instanceof ZipSlip::Source }
@@ -36,3 +37,30 @@ class Configuration extends TaintTracking::Configuration {
 
   override predicate isSanitizer(DataFlow::Node node) { node instanceof ZipSlip::Sanitizer }
 }
+
+private module ZipSlipConfig implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof ZipSlip::Source }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof ZipSlip::Sink }
+
+  /**
+   * This should actually be
+   * `and cn = API::getTopLevelMember("Gem").getMember("Package").getMember("TarReader").getMember("Entry").getAMethodCall("full_name")` and similar for other classes
+   * but I couldn't make it work so there's only check for the method name called on the entry. It is `full_name` for `Gem::Package::TarReader::Entry` and `Zlib`
+   * and `name` for `Zip::File`
+   */
+  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    exists(DataFlow::CallNode cn |
+      cn.getReceiver() = nodeFrom and
+      cn.getMethodName() in ["full_name", "name"] and
+      cn = nodeTo
+    )
+  }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof ZipSlip::Sanitizer }
+}
+
+/**
+ * Taint-tracking for reasoning about zip slip vulnerabilities.
+ */
+module ZipSlipFlow = TaintTracking::Global<ZipSlipConfig>;

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

@@ -2,33 +2,51 @@
  * Provides a taint-tracking configuration for "Clear-text logging of sensitive information".
  *
  * Note, for performance reasons: only import this file if
- * `CleartextLogging::Configuration` is needed, otherwise
+ * `CleartextLoggingFlow` is needed, otherwise
  * `CleartextLoggingCustomizations` should be imported instead.
  */
 
 private import codeql.ruby.AST
 private import codeql.ruby.DataFlow
 private import codeql.ruby.TaintTracking
 import CleartextLoggingCustomizations::CleartextLogging
-private import CleartextLoggingCustomizations::CleartextLogging as CleartextLogging
+private import CleartextLoggingCustomizations::CleartextLogging as CL
 
 /**
  * A taint-tracking configuration for detecting "Clear-text logging of sensitive information".
+ * DEPRECATED: Use `CleartextLoggingFlow` instead
  */
-class Configuration extends TaintTracking::Configuration {
+deprecated class Configuration extends TaintTracking::Configuration {
   Configuration() { this = "CleartextLogging" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof CleartextLogging::Source }
+  override predicate isSource(DataFlow::Node source) { source instanceof CL::Source }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof CleartextLogging::Sink }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof CL::Sink }
 
   override predicate isSanitizer(DataFlow::Node node) {
     super.isSanitizer(node)
     or
-    node instanceof CleartextLogging::Sanitizer
+    node instanceof CL::Sanitizer
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-    CleartextLogging::isAdditionalTaintStep(nodeFrom, nodeTo)
+    CL::isAdditionalTaintStep(nodeFrom, nodeTo)
   }
 }
+
+private module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof CL::Source }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof CL::Sink }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof CL::Sanitizer }
+
+  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    CL::isAdditionalTaintStep(nodeFrom, nodeTo)
+  }
+}
+
+/**
+ * Taint-tracking for detecting "Clear-text logging of sensitive information".
+ */
+module CleartextLoggingFlow = TaintTracking::Global<Config>;

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

@@ -2,32 +2,50 @@
  * Provides a taint-tracking configuration for "Clear-text storage of sensitive information".
  *
  * Note, for performance reasons: only import this file if
- * `Configuration` is needed, otherwise `CleartextStorageCustomizations` should be
- * imported instead.
+ * `CleartextStorageFlow` is needed, otherwise
+ * `CleartextStorageCustomizations` should be imported instead.
  */
 
 private import codeql.ruby.AST
 private import codeql.ruby.DataFlow
 private import codeql.ruby.TaintTracking
-private import CleartextStorageCustomizations::CleartextStorage as CleartextStorage
+private import CleartextStorageCustomizations::CleartextStorage as CS
 
 /**
  * A taint-tracking configuration for detecting "Clear-text storage of sensitive information".
+ * DEPRECATED: Use `CleartextStorageFlow` instead
  */
-class Configuration extends TaintTracking::Configuration {
+deprecated class Configuration extends TaintTracking::Configuration {
   Configuration() { this = "CleartextStorage" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof CleartextStorage::Source }
+  override predicate isSource(DataFlow::Node source) { source instanceof CS::Source }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof CleartextStorage::Sink }
+  override predicate isSink(DataFlow::Node sink) { sink instanceof CS::Sink }
 
   override predicate isSanitizer(DataFlow::Node node) {
     super.isSanitizer(node)
     or
-    node instanceof CleartextStorage::Sanitizer
+    node instanceof CS::Sanitizer
   }
 
   override predicate isAdditionalTaintStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
-    CleartextStorage::isAdditionalTaintStep(nodeFrom, nodeTo)
+    CS::isAdditionalTaintStep(nodeFrom, nodeTo)
   }
 }
+
+private module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { source instanceof CS::Source }
+
+  predicate isSink(DataFlow::Node sink) { sink instanceof CS::Sink }
+
+  predicate isBarrier(DataFlow::Node node) { node instanceof CS::Sanitizer }
+
+  predicate isAdditionalFlowStep(DataFlow::Node nodeFrom, DataFlow::Node nodeTo) {
+    CS::isAdditionalTaintStep(nodeFrom, nodeTo)
+  }
+}
+
+/**
+ * Taint-tracking for detecting "Clear-text storage of sensitive information".
+ */
+module CleartextStorageFlow = TaintTracking::Global<Config>;

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

@@ -13,38 +13,100 @@ private import codeql.ruby.dataflow.BarrierGuards
 module CodeInjection {
   /** Flow states used to distinguish whether an attacker controls the entire string. */
   module FlowState {
-    /** Flow state used for normal tainted data, where an attacker might only control a substring. */
-    DataFlow::FlowState substring() { result = "substring" }
+    /**
+     * Flow state used for normal tainted data, where an attacker might only control a substring.
+     * DEPRECATED: Use `Full()`
+     */
+    deprecated DataFlow::FlowState substring() { result = "substring" }
+
+    /**
+     * Flow state used for data that is entirely controlled by the attacker.
+     * DEPRECATED: Use `Full()`
+     */
+    deprecated DataFlow::FlowState full() { result = "full" }
 
-    /** Flow state used for data that is entirely controlled by the attacker. */
-    DataFlow::FlowState full() { result = "full" }
+    private newtype TState =
+      TFull() or
+      TSubString()
+
+    /** A flow state used to distinguish whether an attacker controls the entire string. */
+    class State extends TState {
+      /**
+       * Gets a string representation of this state.
+       */
+      string toString() { result = this.getStringRepresentation() }
+
+      /**
+       * Gets a canonical string representation of this state.
+       */
+      string getStringRepresentation() {
+        this = TSubString() and result = "substring"
+        or
+        this = TFull() and result = "full"
+      }
+    }
+
+    /**
+     * A flow state used for normal tainted data, where an attacker might only control a substring.
+     */
+    class SubString extends State, TSubString { }
+
+    /**
+     * A flow state used for data that is entirely controlled by the attacker.
+     */
+    class Full extends State, TFull { }
   }
 
   /**
    * A data flow source for "Code injection" vulnerabilities.
    */
   abstract class Source extends DataFlow::Node {
+    /**
+     * Gets a flow state for which this is a source.
+     * DEPRECATED: Use `getAState()`
+     */
+    deprecated DataFlow::FlowState getAFlowState() {
+      result = [FlowState::substring(), FlowState::full()]
+    }
+
     /** Gets a flow state for which this is a source. */
-    DataFlow::FlowState getAFlowState() { result = [FlowState::substring(), FlowState::full()] }
+    FlowState::State getAState() {
+      result instanceof FlowState::SubString or result instanceof FlowState::Full
+    }
   }
 
   /**
    * A data flow sink for "Code injection" vulnerabilities.
    */
   abstract class Sink extends DataFlow::Node {
+    /**
+     * Holds if this sink is safe for an attacker that only controls a substring.
+     * DEPRECATED: Use `getAState()`
+     */
+    deprecated DataFlow::FlowState getAFlowState() {
+      result = [FlowState::substring(), FlowState::full()]
+    }
+
     /** Holds if this sink is safe for an attacker that only controls a substring. */
-    DataFlow::FlowState getAFlowState() { result = [FlowState::substring(), FlowState::full()] }
+    FlowState::State getAState() { any() }
   }
 
   /**
    * A sanitizer for "Code injection" vulnerabilities.
    */
   abstract class Sanitizer extends DataFlow::Node {
+    /**
+     * Gets a flow state for which this is a sanitizer.
+     * Sanitizes all states if the result is empty.
+     * DEPRECATED: Use `getAState()`
+     */
+    deprecated DataFlow::FlowState getAFlowState() { none() }
+
     /**
      * Gets a flow state for which this is a sanitizer.
      * Sanitizes all states if the result is empty.
      */
-    DataFlow::FlowState getAFlowState() { none() }
+    FlowState::State getAState() { none() }
   }
 
   /**
@@ -67,12 +129,17 @@ module CodeInjection {
 
     CodeExecutionAsSink() { this = c.getCode() }
 
-    /** Gets a flow state for which this is a sink. */
-    override DataFlow::FlowState getAFlowState() {
+    deprecated override DataFlow::FlowState getAFlowState() {
       if c.runsArbitraryCode()
       then result = [FlowState::substring(), FlowState::full()] // If it runs arbitrary code then it's always vulnerable.
       else result = FlowState::full() // If it "just" loads something, then it's only vulnerable if the attacker controls the entire string.
     }
+
+    override FlowState::State getAState() {
+      if c.runsArbitraryCode()
+      then any() // If it runs arbitrary code then it's always vulnerable.
+      else result instanceof FlowState::Full // If it "just" loads something, then it's only vulnerable if the attacker controls the entire string.
+    }
   }
 
   private import codeql.ruby.AST as Ast
@@ -92,6 +159,8 @@ module CodeInjection {
       )
     }
 
-    override DataFlow::FlowState getAFlowState() { result = FlowState::full() }
+    deprecated override DataFlow::FlowState getAFlowState() { result = FlowState::full() }
+
+    override FlowState::State getAState() { result instanceof FlowState::Full }
   }
 }