Merge pull request #14308 from hmac/hmac-rb-csrf-not-enabled

Ruby: Add a query for CSRF protection not enabled

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

@@ -22,6 +22,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
+    }
+  }
 }
 
 /**
@@ -39,18 +68,12 @@ module ActionController {
  */
 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()
+    // In Rails applications `ApplicationController` typically extends `ActionController::Base`, but we
+    // treat it separately in case the `ApplicationController` definition is not in the database.
+    this = DataFlow::getConstant("ApplicationController").getADescendentModule()
+    or
+    this = actionControllerBaseClass().getADescendentModule() and
+    not exists(DataFlow::ModuleNode m | m = actionControllerBaseClass().asModule() | this = m)
   }
 
   /**
@@ -74,6 +97,18 @@ class ActionControllerClass extends DataFlow::ClassNode {
   }
 }
 
+private DataFlow::ConstRef actionControllerBaseClass() {
+  result =
+    [
+      DataFlow::getConstant("ActionController").getConstant("Base"),
+      // ActionController::Metal and ActionController::API technically don'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"),
+      DataFlow::getConstant("ActionController").getConstant("API")
+    ]
+}
+
 private API::Node actionControllerInstance() {
   result = any(ActionControllerClass cls).getSelf().track()
 }
@@ -407,27 +442,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/lib/codeql/ruby/frameworks/Gemfile.qll

@@ -0,0 +1,254 @@
+/**
+ * Provides classes and predicates for Gemfiles, including version constraint logic.
+ */
+
+private import codeql.ruby.AST
+
+/**
+ * Provides classes and predicates for Gemfiles, including version constraint logic.
+ */
+module Gemfile {
+  private File getGemfile() { result.getBaseName() = "Gemfile" }
+
+  /**
+   * A call to `gem` inside a gemfile. This defines a dependency. For example:
+   *
+   * ```rb
+   * gem "actionpack", "~> 7.0.0"
+   * ```
+   *
+   * This call defines a dependency on the `actionpack` gem, with version constraint `~> 7.0.0`.
+   * For detail on version constraints, see the `VersionConstraint` class.
+   */
+  class Gem extends MethodCall {
+    Gem() { this.getMethodName() = "gem" and this.getFile() = getGemfile() }
+
+    /**
+     * Gets the name of the gem in this version constraint.
+     */
+    string getName() { result = this.getArgument(0).getConstantValue().getStringlikeValue() }
+
+    /**
+     * Gets the `i`th version string for this gem. A single `gem` call may have multiple version constraints, for example:
+     *
+     * ```rb
+     * gem "json", "3.4.0", ">= 3.0"
+     * ```
+     */
+    string getVersionString(int i) {
+      result = this.getArgument(i + 1).getConstantValue().getStringlikeValue()
+    }
+
+    /**
+     * Gets a version constraint defined by this call.
+     */
+    VersionConstraint getAVersionConstraint() { result = this.getVersionString(_) }
+  }
+
+  private newtype TComparator =
+    TEq() or
+    TNeq() or
+    TGt() or
+    TLt() or
+    TGeq() or
+    TLeq() or
+    TPGeq()
+
+  /**
+   * A comparison operator in a version constraint.
+   */
+  private class Comparator extends TComparator {
+    string toString() { result = this.toSourceString() }
+
+    /**
+     * Gets the representation of the comparator in source code.
+     * This is defined separately so that we can change the `toString` implementation without breaking `parseConstraint`.
+     */
+    string toSourceString() {
+      this = TEq() and result = "="
+      or
+      this = TNeq() and result = "!="
+      or
+      this = TGt() and result = ">"
+      or
+      this = TLt() and result = "<"
+      or
+      this = TGeq() and result = ">="
+      or
+      this = TLeq() and result = "<="
+      or
+      this = TPGeq() and result = "~>"
+    }
+  }
+
+  bindingset[s]
+  private predicate parseExactVersion(string s, string version) {
+    version = s.regexpCapture("\\s*(\\d+\\.\\d+\\.\\d+)\\s*", 1)
+  }
+
+  bindingset[s]
+  private predicate parseConstraint(string s, Comparator c, string version) {
+    exists(string pattern | pattern = "(=|!=|>=?|<=?|~>)\\s+(.+)" |
+      c.toSourceString() = s.regexpCapture(pattern, 1) and version = s.regexpCapture(pattern, 2)
+    )
+  }
+
+  /**
+   * A version constraint in a `gem` call. This consists of a version number and an optional comparator, for example
+   * `>= 1.2.3`.
+   */
+  class VersionConstraint extends string {
+    Comparator comp;
+    string versionString;
+
+    VersionConstraint() {
+      this = any(Gem g).getVersionString(_) and
+      (
+        parseConstraint(this, comp, versionString)
+        or
+        parseExactVersion(this, versionString) and comp = TEq()
+      )
+    }
+
+    /**
+     * Gets the string defining the version number used in this constraint.
+     */
+    string getVersionString() { result = versionString }
+
+    /**
+     * Gets the `Version` used in this constraint.
+     */
+    Version getVersion() { result = this.getVersionString() }
+
+    /**
+     * Holds if `other` is a version which is strictly greater than the range described by this version constraint.
+     */
+    bindingset[other]
+    predicate before(string other) {
+      comp = TEq() and this.getVersion().before(other)
+      or
+      comp = TLt() and
+      (this.getVersion().before(other) or this.getVersion().equal(other))
+      or
+      comp = TLeq() and this.getVersion().before(other)
+      or
+      // ~> x.y.z <=> >= x.y.z && < x.(y+1).0
+      // ~> x.y <=> >= x.y && < (x+1).0
+      comp = TPGeq() and
+      exists(int thisMajor, int thisMinor, int otherMajor, int otherMinor |
+        thisMajor = this.getVersion().getMajor() and
+        thisMinor = this.getVersion().getMinor() and
+        exists(string maj, string mi | normalizeSemver(other, _, maj, mi, _) |
+          otherMajor = maj.toInt() and otherMinor = mi.toInt()
+        )
+      |
+        exists(this.getVersion().getPatch()) and
+        (
+          thisMajor < otherMajor
+          or
+          thisMajor = otherMajor and
+          thisMinor < otherMinor
+        )
+        or
+        not exists(this.getVersion().getPatch()) and
+        thisMajor < otherMajor
+      )
+      // if the comparator is > or >=, it has no upper bound and therefore isn't guaranteed to be before any other version.
+    }
+  }
+
+  /**
+   * A version number in a version constraint. For example, in the following code
+   *
+   * ```rb
+   * gem "json", ">= 3.4.5"
+   * ```
+   *
+   * The version is `3.4.5`.
+   */
+  private class Version extends string {
+    string normalized;
+
+    Version() {
+      this = any(Gem c).getAVersionConstraint().getVersionString() and
+      normalized = normalizeSemver(this)
+    }
+
+    /**
+     * Holds if this version is strictly before the version defined by `other`.
+     */
+    bindingset[other]
+    predicate before(string other) { normalized < normalizeSemver(other) }
+
+    /**
+     * Holds if this versino is equal to the version defined by `other`.
+     */
+    bindingset[other]
+    predicate equal(string other) { normalized = normalizeSemver(other) }
+
+    /**
+     * Holds if this version is strictly after the version defined by `other`.
+     */
+    bindingset[other]
+    predicate after(string other) { normalized > normalizeSemver(other) }
+
+    /**
+     * Holds if this version defines a patch number.
+     */
+    predicate hasPatch() { exists(getPatch(this)) }
+
+    /**
+     * Gets the major number of this version.
+     */
+    int getMajor() { result = getMajor(normalized).toInt() }
+
+    /**
+     * Gets the minor number of this version, if it exists.
+     */
+    int getMinor() { result = getMinor(normalized).toInt() }
+
+    /**
+     * Gets the patch number of this version, if it exists.
+     */
+    int getPatch() { result = getPatch(normalized).toInt() }
+  }
+
+  /**
+   * Normalizes a SemVer string such that the lexicographical ordering
+   * of two normalized strings is consistent with the SemVer ordering.
+   *
+   * Pre-release information and build metadata is not supported.
+   */
+  bindingset[orig]
+  private predicate normalizeSemver(
+    string orig, string normalized, string major, string minor, string patch
+  ) {
+    major = getMajor(orig) and
+    (
+      minor = getMinor(orig)
+      or
+      not exists(getMinor(orig)) and minor = "0"
+    ) and
+    (
+      patch = getPatch(orig)
+      or
+      not exists(getPatch(orig)) and patch = "0"
+    ) and
+    normalized = leftPad(major) + "." + leftPad(minor) + "." + leftPad(patch)
+  }
+
+  bindingset[orig]
+  private string normalizeSemver(string orig) { normalizeSemver(orig, result, _, _, _) }
+
+  bindingset[s]
+  private string getMajor(string s) { result = s.regexpCapture("(\\d+).*", 1) }
+
+  bindingset[s]
+  private string getMinor(string s) { result = s.regexpCapture("(\\d+)\\.(\\d+).*", 2) }
+
+  bindingset[s]
+  private string getPatch(string s) { result = s.regexpCapture("(\\d+)\\.(\\d+)\\.(\\d+).*", 3) }
+
+  bindingset[str]
+  private string leftPad(string str) { result = ("000" + str).suffix(str.length()) }
+}

ruby/ql/src/change-notes/2023-09-25-csrf-protection-not-enabled.md

@@ -0,0 +1,4 @@
+---
+category: newQuery
+---
+* Added a new query, `rb/csrf-protection-not-enabled`, to detect cases where Cross-Site Request Forgery protection is not enabled in Ruby on Rails controllers.

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,68 @@
+/**
+ * @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
+import codeql.ruby.frameworks.Gemfile
+import codeql.ruby.DataFlow
+
+/**
+ * Holds if a call to `protect_from_forgery` is made in the controller class `definedIn`,
+ * which is inherited by the controller class `child`. These classes may be the same.
+ */
+private predicate protectFromForgeryCall(
+  ActionControllerClass definedIn, ActionControllerClass child,
+  ActionController::ProtectFromForgeryCall call
+) {
+  definedIn.getSelf().flowsTo(call.getReceiver()) and child = definedIn.getADescendent()
+}
+
+/**
+ * Holds if the Gemfile for this application specifies a version of "rails" or "actionpack" < 5.2.
+ * Rails versions prior to 5.2 do not enable CSRF protection by default.
+ */
+private predicate railsPreVersion5_2() {
+  exists(Gemfile::Gem g |
+    g.getName() = ["rails", "actionpack"] and g.getAVersionConstraint().before("5.2")
+  )
+}
+
+private float getRailsConfigDefaultVersion() {
+  exists(DataFlow::CallNode config, DataFlow::CallNode loadDefaultsCall |
+    DataFlow::getConstant("Rails")
+        .getConstant("Application")
+        .getADescendentModule()
+        .getAnImmediateReference()
+        .flowsTo(config.getReceiver()) and
+    config.getMethodName() = "config" and
+    loadDefaultsCall.getReceiver() = config and
+    loadDefaultsCall.getMethodName() = "load_defaults" and
+    result = loadDefaultsCall.getArgument(0).getConstantValue().getFloat()
+  )
+}
+
+from ActionControllerClass c
+where
+  not protectFromForgeryCall(_, c, _) and
+  (
+    // Rails versions prior to 5.2 require CSRF protection to be explicitly enabled.
+    railsPreVersion5_2()
+    or
+    // For Rails >= 5.2, CSRF protection is enabled by default if there is a `load_defaults` call in the root application class
+    // which specifies a version >= 5.2.
+    not getRailsConfigDefaultVersion() >= 5.2
+  ) and
+  // Only generate alerts for the topmost controller in the tree.
+  not exists(ActionControllerClass parent | c = parent.getAnImmediateDescendent())
+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/library-tests/frameworks/gemfile/Gemfile

@@ -0,0 +1,9 @@
+source "https://rubygems.org"
+
+gem "rails", "7.0.0"
+gem "json", "~> 2.6.0"
+gem "jwt"
+
+gem "loofah", ">= 2"
+
+gem "invalid-version", "abc"

ruby/ql/test/library-tests/frameworks/gemfile/Gemfile.expected

@@ -0,0 +1,8 @@
+gemCalls
+| Gemfile:3:1:3:20 | call to gem | rails | 7.0.0 | 7.0.0 |
+| Gemfile:4:1:4:22 | call to gem | json | ~> 2.6.0 | 2.6.0 |
+| Gemfile:7:1:7:20 | call to gem | loofah | >= 2 | 2 |
+versionBefore
+| 2 | 2.6.0 |
+| 2 | 7.0.0 |
+| 2.6.0 | 7.0.0 |

ruby/ql/test/library-tests/frameworks/gemfile/Gemfile.ql

@@ -0,0 +1,17 @@
+import codeql.ruby.frameworks.Gemfile
+
+query predicate gemCalls(
+  Gemfile::Gem gem, string name, Gemfile::VersionConstraint constraint, string version
+) {
+  name = gem.getName() and
+  constraint = gem.getAVersionConstraint() and
+  version = constraint.getVersion()
+}
+
+query predicate versionBefore(string before, string after) {
+  exists(Gemfile::VersionConstraint c1, Gemfile::VersionConstraint c2 |
+    c1.getVersion() = before and c2.getVersion() = after
+  |
+    c1.getVersion().before(after)
+  )
+}

ruby/ql/test/library-tests/frameworks/gemfile/not_gemfile.rb

@@ -0,0 +1 @@
+gem "this-gem-not-in-gemfile", "1.2"

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

@@ -1,5 +1,5 @@
 | railsapp/app/controllers/application_controller.rb:5:3:5:22 | call to protect_from_forgery | Potential CSRF vulnerability due to forgery protection being disabled or weakened. |
 | railsapp/app/controllers/users_controller.rb:4:3:4:47 | call to skip_before_action | Potential CSRF vulnerability due to forgery protection being disabled or weakened. |
-| railsapp/config/application.rb:15:5:15:53 | call to allow_forgery_protection= | Potential CSRF vulnerability due to forgery protection being disabled or weakened. |
+| railsapp/config/application.rb:16:5:16:53 | call to allow_forgery_protection= | Potential CSRF vulnerability due to forgery protection being disabled or weakened. |
 | railsapp/config/environments/development.rb:5:3:5:51 | call to allow_forgery_protection= | Potential CSRF vulnerability due to forgery protection being disabled or weakened. |
 | railsapp/config/environments/production.rb:5:3:5:51 | call to allow_forgery_protection= | Potential CSRF vulnerability due to forgery protection being disabled or weakened. |

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

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

@@ -0,0 +1,3 @@
+class SubscriptionsController < AlternativeRootController
+    protect_from_forgery with: :exception
+end

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

@@ -0,0 +1,2 @@
+class TagsController < AlternativeRootController
+end

ruby/ql/test/query-tests/security/cwe-352/railsapp/config/application.rb

@@ -9,7 +9,8 @@ Bundler.require(*Rails.groups)
 module Railsapp
   class Application < Rails::Application
     # Initialize configuration defaults for originally generated Rails version.
-    config.load_defaults 6.0
+    # This defaults version does NOT enable CSRF protection by default.
+    config.load_defaults 5.1
 
     # BAD: Disabling forgery protection may open the application to CSRF attacks
     config.action_controller.allow_forgery_protection = false