Lighthouse has a new layout. Prefer the old one? Return to the old layout, and switch back any time from the link at the top of each page.

This project is archived and is in readonly mode.

redirect_to skips generation of headers (security problem)

#3663

I'm using RestfulAuthentication and discovered a session hijacking issue related due to rails skipping the generation of "Cache-Control" headers during redirections. Every default deployment of rails sends headers to make responses non-cacheable which is a security-wise decision. But the problem is, as soon as you call "redirect_to" it does no longer generate these default headers BUT it still sends the "Set-Cookie" header if you accessed the session before (which you usually do if you create session-based authenticators and redirect back to a previous page). But http responses without "Cache-Control" headers are allowed to be cached by either reverse proxies or client caches - and this includes the "Set-Cookie" header!

What follows is a whole cascade of other problems:

  • "Location" headers become cachable (we loose control over if a request is to be redirected or not because the cache decides)
  • "Set-Cookie" headers become cacheable which opens Rails to session hijacking on redirects
  • ...

Counter measures:

  • Rails should not send a "Set-Cookie" handler if I only did a read access to the session[] hash (it won't change, why send again?)
  • Rails should not skip default headers on redirects
  • Rails should handle bad combinations of Set-Cookie and Cache-Control gracefully (except the user EXPLICITLY wants to make Set-Cookie cachable) and especially never generate those bad combinations
  • Rails should never skip sending default Cache-Control headers if Set-Cookie is to be send (which actually happens in current versions when you use redirect_to)

Quickfix:

Overwrite redirect_to_full_url in your ApplicationController like so:

class ApplicationController < ActionController::Base

  private

  def redirect_to_full_url(url, status)
    headers["Cache-Control"] = "private, no-cache, max-age=0, must-revalidate"
    super url, status
  end
end

Reported by Kai Krakow · January 5th, 2010 @ 10:46 PM

State: needs-more-info
Milestone: none
Assigned to: Andrew White Andrew White
Importance: Low

Activity

  1. Rohit Arondekar
    Rohit Arondekar
    • State changed from new to stale
    • Importance changed from to Low

    Marking ticket as stale. If this is still an issue please leave a comment with suggested changes, creating a patch with tests, rebasing an existing patch or just confirming the issue on a latest release or master/branches.

    If this is still relevant can you please move this to http://groups.google.com/group/rubyonrails-security Which is listed here http://rubyonrails.org/community for critical patches regarding security. Thanks!

    October 8th, 2010 @ 03:18 AM

  2. Kai Krakow
    Kai Krakow

    Currently I can still reproduce with Rails 2.3.8, I'll check later versions soon.

    October 11th, 2010 @ 05:44 PM

  3. Rohit Arondekar
    Rohit Arondekar
    • State changed from stale to incomplete
    • Assigned user set to Andrew White

    Kai, thanks for the quick response! :)

    October 14th, 2010 @ 10:19 AM

  4. Andrew White
    Andrew White
    • State changed from incomplete to needs-more-info

    I can't reproduce this in a test application in either 2.3.10 or 3.0.1 - here's the output I get:

    curl -I -b "_testapp2_session=BAh7BzoJcGFnZSIJdGVzdDoPc2Vzc2lvbl9pZCIlNzRiZGY3ZjRmY2QxZDA1OGFlNTlmMDg5MWEwNmY0Mzk%3D--56ad78acb2575e17b8d2131b2fa81b6f9332bee4"  http://127.0.0.1:3000/
    HTTP/1.1 302 Moved Temporarily
    Connection: close
    Date: Fri, 15 Oct 2010 09:01:36 GMT
    Location: http://127.0.0.1:3000/pages/test
    Content-Type: text/html; charset=utf-8
    X-Runtime: 2
    Content-Length: 98
    Set-Cookie: _testapp2_session=BAh7BzoJcGFnZSIJdGVzdDoPc2Vzc2lvbl9pZCIlNzRiZGY3ZjRmY2QxZDA1OGFlNTlmMDg5MWEwNmY0Mzk%3D--56ad78acb2575e17b8d2131b2fa81b6f9332bee4; path=/; HttpOnly
    Cache-Control: no-cache
    

    I do get the Set-Cookie header when just reading the session but there's also a Cache-Control: no-cache header. The session is just a hash stored in the Rack environment under the key 'rack.session' so there's no straightforward way of telling whether it's been modified so it's safer just to write it out again.

    Kai, if you're still seeing the problem can you please upload a sample app here or create a repo on GitHub.

    October 15th, 2010 @ 10:33 AM

  5. Kai Krakow
    Kai Krakow

    I'm pretty sure that Rails should send "Cache-Control: private" (which it does on normal, unredirected responses) to protect the Set-Cookie header from being disclosed in a non-private cache. See http://code.google.com/p/doctype/wiki/ArticleHttpCaching.

    However, I wonder why it sends a "no-cache" value - it never did that in my application: This seems to be a new behaviour since 2.3.8 (which I am currently using).

    Essentially, what the link says is that sending Set-Cookie (or Location) with a missing Cache-Control may make public caches store these headers for later responses from the cache. Rails should have counter-measures to prevent such cases if the user/programmer/developer does not implicitly create such situations.

    Although the document also says that special response codes like 302 would never be cached, I found that some caches (eg mod_cache in Apache) cache that.

    October 15th, 2010 @ 11:03 AM

  6. Kai Krakow
    Kai Krakow

    To reproduce, create a test application:

      $ rails test
      $ ruby script/generate controller Test index redirect
      $ rm public/index.html
      $ vim app/controllers/test_controller.rb
    

    Change TestController#redirect to "redirect_to '/test/index'"

      $ ruby script/server
    

    Now on another console run

      $ curl -I "http://localhost:3000/test/index"
      HTTP/1.1 200 OK
      [...]
      Cache-Control: private, max-age=0, must-revalidate
    

    Now compare the redirect action:

      $ curl -I "http://localhost:3000/test/redirect"
      HTTP/1.1 302 Moved Temporarily
      [...]
      Location: http://localhost:3000/test/index
      Cache-Control: no-cache
    

    It's no longer sending "Cache-Control: private". Previous versions of Rails sent no "Cache-Control" header at all in this situation (but still generated a Set-Cookie header!). I'm still thinking "no-cache" is not enough otherwise it would be default for non-redirected responses. (compare anti-patterns in the link I posted above)

    Edited by Rohit Arondekar for formatting.

    October 15th, 2010 @ 11:21 AM

  7. Kai Krakow
    Kai Krakow

    Sorry, lighthouse killed my formatting and there's no way to edit a post. sigh

    October 15th, 2010 @ 11:23 AM

  8. Rohit Arondekar
    Rohit Arondekar

    Fixed the formatting. And yes it's a real pain that you can't edit your own posts. :(

    October 15th, 2010 @ 11:27 AM

  9. Andrew White
    Andrew White

    I'm still getting a no-cache header with Rails 2.3.8 - is there a plugin that may be affecting things?

    The setting of no-cache for redirection seems logical to me. One of the use cases for redirecting is to redirect to a login page when a protected page (e.g. admin page) is accessed. If we used private then this would allow the browser to cache the response. The default max-age of zero would mean that in effect it would work but the explicit no-cache seems the right response and is what the link recommends.

    In terms of the anti-pattern you specify that relates to a missing expires header with a set-cookie header being cached with HTTP/1.0 proxies. In this case we are sending a 302 response which shouldn't be cached unless explicitly specified using cc:public or an expires header set in the future.

    As the comment points out at the bottom of the article, the chances of running into a HTTP/1.0 proxy that doesn't respond to cache-control headers is virtually zero anyway.

    October 15th, 2010 @ 12:35 PM

  10. Ryan Bigg
    Ryan Bigg
    • Tag cleared.

    Automatic cleanup of spam.

    October 19th, 2010 @ 08:33 AM

  11. Jeff Kreeftmeijer