Ruby: generalize rails flow step for accessing render locals hash in view

2026-04-28 18:25:24 +02:00 · 2023-01-03 14:09:00 +00:00
parent 976b0401be
commit e5fbc92856
4 changed files with 116 additions and 45 deletions
--- a/ruby/ql/lib/codeql/ruby/dataflow/FlowSummary.qll
+++ b/ruby/ql/lib/codeql/ruby/dataflow/FlowSummary.qll
@@ -30,6 +30,8 @@ module SummaryComponent {

  predicate withContent = SC::withContent/1;

+  class SyntheticGlobal = SC::SyntheticGlobal;
+
  /** Gets a summary component that represents a receiver. */
  SummaryComponent receiver() { result = argument(any(ParameterPosition pos | pos.isSelf())) }

--- a/ruby/ql/lib/codeql/ruby/frameworks/Rails.qll
+++ b/ruby/ql/lib/codeql/ruby/frameworks/Rails.qll
@@ -10,6 +10,7 @@ private import codeql.ruby.frameworks.ActiveStorage
 private import codeql.ruby.frameworks.internal.Rails
 private import codeql.ruby.ApiGraphs
 private import codeql.ruby.security.OpenSSL
+private import codeql.ruby.dataflow.FlowSummary

 /**
 * Provides classes for working with Rails.
@@ -287,5 +288,104 @@ private class CookiesSameSiteProtectionSetting extends Settings::NillableStringl
    result = "Unsetting 'SameSite' can disable same-site cookie restrictions in some browsers."
  }
 }
+
 // TODO: initialization hooks, e.g. before_configuration, after_initialize...
 // TODO: initializers
+private string getErbFileIdentifier(ErbFile erbFile) { result = erbFile.getRelativePath() }
+
+/** A synthetic global to represent the value passed to the `locals` argument of a render call for a specific ERB file. */
+private class LocalAssignsHashSyntheticGlobal extends SummaryComponent::SyntheticGlobal {
+  private ErbFile erbFile;
+  private string id;
+
+  LocalAssignsHashSyntheticGlobal() {
+    this = "LocalAssignsHashSyntheticGlobal+" + id and
+    id = getErbFileIdentifier(erbFile)
+  }
+
+  /** Gets the `ErbFile` which this locals hash is accessible from. */
+  ErbFile getErbFile() { result = erbFile }
+
+  /** Gets the identifier for this particular locals hash synthetic global. */
+  string getId() { result = id }
+}
+
+/** A summary for `render` calls linked to some specific ERB file. */
+private class RenderLocalsSummary extends SummarizedCallable {
+  private string id;
+  private LocalAssignsHashSyntheticGlobal glob;
+
+  RenderLocalsSummary() {
+    this = "rails_render_locals()" + id and
+    glob.getId() = id
+  }
+
+  override Rails::RenderCall getACall() { result.getTemplateFile() = glob.getErbFile() }
+
+  override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+    input = "Argument[locals:]" and
+    output = "SyntheticGlobal[" + glob + "]" and
+    preservesValue = true
+  }
+}
+
+/** A summary for calls to `local_assigns` in a view to access a `render` call `locals` hash. */
+private class AccessLocalsSummary extends SummarizedCallable {
+  private string id;
+  private LocalAssignsHashSyntheticGlobal glob;
+
+  AccessLocalsSummary() {
+    this = "rails_local_assigns()" + id and
+    glob.getId() = id
+  }
+
+  override MethodCall getACall() {
+    id = getErbFileIdentifier(result.getLocation().getFile()) and
+    result.getMethodName() = "local_assigns"
+  }
+
+  override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+    input = "SyntheticGlobal[" + glob + "]" and
+    output = "ReturnValue" and
+    preservesValue = true
+  }
+}
+
+private string getAMethodNameFromErbFile(ErbFile f) {
+  result = any(MethodCall c | c.getLocation().getFile() = f).getMethodName()
+}
+
+private predicate renderHasLocalsKey(Rails::RenderCall c, string key) {
+  exists(DataFlow::HashLiteralNode hashLitNode, DataFlow::CallNode renderCall |
+    renderCall.asExpr().getExpr() = c and
+    hashLitNode.flowsTo(renderCall.getKeywordArgument("locals"))
+  |
+    key = hashLitNode.getAKeyValuePair().getKey().getConstantValue().getStringlikeValue()
+  )
+}
+
+private class AccessLocalsKeySummary extends SummarizedCallable {
+  private string id;
+  private LocalAssignsHashSyntheticGlobal glob;
+  private string methodName;
+
+  AccessLocalsKeySummary() {
+    this = "rails_locals_key()" + id and
+    id = glob.getId() + "#" + methodName and
+    methodName = getAMethodNameFromErbFile(glob.getErbFile())
+    // TODO: this would cut down massively on impossible flow steps, but fails due to non-monotonic recusrion problems
+    //    and
+    //    renderHasLocalsKey(any(Rails::RenderCall c | c.getTemplateFile() = erbFile), methodName))
+  }
+
+  override MethodCall getACall() {
+    result.getLocation().getFile() = glob.getErbFile() and
+    result.getMethodName() = methodName
+  }
+
+  override predicate propagatesFlowExt(string input, string output, boolean preservesValue) {
+    input = "SyntheticGlobal[" + glob + "].Element[:" + methodName + "]" and
+    output = "ReturnValue" and
+    preservesValue = false
+  }
+}
--- a/ruby/ql/lib/codeql/ruby/security/XSS.qll
+++ b/ruby/ql/lib/codeql/ruby/security/XSS.qll
@@ -162,46 +162,6 @@ private module Shared {
    erb = call.getLocation().getFile()
  }

-  /**
-   * Holds if some render call passes `value` for `hashKey` in the `locals`
-   * argument, in ERB file `erb`.
-   */
-  pragma[noinline]
-  private predicate renderCallLocals(string hashKey, Expr value, ErbFile erb) {
-    exists(Rails::RenderCall call, Pair kvPair |
-      call.getLocals().getAKeyValuePair() = kvPair and
-      kvPair.getValue() = value and
-      kvPair.getKey().getConstantValue().isStringlikeValue(hashKey) and
-      call.getTemplateFile() = erb
-    )
-  }
-
-  pragma[noinline]
-  private predicate isFlowFromLocals0(
-    CfgNodes::ExprNodes::ElementReferenceCfgNode refNode, string hashKey, ErbFile erb
-  ) {
-    exists(DataFlow::Node argNode |
-      argNode.asExpr() = refNode.getArgument(0) and
-      refNode.getReceiver().getExpr().(MethodCall).getMethodName() = "local_assigns" and
-      argNode.getALocalSource().getConstantValue().isStringlikeValue(hashKey) and
-      erb = refNode.getFile()
-    )
-  }
-
-  private predicate isFlowFromLocals(DataFlow::Node node1, DataFlow::Node node2) {
-    exists(string hashKey, ErbFile erb |
-      // node1 is a `locals` argument to a render call...
-      renderCallLocals(hashKey, node1.asExpr().getExpr(), erb)
-    |
-      // node2 is an element reference against `local_assigns`
-      isFlowFromLocals0(node2.asExpr(), hashKey, erb)
-      or
-      // ...node2 is a "method call" to a "method" with `hashKey` as its name
-      // TODO: This may be a variable read in reality that we interpret as a method call
-      isMethodCall(node2.asExpr().getExpr(), hashKey, erb)
-    )
-  }
-
  /**
   * Holds if `action` contains an assignment of `value` to an instance
   * variable named `name`, in ERB file `erb`.
@@ -275,8 +235,6 @@ private module Shared {
   * An additional step that is preserves dataflow in the context of XSS.
   */
  predicate isAdditionalXssFlowStep(DataFlow::Node node1, DataFlow::Node node2) {
-    isFlowFromLocals(node1, node2)
-    or
    isFlowFromControllerInstanceVariable(node1, node2)
    or
    isFlowIntoHelperMethod(node1, node2)
--- a/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected
+++ b/ruby/ql/test/query-tests/security/cwe-079/ReflectedXSS.expected
@@ -15,14 +15,19 @@ edges
 | app/controllers/foo/bars_controller.rb:24:39:24:44 | call to params :  | app/controllers/foo/bars_controller.rb:24:39:24:59 | ...[...] :  |
 | app/controllers/foo/bars_controller.rb:24:39:24:59 | ...[...] :  | app/controllers/foo/bars_controller.rb:24:39:24:59 | ... = ... |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text |
-| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
-| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:8:9:8:21 | call to local_assigns [element :display_text] :  |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:12:9:12:21 | call to local_assigns [element :display_text] :  |
+| app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:18:15:18:27 | call to local_assigns [element :display_text] :  |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text |
 | app/controllers/foo/bars_controller.rb:26:53:26:54 | dt :  | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text :  |
 | app/controllers/foo/bars_controller.rb:30:11:30:16 | call to params :  | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] :  |
 | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] :  | app/controllers/foo/bars_controller.rb:31:5:31:7 | str |
+| app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text] :  | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
+| app/views/foo/bars/show.html.erb:8:9:8:21 | call to local_assigns [element :display_text] :  | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] |
+| app/views/foo/bars/show.html.erb:12:9:12:21 | call to local_assigns [element :display_text] :  | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] |
+| app/views/foo/bars/show.html.erb:18:15:18:27 | call to local_assigns [element :display_text] :  | app/views/foo/bars/show.html.erb:18:15:18:32 | ...[...] |
 | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text |
-| app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] |
+| app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  | app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text] :  |
 | app/views/foo/bars/show.html.erb:44:76:44:87 | call to display_text :  | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  |
 | app/views/foo/bars/show.html.erb:54:29:54:34 | call to params :  | app/views/foo/bars/show.html.erb:54:29:54:44 | ...[...] |
 | app/views/foo/bars/show.html.erb:57:13:57:18 | call to params :  | app/views/foo/bars/show.html.erb:57:13:57:28 | ...[...] |
@@ -47,11 +52,16 @@ nodes
 | app/controllers/foo/bars_controller.rb:30:11:30:28 | ...[...] :  | semmle.label | ...[...] :  |
 | app/controllers/foo/bars_controller.rb:31:5:31:7 | str | semmle.label | str |
 | app/views/foo/bars/_widget.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
+| app/views/foo/bars/_widget.html.erb:8:9:8:21 | call to local_assigns [element :display_text] :  | semmle.label | call to local_assigns [element :display_text] :  |
 | app/views/foo/bars/_widget.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/show.html.erb:2:18:2:30 | @user_website | semmle.label | @user_website |
 | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text | semmle.label | call to display_text |
+| app/views/foo/bars/show.html.erb:8:9:8:21 | call to local_assigns [element :display_text] :  | semmle.label | call to local_assigns [element :display_text] :  |
 | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | semmle.label | ...[...] |
+| app/views/foo/bars/show.html.erb:12:9:12:21 | call to local_assigns [element :display_text] :  | semmle.label | call to local_assigns [element :display_text] :  |
 | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | semmle.label | ...[...] |
+| app/views/foo/bars/show.html.erb:18:15:18:27 | call to local_assigns [element :display_text] :  | semmle.label | call to local_assigns [element :display_text] :  |
+| app/views/foo/bars/show.html.erb:18:15:18:32 | ...[...] | semmle.label | ...[...] |
 | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text | semmle.label | call to display_text |
 | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | semmle.label | @instance_text |
 | app/views/foo/bars/show.html.erb:44:64:44:87 | ... + ... :  | semmle.label | ... + ... :  |
@@ -76,6 +86,7 @@ subpaths
 | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/show.html.erb:5:9:5:20 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/show.html.erb:8:9:8:36 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/show.html.erb:12:9:12:26 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
+| app/views/foo/bars/show.html.erb:18:15:18:32 | ...[...] | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/show.html.erb:18:15:18:32 | ...[...] | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/show.html.erb:36:3:36:14 | call to display_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params :  | app/views/foo/bars/show.html.erb:41:3:41:16 | @instance_text | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:18:10:18:15 | call to params | user-provided value |
 | app/views/foo/bars/show.html.erb:47:5:47:13 | call to user_name | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params :  | app/views/foo/bars/show.html.erb:47:5:47:13 | call to user_name | Cross-site scripting vulnerability due to a $@. | app/controllers/foo/bars_controller.rb:9:12:9:17 | call to params | user-provided value |