Do not send multiple cookies with same name/domain/path #599

Closed
wants to merge 1 commit into
from

2 participants

@atotic

I've triggered this bug in the wild. I had a client polling rack server every second, and connection was kept alive. All of these are necessary to trigger the bug.

Bug:
I use 'rack.session' with :expires setting.
This causes Rack::Session::Abstract::ID.set_cookie to call
Utils.set_cookie_header for every response.

Every time Utils.set_cookie_header was called, new cookie was added. It was a duplicate of an existing session cookie, but because of :expires flag, cookie was being saved anyway.

Because I was polling every second, and keep-alive kept the connection open, the number of cookie headers kept growing. I noticed the bug when my response headers were 250K! Simple HTTP dump is attached showing the respone.

Fix:
Fix can either happen in Rack::Session::Abstract::ID.set_cookie, or in Utils.set_cookie_header.

I was unclear why does Rack::Session::Abstract::ID.set_cookie always sets the cookie if session has :expires set, so I did not touch that code.

I decided that the right fix is inside Utils.set_cookie_header. According to cookie spec, RFC 6265 section 4.1.2
"If the user agent receives a new cookie with the same cookie-name,
domain-value, and path-value as a cookie that it has already stored, the existing cookie is evicted and replaced with the new cookie."

I've modified Rack::Utils.set_cookie header to remove other cookies with same name/domain/path.

Sample bug session:

~> telnet localhost 27000
Trying ::1...
telnet: connect to address ::1: Connection refused
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /get_work HTTP/1.1
hostname: localhost

HTTP/1.1 204 No Content
Content-Type: text/plain
Content-Length: 0
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000

@atotic atotic Do not send multiple cookies with same name/domain/path
I've triggered this bug in the wild. I had a client polling rack server every second, and connection was kept alive. All of these are necessary to trigger the bug.

Bug:
I use 'rack.session' with :expires setting.
This causes Rack::Session::Abstract::ID.set_cookie to call 
Utils.set_cookie_header for every response.

Every time Utils.set_cookie_header was called, new cookie was added. It was a duplicate of an existing session cookie, but because of :expires flag, cookie was being saved anyway.

Because I was polling every second, and keep-alive kept the connection open, the number of cookie headers kept growing. I noticed the bug when my response headers were 250K! Simple HTTP dump is attached showing the respone.

Fix:
Fix can either happen in Rack::Session::Abstract::ID.set_cookie, or in Utils.set_cookie_header.

I was unclear why does Rack::Session::Abstract::ID.set_cookie always sets the cookie if session has :expires set, so I did not touch that code.

I decided that the right fix is inside Utils.set_cookie_header. According to cookie spec, RFC 6265 section 4.1.2 
"If the user agent receives a new cookie with the same cookie-name,
domain-value, and path-value as a cookie that it has already stored, the existing cookie is evicted and replaced with the new cookie."

I've modified Rack::Utils.set_cookie header to remove other cookies with same name/domain/path.

Sample bug session:

~> telnet localhost 27000
Trying ::1...
telnet: connect to address ::1: Connection refused
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
GET /get_work HTTP/1.1
hostname: localhost

HTTP/1.1 204 No Content
Content-Type: text/plain
Content-Length: 0
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
Set-Cookie: rack.session=%7B%22session_id%22%3A%22e54c5fc4f77c4349%22%2C%22user_id%22%3A2%2C%22user_id_expires%22%3A1377981804%7D--bb1c857cf9e6e5e776f791de4a4ec6d1a5905e80; path=/; expires=Sat, 31 Aug 2013 20:43:24 -0000
5610484
@raggi
Official Rack repositories member

The problem is that rack does not know from a /request/ if the domain and path are the same (as these are not included in the request headers).

A patch that prevents multiple sets here, prevents full session clears from functioning in the case where there are multiple cookies with the same name.

This is a big problem for customers trying to conform to EU law.

@raggi raggi added this to the Rack 1.6 milestone Jul 12, 2014
@raggi
Official Rack repositories member

Any further thoughts here? AFAICS you're battling a broken user agent, is that true?

@atotic

I've moved to node, so feel free to kill this pull request.

@atotic

PS: it was not a broken user agent, just an unusual usage case. I was polling rack server from a chrome extension running on the same machine.

@raggi
Official Rack repositories member

@atotic chrome de-dups based on name, domain and path. If it wasn't the user agent, were you altering one of these with each request, thus creating new cookies?

@atotic

Here is my vague recollection of the problem:

  • Chrome and rack are running on the same machine
  • Chrome extension polls rack for work (GET /work) once a second. The calls are frequent enough that keep-alive connection never closes.
  • Chrome's requests are correct.
  • Rack adds an additional Set-Cookie header in every response. 1st response gets one Set-Cookie, 2nd response gets two, 100th response gets 100 Set-Cookie headers. All Set-Cookies are identical.
  • So, Rack's response grows indefinitely until I run out of memory. Chrome is fine.
@raggi
Official Rack repositories member

There's something else involved here that I cannot replicate. Rack will not generate multiple set-cookie headers under these conditions. Do you have some code that replicates this issue?

If you don't have the original code, this may serve as a starting point: https://gist.github.com/raggi/5ce37897f5f1321aaddb

@atotic

You are persistent, I appreciate the thoroughfullness. I am no longer able to reproduce the bug. I updated all the gems, fired up the old server, and used your code as a starting point for testing. Not sure what fixed it, lots of moving parts

@atotic atotic closed this Jul 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment