Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSRF protection from cross-origin <script> tags #13345

Merged
merged 1 commit into from Dec 17, 2013
Merged

Conversation

@jeremy
Copy link
Member

@jeremy jeremy commented Dec 17, 2013

Extend protect_from_forgery to cover GET requests with JavaScript responses, protecting apps from cross-origin <script> embedding that may leak sensitive data.

TODO

  • Changelog
  • Update Security guide
  • Test actions that use Accept rather than an explicit format
  • Test actions that had neither .format nor Accept
@jeremy
jeremy reviewed Dec 17, 2013
View changes
actionpack/lib/action_controller/metal/request_forgery_protection.rb Outdated
end

def non_xhr_javascript_response?
content_type =~ %r(\Atext/javascript) && !request.xhr?

This comment has been minimized.

@jeremy

jeremy Dec 17, 2013
Author Member

This content type comparison is gross but I couldn't find a better alternative.

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 17, 2013
Member

Can content type be some other like application/javascript?

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 17, 2013
Member

I think so.

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 17, 2013
Member

text/ecmascript and application/ecmascript are also valid values and I think there are more supported content types for javascript execution.

Whitelisting is not an option right?

This comment has been minimized.

@pothibo

pothibo Dec 17, 2013
Contributor

unless I'm mistaken, doesn't the Mime types already take care of this?

request.format == :js && !request.xhr?

This comment has been minimized.

@jeremy

jeremy Dec 17, 2013
Author Member

@pothibo request.format is the requested format, not the response Content-Type.

For example, in #test_should_only_allow_same_origin_js_get_with_xhr_header:

> [content_type, request.format, rendered_format]
=> ["text/javascript", #<Mime::Type:0x007ff4d96d15e8 @synonyms=["application/xhtml+xml"], @symbol=:html, @string="text/html">, #<Mime::NullType:0x007ff4d8438530>]

This comment has been minimized.

@jeremy

jeremy Dec 17, 2013
Author Member

@rafaelfranca You'll only get those Content-Type if you manually specify them with render ..., type: 'application/ecmascript'. Mime::JS will always be text/javascript.

We may want a whitelist to extend verification to other Content-Type (including JSON for those supporting ancient browsers) in the future, but I don't think it's necessary to start.

This comment has been minimized.

@acapilleri
Copy link
Contributor

@acapilleri acapilleri commented Dec 17, 2013

👍 Thanks!

@carlosantoniodasilva
carlosantoniodasilva reviewed Dec 17, 2013
View changes
actionpack/lib/action_controller/metal/request_forgery_protection.rb Outdated
@@ -89,6 +98,8 @@ def protect_from_forgery(options = {})
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
prepend_before_action :mark_for_same_origin_verification, options
append_after_action :verify_same_origin_request

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Dec 17, 2013
Member

If people can skip verify_authenticity_token in a controller that inherits protect_from_forgery, then mark_for_same_origin_verification would still be triggered and thus verify_same_origin_request too, not? Would they need to skip that too, independently?

This comment has been minimized.

@jeremy

jeremy Dec 17, 2013
Author Member

Yes. I'll move the marker to verify_authenticity_token to fix that.

I used a separate, private method because we encourage people to change the builtin method:

      # The actual before_action that is used. Modify this to change how you handle unverified requests.
      def verify_authenticity_token

So an overriding method would miss the marking and skip same-origin verification.

I'm removing that comment. Shouldn't be overriding that method.

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 17, 2013

Tests are broken 😢

@rafaelfranca
rafaelfranca reviewed Dec 17, 2013
View changes
actionpack/CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
* Extend protect_from_forgery to GET requests with JavaScript responses,

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 17, 2013
Member

Extend protect_from_forgery to GET requests with JavaScript responses,

@rafaelfranca
rafaelfranca reviewed Dec 17, 2013
View changes
actionpack/CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
* Extend protect_from_forgery to GET requests with JavaScript responses,
protecting apps from cross-origin <script> tags. #13345

This comment has been minimized.

@rafaelfranca

rafaelfranca Dec 17, 2013
Member

protecting apps from cross-origin <script> tags.

Thanks to @homakov for sounding the alarm about JSONP-style data leaking
@jeremy
Copy link
Member Author

@jeremy jeremy commented Dec 17, 2013

@rafaelfranca fixed the other broken tests, they hit the same-origin verification 😁

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Dec 17, 2013

:shipit:

@carlosantoniodasilva
Copy link
Member

@carlosantoniodasilva carlosantoniodasilva commented Dec 17, 2013

👍

jeremy added a commit that referenced this pull request Dec 17, 2013
CSRF protection from cross-origin <script> tags
@jeremy jeremy merged commit 39ca25f into rails:master Dec 17, 2013
@jeremy jeremy deleted the jeremy:get-csrf branch Dec 17, 2013
@homakov
Copy link
Contributor

@homakov homakov commented Dec 19, 2013

@jeremy probably instead of exception it should respond with "please send x requested with header to verify request origin"

@jeremy
Copy link
Member Author

@jeremy jeremy commented Dec 19, 2013

@homakov thought about that.. responding with 403 Forbidden so it's clear it's a client error not an application error

App devs can use rescue_from to do that today. But then they'd never get exception emails / notifications and they remain ignorant of the issue. We do that with CSRF also, and it feels strange for an app to ignore unauthorized requests. We just don't have a good convention or API for alerting devs about attacks that were thwarted. Could do that with an AS notification channel: publish security events, let apps/plugins consume them and report and alert.

In any case, this is starting as small as possible. I hope we'll see PRs that do a default rescue_from for this exception, or add separate response strategies like CSRF token verification handling uses.

@jeremy
Copy link
Member Author

@jeremy jeremy commented Apr 15, 2014

@TechFounder Sounds like you're making a GET request that has a JavaScript response, but you aren't passing the X-Requested-With request header. You'll need to track down where and why.

@harmdewit
Copy link

@harmdewit harmdewit commented Apr 22, 2014

In our production server i'm getting ActionController::InvalidCrossOriginRequest:Security warnings in my app for requests with format=*/*. The strange thing is that most requests are correctly called with format=js and some with the strange format=html and even fewer with format=*/*. Only the requests with format=*/* are raising the error and return status 500. I'm trying to reproduce requests with format=*/* but without success.

Am i seeing these inconsistencies because of browser differences? I'm using jquery-rails, is it incorrectly setting the dataType?

@masonforest
Copy link

@masonforest masonforest commented Jan 22, 2015

@harmdewit did you ever figure this out? I'm having the same issue.

@harmdewit
Copy link

@harmdewit harmdewit commented Jan 22, 2015

I'm not sure what the issue was after this long time but putting this into our ApplicationController seemed to fix it

+  def non_xhr_javascript_response?
+    unless request.get?
+      content_type =~ %r(\Atext/javascript) && !request.xhr?
+    end
+  end
@masonforest
Copy link

@masonforest masonforest commented Jan 22, 2015

Thanks @harmdewit! 😃

@masonforest
Copy link

@masonforest masonforest commented Jan 22, 2015

I'm not sure what is requesting format=/ either.

Another solution I came up with is to reorder the respond_to formats:

So I changed this:

respond_to do |format|
     format.js do
       ...
     end
     format.html
end

to this:

respond_to do |format|
     format.html
     format.js do
       ...
     end
end

That way format=/ will return html.

oliverguenther added a commit to oliverguenther/openproject that referenced this pull request Aug 10, 2015
rails/rails#13345 introduced CSRF protection
to gets with JavaScript responses.
myabc added a commit to myabc/openproject that referenced this pull request Aug 19, 2015
rails/rails#13345 introduced CSRF protection
to gets with JavaScript responses.
@flyfy1

This comment has been minimized.

What case it's trying to defence against? Why do we need to check if content_type =~ %r(\Atext/javascript)?

This comment has been minimized.

Copy link
Member Author

@jeremy jeremy replied Apr 10, 2017

This comment has been minimized.

Copy link

@flyfy1 flyfy1 replied Apr 11, 2017

Thanks, that's really helpful :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.