Merge pull request #13345 from jeremy/get-csrf

CSRF protection from cross-origin <script> tags

6 files changed, 143 insertions, 28 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index b4d3da3603..3324dfa623 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Extend cross-site request forgery (CSRF) protection to GET requests with
+    JavaScript responses, protecting apps from cross-origin `<script>` tags.
+
+    *Jeremy Kemper*
+
 *   Fix generating a path for engine inside a resources block.
 
     Fixes #8533.
diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb
index bd64b1f812..8cdb9a7655 100644
--- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb
+++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb
@@ -5,14 +5,24 @@ module ActionController #:nodoc:
   class InvalidAuthenticityToken < ActionControllerError #:nodoc:
   end
 
+  class InvalidCrossOriginRequest < ActionControllerError #:nodoc:
+  end
+
   # Controller actions are protected from Cross-Site Request Forgery (CSRF) attacks
   # by including a token in the rendered html for your application. This token is
   # stored as a random string in the session, to which an attacker does not have
   # access. When a request reaches your application, \Rails verifies the received
   # token with the token in the session. Only HTML and JavaScript requests are checked,
   # so this will not protect your XML API (presumably you'll have a different
-  # authentication scheme there anyway). Also, GET requests are not protected as these
-  # should be idempotent.
+  # authentication scheme there anyway).
+  #
+  # GET requests are not protected since they don't have side effects like writing
+  # to the database and don't leak sensitive information. JavaScript requests are
+  # an exception: a third-party site can use a <script> tag to reference a JavaScript
+  # URL on your site. When your JavaScript response loads on their site, it executes.
+  # With carefully crafted JavaScript on their end, sensitive data in your JavaScript
+  # response may be extracted. To prevent this, only XmlHttpRequest (known as XHR or
+  # Ajax) requests are allowed to make GET requests for JavaScript responses.
   #
   # It's important to remember that XML or JSON requests are also affected and if
   # you're building an API you'll need something like:
@@ -65,17 +75,16 @@ module ActionController #:nodoc:
     module ClassMethods
       # Turn on request forgery protection. Bear in mind that only non-GET, HTML/JavaScript requests are checked.
       #
+      #   class ApplicationController < ActionController::Base
+      #     protect_from_forgery
+      #   end
+      #
       #   class FooController < ApplicationController
       #     protect_from_forgery except: :index
       #
-      # You can disable csrf protection on controller-by-controller basis:
-      #
+      # You can disable CSRF protection on controller by skipping the verification before_action:
       #   skip_before_action :verify_authenticity_token
       #
-      # It can also be disabled for specific controller actions:
-      #
-      #   skip_before_action :verify_authenticity_token, except: [:create]
-      #
       # Valid Options:
       #
       # * <tt>:only/:except</tt> - Passed to the <tt>before_action</tt> call. Set which actions are verified.
@@ -89,6 +98,7 @@ module ActionController #:nodoc:
         self.forgery_protection_strategy = protection_method_class(options[:with] || :null_session)
         self.request_forgery_protection_token ||= :authenticity_token
         prepend_before_action :verify_authenticity_token, options
+        append_after_action :verify_same_origin_request
       end
 
       private
@@ -169,18 +179,56 @@ module ActionController #:nodoc:
     end
 
     protected
+      # The actual before_action that is used to verify the CSRF token.
+      # Don't override this directly. Provide your own forgery protection
+      # strategy instead. If you override, you'll disable same-origin
+      # `<script>` verification.
+      #
+      # Lean on the protect_from_forgery declaration to mark which actions are
+      # due for same-origin request verification. If protect_from_forgery is
+      # enabled on an action, this before_action flags its after_action to
+      # verify that JavaScript responses are for XHR requests, ensuring they
+      # follow the browser's same-origin policy.
+      def verify_authenticity_token
+        @marked_for_same_origin_verification = true
+
+        if !verified_request?
+          logger.warn "Can't verify CSRF token authenticity" if logger
+          handle_unverified_request
+        end
+      end
+
       def handle_unverified_request
         forgery_protection_strategy.new(self).handle_unverified_request
       end
 
-      # The actual before_action that is used. Modify this to change how you handle unverified requests.
-      def verify_authenticity_token
-        unless verified_request?
-          logger.warn "Can't verify CSRF token authenticity" if logger
-          handle_unverified_request
+      CROSS_ORIGIN_JAVASCRIPT_WARNING = "Security warning: an embedded " \
+        "<script> tag on another site requested protected JavaScript. " \
+        "If you know what you're doing, go ahead and disable forgery " \
+        "protection on this action to permit cross-origin JavaScript embedding."
+      private_constant :CROSS_ORIGIN_JAVASCRIPT_WARNING
+
+      # If `verify_authenticity_token` was run (indicating that we have
+      # forgery protection enabled for this request) then also verify that
+      # we aren't serving an unauthorized cross-origin response.
+      def verify_same_origin_request
+        if marked_for_same_origin_verification? && non_xhr_javascript_response?
+          logger.warn CROSS_ORIGIN_JAVASCRIPT_WARNING if logger
+          raise ActionController::InvalidCrossOriginRequest, CROSS_ORIGIN_JAVASCRIPT_WARNING
         end
       end
 
+      # If the `verify_authenticity_token` before_action ran, verify that
+      # JavaScript responses are only served to same-origin GET requests.
+      def marked_for_same_origin_verification?
+        defined? @marked_for_same_origin_verification
+      end
+
+      # Check for cross-origin JavaScript responses.
+      def non_xhr_javascript_response?
+        content_type =~ %r(\Atext/javascript) && !request.xhr?
+      end
+
       # Returns true or false if a request is verified. Checks:
       #
       # * is it a GET or HEAD request?  Gets should be safe and idempotent
diff --git a/actionpack/test/controller/render_js_test.rb b/actionpack/test/controller/render_js_test.rb
index f070109b27..d550422a2f 100644
--- a/actionpack/test/controller/render_js_test.rb
+++ b/actionpack/test/controller/render_js_test.rb
@@ -22,7 +22,7 @@ class RenderJSTest < ActionController::TestCase
   tests TestController
 
   def test_render_vanilla_js
-    get :render_vanilla_js_hello
+    xhr :get, :render_vanilla_js_hello
     assert_equal "alert('hello')", @response.body
     assert_equal "text/javascript", @response.content_type
   end
diff --git a/actionpack/test/controller/render_json_test.rb b/actionpack/test/controller/render_json_test.rb
index 7c0a6bd67e..de8d1cbd9b 100644
--- a/actionpack/test/controller/render_json_test.rb
+++ b/actionpack/test/controller/render_json_test.rb
@@ -100,13 +100,13 @@ class RenderJsonTest < ActionController::TestCase
   end
 
   def test_render_json_with_callback
-    get :render_json_hello_world_with_callback
+    xhr :get, :render_json_hello_world_with_callback
     assert_equal 'alert({"hello":"world"})', @response.body
     assert_equal 'text/javascript', @response.content_type
   end
 
   def test_render_json_with_custom_content_type
-    get :render_json_with_custom_content_type
+    xhr :get, :render_json_with_custom_content_type
     assert_equal '{"hello":"world"}', @response.body
     assert_equal 'text/javascript', @response.content_type
   end
diff --git a/actionpack/test/controller/request_forgery_protection_test.rb b/actionpack/test/controller/request_forgery_protection_test.rb
index 727db79241..f1ed545205 100644
--- a/actionpack/test/controller/request_forgery_protection_test.rb
+++ b/actionpack/test/controller/request_forgery_protection_test.rb
@@ -52,18 +52,36 @@ module RequestForgeryProtectionActions
     render :inline => "<%= form_for(:some_resource, :remote => true, :authenticity_token => 'external_token') {} %>"
   end
 
+  def same_origin_js
+    render js: 'foo();'
+  end
+
+  def negotiate_same_origin
+    respond_to do |format|
+      format.js { same_origin_js }
+    end
+  end
+
+  def cross_origin_js
+    same_origin_js
+  end
+
+  def negotiate_cross_origin
+    negotiate_same_origin
+  end
+
   def rescue_action(e) raise e end
 end
 
 # sample controllers
 class RequestForgeryProtectionControllerUsingResetSession < ActionController::Base
   include RequestForgeryProtectionActions
-  protect_from_forgery :only => %w(index meta), :with => :reset_session
+  protect_from_forgery :only => %w(index meta same_origin_js negotiate_same_origin), :with => :reset_session
 end
 
 class RequestForgeryProtectionControllerUsingException < ActionController::Base
   include RequestForgeryProtectionActions
-  protect_from_forgery :only => %w(index meta), :with => :exception
+  protect_from_forgery :only => %w(index meta same_origin_js negotiate_same_origin), :with => :exception
 end
 
 class RequestForgeryProtectionControllerUsingNullSession < ActionController::Base
@@ -201,7 +219,7 @@ module RequestForgeryProtectionTests
   end
 
   def test_should_not_allow_post_without_token_irrespective_of_format
-    assert_blocked { post :index, :format=>'xml' }
+    assert_blocked { post :index, format: 'xml' }
   end
 
   def test_should_not_allow_patch_without_token
@@ -271,6 +289,38 @@ module RequestForgeryProtectionTests
     end
   end
 
+  def test_should_only_allow_same_origin_js_get_with_xhr_header
+    assert_cross_origin_blocked { get :same_origin_js }
+    assert_cross_origin_blocked { get :same_origin_js, format: 'js' }
+    assert_cross_origin_blocked do
+      @request.accept = 'text/javascript'
+      get :negotiate_same_origin
+    end
+
+    assert_cross_origin_not_blocked { xhr :get, :same_origin_js }
+    assert_cross_origin_not_blocked { xhr :get, :same_origin_js, format: 'js' }
+    assert_cross_origin_not_blocked do
+      @request.accept = 'text/javascript'
+      xhr :get, :negotiate_same_origin
+    end
+  end
+
+  def test_should_only_allow_cross_origin_js_get_without_xhr_header_if_protection_disabled
+    assert_cross_origin_not_blocked { get :cross_origin_js }
+    assert_cross_origin_not_blocked { get :cross_origin_js, format: 'js' }
+    assert_cross_origin_not_blocked do
+      @request.accept = 'text/javascript'
+      get :negotiate_cross_origin
+    end
+
+    assert_cross_origin_not_blocked { xhr :get, :cross_origin_js }
+    assert_cross_origin_not_blocked { xhr :get, :cross_origin_js, format: 'js' }
+    assert_cross_origin_not_blocked do
+      @request.accept = 'text/javascript'
+      xhr :get, :negotiate_cross_origin
+    end
+  end
+
   def assert_blocked
     session[:something_like_user_id] = 1
     yield
@@ -282,6 +332,16 @@ module RequestForgeryProtectionTests
     assert_nothing_raised { yield }
     assert_response :success
   end
+
+  def assert_cross_origin_blocked
+    assert_raises(ActionController::InvalidCrossOriginRequest) do
+      yield
+    end
+  end
+
+  def assert_cross_origin_not_blocked
+    assert_not_blocked { yield }
+  end
 end
 
 # OK let's get our test on
@@ -305,13 +365,13 @@ class RequestForgeryProtectionControllerUsingResetSessionTest < ActionController
   end
 end
 
-class NullSessionDummyKeyGenerator
-  def generate_key(secret)
-    '03312270731a2ed0d11ed091c2338a06'
+class RequestForgeryProtectionControllerUsingNullSessionTest < ActionController::TestCase
+  class NullSessionDummyKeyGenerator
+    def generate_key(secret)
+      '03312270731a2ed0d11ed091c2338a06'
+    end
   end
-end
 
-class RequestForgeryProtectionControllerUsingNullSessionTest < ActionController::TestCase  
   def setup
     @request.env[ActionDispatch::Cookies::GENERATOR_KEY] = NullSessionDummyKeyGenerator.new
   end
@@ -375,8 +435,8 @@ end
 
 class CustomAuthenticityParamControllerTest < ActionController::TestCase
   def setup
-    ActionController::Base.request_forgery_protection_token = :custom_token_name
     super
+    ActionController::Base.request_forgery_protection_token = :custom_token_name
   end
 
   def teardown
diff --git a/guides/source/security.md b/guides/source/security.md
index c698959a2c..21cc3deb8a 100644
--- a/guides/source/security.md
+++ b/guides/source/security.md
@@ -230,13 +230,15 @@ Or the attacker places the code into the onmouseover event handler of an image:
 <img src="http://www.harmless.com/img" width="400" height="400" onmouseover="..." />
 ```
 
-There are many other possibilities, including Ajax to attack the victim in the background.
The _solution to this is including a security token in non-GET requests_ which check on the server-side. In Rails 2 or higher, this is a one-liner in the application controller:
+There are many other possibilities, like using a `<script>` tag to make a cross-site request to a URL with a JSONP or JavaScript response. The response is executable code that the attacker can find a way to run, possibly extracting sensitive data. To protect against this data leakage, we disallow cross-site `<script>` tags. Only Ajax requests may have JavaScript responses since XmlHttpRequest is subject to the browser Same-Origin policy - meaning only your site can initiate the request.
+
+To protect against all other forged requests, we introduce a _required security token_ that our site knows but other sites don't know. We include the security token in requests and verify it on the server. This is a one-liner in your application controller:
 
 ```ruby
-protect_from_forgery secret: "123456789012345678901234567890..."
+protect_from_forgery
 ```
 
-This will automatically include a security token, calculated from the current session and the server-side secret, in all forms and Ajax requests generated by Rails. You won't need the secret, if you use CookieStorage as session storage. If the security token doesn't match what was expected, the session will be reset. **Note:** In Rails versions prior to 3.0.4, this raised an `ActionController::InvalidAuthenticityToken` error.
+This will automatically include a security token in all forms and Ajax requests generated by Rails. If the security token doesn't match what was expected, the session will be reset.
 
 It is common to use persistent cookies to store user information, with `cookies.permanent` for example. In this case, the cookies will not be cleared and the out of the box CSRF protection will not be effective. If you are using a different cookie store than the session for this information, you must handle what to do with it yourself: