Ruby: Add a query for CSRF protection not enabled

Specifically in Rails apps, we look for root ActionController classes
without a call to `protect_from_forgery`.

Author: hmac

ruby/ql/lib/codeql/ruby/frameworks/ActionController.qll

@@ -21,6 +21,35 @@ private import codeql.ruby.dataflow.internal.DataFlowDispatch
 module ActionController {
   // TODO: move the rest of this file inside this module.
   import codeql.ruby.frameworks.actioncontroller.Filters
+
+  /**
+   * An ActionController class which sits at the top of the class hierarchy.
+   * In other words, it does not subclass any other class in source code.
+   */
+  class RootController extends ActionControllerClass {
+    RootController() {
+      not exists(ActionControllerClass parent | this != parent and this = parent.getADescendent())
+    }
+  }
+
+  /**
+   * A call to `protect_from_forgery`.
+   */
+  class ProtectFromForgeryCall extends CsrfProtectionSetting::Range, DataFlow::CallNode {
+    ProtectFromForgeryCall() {
+      this = actionControllerInstance().getAMethodCall("protect_from_forgery")
+    }
+
+    private string getWithValueText() {
+      result = this.getKeywordArgument("with").getConstantValue().getSymbol()
+    }
+
+    // Calls without `with: :exception` can allow for bypassing CSRF protection
+    // in some scenarios.
+    override boolean getVerificationSetting() {
+      if this.getWithValueText() = "exception" then result = true else result = false
+    }
+  }
 }
 
 /**
@@ -48,18 +77,10 @@ deprecated class CookiesCall = Rails::CookiesCall;
  */
 class ActionControllerClass extends DataFlow::ClassNode {
   ActionControllerClass() {
-    this =
-      [
-        DataFlow::getConstant("ActionController").getConstant("Base"),
-        // In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we
-        // treat it separately in case the `ApplicationController` definition is not in the database.
-        DataFlow::getConstant("ApplicationController"),
-        // ActionController::Metal technically doesn't contain all of the
-        // methods available in Base, such as those for rendering views.
-        // However we prefer to be over-sensitive in this case in order to find
-        // more results.
-        DataFlow::getConstant("ActionController").getConstant("Metal")
-      ].getADescendentModule()
+    this = DataFlow::getConstant("ApplicationController").getADescendentModule()
+    or
+    this = actionControllerBaseClass().getADescendentModule() and
+    not exists(DataFlow::ModuleNode m | m = actionControllerBaseClass().asModule() | this = m)
   }
 
   /**
@@ -83,6 +104,20 @@ class ActionControllerClass extends DataFlow::ClassNode {
   }
 }
 
+private DataFlow::ConstRef actionControllerBaseClass() {
+  result =
+    [
+      // In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we
+      // treat it separately in case the `ApplicationController` definition is not in the database.
+      DataFlow::getConstant("ActionController").getConstant("Base"),
+      // ActionController::Metal technically doesn't contain all of the
+      // methods available in Base, such as those for rendering views.
+      // However we prefer to be over-sensitive in this case in order to find
+      // more results.
+      DataFlow::getConstant("ActionController").getConstant("Metal")
+    ]
+}
+
 private API::Node actionControllerInstance() {
   result = any(ActionControllerClass cls).getSelf().track()
 }
@@ -431,27 +466,6 @@ class ActionControllerSkipForgeryProtectionCall extends CsrfProtectionSetting::R
   override boolean getVerificationSetting() { result = false }
 }
 
-/**
- * A call to `protect_from_forgery`.
- */
-private class ActionControllerProtectFromForgeryCall extends CsrfProtectionSetting::Range,
-  DataFlow::CallNode
-{
-  ActionControllerProtectFromForgeryCall() {
-    this = actionControllerInstance().getAMethodCall("protect_from_forgery")
-  }
-
-  private string getWithValueText() {
-    result = this.getKeywordArgument("with").getConstantValue().getSymbol()
-  }
-
-  // Calls without `with: :exception` can allow for bypassing CSRF protection
-  // in some scenarios.
-  override boolean getVerificationSetting() {
-    if this.getWithValueText() = "exception" then result = true else result = false
-  }
-}
-
 /**
  * A call to `send_file`, which sends the file at the given path to the client.
  */

ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.qhelp

@@ -0,0 +1,65 @@
+<!DOCTYPE qhelp PUBLIC
+ "-//Semmle//qhelp//EN"
+ "qhelp.dtd">
+<qhelp>
+
+  <overview>
+    <p>
+      Cross-site request forgery (CSRF) is a type of vulnerability in which an
+      attacker is able to force a user to carry out an action that the user did
+      not intend.
+    </p>
+
+    <p>
+      The attacker tricks an authenticated user into submitting a request to the
+      web application. Typically this request will result in a state change on
+      the server, such as changing the user's password. The request can be
+      initiated when the user visits a site controlled by the attacker. If the
+      web application relies only on cookies for authentication, or on other
+      credentials that are automatically included in the request, then this
+      request will appear as legitimate to the server.
+    </p>
+
+    <p>
+      A common countermeasure for CSRF is to generate a unique token to be
+      included in the HTML sent from the server to a user. This token can be
+      used as a hidden field to be sent back with requests to the server, where
+      the server can then check that the token is valid and associated with the
+      relevant user session.
+    </p>
+  </overview>
+
+  <recommendation>
+    <p>
+      In the Rails web framework, CSRF protection is enabled by the adding a call to
+      the <code>protect_from_forgery</code> method inside an
+      <code>ActionController</code> class. Typically this is done in the
+      <code>ApplicationController</code> class, or an equivalent class from which
+      other controller classes are subclassed.
+
+      The default behaviour of this method is to null the session when an invalid
+      CSRF token is provided. This may not be sufficient to avoid a CSRF
+      vulnerability - for example if parts of the session are memoized. Calling
+      <code>protect_from_forgery with: :exception</code> can help to avoid this
+      by raising an exception on an invalid CSRF token instead.
+    </p>
+  </recommendation>
+
+  <example>
+    <p>
+      The following example shows a case where CSRF protection is enabled with
+      a secure request handling strategy of <code>:exception</code>.
+    </p>
+
+    <sample src="examples/ProtectFromForgeryGood.rb"/>
+
+  </example>
+
+  <references>
+    <li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Cross-site_request_forgery">Cross-site request forgery</a></li>
+    <li>OWASP: <a href="https://owasp.org/www-community/attacks/csrf">Cross-site request forgery</a></li>
+    <li>Securing Rails Applications: <a href="https://guides.rubyonrails.org/security.html#cross-site-request-forgery-csrf">Cross-Site Request Forgery (CSRF)</a></li>
+    <li>Veracode: <a href="https://www.veracode.com/blog/managing-appsec/when-rails-protectfromforgery-fails">When Rails' protect_from_forgery Fails</a>.</li>
+  </references>
+
+</qhelp>

ruby/ql/src/queries/security/cwe-352/CSRFProtectionNotEnabled.ql

@@ -0,0 +1,23 @@
+/**
+ * @name CSRF protection not enabled
+ * @description Not enabling CSRF protection may make the application
+ *              vulnerable to a Cross-Site Request Forgery (CSRF) attack.
+ * @kind problem
+ * @problem.severity warning
+ * @security-severity 8.8
+ * @precision high
+ * @id rb/csrf-protection-not-enabled
+ * @tags security
+ *       external/cwe/cwe-352
+ */
+
+import codeql.ruby.AST
+import codeql.ruby.Concepts
+import codeql.ruby.frameworks.ActionController
+
+from ActionController::RootController c
+where
+  not exists(ActionController::ProtectFromForgeryCall call |
+    c.getSelf().flowsTo(call.getReceiver())
+  )
+select c, "Potential CSRF vulnerability due to forgery protection not being enabled"

ruby/ql/src/queries/security/cwe-352/examples/ProtectFromForgeryGood.rb

@@ -0,0 +1,4 @@
+class ApplicationController < ActionController::Base
+  protect_from_forgery with: :exception
+end
+  

ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.expected

@@ -0,0 +1 @@
+| railsapp/app/controllers/alternative_root_controller.rb:1:1:3:3 | AlternativeRootController | Potential CSRF vulnerability due to forgery protection not being enabled |

ruby/ql/test/query-tests/security/cwe-352/CSRFProtectionNotEnabled.qlref

@@ -0,0 +1 @@
+queries/security/cwe-352/CSRFProtectionNotEnabled.ql

ruby/ql/test/query-tests/security/cwe-352/railsapp/app/controllers/alternative_root_controller.rb

@@ -0,0 +1,3 @@
+class AlternativeRootController < ActionController::Base
+    # BAD: no protect_from_forgery call
+end