Merge pull request #9696 from thiggy1342/experimental-strong-params

RB: Experimental strong params query

ruby/ql/src/change-notes/2022-07-21-weak-params.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new experimental query, `rb/weak-params`, to detect cases when the rails strong parameters pattern isn't followed and values flow into persistent store writes.

ruby/ql/src/experimental/weak-params/WeakParams.qhelp

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      Directly checking request parameters without following a strong params
+      pattern can lead to unintentional avenues for injection attacks.
+    </p>
+  </overview>
+  <recommendation>
+    <p>
+      Instead of manually checking parameters from the `param` object, it is
+      recommended that you follow the strong parameters pattern established in
+      Rails: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html
+    </p>
+    <p>
+      In the strong parameters pattern, you are able to specify required and allowed
+      parameters for each action called by your controller methods. This acts as an
+      additional layer of data validation before being passed along to other areas
+      of your application, such as the model.
+    </p>
+  </recommendation>
+  
+  <references>
+  
+  </references>
+</qhelp>

ruby/ql/src/experimental/weak-params/WeakParams.ql

@@ -0,0 +1,61 @@
+/**
+ * @name Weak or direct parameter references are used
+ * @description Directly checking request parameters without following a strong params pattern can lead to unintentional avenues for injection attacks.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 5.0
+ * @precision medium
+ * @id rb/weak-params
+ * @tags security
+ */
+
+import ruby
+import codeql.ruby.Concepts
+import codeql.ruby.DataFlow
+import codeql.ruby.TaintTracking
+import codeql.ruby.frameworks.ActionController
+import DataFlow::PathGraph
+
+/**
+ * A call to `request` in an ActionController controller class.
+ * This probably refers to the incoming HTTP request object.
+ */
+class ActionControllerRequest extends DataFlow::Node {
+  ActionControllerRequest() {
+    exists(DataFlow::CallNode c |
+      c.asExpr().getExpr().getEnclosingModule() instanceof ActionControllerControllerClass and
+      c.getMethodName() = "request"
+    |
+      c.flowsTo(this)
+    )
+  }
+}
+
+/**
+ * A direct parameters reference that happens inside a controller class.
+ */
+class WeakParams extends DataFlow::CallNode {
+  WeakParams() {
+    this.getReceiver() instanceof ActionControllerRequest and
+    this.getMethodName() =
+      ["path_parameters", "query_parameters", "request_parameters", "GET", "POST"]
+  }
+}
+
+/**
+ * A Taint tracking config where the source is a weak params access in a controller and the sink
+ * is a method call of a model class
+ */
+class Configuration extends TaintTracking::Configuration {
+  Configuration() { this = "WeakParamsConfiguration" }
+
+  override predicate isSource(DataFlow::Node node) { node instanceof WeakParams }
+
+  // the sink is an instance of a Model class that receives a method call
+  override predicate isSink(DataFlow::Node node) { node = any(PersistentWriteAccess a).getValue() }
+}
+
+from Configuration config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "By exposing all keys in request parameters or by blindy accessing them, unintended parameters could be used and lead to mass-assignment or have other unexpected side-effects. It is safer to follow the 'strong parameters' pattern in Rails, which is outlined here: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html"

ruby/ql/test/query-tests/experimental/weak-params/WeakParams.expected

@@ -0,0 +1,20 @@
+edges
+| WeakParams.rb:5:28:5:53 | call to request_parameters :  | WeakParams.rb:5:28:5:59 | ...[...] |
+| WeakParams.rb:10:28:10:51 | call to query_parameters :  | WeakParams.rb:10:28:10:57 | ...[...] |
+| WeakParams.rb:15:28:15:39 | call to POST :  | WeakParams.rb:15:28:15:45 | ...[...] |
+| WeakParams.rb:20:28:20:38 | call to GET :  | WeakParams.rb:20:28:20:44 | ...[...] |
+nodes
+| WeakParams.rb:5:28:5:53 | call to request_parameters :  | semmle.label | call to request_parameters :  |
+| WeakParams.rb:5:28:5:59 | ...[...] | semmle.label | ...[...] |
+| WeakParams.rb:10:28:10:51 | call to query_parameters :  | semmle.label | call to query_parameters :  |
+| WeakParams.rb:10:28:10:57 | ...[...] | semmle.label | ...[...] |
+| WeakParams.rb:15:28:15:39 | call to POST :  | semmle.label | call to POST :  |
+| WeakParams.rb:15:28:15:45 | ...[...] | semmle.label | ...[...] |
+| WeakParams.rb:20:28:20:38 | call to GET :  | semmle.label | call to GET :  |
+| WeakParams.rb:20:28:20:44 | ...[...] | semmle.label | ...[...] |
+subpaths
+#select
+| WeakParams.rb:5:28:5:59 | ...[...] | WeakParams.rb:5:28:5:53 | call to request_parameters :  | WeakParams.rb:5:28:5:59 | ...[...] | By exposing all keys in request parameters or by blindy accessing them, unintended parameters could be used and lead to mass-assignment or have other unexpected side-effects. It is safer to follow the 'strong parameters' pattern in Rails, which is outlined here: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html |
+| WeakParams.rb:10:28:10:57 | ...[...] | WeakParams.rb:10:28:10:51 | call to query_parameters :  | WeakParams.rb:10:28:10:57 | ...[...] | By exposing all keys in request parameters or by blindy accessing them, unintended parameters could be used and lead to mass-assignment or have other unexpected side-effects. It is safer to follow the 'strong parameters' pattern in Rails, which is outlined here: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html |
+| WeakParams.rb:15:28:15:45 | ...[...] | WeakParams.rb:15:28:15:39 | call to POST :  | WeakParams.rb:15:28:15:45 | ...[...] | By exposing all keys in request parameters or by blindy accessing them, unintended parameters could be used and lead to mass-assignment or have other unexpected side-effects. It is safer to follow the 'strong parameters' pattern in Rails, which is outlined here: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html |
+| WeakParams.rb:20:28:20:44 | ...[...] | WeakParams.rb:20:28:20:38 | call to GET :  | WeakParams.rb:20:28:20:44 | ...[...] | By exposing all keys in request parameters or by blindy accessing them, unintended parameters could be used and lead to mass-assignment or have other unexpected side-effects. It is safer to follow the 'strong parameters' pattern in Rails, which is outlined here: https://api.rubyonrails.org/classes/ActionController/StrongParameters.html |

ruby/ql/test/query-tests/experimental/weak-params/WeakParams.qlref

@@ -0,0 +1 @@
+experimental/weak-params/WeakParams.ql

ruby/ql/test/query-tests/experimental/weak-params/WeakParams.rb

@@ -0,0 +1,40 @@
+class TestController < ActionController::Base
+  
+  # Should catch
+  def create
+    TestObject.create(foo: request.request_parameters[:foo])
+  end
+
+  # Should catch
+  def create_query
+    TestObject.create(foo: request.query_parameters[:foo])
+  end
+
+  # Should catch
+  def update_unsafe
+    TestObject.update(foo: request.POST[:foo])
+  end
+
+  # Should catch
+  def update_unsafe_get
+    TestObject.update(foo: request.GET[:foo])
+  end
+
+  # Should not catch
+  def update
+    TestObject.update(object_params)
+  end
+
+  # strong params method
+  def object_params
+    params.require(:uuid).permit(:notes)
+  end
+
+  # Should not catch
+  def test_non_sink
+    puts request.request_parameters
+  end
+end
+
+class TestObject < ActiveRecord::Base
+end