Merge pull request github#7611 from github/ruby/protect_from_forgery-without-exception

Ruby: flag up `protect_from_forgery` calls without an exception strategy

Author: alexrford

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

@@ -297,3 +297,23 @@ class ActionControllerSkipForgeryProtectionCall extends CSRFProtectionSetting::R
 
   override boolean getVerificationSetting() { result = false }
 }
+
+/**
+ * A call to `protect_from_forgery`.
+ */
+private class ActionControllerProtectFromForgeryCall extends CSRFProtectionSetting::Range {
+  private ActionControllerContextCall callExpr;
+
+  ActionControllerProtectFromForgeryCall() {
+    callExpr = this.asExpr().getExpr() and
+    callExpr.getMethodName() = "protect_from_forgery"
+  }
+
+  private string getWithValueText() { result = callExpr.getKeywordArgument("with").getValueText() }
+
+  // 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
+  }
+}

ruby/ql/src/change-notes/2022-01-19-csrf-protection-weakened.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+lgtm,codescanning
+* The query `rb/csrf-protection-disabled` has been extended to find calls to the Rails method `protect_from_forgery` that may weaken CSRF protection.

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

@@ -50,12 +50,23 @@
       <code>skip_before_action</code>.
     </p>
 
+    <p>
+      Care should be taken when using the Rails
+      <code>protect_from_forgery</code> method to prevent CSRF. 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>
+
   </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/CSRFProtectionDisabled.ql

@@ -1,7 +1,7 @@
 /**
- * @name CSRF protection disabled
- * @description Disabling CSRF protection makes the application vulnerable to
- *              a Cross-Site Request Forgery (CSRF) attack.
+ * @name CSRF protection weakened or disabled
+ * @description Disabling or weakening CSRF protection may make the application
+ *              vulnerable to a Cross-Site Request Forgery (CSRF) attack.
  * @kind problem
  * @problem.severity warning
  * @security-severity 8.8
@@ -16,4 +16,4 @@ import codeql.ruby.Concepts
 
 from CSRFProtectionSetting s
 where s.getVerificationSetting() = false
-select s, "Potential CSRF vulnerability due to forgery protection being disabled."
+select s, "Potential CSRF vulnerability due to forgery protection being disabled or weakened."

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

@@ -1,4 +1,5 @@
-| railsapp/app/controllers/users_controller.rb:4:3:4:47 | call to skip_before_action | Potential CSRF vulnerability due to forgery protection being disabled. |
-| railsapp/config/application.rb:15:5:15:53 | call to allow_forgery_protection= | Potential CSRF vulnerability due to forgery protection being disabled. |
-| railsapp/config/environments/development.rb:5:3:5:51 | call to allow_forgery_protection= | Potential CSRF vulnerability due to forgery protection being disabled. |
-| railsapp/config/environments/production.rb:5:3:5:51 | call to allow_forgery_protection= | Potential CSRF vulnerability due to forgery protection being disabled. |
+| 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/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/railsapp/app/controllers/application_controller.rb

@@ -1,2 +1,20 @@
 class ApplicationController < ActionController::Base
+
+  # BAD: `protect_from_forgery` without `with: :exception` can expose an
+  # application to CSRF attacks in some circumstances
+  protect_from_forgery
+
+  before_action authz_guard
+
+  def current_user
+    @current_user ||= User.find_by_id(session[:user_id])
+  end
+
+  def logged_in?
+    !current_user.nil?
+  end
+
+  def authz_guard
+    render(plain: "not logged in") unless logged_in?
+  end
 end

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

@@ -0,0 +1,24 @@
+class ArticlesController < ApplicationController
+  prepend_before_action :user_authored_article?, only: [:delete_authored_article, :change_title]
+
+  # GOOD: `with: :exception` provides more effective CSRF protection than
+  # `with: :null_session` or `with: :reset_session`.
+  protect_from_forgery with: :exception, only: [:change_title]
+
+  def delete_authored_article
+    article.destroy
+  end
+
+  def change_title
+    article.title = params[:article_title]
+    article.save!
+  end
+
+  def article
+    @article ||= Article.find(params[:article_id])
+  end
+
+  def user_authored_article?
+    @article.author_id = current_user.id
+  end
+end

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

@@ -4,7 +4,7 @@ class UsersController < ApplicationController
   skip_before_action :verify_authenticity_token
 
   def change_email
-    user = User.find_by(name: params[:user_name])
+    user = current_user
     user.email = params[:new_email]
     user.save!
   end