RB: Experimental query to manually check request verb

ruby/ql/src/change-notes/2022-07-21-check-http-verb.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new experimental query, `rb/manually-checking-http-verb`, to detect cases when the HTTP verb for an incoming request is checked and then used as part of control flow.

ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qhelp

@@ -0,0 +1,23 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+  <overview>
+    <p>
+      Manually checking the HTTP request verb inside of a controller method can lead to
+      CSRF bypass if GET or HEAD requests are handled improperly.
+    </p>
+  </overview>
+  <recommendation>
+    <p>
+      It is better to use different controller methods for each resource/http verb combination
+      and configure the Rails routes in your application to call them accordingly.
+    </p>
+  </recommendation>
+
+  <references>
+    <li>
+      See https://guides.rubyonrails.org/routing.html for more information.
+    </li>
+  </references>
+</qhelp>

ruby/ql/src/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql

@@ -0,0 +1,96 @@
+/**
+ * @name Manually checking http verb instead of using built in rails routes and protections
+ * @description Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods.
+ * @kind path-problem
+ * @problem.severity error
+ * @security-severity 5.0
+ * @precision low
+ * @id rb/manually-checking-http-verb
+ * @tags security
+ */
+
+import ruby
+import codeql.ruby.DataFlow
+import codeql.ruby.controlflow.CfgNodes
+import codeql.ruby.frameworks.ActionController
+import codeql.ruby.TaintTracking
+import DataFlow::PathGraph
+
+// any `request` calls in an action method
+class Request extends DataFlow::CallNode {
+  Request() {
+    this.getMethodName() = "request" and
+    this.asExpr().getExpr().getEnclosingMethod() instanceof ActionControllerActionMethod
+  }
+}
+
+// `request.env`
+class RequestEnvMethod extends DataFlow::CallNode {
+  RequestEnvMethod() {
+    this.getMethodName() = "env" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+// `request.request_method`
+class RequestRequestMethod extends DataFlow::CallNode {
+  RequestRequestMethod() {
+    this.getMethodName() = "request_method" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+// `request.method`
+class RequestMethod extends DataFlow::CallNode {
+  RequestMethod() {
+    this.getMethodName() = "method" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+// `request.raw_request_method`
+class RequestRawRequestMethod extends DataFlow::CallNode {
+  RequestRawRequestMethod() {
+    this.getMethodName() = "raw_request_method" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+// `request.request_method_symbol`
+class RequestRequestMethodSymbol extends DataFlow::CallNode {
+  RequestRequestMethodSymbol() {
+    this.getMethodName() = "request_method_symbol" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+// `request.get?`
+class RequestGet extends DataFlow::CallNode {
+  RequestGet() {
+    this.getMethodName() = "get?" and
+    any(Request r).flowsTo(this.getReceiver())
+  }
+}
+
+class HttpVerbConfig extends TaintTracking::Configuration {
+  HttpVerbConfig() { this = "HttpVerbConfig" }
+
+  override predicate isSource(DataFlow::Node source) {
+    source instanceof RequestMethod or
+    source instanceof RequestRequestMethod or
+    source instanceof RequestEnvMethod or
+    source instanceof RequestRawRequestMethod or
+    source instanceof RequestRequestMethodSymbol or
+    source instanceof RequestGet
+  }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(ExprNodes::ConditionalExprCfgNode c | c.getCondition() = sink.asExpr()) or
+    exists(ExprNodes::CaseExprCfgNode c | c.getValue() = sink.asExpr())
+  }
+}
+
+from HttpVerbConfig config, DataFlow::PathNode source, DataFlow::PathNode sink
+where config.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods."

ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.expected

@@ -0,0 +1,32 @@
+edges
+| ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] :  |
+| ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] :  | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... |
+| ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... |
+| ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... |
+| ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... |
+| ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... |
+| ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] |
+nodes
+| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | semmle.label | call to get? |
+| ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | semmle.label | call to env :  |
+| ManuallyCheckHttpVerb.rb:11:14:11:42 | ...[...] :  | semmle.label | ...[...] :  |
+| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | semmle.label | call to request_method :  |
+| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | semmle.label | call to method :  |
+| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | semmle.label | call to raw_request_method :  |
+| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | semmle.label | call to request_method_symbol :  |
+| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | semmle.label | ... == ... |
+| ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | semmle.label | call to env :  |
+| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | semmle.label | ...[...] |
+subpaths
+#select
+| ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | ManuallyCheckHttpVerb.rb:4:8:4:19 | call to get? | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
+| ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | ManuallyCheckHttpVerb.rb:11:14:11:24 | call to env :  | ManuallyCheckHttpVerb.rb:12:8:12:22 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
+| ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | ManuallyCheckHttpVerb.rb:19:14:19:35 | call to request_method :  | ManuallyCheckHttpVerb.rb:20:8:20:22 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
+| ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | ManuallyCheckHttpVerb.rb:27:14:27:27 | call to method :  | ManuallyCheckHttpVerb.rb:28:8:28:22 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
+| ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | ManuallyCheckHttpVerb.rb:35:14:35:39 | call to raw_request_method :  | ManuallyCheckHttpVerb.rb:36:8:36:22 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
+| ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | ManuallyCheckHttpVerb.rb:51:16:51:44 | call to request_method_symbol :  | ManuallyCheckHttpVerb.rb:52:10:52:23 | ... == ... | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |
+| ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | ManuallyCheckHttpVerb.rb:59:10:59:20 | call to env :  | ManuallyCheckHttpVerb.rb:59:10:59:38 | ...[...] | Manually checking HTTP verbs is an indication that multiple requests are routed to the same controller action. This could lead to bypassing necessary authorization methods and other protections, like CSRF protection. Prefer using different controller actions for each HTTP method and relying Rails routing to handle mappting resources and verbs to specific methods. |

ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.qlref

@@ -0,0 +1 @@
+experimental/manually-check-http-verb/ManuallyCheckHttpVerb.ql

ruby/ql/test/query-tests/experimental/manually-check-http-verb/ManuallyCheckHttpVerb.rb

@@ -0,0 +1,117 @@
+class ExampleController < ActionController::Base
+  # Should find
+  def example_action
+    if request.get?
+      Resource.find(id: params[:example_id])
+    end
+  end
+
+  # Should find
+  def other_action
+    method = request.env['REQUEST_METHOD']
+    if method == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+  # Should find
+  def foo
+    method = request.request_method
+    if method == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+  # Should find
+  def bar
+    method = request.method
+    if method == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+  # Should find
+  def baz
+    method = request.raw_request_method
+    if method == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+  # Should not find
+  def baz2
+    method = request.raw_request_method
+    if some_other_function == "GET"
+      Resource.find(id: params[:id])
+    end
+  end
+
+    # Should find
+    def foobarbaz
+      method = request.request_method_symbol
+      if method == :GET
+        Resource.find(id: params[:id])
+      end
+    end
+
+  # Should find
+  def resource_action
+    case request.env['REQUEST_METHOD']
+    when "GET"
+      Resource.find(id: params[:id])
+    when "POST"
+      Resource.new(id: params[:id], details: params[:details])
+    end
+  end
+
+  # Should not find
+  def resource_action
+    case request.random_method
+    when "GET"
+      Resource.find(id: params[:id])
+    when "POST"
+      Resource.new(id: params[:id], details: params[:details])
+    end
+  end
+end
+
+class SafeController < ActionController::Base
+  # this class should have no hits because controllers rely on conventional Rails routes
+  def index
+    Resource.find(id: params[:id])
+  end
+
+  def create
+    Resource.new(id: params[:id], details: params[:details])
+  end
+
+  def update
+    Resource.update(id: params[:id], details: params[:details])
+  end
+
+  def delete
+    s = Resource.find(id: params[:id])
+    s.delete
+  end
+end
+
+# There should be no hits from this class because it does not inherit from ActionController
+class NotAController
+  def example_action
+    if request.get?
+      Resource.find(params[:example_id])
+    end
+  end
+
+  def resource_action
+    case env['REQUEST_METHOD']
+    when "GET"
+      Resource.find(params[:id])
+    when "POST"
+      Resource.new(params[:id], params[:details])
+    end
+  end
+end
+
+class Resource < ActiveRecord::Base
+end