Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Rack::Cache caches Set-Cookie response headers yielding potential security holes in apps #52

Merged
merged 7 commits into from

10 participants

@rmm5t

Rack::Cache v0.3.0 (see changelog) introduced the behavior such that it will cache "responses marked as explicitly public even when the request includes an Authorization or Cookie header."

We're having a difficult time understanding why a Set-Cookie header should ever be cached.

We have a Rails application that has a few fresh_when and stale? checks that also happen to have a Cookie header sent in the request because the cookie's domain matches. Specifically, this cookie is our Rails session_store, and we started to experience hijacked sessions between users.

These resources that end up getting a public cache control as part of the response also have the Set-Cookie header set and Rack::Cache will cache the cookies because the response was set explicitly public.

We'd like to propose preventing Rack::Cache from ever caching a Set-Cookie header. @rtomayko touched on this as a potential solution to a related problem that dealt with cached assets.

@rtomayko
Owner

Yeah, let's just special case Set-Cookie and be done with it. There's no case where it's useful to cache.

@rmm5t

@rtomayko Great. Are you leaning towards stripping the Set-Cookie header before writing the response to the cache or towards not caching anything with a Set-Cookie header?

Personally, I think I lean towards stripping the Set-Cookie header, but for comparison, the default behavior of Varnish is to avoid caching any response with a Set-Cookie header (though Varnish is configurable to strip the header before writing the response to the cache).

We just wrote a small piece of middleware today to strip the Set-Cookie header on cacheable resources, and it would be great to throw that away in favor of Rack::Cache working more securely out of the box.

@mloughran

+1 for stripping Set-Cookie – this has bitten me far too many times already.

I would say strip rather than replicating Varnish's behaviour (not caching) simply because Rails makes controlling the Set-Cookie header so very difficult.

@rtomayko
Owner

Definitely going to strip it out, most likely before it's written to cache storage.

FWIW, Varnish isn't a very good thing to base behavior on because it doesn't even attempt to conform to RFC 2616's definition of a cache. It only handles caching on expiration and is very much optimized for serving static content. Not that it isn't absolutely wonderful for that, but rack-cache has always tried to follow RFC 2616 to the letter. Not caching responses that have an explicit "Cache-Control: public" definitely feels like it goes against the spec. That header basically translates to "please cache me all over the place".

I'd be much more interested in how squid, akamai, and other spec loving caches handle this.

@rmm5t

@rtomayko, I did some limited research -- hoping it helps for future onlookers.

Squid: "The proper way to deal with Set-Cookie reply headers, according to RFC 2109 is to cache the whole object, EXCEPT the Set-Cookie header lines." In fairness, RFC 2109 muddies the water with a Cache-control: no-cache="set-cookie", which seems like more hassle than the correct approach. I would have written that spec to always strip the Set-Cookie header unless the Cache-control had something to explicitly state that you want the cookie cached.

Akamai: I had trouble finding a definitive answer here about their default behavior, but it looks like stripping the Set-Cookie header is at least configurable.

Nginx proxy_cache: Responses with a Set-Cookie header are uncacheable unless the header is configured as "ignored."

Apache mod_cache: Seems to default to caching Set-Cookie headers, but is configurable. The typical (and presumably recommended) configuration is to set it to ignore (i.e. strip) the Set-Cookie header.

In summary, stripping the Set-Cookie looks like the correct approach, and I think it's certainly the best default for a cache layer like Rack::Cache to take.

@rtomayko
Owner

Thanks for running that down. It definitely looks like stripping before going into cache is the way to go.

I'm not sure how soon I'll be able to look at this. If someone wants to take a crack at a patch it'd be greatly appreciated.

rmm5t added some commits
@rmm5t rmm5t Removed Gemfile.lock
Gemfile.lock was correctly in the .gitignore file, but was also committed to the
repository.  See
http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/

Furthermore, it was locked to a version of memcached that I could get to compile
on Lion.
c01c0a9
@rmm5t rmm5t Added `bundle exec` to the `bacon` calls for test and spec tasks 13a41c3
@rmm5t rmm5t Added ignore_headers configuration option
Defaults to ['Set-Cookie'] thereby stripping cookies from cacheable responses
566f5d5
@rmm5t rmm5t Removed all ignore_headers before writing to the cache
By default, this will strip the Set-Cookie response header before storing a
cacheable response.
d668c88
@rmm5t rmm5t Added 'ignore' to the trace if a response header was stripped 0ed9a12
@rmm5t

@rtomayko Please have a look at this pull-request to strip the Set-Cookie header on cacheable responses. Note, it was built upon pull request #53 (which was necessary before I could get the test suite working in my environment).

@rmm5t

I should also mention that I'm not entirely happy the the trace name "ignore" that I used in 0ed9a12. I was trying to stick to what seemed like a convention of using one-word trace names. I'm open to suggestions.

@saturnflyer

+1 thanks @rmm5t. radiant is bitten by this problem and I haven't had time to track it down.

lib/rack/cache/context.rb
@@ -269,6 +270,16 @@ def store(response)
record :store
end
+ # Remove all ignored response headers before writing to the cache.
+ def strip_ignore_headers(response)
+ stripped = false
+ ignore_headers.each do |name|
+ stripped ||= response.headers.delete(name)
+ response.headers.delete(name)
+ end
+ record :ignore if stripped
@nbibler
nbibler added a note

The seemingly double-call to response.headers.delete reads very strangely. I understand that it only gets called for the first time that it passes through (in theory, because the header should have a truthy value which would be returned and set, although for a nil header value that catch / logic would fail).

I think something more like:

def strip_ignore_headers(response)
  record :ignore if response.headers.reject! { |k,v| ignore_headers.include? k }
end

See the Hash#reject! docs for more info.

There may be some issues with String comparisons on the include? call since the ignore headers are set by a developer and therefore may be either symbols or strings, not to mention casing issues may arrise. But that's probably beyond the scope of this issue.

@rmm5t
rmm5t added a note

Whoops. Good catch @nbibler. That was the result of a messy/botched refactor on my part after getting the tests to pass. Cleanup forthcoming.

@rmm5t
rmm5t added a note

@nbibler, Oh, I just realized that the reject! doesn't cover everything because the headers hash is a case-insensitive HeaderHash object from Rack::Utils. New test and another refactor coming.

@nbibler
nbibler added a note

Hah! Yikes. Good catch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nbibler

:+1: for the pull request. My code comment is more of a clarity / maintenance / readability comment or thought than a "THIS [request] SHALL NOT PASS" statement. :)

Regarding the question of recording an :ignore activity: it could probably be named something more interesting, but since that's basically internal, I don't know that it's a significant problem than needs to be solved, immediately.

@joemsak

Should I still see Set-Cookie: in the curl 304 response when using this branch?

example:

curl -I http://www.theydrawandtravel.com -c cookies.txt
curl -I http://www.theydrawandtravel.com -b cookies.txt --header 'If-Modified-Since: Fri, 10 Feb 2012 18:24:07 GMT'

@nbibler

@joemsak I believe with this update you will still see the Set-Cookie header in the response, if you make an uncached query, but that header would not be cached - Set-Cookie is specifically stripped out - and therefore not served to the next client.

@joemsak
@nbibler

@joemsak Sure. If you make the first request with a cookie, which in theory would've been cached, previously. Then make a subsequent request from a new cookie-less session. That new request should not contain the previous session's Set-Cookie header. Of course, it may send a new Set-Cookie header, depending on your system configuration... but if it's served purely from cache, then there should be no cookie header, at all, for the new requester.

@joemsak

I see this in firebug:

No "set-cookie" -- does that confirm this is resolved for me? (no more crossed user sessions?)

Response Headers From Cache
Cache-Control max-age=300, public
Content-Length 507535
Content-Type text/html; charset=utf-8
Date Fri, 10 Feb 2012 19:09:43 GMT
Last-Modified Fri, 10 Feb 2012 18:53:50 GMT
Server thin 1.3.1 codename Triple Espresso
X-Rack-Cache pass
X-Runtime 0.378382
X-Ua-Compatible IE=Edge,chrome=1

@nbibler

As another point of reference: Amazon CloudFront (the Amazon CDN) originally intended to strip Set-Cookie headers, but they accidentally disabled the cookie header removal in a bug released along with the custom origins update - Nov 2010. Only "recently" (about 2 months ago) did they deploy a fix for it.

Caching is so much fun.

@nbibler

@joemsak Yeah, if no Set-Cookie header is being transmitted, then user sessions cannot be crossed/mixed up from that end point. :)

@rmm5t

@joemsak The curl responses in your example are getting bypassed by Rack::Cache for other reasons. Note the X-Rack-Cache: pass header. Your responses are not being cached by Rack::Cache. This is sometimes caused by an error with the entitystore. Do you have any errors in the logs?

@joemsak

@rmm5t based on @nbibler's response to my latest question (demonstrating that Set-Cookie is not in my cached response), I want to say we are out of the woods with the crisis we've been having for 2 days over this, thanks to your strip_set_cookie branch... tentatively... ; ) I've been claiming to be out of the woods a little too frequently with this client

lib/rack/cache/context.rb
@@ -269,6 +270,12 @@ def store(response)
record :store
end
+ # Remove all ignored response headers before writing to the cache.
+ def strip_ignore_headers(response)
+ stripped_values = ignore_headers.map { |name, value| response.headers.delete(name) }
@nbibler
nbibler added a note

I'm afraid you might've been right the first time through (before the "fix" commit). You're mapping an Array instance (ignore_headers, as seen in cache/options.rb line 149, is an array). So, it should just be:

def strip_ignore_headers(response)
  record :ignore if ignore_headers.map { |h| response.headers.delete(h) }.any?
end
@rmm5t
rmm5t added a note

Thanks. Brain fart on that one. Will reset and force push that commit away.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rtomayko rtomayko commented on the diff
lib/rack/cache/context.rb
@@ -269,6 +270,12 @@ def store(response)
record :store
end
+ # Remove all ignored response headers before writing to the cache.
+ def strip_ignore_headers(response)
+ stripped_values = ignore_headers.map { |name| response.headers.delete(name) }
+ record :ignore if stripped_values.any?
@rtomayko Owner

We might not even need to log this. When is it useful? Just to verify that the header is actually being stripped?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rtomayko
Owner

This looks great guys. Thanks so much. Merging.

@rtomayko rtomayko merged commit 2e3a64d into rtomayko:master
@chanks

Am I reading this right that the Set-Cookie header is stripped not only from the response set in the cache, but the response passed back to the client? This seems like a bad idea to me. I mean, it's inadvisable for a developer to change the session in an action that's intended to be publically cached, but if they do, that information should probably still make it back to the initial client (the one who caused the cache to be populated).

@rtomayko
Owner

That's a damn good point. Hmmmm.

@rmm5t

@chanks, @rtomayko, True. That's the current behavior, but I figured it wasn't an issue. I figured that if the response can be served from the cache for everyone else without the Cookie, then it should be able to be delivered without a Cookie to the original request. I do understand and empathize with @chanks's concern though.

The main reason stripping Set-Cookie headers was necessary in the first place is because Rails will aggressively include Set-Cookie responses on all responses after a session has been initiated. It does this with middleware making it somewhat of a burden to ensure public cacheable content does not have a Set-Cookie header. If nothing else, the automatic inclusion of Rack-Cache in production was potentially introducing an unexpected session security hole -- one that most people would not expect until it was too late. I feel that if you're explicitly setting a cacheable response, there probably shouldn't have been a Set-Cookie header there in the first place. This new feature in Rack-Cache just enforces that behavior by default.

@chanks

@rmm5t @rtomayko I've been considering this more. I think it's reasonable (and perhaps common?) for a developer to use a before/after filter across the entire app that affects the session (as a part of their authentication system, for example - think of checking and updating a user-specific token in the session and DB on each request, or after a given length of time has passed). It would, of course, also fire during actions that are intended to be publically cached.

Maybe it's fair to say that the developer should go out of their way to skip filters like that on publically-cached actions, but if they're unaware of the issue (or they forget) I think this will lead to strange bugs in production that they won't be able to reproduce in the development or test environments (since I believe Rails doesn't include Rack::Cache there by default).

@barttenbrinke

We encountered this yesterday in production. As this is a MAJOR security issue, why hasn't there been a gem release yet? And I assume this will be back ported to all old versions of the gem?

As for the response filtering of the active cache: we currently actively strip set cookie headers from anything coming from /asset with apache until this is resolved.

@barttenbrinke

@thisduck \0/ Thank you!

@rmm5t

@barttenbrinke @thisduck I'm not sure back porting is the best idea. Caching cookies was a known and documented feature of Rack::Cache before this change. I'd argue that this change is a backwards-incompatibility and needs to at least be part of a minor version bump.

At the end of the day, it's up to @rtomayko.

@thisduck

@rmm5t @barttenbrinke @rtomayko

There is a slight issue here with documentation and expectation. I'm also not sure that this is a well known feature.

The problem with caching cookies on a session based site is that the cookie sends the browser your session id. If this is cached for certain resources, then you have a major issue where your session id is being sent to every browser that is requesting the cached resource.

I would assume the default expectation is that any cached resources don't potentially exploit the session id. Seems like a fairly major issue. It's further complicated because it's very hard to debug. The default expectation (at least that I had) was that any caching layer along the way should strip cookies, precisely because of the session issue.

If you're on Heroku on Rails 3.1, rack-cache is enabled by default using 1.0.x. If I know this, I can scan for cached resources that return a Set-Cookie header and potentially steal someone's session. If you're using rail's default with entire cookie sessions, then I have your entire session data.

So even if it's a documented feature, in practice it's a major hole.

EDIT: of course you touched upon this in the initial post up there. But if I'm on Rails 3.1 and not updating any time soon to 3.2, then I might have a hijacking issue on my hands.

@rmm5t

@thisduck, Correct me if I'm mistaken, but Heroku doesn't enable rack-cache. Rails 3.1.3 (specifically actionpack) depends on rack-cache. Furthermore, actionpack 3.1.3 depends on rack-cache ~> 1.1. which means that you'll be able to use rack-cache 1.2+ (assuming that this change doesn't get pushed into a major version bump). After a new rack-cache release, all you will need to do is bundle update rack-cache in your app and redeploy. Actually, in theory, you should be able to do that now by specifying the following in your Gemfile:

gem "rack-cache", :git => "https://github.com/rtomayko/rack-cache.git"
@thisduck

@rmm5t

Aah, okay. Didn't know about rails 3.1.3.

But Rails 3.1 on Heroku does use rack-cache by default for action caching. You can see this in action by turned action caching on in dev or running in production mode.

@barttenbrinke

All I'm asking is to release a new gem as soon as possible, as 99% of the developers will not be aware of this issue.
Please try to prevent another Github/rails drama like we had this weekend.

@rtomayko
Owner

Yep. I'm dealing with this right now.

@rtomayko
Owner

Kind of not happy with this solution still to be honest. It fixes the cookie leak but doesn't address @chanks comment here:

#52 (comment)

I'm going ahead with a 1.2 release because I'm more concerned about the cookie leak but I'm almost positive this solution will change in a 1.3 or 2.0 release.

@barttenbrinke

+1 for @chanks comment. Maybe a warning in the 1.2 release notes to flush all cache would also help a lot.

@rtomayko
Owner

Alright, rack-cache 1.2 is now available on rubygems.org. Please let me know if this plugs all holes for you.

@foeken

So no backport in older versions? Hope people update those gems...

@jperkin jperkin referenced this pull request from a commit in joyent/pkgsrc
taca Update ruby-rack-cache package to 1.2.
## 1.2 / March 2012

  * Fix a cookie leak vulnerability effecting large numbers of Rails 3.x
    installs: rtomayko/rack-cache#52

  * Never 304 on PUT or POST requests.

  * Misc bundler and test tooling fixes.
194868d
@jsonn jsonn referenced this pull request from a commit in jsonn/pkgsrc
taca Update ruby-rack-cache package to 1.2.
## 1.2 / March 2012

  * Fix a cookie leak vulnerability effecting large numbers of Rails 3.x
    installs: rtomayko/rack-cache#52

  * Never 304 on PUT or POST requests.

  * Misc bundler and test tooling fixes.
f8be9b6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 10, 2012
  1. @rmm5t

    Removed Gemfile.lock

    rmm5t authored
    Gemfile.lock was correctly in the .gitignore file, but was also committed to the
    repository.  See
    http://yehudakatz.com/2010/12/16/clarifying-the-roles-of-the-gemspec-and-gemfile/
    
    Furthermore, it was locked to a version of memcached that I could get to compile
    on Lion.
  2. @rmm5t
  3. @rmm5t

    Added ignore_headers configuration option

    rmm5t authored
    Defaults to ['Set-Cookie'] thereby stripping cookies from cacheable responses
  4. @rmm5t

    Removed all ignore_headers before writing to the cache

    rmm5t authored
    By default, this will strip the Set-Cookie response header before storing a
    cacheable response.
  5. @rmm5t
  6. @rmm5t
  7. @rmm5t
This page is out of date. Refresh to see the latest.
View
22 Gemfile.lock
@@ -1,22 +0,0 @@
-PATH
- remote: .
- specs:
- rack-cache (1.0.3)
- rack (>= 0.4)
-
-GEM
- remote: http://rubygems.org/
- specs:
- bacon (1.1.0)
- dalli (1.0.5)
- memcached (1.3)
- rack (1.3.2)
-
-PLATFORMS
- ruby
-
-DEPENDENCIES
- bacon
- dalli
- memcached
- rack-cache!
View
4 Rakefile
@@ -15,13 +15,13 @@ end
desc 'Run specs with unit test style output'
task :test => FileList['test/*_test.rb'] do |t|
suite = t.prerequisites
- sh "bacon -q -I.:lib:test #{suite.join(' ')}", :verbose => false
+ sh "bundle exec bacon -q -I.:lib:test #{suite.join(' ')}", :verbose => false
end
desc 'Run specs with story style output'
task :spec => FileList['test/*_test.rb'] do |t|
suite = t.prerequisites
- sh "bacon -I.:lib:test #{suite.join(' ')}", :verbose => false
+ sh "bundle exec bacon -I.:lib:test #{suite.join(' ')}", :verbose => false
end
desc 'Generate test coverage report'
View
7 lib/rack/cache/context.rb
@@ -260,6 +260,7 @@ def fetch
# Write the response to the cache.
def store(response)
+ strip_ignore_headers(response)
metastore.store(@request, response, entitystore)
response.headers['Age'] = response.age.to_s
rescue Exception => e
@@ -269,6 +270,12 @@ def store(response)
record :store
end
+ # Remove all ignored response headers before writing to the cache.
+ def strip_ignore_headers(response)
+ stripped_values = ignore_headers.map { |name| response.headers.delete(name) }
+ record :ignore if stripped_values.any?
@rtomayko Owner

We might not even need to log this. When is it useful? Just to verify that the header is actually being stripped?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ end
+
def log_error(exception)
@env['rack.errors'].write("cache error: #{exception.message}\n#{exception.backtrace.join("\n")}\n")
end
View
9 lib/rack/cache/options.rb
@@ -78,6 +78,14 @@ def option_name(key)
# Default: 0
option_accessor :default_ttl
+ # Set of response headers that are removed before storing them in the
+ # cache. These headers are only removed for cacheable responses. For
+ # example, in most cases, it makes sense to prevent cookies from being
+ # stored in the cache.
+ #
+ # Default: ['Set-Cookie']
+ option_accessor :ignore_headers
+
# Set of request headers that trigger "private" cache-control behavior
# on responses that don't explicitly state whether the response is
# public or private via a Cache-Control directive. Applications that use
@@ -138,6 +146,7 @@ def initialize_options(options={})
'rack-cache.metastore' => 'heap:/',
'rack-cache.entitystore' => 'heap:/',
'rack-cache.default_ttl' => 0,
+ 'rack-cache.ignore_headers' => ['Set-Cookie'],
'rack-cache.private_headers' => ['Authorization', 'Cookie'],
'rack-cache.allow_reload' => false,
'rack-cache.allow_revalidate' => false,
View
35 test/context_test.rb
@@ -57,6 +57,7 @@
response.should.be.ok
cache.trace.should.include :miss
cache.trace.should.include :store
+ cache.trace.should.not.include :ignore
response.headers.should.include 'Age'
response.headers['Cache-Control'].should.equal 'public'
end
@@ -85,6 +86,40 @@
response.headers['Cache-Control'].should.equal 'private'
end
+ it 'does remove Set-Cookie response header from a cacheable response' do
+ respond_with 200, 'Cache-Control' => 'public', 'ETag' => '"FOO"', 'Set-Cookie' => 'TestCookie=OK'
+ get '/'
+
+ app.should.be.called
+ response.should.be.ok
+ cache.trace.should.include :store
+ cache.trace.should.include :ignore
+ response.headers['Set-Cookie'].should.be.nil
+ end
+
+ it 'does remove all configured ignore_headers from a cacheable response' do
+ respond_with 200, 'Cache-Control' => 'public', 'ETag' => '"FOO"', 'SET-COOKIE' => 'TestCookie=OK', 'X-Strip-Me' => 'Secret'
+ get '/', 'rack-cache.ignore_headers' => ['set-cookie', 'x-strip-me']
+
+ app.should.be.called
+ response.should.be.ok
+ cache.trace.should.include :store
+ cache.trace.should.include :ignore
+ response.headers['Set-Cookie'].should.be.nil
+ response.headers['x-strip-me'].should.be.nil
+ end
+
+ it 'does not remove Set-Cookie response header from a private response' do
+ respond_with 200, 'Cache-Control' => 'private', 'Set-Cookie' => 'TestCookie=OK'
+ get '/'
+
+ app.should.be.called
+ response.should.be.ok
+ cache.trace.should.not.include :store
+ cache.trace.should.not.include :ignore
+ response.headers['Set-Cookie'].should.equal 'TestCookie=OK'
+ end
+
it 'responds with 304 when If-Modified-Since matches Last-Modified' do
timestamp = Time.now.httpdate
respond_with do |req,res|
Something went wrong with that request. Please try again.