Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Matthew Moore September 30th, 2008 @ 08:39 PM

  This happens because the HTTP_CONTENT_TYPE isn't specified for a delete request.

  However, HTTP_ACCEPT is set correctly to 'application/xml', which request_forgery_protection.rb plainly ignores.

  I vote that request_forgery_protection.rb should check HTTP_ACCEPT as well as HTTP_CONTENT_TYPE when checking if a request should be verified for forgery protection.

• You flagged this item as spam.
  

  Matthew Moore September 30th, 2008 @ 08:45 PM

  Also related ticket: SECURITY BUG - Request Forgery protection checks for 'Accept' header instead of 'Content-Type' header

• You flagged this item as spam.
  

  Matthew Moore September 30th, 2008 @ 08:51 PM

  • Assigned user set to “Rick”

• You flagged this item as spam.
  

  Mathijs Kwik October 1st, 2008 @ 07:06 AM

  Matthew,

  Please read the thread you linked to entirely (especially my may-1st post).

  The reason why we changed to content-type instead of accept-header is simple: A web-form can fake the accept type by using an extension in the form url (/posts.xml) and get through.

  Like I said before, RF-protection is only meant to protect against standard web forms being forged on malicious sites. Any other type of request is already protected. Ajax requests (asking for xml/json or html data) can't be forged, because browsers have the same-origin-policy (so the malicious site can only make your browser send malicious requests to itself). Flash can connect to every domain, but it can't tap into your browsers session/cookie data for other domains, so the requests can't spoof as a different user.

  Checking for accept instead of content brought another strange thing. It would mean that for ajax requests that post/put/delete something and want html in return(accept-header), the protection token is needed, but for exactly the same ajax request doing the same post/put/delete but wanting xml back as a result, no token is needed. This would be very strange behavior.

  But now on to your problem: I think the problem is activeresource setting no content type for delete requests, which is correct, since delete requests are empty (no body). No need to identify empty post bodies.

  since normal web forms won't be able to send requests without a content-type, we should see empty-content-type-requests as safe.

  So changing this:

  
    def verifiable_request_format?
      request.content_type.nil? || request.content_type.verify_request?
    end
  

  to this:

  
    def verifiable_request_format?
      !request.content_type.nil? && request.content_type.verify_request?
    end
  

  in ActionController::RequestForgeryProtection::ClassMethods

  this would fix your problem without harming RF-protection, since -like I said- web forms will always set www-formencoded as content-type, and this is the only type RF-protection needs to protect anyway.

• You flagged this item as spam.
  

  Jeff Cohen October 2nd, 2008 @ 09:00 PM

  So has anyone contributed a patch for this, or not? I don't see one attached to this ticket. I can try to do so next week if needed, but I don't want to repeat someone else's effort since there seems to have been a fair amount of discussion already.

• You flagged this item as spam.
  

  Matthew Moore October 2nd, 2008 @ 09:03 PM

  I will not be doing so any time soon, for the record. I would very much appreciate it if you were able to!

• You flagged this item as spam.
  

  Marcelo Barbudas October 27th, 2008 @ 01:06 PM

  • Tag changed from “request-forgery-protection, actionpack, activeresource, bug” to “actionpack, activeresource, bug, request-forgery-protection”

  I've encountered the same problem.

  Will this bug be fixed any time soon?

• You flagged this item as spam.
  

  Jeff Cohen October 29th, 2008 @ 12:23 AM

  Trying Mathjis's suggestion:

  !request.content_type.nil? && request.content_type.verify_request?

  did not work - it broke a lot of existing tests that expected exceptions that are not being raised anymore.

  I'll try to look into it this week, but I'm now a bit confused: are we worrying about the Accept header being empty, or the content format of the request, or both?

  Seems to me, if the request content format is empty, AND the verb is DELETE, AND the requested format is XML (the only case by which we have a real reproducible bug), THEN we should NOT check for forgery protection.

  Or, we'd have to change ActiveResource to somehow emit the content format as XML, but I don't think that makes sense, either.

  Any other thoughts before I charge ahead?

• You flagged this item as spam.
  

  Mathijs Kwik October 29th, 2008 @ 05:52 AM

  Well, you are right, checking for an empty content format + DELETE + request format XML is the thing that needs to get fixed now.

  But looking at the problem from a bit more distance, this solution feels like a patch, not a fix.

  The requested format doesn't have anything to do with it. Right now, the bug was found on activeresource, which requests xml. I'm pretty sure the same thing will happen when asking for json data. or any kind of data, since the problem is in the missing content format.

  So in that case we should be looking for empty content (header) + DELETE (http verb) and allow that one.

  The thing is, the way I see it, RF-protection is a bit over-protective in general (I know, better safe than sorry), since the only things that can get forged are standard (non-ajax) web forms, which always have the same content-type. So protecting all and then opening up all kinds of things that are allowed is a bit tedious here I guess and it would be a lot easier to check just for standard webforms. Maybe I'm wrong, in that case please point me to other ways attackers could forge requests.

  Anyway, my suggestion (or at least something close to that, I didn't test it) will work, I'm pretty sure of that. A lot of tests will break, like you noticed, so either those need to get fixed too (if all agree on trusting empty content(type)), or a more careful fix (like you proposed) can be crafted. I'm fine with that too, as long as we won't start blocking request because of an accept header (requested format) like it was a while back.

• 

  DHH October 30th, 2008 @ 10:41 AM

  • Milestone cleared.

  I agree that we should change this to only offer protection for html.

• You flagged this item as spam.
  

  Jeff Cohen October 31st, 2008 @ 02:06 AM

  Ok, I think I'm almost there, but I have a question about what we want to do with existing tests that no longer seem valid. Do I delete them, or change the assertion inside? For example, this existing test is no longer valid if we're only going to worry about non-ajax html forms:

  
    def test_should_not_allow_api_formatted_post_without_token
      assert_raises(ActionController::InvalidAuthenticityToken) do
        post :index, :format => 'xml'
      end
    end
  

  I was going to just delete all of these kinds of tests, but perhaps we'd prefer to change the assertion instead:

  
    def test_should_not_allow_api_formatted_post_without_token
      assert_nothing_raised do
        post :index, :format => 'xml'
      end
    end
  

  What's the Rails opinion on this sort of thing? I'd probably just toast the tests if it were me, but I thought I'd ask to be sure.

• You flagged this item as spam.
  

  Mathijs Kwik October 31st, 2008 @ 08:40 AM

  Interesting question, I can't answer that.

  For example, this existing test is no longer valid if we're only going to worry about non-ajax html forms:

  I think we should still check ajax html forms. We just don't care about xml/json/empty requests anymore. This basically boils down to protecting requests with www-formencoded content-type, no matter if they are submitted by ajax or by normal form.

  Strictly speaking checking for (non)ajax might work too, but it's a lot easier to just protect by content type, like it is now.

• You flagged this item as spam.
  

  Jeff Cohen October 31st, 2008 @ 02:23 PM

  Running into one annoyance, which is that the tests don't seem to set the default content type to html. So tests like this,

  
    def test_should_not_allow_post_without_token
      assert_raises(ActionController::InvalidAuthenticityToken) { post :index }
    end
  

  Are sending an empty content-type, which we normally want to ignore from token validation. But the intent of this test is to send a normal HTML POST request, so this test is broken.

  For some reason TestRequest doesn't populate with a default content-type of text/html; I wonder if I should do that, but now I'm getting in deeper than I had hoped to with 2.2 so close to shipping already.

  I'll try to continue to look at it today and see what I can do about this.

  It could be that I punt on this for now, and only change the existing code to let DELETE requests with empty content-type pass through without a token, which will at least allow ActiveResource to work again.

• You flagged this item as spam.
  

  Jeff Cohen November 1st, 2008 @ 04:53 AM

  • Tag changed from “actionpack, activeresource, bug, request-forgery-protection” to “actionpack, activeresource, bug, patch, request-forgery-protection”

  Ok, patch attached. Here's the final summary:

  • Changed Mime::Type#verify_request? to only worry about HTML-formatted content.
  • Only HTML and Ajax requests that are sending HTML (form encoded) content will be examined.
  • There was one test for when sessions are off that was actually passing for the wrong reason; I've commented it out because I'm not sure how to fix it.
  • Not much actually changed in the actual code - most of it was rewriting the tests.

  If someone could please look at the new tests and make sure they make sense, and give this ticket some +1's if everything's ok, that would be great. And if I messed something up, let me know.

  • csrf_html_ajax_only.diff 12.5 KB
• 

  Michael Koziarski November 1st, 2008 @ 04:51 PM

  So are we sure that flash can't forge requests with these content types?

  This change looks mostly sensible to me, but I'd really like to get the 'final word' on whether it's safe. Anyone have some references they can cite which shows it is?

• You flagged this item as spam.
  

  Mathijs Kwik November 2nd, 2008 @ 07:10 PM

  Flash can forge every kind of request they want, but as far as I know (don't have any reference, and I don't know flash, so I must have picked it up somewhere) flash can't tap into cookies/sessions for other domains, so in that case it can't pose as a user that happens to be logged in.

  We should check this to make sure though.

• You flagged this item as spam.
  

  Alex MacCaw November 2nd, 2008 @ 10:43 PM

  Mathijs Kwik, That's right - unless that domain has a crossdomain.xml file permitting it.

• You flagged this item as spam.
  

  Mathijs Kwik November 3rd, 2008 @ 12:22 AM

  @Alex

  Ok, but 'that domain' would happen to be the rails-site right? so an attacker can't set that. In case I want to enable it for my site, I should already make very very very sure how to authenticate requests and such. Also, (in rails 2.1 .. edge at least) if I happen to have a crossdomain.xml file, flash can already send xml/json requests (they aren't under RF-protection), so the changes discussed here won't be a regression.

  The way I see it is that most people don't want crossdomain.xml, and people that do use it (mashups and such) will already have to secure their 'service' in a lot of ways. Rails' RF-protection shouldn't intervene, because using crossdomain.xml means you want cross-site stuff.

  People can always use the helper methods and manually check request using rails' helper methods for generating/checking tokens in case they need it.

• 

  DHH November 4th, 2008 @ 04:59 PM

  Do we need to update any documentation as part of this?

• 

  Rick November 12th, 2008 @ 09:26 PM

  What does content type even have to do with CSRF? I've run into two cases where this assumption doesn't work.

  Exhibit A: I added an API call that uses form params. I didn't feel like dealing with xml or json, since the request as simple as /foo?bar=blah.

  Exhibit B: When adding a JSON widget to an application, I created a csrf vulnerability where anyone viewing the widget would 'log in' as the token the JSON widget was using. This just meant I had to ensure the session was disabled for API requests.

  session :off, :if => Proc.new { |req| req.format.json? }
  
  

  I bring this up because of this failure:

  
  test_verifiable_mime_types(MimeTypeTest)
      [controller/mime_type_test.rb:85:in `test_verifiable_mime_types'
       controller/mime_type_test.rb:85:in `each'
       controller/mime_type_test.rb:85:in `test_verifiable_mime_types'
       ./../../activesupport/lib/active_support/testing/setup_and_teardown.rb:94:in `__send__'
       ./../../activesupport/lib/active_support/testing/setup_and_teardown.rb:94:in `run']:
  Mime Type is not verified: :js.
  <nil> is not true.
  

  The Mime::JS is not verified because Mime::JS.html? == false. But, I consider ajax requests just as dangerous because they still use the cookie. Maybe that's a moot point though because of the ajax cross domain submissions though?

• 

  Rick November 12th, 2008 @ 09:35 PM

  Going with the assumption that I'm wrong here, I've fixed up two failing ActionPack tests. I'm attaching a patch. However, Mime::Type.unverifiable_types is now unused. Should it be deprecated? Since Mime::Type.html_types are all that is used to check for verified types, it's not really required anymore.

  • csrf_html_ajax_only_2.diff 13.9 KB
• 

  Michael Koziarski November 13th, 2008 @ 09:27 AM

  We can use the Content-Type of the request because browsers can't change it. They will only ever send html, url encoded or multipart.

  Flash can change that header, but only when the crossdomain.xml thing allows it

  Ajax requests can change it, but same origin policy protects us there.

  Java and the like can probably do all sorts, but they can't get at the cookie store

  Perhaps the real problem with understanding is that we're calling these 'html types' when in reality they're 'things the browser will send'? Mime::Type.browser_generated_types?

  +1 on deprecating the unverifiable_types

• 

  Repository November 13th, 2008 @ 10:24 AM

  • State changed from “new” to “committed”

  (from [f1ad8b48aae3ee26613b3e77bc0056e120096846]) Instead of overriding html_types, base the verification on browser_generated_types.

  Also Deprecate the old unverifiable types.

  [#1145 state:committed] http://github.com/rails/rails/co...

• 

  Michael Koziarski November 13th, 2008 @ 10:27 AM

  OK, so this is applied now. I rejigged the logic slightly in another commit, but it's mostly as jeff originally applied it with Rick's test fixes.

  So I think we're good to go, but let me know if we missed anything.

• You flagged this item as spam.
  

  Jeff Cohen November 13th, 2008 @ 01:48 PM

  Thanks everyone!

  I'm glad this not only got fixed, but it was certainly a community effort to get it done, which I personally think is pretty cool :-)

  Jeff

• 

  Michael Koziarski November 13th, 2008 @ 04:51 PM

  Indeed, great work everyone :)

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) November 16th, 2008 @ 04:54 PM

  • Assigned user changed from “Rick” to “Michael Koziarski”

  guys, i was browsing through the firefox source after reading this thread, and i believe you'll want to add 'text/plain' to the browser_generated_types:

  will get past the auth token check and allow a CSRF:

  http://mxr.mozilla.org/mozilla-c...

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) November 16th, 2008 @ 04:57 PM

  my bad, html got stripped...

  guys, i was browsing through the firefox source after reading this thread, and i believe you'll want to add 'text/plain' to the browser_generated_types:

  
  <form method="post" enctype="text/plain" action="/delete?user_id=5">
  

  will get past the auth token check and allow a CSRF:

  http://mxr.mozilla.org/mozilla-c...

• You flagged this item as spam.
  

  ronin-37814 (at lighthouseapp) November 16th, 2008 @ 05:55 PM

  same deal with webkit:

  http://trac.webkit.org/browser/r...

  I also verified this is an issue in rails 2.1, but not in 2.0.

• 

  Michael Koziarski November 16th, 2008 @ 07:45 PM

  Nice find, I've updated 2-2-stable and master.

  2-1-stable still uses @@unverifiable_types and will need a separate fix which I'll add shortly.

• You flagged this item as spam.
  

  Steve April 19th, 2009 @ 08:24 PM

  Sorry, I'm not sure I understand this comment: "Ajax requests can change it, but same origin policy protects us there."

  How does the same-origin policy protect us? We're not worried about valid requests, we're worried about forged requests. What's to stop me from forging a request to an ajax endpoint?
  

  Or, to put it differently, can't I get around the forgery protection simply by making a POST request where the content-type is not specified?

  (In point of fact, I can do that, am shocked that it just skips the whole "forgery protection".... and that's why I'm here reading this thread...)

• You flagged this item as spam.
  

  Steve April 19th, 2009 @ 08:41 PM

  My mistake - my test was on a file:// url and same origin policy doesn't apply there.

• You flagged this item as spam.
  

  Lisa32 January 16th, 2010 @ 01:51 PM

  • Tag cleared.

  You would hear about this post therefore, I propose to find the essays writers and buy essays moreover, that is real to see pre written essays.

• 

  Ryan Bigg October 9th, 2010 @ 09:52 PM

  • Importance changed from “” to “Low”

  Automatic cleanup of spam.