Ruby: First draft of rails callback flow

Author: hmac

ruby/ql/lib/codeql/ruby/frameworks/actioncontroller/Filters.qll

@@ -387,4 +387,119 @@ module Filters {
    * `pred` and `succ` may be methods bound to callbacks or controller actions.
    */
   predicate next(Method pred, Method succ) { next(_, pred, succ) }
+
+  /**
+   * Holds if `n` is the post-update node of a self-variable access in method
+   * `m` which stores content in the self variable.
+   * In the example below, `n` is the post-update node for `@x = 1`.
+   * ```rb
+   * def m
+   *   @x = 1
+   * end
+   * ```
+   */
+  private predicate variableAccessPostUpdate(DataFlow::PostUpdateNode n, Method m) {
+    n.getPreUpdateNode().asExpr() instanceof SelfVariableAccessCfgNode and
+    m = n.getPreUpdateNode().asExpr().getExpr().getEnclosingCallable() and
+    DataFlowPrivate::storeStep(_, _, n)
+  }
+
+  /**
+   * Holds if `n` is the syntactically last dataflow node in `m` that satisfies `variableAccessPostUpdate`.
+   * In the example below, `n` is the post-update node for `@y = 2`.
+   * ```rb
+   * def m
+   *   @x = 1
+   *   @y = 2
+   * end
+   * ```
+   */
+  private predicate lastVariableAccessPostUpdate(DataFlow::PostUpdateNode n, Method m) {
+    variableAccessPostUpdate(n, m) and
+    not exists(DataFlow::PostUpdateNode n2 |
+      variableAccessPostUpdate(n2, m) and
+      n.getPreUpdateNode().asExpr().getASuccessor+() = n2.getPreUpdateNode().asExpr()
+    )
+  }
+
+  /**
+   * Holds if `a` is the syntactically last access of the self variable `v` in method `m`.
+   */
+  private predicate lastMethodSelfVarAccess(SelfVariableAccessCfgNode a, SelfVariable v, Method m) {
+    a.getExpr().getEnclosingMethod() = m and
+    v = a.getExpr().getVariable() and
+    not exists(SelfVariableAccessCfgNode a2 |
+      a2.getExpr().getVariable() = v and
+      a.getASuccessor+() = a2
+    )
+  }
+
+  /**
+   * Holds if there is no access to `self` in method `m`.
+   */
+  private predicate noSelfVarAccess(Method m) {
+    not exists(SelfVariableAccess a | a.getEnclosingMethod() = m)
+  }
+
+  /**
+   * Holds if `n` is the self parameter of method `m`.
+   */
+  private predicate selfParameter(DataFlowPrivate::SelfParameterNode n, Method m) {
+    m = n.getMethod()
+  }
+
+  /**
+   * A class defining additional jump steps arising from filters.
+   */
+  class FilterJumpStep extends DataFlowPrivate::AdditionalJumpStep {
+    /**
+     * Holds if data can flow from `pred` to `succ` via a callback chain.
+     * `pred` is the post-update node of the self parameter in a method, and
+     * `succ` is the self parameter of a subsequent method that is executed as
+     * part of the callback chain.
+     */
+    override predicate step(DataFlow::Node pred, DataFlow::Node succ) {
+      exists(Method predMethod, Method succMethod |
+        next(predMethod, succMethod) and
+        (
+          // Flow from an update of self in `pred` to the self parameter of `succ`
+          //
+          // def a
+          //   @x = 1 ---+
+          // end         |
+          //             |
+          // def b  <----+
+          //  ...
+          //
+          lastVariableAccessPostUpdate(pred, predMethod) and
+          selfParameter(succ, succMethod)
+          or
+          // When there is no update of self in `pred`, flow from the last access to self to the self parameter of `succ`
+          //
+          // def a
+          //   foo() ---+
+          //   x = 1    |
+          // end        |
+          //            |
+          // def b  <---+
+          // ...
+          //
+          not variableAccessPostUpdate(_, predMethod) and
+          lastMethodSelfVarAccess(pred.asExpr(), _, predMethod) and
+          selfParameter(succ, succMethod)
+          or
+          // When there is no access to self in `pred`, flow from the self parameter of `pred` to the self parameter of `succ`
+          //
+          // def a ---+
+          // end      |
+          //          |
+          // def b <--+
+          // ...
+          noSelfVarAccess(predMethod) and
+          selfParameter(pred, predMethod) and
+          selfParameter(succ, succMethod)
+        )
+      )
+    }
+  }
 }

ruby/ql/test/library-tests/frameworks/action_controller/Filters.expected

@@ -1,3 +1,36 @@
+additionalFlowSteps
+| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:74:3:77:5 | self in ensure_user_can_edit_comments |
+| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
+| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
+| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/posts_controller.rb:12:3:13:5 | self in index |
+| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/posts_controller.rb:15:3:16:5 | self in show |
+| controllers/application_controller.rb:7:5:7:9 | [post] self | controllers/posts_controller.rb:18:3:19:5 | self in upvote |
+| controllers/application_controller.rb:11:53:11:59 | self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
+| controllers/application_controller.rb:11:53:11:59 | self | controllers/photos_controller.rb:3:3:6:5 | self in show |
+| controllers/application_controller.rb:11:53:11:59 | self | controllers/posts_controller.rb:23:3:25:5 | self in set_post |
+| controllers/comments_controller.rb:50:5:50:12 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
+| controllers/comments_controller.rb:53:3:54:5 | self in create | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
+| controllers/comments_controller.rb:57:5:61:7 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
+| controllers/comments_controller.rb:58:33:58:48 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
+| controllers/comments_controller.rb:60:58:60:63 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
+| controllers/comments_controller.rb:65:15:65:20 | self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
+| controllers/comments_controller.rb:69:12:69:18 | self | controllers/comments_controller.rb:83:3:85:5 | self in log_comment_change |
+| controllers/comments_controller.rb:76:5:76:68 | self | controllers/comments_controller.rb:79:3:81:5 | self in set_comment |
+| controllers/comments_controller.rb:76:5:76:68 | self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
+| controllers/comments_controller.rb:80:5:80:12 | [post] self | controllers/comments_controller.rb:99:3:100:5 | self in foo |
+| controllers/comments_controller.rb:84:61:84:68 | self | controllers/comments_controller.rb:87:3:89:5 | self in check_feature_flags |
+| controllers/comments_controller.rb:88:5:88:28 | self | controllers/comments_controller.rb:95:3:97:5 | self in this_must_run_last |
+| controllers/comments_controller.rb:91:3:93:5 | self in this_must_run_first | controllers/application_controller.rb:10:3:12:5 | self in log_request |
+| controllers/comments_controller.rb:99:3:100:5 | self in foo | controllers/comments_controller.rb:102:3:103:5 | self in bar |
+| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:17:3:51:5 | self in index |
+| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:53:3:54:5 | self in create |
+| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:56:3:62:5 | self in show |
+| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:64:3:66:5 | self in photo |
+| controllers/comments_controller.rb:102:3:103:5 | self in bar | controllers/comments_controller.rb:68:3:70:5 | self in destroy |
+| controllers/photos_controller.rb:5:5:5:6 | [post] self | controllers/photos_controller.rb:8:3:9:5 | self in foo |
+| controllers/posts_controller.rb:18:3:19:5 | self in upvote | controllers/posts_controller.rb:27:3:29:5 | self in log_upvote |
+| controllers/posts_controller.rb:24:5:24:9 | [post] self | controllers/application_controller.rb:6:3:8:5 | self in set_user |
+filterChain
 | controllers/comments_controller.rb:17:3:51:5 | index | controllers/application_controller.rb:6:3:8:5 | set_user | controllers/comments_controller.rb:99:3:100:5 | foo |
 | controllers/comments_controller.rb:17:3:51:5 | index | controllers/application_controller.rb:10:3:12:5 | log_request | controllers/application_controller.rb:6:3:8:5 | set_user |
 | controllers/comments_controller.rb:17:3:51:5 | index | controllers/comments_controller.rb:17:3:51:5 | index | controllers/comments_controller.rb:87:3:89:5 | check_feature_flags |

ruby/ql/test/library-tests/frameworks/action_controller/Filters.ql

@@ -3,3 +3,7 @@ private import codeql.ruby.frameworks.ActionController
 private import codeql.ruby.DataFlow
 
 query predicate filterChain = ActionController::Filters::next/3;
+
+query predicate additionalFlowSteps(DataFlow::Node pred, DataFlow::Node succ) {
+  any(ActionController::Filters::FilterJumpStep s).step(pred, succ)
+}

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

@@ -21,6 +21,20 @@ edges
 | CodeInjection.rb:78:12:78:24 | ...[...] :  | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" |
 | CodeInjection.rb:78:12:78:24 | ...[...] :  | CodeInjection.rb:90:10:90:13 | code |
 | CodeInjection.rb:78:12:78:24 | ...[...] :  | CodeInjection.rb:90:10:90:13 | code |
+| CodeInjection.rb:101:3:102:5 | self in index [@foo] :  | CodeInjection.rb:111:3:113:5 | self in baz [@foo] :  |
+| CodeInjection.rb:101:3:102:5 | self in index [@foo] :  | CodeInjection.rb:111:3:113:5 | self in baz [@foo] :  |
+| CodeInjection.rb:105:5:105:8 | [post] self [@foo] :  | CodeInjection.rb:108:3:109:5 | self in bar [@foo] :  |
+| CodeInjection.rb:105:5:105:8 | [post] self [@foo] :  | CodeInjection.rb:108:3:109:5 | self in bar [@foo] :  |
+| CodeInjection.rb:105:12:105:17 | call to params :  | CodeInjection.rb:105:12:105:23 | ...[...] :  |
+| CodeInjection.rb:105:12:105:17 | call to params :  | CodeInjection.rb:105:12:105:23 | ...[...] :  |
+| CodeInjection.rb:105:12:105:23 | ...[...] :  | CodeInjection.rb:105:5:105:8 | [post] self [@foo] :  |
+| CodeInjection.rb:105:12:105:23 | ...[...] :  | CodeInjection.rb:105:5:105:8 | [post] self [@foo] :  |
+| CodeInjection.rb:108:3:109:5 | self in bar [@foo] :  | CodeInjection.rb:101:3:102:5 | self in index [@foo] :  |
+| CodeInjection.rb:108:3:109:5 | self in bar [@foo] :  | CodeInjection.rb:101:3:102:5 | self in index [@foo] :  |
+| CodeInjection.rb:111:3:113:5 | self in baz [@foo] :  | CodeInjection.rb:112:10:112:13 | self [@foo] :  |
+| CodeInjection.rb:111:3:113:5 | self in baz [@foo] :  | CodeInjection.rb:112:10:112:13 | self [@foo] :  |
+| CodeInjection.rb:112:10:112:13 | self [@foo] :  | CodeInjection.rb:112:10:112:13 | @foo |
+| CodeInjection.rb:112:10:112:13 | self [@foo] :  | CodeInjection.rb:112:10:112:13 | @foo |
 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 :  |
@@ -50,6 +64,22 @@ nodes
 | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | semmle.label | "prefix_#{...}_suffix" |
 | CodeInjection.rb:90:10:90:13 | code | semmle.label | code |
 | CodeInjection.rb:90:10:90:13 | code | semmle.label | code |
+| CodeInjection.rb:101:3:102:5 | self in index [@foo] :  | semmle.label | self in index [@foo] :  |
+| CodeInjection.rb:101:3:102:5 | self in index [@foo] :  | semmle.label | self in index [@foo] :  |
+| CodeInjection.rb:105:5:105:8 | [post] self [@foo] :  | semmle.label | [post] self [@foo] :  |
+| CodeInjection.rb:105:5:105:8 | [post] self [@foo] :  | semmle.label | [post] self [@foo] :  |
+| CodeInjection.rb:105:12:105:17 | call to params :  | semmle.label | call to params :  |
+| CodeInjection.rb:105:12:105:17 | call to params :  | semmle.label | call to params :  |
+| CodeInjection.rb:105:12:105:23 | ...[...] :  | semmle.label | ...[...] :  |
+| CodeInjection.rb:105:12:105:23 | ...[...] :  | semmle.label | ...[...] :  |
+| CodeInjection.rb:108:3:109:5 | self in bar [@foo] :  | semmle.label | self in bar [@foo] :  |
+| CodeInjection.rb:108:3:109:5 | self in bar [@foo] :  | semmle.label | self in bar [@foo] :  |
+| CodeInjection.rb:111:3:113:5 | self in baz [@foo] :  | semmle.label | self in baz [@foo] :  |
+| CodeInjection.rb:111:3:113:5 | self in baz [@foo] :  | semmle.label | self in baz [@foo] :  |
+| CodeInjection.rb:112:10:112:13 | @foo | semmle.label | @foo |
+| CodeInjection.rb:112:10:112:13 | @foo | semmle.label | @foo |
+| CodeInjection.rb:112:10:112:13 | self [@foo] :  | semmle.label | self [@foo] :  |
+| CodeInjection.rb:112:10:112:13 | self [@foo] :  | semmle.label | self [@foo] :  |
 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 |
@@ -64,3 +94,4 @@ subpaths
 | CodeInjection.rb:86:10:86:37 | ... + ... | CodeInjection.rb:78:12:78:17 | call to params :  | CodeInjection.rb:86:10:86:37 | ... + ... | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |
 | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | CodeInjection.rb:78:12:78:17 | call to params :  | CodeInjection.rb:88:10:88:32 | "prefix_#{...}_suffix" | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |
 | CodeInjection.rb:90:10:90:13 | code | CodeInjection.rb:78:12:78:17 | call to params :  | CodeInjection.rb:90:10:90:13 | code | This code execution depends on a $@. | CodeInjection.rb:78:12:78:17 | call to params | user-provided value |
+| CodeInjection.rb:112:10:112:13 | @foo | CodeInjection.rb:105:12:105:17 | call to params :  | CodeInjection.rb:112:10:112:13 | @foo | This code execution depends on a $@. | CodeInjection.rb:105:12:105:17 | call to params | user-provided value |

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

@@ -89,4 +89,26 @@ def create
 
     eval(code); # BAD
   end
-end
+end
+
+Rails.application.routes.draw { resources :posts }
+
+class PostsController < ActionController::Base
+  before_action :foo
+  before_action :bar
+  after_action :baz
+
+  def index
+  end
+
+  def foo
+    @foo = params[:foo]
+  end
+
+  def bar
+  end
+
+  def baz
+    eval(@foo)
+  end
+end