Rack::Cache caches Set-Cookie response headers yielding potential security holes in apps #52
Conversation
|
Yeah, let's just special case |
|
@rtomayko Great. Are you leaning towards stripping the Personally, I think I lean towards stripping the We just wrote a small piece of middleware today to strip the |
|
+1 for stripping I would say strip rather than replicating Varnish's behaviour (not caching) simply because Rails makes controlling the Set-Cookie header so very difficult. |
|
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. |
|
@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 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. |
|
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. |
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.
Defaults to ['Set-Cookie'] thereby stripping cookies from cacheable responses
By default, this will strip the Set-Cookie response header before storing a cacheable response.
|
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. |
|
+1 thanks @rmm5t. radiant is bitten by this problem and I haven't had time to track it down. |
| stripped ||= response.headers.delete(name) | ||
| response.headers.delete(name) | ||
| end | ||
| record :ignore if stripped |
There was a problem hiding this comment.
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 }
endSee 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.
There was a problem hiding this comment.
Whoops. Good catch @nbibler. That was the result of a messy/botched refactor on my part after getting the tests to pass. Cleanup forthcoming.
There was a problem hiding this comment.
@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.
|
👍 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 |
|
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 |
|
@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. |
|
Is there a reliable test to be sure this is working correctly? |
|
@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. |
|
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 |
|
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. |
|
@joemsak Yeah, if no |
|
@joemsak The curl responses in your example are getting bypassed by Rack::Cache for other reasons. Note the |
|
@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 |
| @@ -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) } | |||
There was a problem hiding this comment.
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?
endThere was a problem hiding this comment.
Thanks. Brain fart on that one. Will reset and force push that commit away.
|
This looks great guys. Thanks so much. Merging. |
Rack::Cache caches Set-Cookie response headers yielding potential security holes in apps
|
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). |
|
That's a damn good point. Hmmmm. |
|
@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. |
|
@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). |
|
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 I've back ported this to 1.0(.3) here: https://github.com/thisduck/rack-cache/tree/backport_set_cookie_fix_to_1.0 |
|
@thisduck \0/ Thank you! |
|
@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. |
|
@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. |
|
@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 |
|
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. |
|
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. |
|
Yep. I'm dealing with this right now. |
|
Kind of not happy with this solution still to be honest. It fixes the cookie leak but doesn't address @chanks comment here: 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. |
|
+1 for @chanks comment. Maybe a warning in the 1.2 release notes to flush all cache would also help a lot. |
|
Alright, rack-cache 1.2 is now available on rubygems.org. Please let me know if this plugs all holes for you. |
|
So no backport in older versions? Hope people update those gems... |
## 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.
## 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.
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-Cookieheader should ever be cached.We have a Rails application that has a few
fresh_whenandstale?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.