Merge pull request #11207 from github/nickrolfe/arel-sql

Ruby: add `SqlConstruction` concept, and implement it for calls to `Arel.sql`

ruby/ql/lib/change-notes/2022-11-10-arel-sql.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* The `codeql.ruby.Concepts` library now has a `SqlConstruction` class, in addition to the existing `SqlExecution` class.
+* Calls to `Arel.sql` are now modeled as instances of the new `SqlConstruction` concept.

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

@@ -11,9 +11,47 @@ private import codeql.ruby.Frameworks
 private import codeql.ruby.dataflow.RemoteFlowSources
 private import codeql.ruby.ApiGraphs
 
+/**
+ * A data-flow node that constructs a SQL statement.
+ *
+ * Often, it is worthy of an alert if a SQL statement is constructed such that
+ * executing it would be a security risk.
+ *
+ * If it is important that the SQL statement is executed, use `SqlExecution`.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `SqlConstruction::Range` instead.
+ */
+class SqlConstruction extends DataFlow::Node instanceof SqlConstruction::Range {
+  /** Gets the argument that specifies the SQL statements to be constructed. */
+  DataFlow::Node getSql() { result = super.getSql() }
+}
+
+/** Provides a class for modeling new SQL execution APIs. */
+module SqlConstruction {
+  /**
+   * A data-flow node that constructs a SQL statement.
+   *
+   * Often, it is worthy of an alert if a SQL statement is constructed such that
+   * executing it would be a security risk.
+   *
+   * If it is important that the SQL statement is executed, use `SqlExecution`.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `SqlConstruction` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the argument that specifies the SQL statements to be constructed. */
+    abstract DataFlow::Node getSql();
+  }
+}
+
 /**
  * A data-flow node that executes SQL statements.
  *
+ * If the context of interest is such that merely constructing a SQL statement
+ * would be valuable to report, consider using `SqlConstruction`.
+ *
  * Extend this class to refine existing API models. If you want to model new APIs,
  * extend `SqlExecution::Range` instead.
  */
@@ -27,6 +65,9 @@ module SqlExecution {
   /**
    * A data-flow node that executes SQL statements.
    *
+   * If the context of interest is such that merely constructing a SQL
+   * statement would be valuable to report, consider using `SqlConstruction`.
+   *
    * Extend this class to model new APIs. If you want to refine existing API models,
    * extend `SqlExecution` instead.
    */

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

@@ -6,6 +6,7 @@
 
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.dataflow.FlowSummary
+private import codeql.ruby.Concepts
 
 /**
  * Provides modeling for Arel, a low level SQL library that powers ActiveRecord.
@@ -28,4 +29,14 @@ module Arel {
       input = "Argument[0]" and output = "ReturnValue" and preservesValue = false
     }
   }
+
+  /** A call to `Arel.sql`, considered as a SQL construction. */
+  private class ArelSqlConstruction extends SqlConstruction::Range, DataFlow::CallNode {
+    ArelSqlConstruction() {
+      this = DataFlow::getConstant("Arel").getAMethodCall() and
+      this.getMethodName() = "sql"
+    }
+
+    override DataFlow::Node getSql() { result = this.getArgument(0) }
+  }
 }

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

@@ -0,0 +1,55 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting SQL injection
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+
+private import codeql.ruby.Concepts
+private import codeql.ruby.DataFlow
+private import codeql.ruby.dataflow.BarrierGuards
+private import codeql.ruby.dataflow.RemoteFlowSources
+
+/**
+ * Provides default sources, sinks and sanitizers for detecting SQL injection
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+module SqlInjection {
+  /** A data flow source for SQL injection vulnerabilities. */
+  abstract class Source extends DataFlow::Node { }
+
+  /** A data flow sink for SQL injection vulnerabilities. */
+  abstract class Sink extends DataFlow::Node { }
+
+  /** A sanitizer for SQL 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 SQL statement of a SQL execution, considered as a flow sink.
+   */
+  private class SqlExecutionAsSink extends Sink {
+    SqlExecutionAsSink() { this = any(SqlExecution e).getSql() }
+  }
+
+  /**
+   * An SQL statement of a SQL construction, considered as a flow sink.
+   */
+  private class SqlConstructionAsSink extends Sink {
+    SqlConstructionAsSink() { this = any(SqlConstruction e).getSql() }
+  }
+
+  /**
+   * 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.
+   */
+  class StringConstArrayInclusionCallAsSanitizer extends Sanitizer,
+    StringConstArrayInclusionCallBarrier { }
+}

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

@@ -0,0 +1,21 @@
+/**
+ * Provides default sources, sinks and sanitizers for detecting SQL injection
+ * vulnerabilities, as well as extension points for adding your own.
+ */
+
+private import codeql.ruby.DataFlow
+private import codeql.ruby.TaintTracking
+import SqlInjectionCustomizations::SqlInjection
+
+/**
+ * A taint-tracking configuration for detecting SQL injection vulnerabilities.
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "SqlInjectionConfiguration" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+  override predicate isSink(DataFlow::Node source) { source instanceof Sink }
+
+  override predicate isSanitizer(DataFlow::Node node) { node instanceof Sanitizer }
+}

ruby/ql/src/change-notes/2022-11-10-arel-sql.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `rb/sql-injection` query now considers consider SQL constructions, such as calls to `Arel.sql`, as sinks.

ruby/ql/src/queries/security/cwe-089/SqlInjection.ql

@@ -11,28 +11,11 @@
  *       external/cwe/cwe-089
  */
 
-import codeql.ruby.AST
-import codeql.ruby.Concepts
 import codeql.ruby.DataFlow
-import codeql.ruby.dataflow.BarrierGuards
-import codeql.ruby.dataflow.RemoteFlowSources
-import codeql.ruby.TaintTracking
+import codeql.ruby.security.SqlInjectionQuery
 import DataFlow::PathGraph
 
-class SqlInjectionConfiguration extends TaintTracking::Configuration {
-  SqlInjectionConfiguration() { this = "SQLInjectionConfiguration" }
-
-  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
-
-  override predicate isSink(DataFlow::Node sink) { sink instanceof SqlExecution }
-
-  override predicate isSanitizer(DataFlow::Node node) {
-    node instanceof StringConstCompareBarrier or
-    node instanceof StringConstArrayInclusionCallBarrier
-  }
-}
-
-from SqlInjectionConfiguration config, DataFlow::PathNode source, DataFlow::PathNode sink
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
 where config.hasFlowPath(source, sink)
 select sink.getNode(), source, sink, "This SQL query depends on a $@.", source.getNode(),
   "user-provided value"

ruby/ql/test/library-tests/frameworks/arel/SqlConstruction.expected

@@ -0,0 +1,2 @@
+| arel.rb:3:8:3:18 | call to sql | arel.rb:3:17:3:17 | x |
+| arel.rb:8:8:8:18 | call to sql | arel.rb:8:17:8:17 | x |

ruby/ql/test/library-tests/frameworks/arel/SqlConstruction.ql

@@ -0,0 +1,4 @@
+import codeql.ruby.Concepts
+import codeql.ruby.DataFlow
+
+query predicate sqlConstructions(SqlConstruction c, DataFlow::Node sql) { sql = c.getSql() }

ruby/ql/test/query-tests/security/cwe-089/ArelInjection.rb

@@ -0,0 +1,8 @@
+
+class PotatoController < ActionController::Base
+  def unsafe_action
+    name = params[:user_name]
+    # BAD: SQL statement constructed from user input
+    sql = Arel.sql("SELECT * FROM users WHERE name = #{name}")
+  end
+end

ruby/ql/test/query-tests/security/cwe-089/SqlInjection.expected

@@ -33,6 +33,8 @@ edges
 | ActiveRecordInjection.rb:137:21:137:44 | ...[...] :  | ActiveRecordInjection.rb:20:22:20:30 | condition :  |
 | ActiveRecordInjection.rb:151:59:151:64 | call to params :  | ActiveRecordInjection.rb:151:59:151:74 | ...[...] :  |
 | ActiveRecordInjection.rb:151:59:151:74 | ...[...] :  | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." |
+| ArelInjection.rb:4:12:4:17 | call to params :  | ArelInjection.rb:4:12:4:29 | ...[...] :  |
+| ArelInjection.rb:4:12:4:29 | ...[...] :  | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." |
 nodes
 | ActiveRecordInjection.rb:8:25:8:28 | name :  | semmle.label | name :  |
 | ActiveRecordInjection.rb:8:31:8:34 | pass :  | semmle.label | pass :  |
@@ -85,6 +87,9 @@ nodes
 | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | semmle.label | "this is an unsafe annotation:..." |
 | ActiveRecordInjection.rb:151:59:151:64 | call to params :  | semmle.label | call to params :  |
 | ActiveRecordInjection.rb:151:59:151:74 | ...[...] :  | semmle.label | ...[...] :  |
+| ArelInjection.rb:4:12:4:17 | call to params :  | semmle.label | call to params :  |
+| ArelInjection.rb:4:12:4:29 | ...[...] :  | semmle.label | ...[...] :  |
+| ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | semmle.label | "SELECT * FROM users WHERE nam..." |
 subpaths
 #select
 | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | ActiveRecordInjection.rb:70:23:70:28 | call to params :  | ActiveRecordInjection.rb:10:33:10:67 | "name='#{...}' and pass='#{...}'" | This SQL query depends on a $@. | ActiveRecordInjection.rb:70:23:70:28 | call to params | user-provided value |
@@ -105,3 +110,4 @@ subpaths
 | ActiveRecordInjection.rb:92:21:92:35 | ...[...] | ActiveRecordInjection.rb:92:21:92:26 | call to params :  | ActiveRecordInjection.rb:92:21:92:35 | ...[...] | This SQL query depends on a $@. | ActiveRecordInjection.rb:92:21:92:26 | call to params | user-provided value |
 | ActiveRecordInjection.rb:104:20:104:32 | ... + ... | ActiveRecordInjection.rb:98:10:98:15 | call to params :  | ActiveRecordInjection.rb:104:20:104:32 | ... + ... | This SQL query depends on a $@. | ActiveRecordInjection.rb:98:10:98:15 | call to params | user-provided value |
 | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | ActiveRecordInjection.rb:151:59:151:64 | call to params :  | ActiveRecordInjection.rb:151:27:151:76 | "this is an unsafe annotation:..." | This SQL query depends on a $@. | ActiveRecordInjection.rb:151:59:151:64 | call to params | user-provided value |
+| ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | ArelInjection.rb:4:12:4:17 | call to params :  | ArelInjection.rb:6:20:6:61 | "SELECT * FROM users WHERE nam..." | This SQL query depends on a $@. | ArelInjection.rb:4:12:4:17 | call to params | user-provided value |