Merge pull request #196 from github/active-record-1

Start modelling some potential SQL fragment sinks in ActiveRecord

ql/src/codeql_ruby/Concepts.qll

@@ -0,0 +1,36 @@
+/**
+ * Provides abstract classes representing generic concepts such as file system
+ * access or system command execution, for which individual framework libraries
+ * provide concrete subclasses.
+ */
+
+private import codeql_ruby.DataFlow
+
+/**
+ * A data-flow node that executes SQL statements.
+ *
+ * Extend this class to refine existing API models. If you want to model new APIs,
+ * extend `SqlExecution::Range` instead.
+ */
+class SqlExecution extends DataFlow::Node {
+  SqlExecution::Range range;
+
+  SqlExecution() { this = range }
+
+  /** Gets the argument that specifies the SQL statements to be executed. */
+  DataFlow::Node getSql() { result = range.getSql() }
+}
+
+/** Provides a class for modeling new SQL execution APIs. */
+module SqlExecution {
+  /**
+   * A data-flow node that executes SQL statements.
+   *
+   * Extend this class to model new APIs. If you want to refine existing API models,
+   * extend `SqlExecution` instead.
+   */
+  abstract class Range extends DataFlow::Node {
+    /** Gets the argument that specifies the SQL statements to be executed. */
+    abstract DataFlow::Node getSql();
+  }
+}

ql/src/codeql_ruby/frameworks/ActiveRecord.qll

@@ -0,0 +1,126 @@
+private import codeql_ruby.AST
+private import codeql_ruby.Concepts
+private import codeql_ruby.controlflow.CfgNodes
+private import codeql_ruby.DataFlow
+private import codeql_ruby.ast.internal.Module
+
+private class ActiveRecordBaseAccess extends ConstantReadAccess {
+  ActiveRecordBaseAccess() {
+    this.getName() = "Base" and
+    this.getScopeExpr().(ConstantAccess).getName() = "ActiveRecord"
+  }
+}
+
+// ApplicationRecord extends ActiveRecord::Base, but we
+// treat it separately in case the ApplicationRecord definition
+// is not in the database
+private class ApplicationRecordAccess extends ConstantReadAccess {
+  ApplicationRecordAccess() { this.getName() = "ApplicationRecord" }
+}
+
+/**
+ * A `ClassDeclaration` for a class that extends `ActiveRecord::Base`. For example,
+ *
+ * ```rb
+ * class UserGroup < ActiveRecord::Base
+ *   has_many :users
+ * end
+ * ```
+ */
+class ActiveRecordModelClass extends ClassDeclaration {
+  ActiveRecordModelClass() {
+    // class Foo < ActiveRecord::Base
+    this.getSuperclassExpr() instanceof ActiveRecordBaseAccess
+    or
+    // class Foo < ApplicationRecord
+    this.getSuperclassExpr() instanceof ApplicationRecordAccess
+    or
+    // class Bar < Foo
+    exists(ActiveRecordModelClass other |
+      other.getModule() = resolveScopeExpr(this.getSuperclassExpr())
+    )
+  }
+}
+
+/** A class method call whose receiver is an `ActiveRecordModelClass`. */
+class ActiveRecordModelClassMethodCall extends MethodCall {
+  ActiveRecordModelClassMethodCall() {
+    // e.g. Foo.where(...)
+    exists(ActiveRecordModelClass recvCls |
+      recvCls.getModule() = resolveScopeExpr(this.getReceiver())
+    )
+    or
+    // e.g. Foo.joins(:bars).where(...)
+    this.getReceiver() instanceof ActiveRecordModelClassMethodCall
+  }
+}
+
+private predicate methodWithSqlFragmentArg(string methodName, int argIndex) {
+  methodName =
+    [
+      "delete_all", "destroy_all", "exists?", "find_by", "find_by_sql", "from", "group", "having",
+      "joins", "lock", "not", "order", "pluck", "where"
+    ] and
+  argIndex = 0
+  or
+  methodName = "calculate" and argIndex = 1
+}
+
+/**
+ * A method call that may result in executing unintended user-controlled SQL
+ * queries if the `getSqlFragmentSinkArgument()` expression is tainted by
+ * unsanitized user-controlled input. For example, supposing that `User` is an
+ * `ActiveRecord` model class, then
+ *
+ * ```rb
+ * User.where("name = '#{user_name}'")
+ * ```
+ *
+ * may be unsafe if `user_name` is from unsanitized user input, as a value such
+ * as `"') OR 1=1 --"` could result in the application looking up all users
+ * rather than just one with a matching name.
+ */
+class PotentiallyUnsafeSqlExecutingMethodCall extends ActiveRecordModelClassMethodCall {
+  // The name of the method invoked
+  private string methodName;
+  // The zero-indexed position of the SQL fragment sink argument
+  private int sqlFragmentArgumentIndex;
+  // The SQL fragment argument itself
+  private Expr sqlFragmentExpr;
+
+  // TODO: `find` with `lock:` option also takes an SQL fragment
+  PotentiallyUnsafeSqlExecutingMethodCall() {
+    methodName = this.getMethodName() and
+    sqlFragmentExpr = this.getArgument(sqlFragmentArgumentIndex) and
+    methodWithSqlFragmentArg(methodName, sqlFragmentArgumentIndex) and
+    (
+      // select only literals containing an interpolated value...
+      exists(StringInterpolationComponent interpolated |
+        interpolated = sqlFragmentExpr.(StringlikeLiteral).getComponent(_)
+      )
+      or
+      // ...or string concatenations...
+      sqlFragmentExpr instanceof AddExpr
+      or
+      // ...or variable reads
+      sqlFragmentExpr instanceof VariableReadAccess
+    )
+  }
+
+  Expr getSqlFragmentSinkArgument() { result = sqlFragmentExpr }
+}
+
+/**
+ * An `SqlExecution::Range` for an argument to a
+ * `PotentiallyUnsafeSqlExecutingMethodCall` that may be vulnerable to being
+ * controlled by user input.
+ */
+class ActiveRecordSqlExecutionRange extends SqlExecution::Range {
+  ActiveRecordSqlExecutionRange() {
+    exists(PotentiallyUnsafeSqlExecutingMethodCall mc |
+      this.asExpr().getNode() = mc.getSqlFragmentSinkArgument()
+    )
+  }
+
+  override DataFlow::Node getSql() { result = this }
+}

ql/test/library-tests/frameworks/ActiveRecord.expected

@@ -0,0 +1,22 @@
+activeRecordModelClasses
+| ActiveRecordInjection.rb:1:1:3:3 | UserGroup |
+| ActiveRecordInjection.rb:5:1:7:3 | User |
+| ActiveRecordInjection.rb:9:1:10:3 | Admin |
+activeRecordSqlExecutionRanges
+| ActiveRecordInjection.rb:22:21:22:41 | "id = #{...}" |
+| ActiveRecordInjection.rb:28:16:28:21 | <<-SQL |
+| ActiveRecordInjection.rb:32:35:32:60 | "user.id = #{...}" |
+| ActiveRecordInjection.rb:45:21:45:33 | ... + ... |
+activeRecordModelClassMethodCalls
+| ActiveRecordInjection.rb:19:5:19:45 | call to calculate |
+| ActiveRecordInjection.rb:22:5:22:42 | call to delete_all |
+| ActiveRecordInjection.rb:25:5:25:45 | call to destroy_all |
+| ActiveRecordInjection.rb:28:5:28:35 | call to where |
+| ActiveRecordInjection.rb:32:5:32:27 | call to joins |
+| ActiveRecordInjection.rb:32:5:32:61 | call to where |
+| ActiveRecordInjection.rb:45:5:45:34 | call to delete_all |
+potentiallyUnsafeSqlExecutingMethodCall
+| ActiveRecordInjection.rb:22:5:22:42 | call to delete_all |
+| ActiveRecordInjection.rb:28:5:28:35 | call to where |
+| ActiveRecordInjection.rb:32:5:32:61 | call to where |
+| ActiveRecordInjection.rb:45:5:45:34 | call to delete_all |

ql/test/library-tests/frameworks/ActiveRecord.ql

@@ -0,0 +1,12 @@
+import codeql_ruby.controlflow.CfgNodes
+import codeql_ruby.frameworks.ActiveRecord
+
+query predicate activeRecordModelClasses(ActiveRecordModelClass cls) { any() }
+
+query predicate activeRecordSqlExecutionRanges(ActiveRecordSqlExecutionRange range) { any() }
+
+query predicate activeRecordModelClassMethodCalls(ActiveRecordModelClassMethodCall call) { any() }
+
+query predicate potentiallyUnsafeSqlExecutingMethodCall(PotentiallyUnsafeSqlExecutingMethodCall call) {
+  any()
+}

ql/test/library-tests/frameworks/ActiveRecordInjection.rb

@@ -0,0 +1,51 @@
+class UserGroup < ActiveRecord::Base
+  has_many :users
+end
+
+class User < ApplicationRecord
+  belongs_to :user_group
+end
+
+class Admin < User
+end
+
+class FooController < ActionController::Base
+
+  MAX_USER_ID = 100_000
+
+  # A string tainted by user input is inserted into an SQL query
+  def some_request_handler
+    # SELECT AVG(#{params[:column]}) FROM "users"
+    User.calculate(:average, params[:column])
+
+    # DELETE FROM "users" WHERE (id = #{params[:id]})
+    User.delete_all("id = #{params[:id]}")
+
+    # SELECT "users".* FROM "users" WHERE (id = #{params[:id]})
+    User.destroy_all(["id = #{params[:id]}"])
+
+    # SELECT "users".* FROM "users" WHERE id BETWEEN #{params[:min_id]} AND 100000
+    User.where(<<-SQL, MAX_USER_ID)
+      id BETWEEN #{params[:min_id]} AND ?
+    SQL
+
+    UserGroup.joins(:users).where("user.id = #{params[:id]}")
+  end
+end
+
+
+class BarController < ApplicationController
+
+  def some_other_request_handler
+    ps = params
+
+    uid = ps[:id]
+
+    # DELETE FROM "users" WHERE (id = #{uid})
+    User.delete_all("id = " + uid)
+  end
+
+end
+
+class BazController < BarController
+end