use flow-states to remove FPs related to an attacker only controlling a substring in code-injection

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

@@ -11,15 +11,30 @@ 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.
@@ -42,6 +57,15 @@ 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.runsImmediately()
+      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.
+    }
   }
 }

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

@@ -9,6 +9,7 @@ import codeql.ruby.DataFlow
 import codeql.ruby.TaintTracking
 import CodeInjectionCustomizations::CodeInjection
 import codeql.ruby.dataflow.BarrierGuards
+private import codeql.ruby.AST as Ast
 
 /**
  * A taint-tracking configuration for detecting "Code injection" vulnerabilities.
@@ -16,9 +17,13 @@ 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
@@ -26,6 +31,18 @@ class Configuration extends TaintTracking::Configuration {
     node instanceof StringConstArrayInclusionCallBarrier
   }
 
+  override predicate isSanitizer(DataFlow::Node node, DataFlow::FlowState state) {
+    // 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
+    |
+      node.asExpr().getExpr() = str and
+      state = FlowState::full()
+    )
+  }
+
   deprecated override predicate isSanitizerGuard(DataFlow::BarrierGuard guard) {
     guard instanceof SanitizerGuard
   }

ruby/ql/test/query-tests/security/cwe-094/CodeInjection.expected

@@ -1,41 +1,57 @@
 edges
 | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:5:12:5:24 | ...[...] :  |
+| CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:5:12:5:24 | ...[...] :  |
+| CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:8:10:8:13 | code |
 | CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:8:10:8:13 | code |
 | CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:20:20:20:23 | code |
+| CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:20:20:20:23 | code |
+| CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:23:21:23:24 | code |
 | CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:23:21:23:24 | code |
 | CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:29:15:29:18 | code |
 | CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:32:19:32:22 | code |
 | CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:38:24:38:27 | code :  |
+| CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:38:24:38:27 | code :  |
 | CodeInjection.rb:5:12:5:24 | ...[...] :  | CodeInjection.rb:41:40:41:43 | code |
 | CodeInjection.rb:38:24:38:27 | code :  | CodeInjection.rb:38:10:38:28 | call to escape |
+| CodeInjection.rb:38:24:38:27 | code :  | CodeInjection.rb:38:10:38:28 | call to escape |
 | CodeInjection.rb:78:12:78:17 | call to params :  | CodeInjection.rb:78:12:78:24 | ...[...] :  |
 | CodeInjection.rb:78:12:78:24 | ...[...] :  | CodeInjection.rb:80:16:80:19 | code |
-| CodeInjection.rb:78:12:78:24 | ...[...] :  | CodeInjection.rb:82:16:82:43 | ... + ... |
 nodes
 | CodeInjection.rb:5:12:5:17 | call to params :  | semmle.label | call to params :  |
+| CodeInjection.rb:5:12:5:17 | call to params :  | semmle.label | call to params :  |
+| CodeInjection.rb:5:12:5:24 | ...[...] :  | semmle.label | ...[...] :  |
 | CodeInjection.rb:5:12:5:24 | ...[...] :  | semmle.label | ...[...] :  |
 | CodeInjection.rb:8:10:8:13 | code | semmle.label | code |
+| CodeInjection.rb:8:10:8:13 | code | semmle.label | code |
+| CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params |
 | CodeInjection.rb:11:10:11:15 | call to params | semmle.label | call to params |
 | CodeInjection.rb:20:20:20:23 | code | semmle.label | code |
+| CodeInjection.rb:20:20:20:23 | code | semmle.label | code |
+| CodeInjection.rb:23:21:23:24 | code | semmle.label | code |
 | CodeInjection.rb:23:21:23:24 | code | semmle.label | code |
 | CodeInjection.rb:29:15:29:18 | code | semmle.label | code |
 | CodeInjection.rb:32:19:32:22 | code | semmle.label | code |
 | CodeInjection.rb:38:10:38:28 | call to escape | semmle.label | call to escape |
+| CodeInjection.rb:38:10:38:28 | call to escape | semmle.label | call to escape |
+| CodeInjection.rb:38:24:38:27 | code :  | semmle.label | code :  |
 | CodeInjection.rb:38:24:38:27 | code :  | semmle.label | code :  |
 | CodeInjection.rb:41:40:41:43 | code | semmle.label | code |
 | CodeInjection.rb:78:12:78:17 | call to params :  | semmle.label | call to params :  |
 | CodeInjection.rb:78:12:78:24 | ...[...] :  | semmle.label | ...[...] :  |
 | CodeInjection.rb:80:16:80:19 | code | semmle.label | code |
-| CodeInjection.rb:82:16:82:43 | ... + ... | semmle.label | ... + ... |
 subpaths
 #select
 | CodeInjection.rb:8:10:8:13 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:8:10:8:13 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
+| CodeInjection.rb:8:10:8:13 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:8:10:8:13 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
+| CodeInjection.rb:11:10:11:15 | call to params | CodeInjection.rb:11:10:11:15 | call to params | CodeInjection.rb:11:10:11:15 | call to params | This code execution depends on a $@. | CodeInjection.rb:11:10:11:15 | call to params | user-provided value |
 | CodeInjection.rb:11:10:11:15 | call to params | CodeInjection.rb:11:10:11:15 | call to params | CodeInjection.rb:11:10:11:15 | call to params | This code execution depends on a $@. | CodeInjection.rb:11:10:11:15 | call to params | user-provided value |
 | CodeInjection.rb:20:20:20:23 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:20:20:20:23 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
+| CodeInjection.rb:20:20:20:23 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:20:20:20:23 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
+| CodeInjection.rb:23:21:23:24 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:23:21:23:24 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
 | CodeInjection.rb:23:21:23:24 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:23:21:23:24 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
 | CodeInjection.rb:29:15:29:18 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:29:15:29:18 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
 | CodeInjection.rb:32:19:32:22 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:32:19:32:22 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
 | CodeInjection.rb:38:10:38:28 | call to escape | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:38:10:38:28 | call to escape | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
+| CodeInjection.rb:38:10:38:28 | call to escape | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:38:10:38:28 | call to escape | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
 | CodeInjection.rb:41:40:41:43 | code | CodeInjection.rb:5:12:5:17 | call to params :  | CodeInjection.rb:41:40:41:43 | code | This code execution depends on a $@. | CodeInjection.rb:5:12:5:17 | call to params | user-provided value |
 | CodeInjection.rb:80:16:80:19 | code | CodeInjection.rb:78:12:78:17 | call to params :  | CodeInjection.rb:80:16:80:19 | code | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |
-| CodeInjection.rb:82:16:82:43 | ... + ... | CodeInjection.rb:78:12:78:17 | call to params :  | CodeInjection.rb:82:16:82:43 | ... + ... | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |

ruby/ql/test/query-tests/security/cwe-094/CodeInjection.rb

@@ -79,6 +79,8 @@ class UsersController < ActionController::Base
 
     obj().send(code, "foo"); # BAD
 
-    obj().send("prefix_" + code + "_suffix", "foo"); # GOOD - but still flagged by this query
+    obj().send("prefix_" + code + "_suffix", "foo"); # GOOD
+
+    obj().send("prefix_#{code}_suffix", "foo"); # GOOD
   end
 end