Merge pull request #10883 from erik-krogh/codeSink

RB: don't flag code-injection for dynamic loading where an attacker only controls a substring

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

@@ -701,6 +701,9 @@ module SystemCommandExecution {
 class CodeExecution extends DataFlow::Node instanceof CodeExecution::Range {
   /** Gets the argument that specifies the code to be executed. */
   DataFlow::Node getCode() { result = super.getCode() }
+
+  /** Holds if this execution runs arbitrary code, as opposed to some restricted subset. E.g. `Object.send` will only run any method on an object. */
+  predicate runsArbitraryCode() { super.runsArbitraryCode() }
 }
 
 /** Provides a class for modeling new dynamic code execution APIs. */
@@ -714,6 +717,9 @@ module CodeExecution {
   abstract class Range extends DataFlow::Node {
     /** Gets the argument that specifies the code to be executed. */
     abstract DataFlow::Node getCode();
+
+    /** Holds if this execution runs arbitrary code, as opposed to some restricted subset. E.g. `Object.send` will only run any method on an object. */
+    predicate runsArbitraryCode() { any() }
   }
 }
 

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

@@ -25,6 +25,8 @@ module ActiveJob {
       }
 
       override DataFlow::Node getCode() { result = this.getArgument(0) }
+
+      override predicate runsArbitraryCode() { none() }
     }
   }
 }

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

@@ -221,5 +221,7 @@ module ActiveStorage {
     }
 
     override DataFlow::Node getCode() { result = this.getArgument(0) }
+
+    override predicate runsArbitraryCode() { none() }
   }
 }

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

@@ -35,6 +35,8 @@ module ActiveSupport {
         }
 
         override DataFlow::Node getCode() { result = this.getReceiver() }
+
+        override predicate runsArbitraryCode() { none() }
       }
 
       /**

ruby/ql/lib/codeql/ruby/frameworks/core/Kernel.qll

@@ -166,6 +166,8 @@ module Kernel {
     SendCallCodeExecution() { this.getMethodName() = "send" }
 
     override DataFlow::Node getCode() { result = this.getArgument(0) }
+
+    override predicate runsArbitraryCode() { none() }
   }
 
   private class TapSummary extends SimpleSummarizedCallable {

ruby/ql/lib/codeql/ruby/frameworks/core/Module.qll

@@ -42,5 +42,7 @@ module Module {
     }
 
     override DataFlow::Node getCode() { result = this.getArgument(0) }
+
+    override predicate runsArbitraryCode() { none() }
   }
 }

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

@@ -11,20 +11,41 @@ private import codeql.ruby.dataflow.BarrierGuards
  * adding your own.
  */
 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 data that is entirely controlled by the attacker. */
+    DataFlow::FlowState full() { result = "full" }
+  }
+
   /**
    * A data flow source for "Code injection" vulnerabilities.
    */
-  abstract class Source extends DataFlow::Node { }
+  abstract class Source extends DataFlow::Node {
+    /** Gets a flow state for which this is a source. */
+    DataFlow::FlowState getAFlowState() { result = [FlowState::substring(), FlowState::full()] }
+  }
 
   /**
    * A data flow sink for "Code injection" vulnerabilities.
    */
-  abstract class Sink extends DataFlow::Node { }
+  abstract class Sink extends DataFlow::Node {
+    /** Holds if this sink is safe for an attacker that only controls a substring. */
+    DataFlow::FlowState getAFlowState() { result = [FlowState::substring(), FlowState::full()] }
+  }
 
   /**
    * A sanitizer for "Code injection" vulnerabilities.
    */
-  abstract class Sanitizer extends DataFlow::Node { }
+  abstract class Sanitizer extends DataFlow::Node {
+    /**
+     * Gets a flow state for which this is a sanitizer.
+     * Sanitizes all states if the result is empty.
+     */
+    DataFlow::FlowState getAFlowState() { none() }
+  }
 
   /**
    * DEPRECATED: Use `Sanitizer` instead.
@@ -42,6 +63,35 @@ module CodeInjection {
    * A call that evaluates its arguments as Ruby code, considered as a flow sink.
    */
   class CodeExecutionAsSink extends Sink {
-    CodeExecutionAsSink() { this = any(CodeExecution c).getCode() }
+    CodeExecution c;
+
+    CodeExecutionAsSink() { this = c.getCode() }
+
+    /** Gets a flow state for which this is a sink. */
+    override DataFlow::FlowState getAFlowState() {
+      if c.runsArbitraryCode()
+      then result = [FlowState::substring(), FlowState::full()] // If it runs immediately, 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.
+    }
+  }
+
+  private import codeql.ruby.AST as Ast
+
+  /**
+   * A string-concatenation that sanitizes the `full()` state.
+   */
+  class StringConcatenationSanitizer extends Sanitizer {
+    StringConcatenationSanitizer() {
+      // string concatenations sanitize the `full` state, as an attacker no longer controls the entire string
+      exists(Ast::AstNode str |
+        str instanceof Ast::StringLiteral
+        or
+        str instanceof Ast::AddExpr
+      |
+        this.asExpr().getExpr() = str
+      )
+    }
+
+    override DataFlow::FlowState getAFlowState() { result = FlowState::full() }
   }
 }

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

@@ -16,16 +16,26 @@ import codeql.ruby.dataflow.BarrierGuards
 class Configuration extends TaintTracking::Configuration {
   Configuration() { this = "CodeInjection" }
 
-  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+  override predicate isSource(DataFlow::Node source, DataFlow::FlowState state) {
+    state = source.(Source).getAFlowState()
+  }
 
-  override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+  override predicate isSink(DataFlow::Node sink, DataFlow::FlowState state) {
+    state = sink.(Sink).getAFlowState()
+  }
 
   override predicate isSanitizer(DataFlow::Node node) {
-    node instanceof Sanitizer or
-    node instanceof StringConstCompareBarrier or
+    node instanceof Sanitizer and not exists(node.(Sanitizer).getAFlowState())
+    or
+    node instanceof StringConstCompareBarrier
+    or
     node instanceof StringConstArrayInclusionCallBarrier
   }
 
+  override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) {
+    node.(Sanitizer).getAFlowState() = state
+  }
+
   deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof SanitizerGuard
   }