4.3.2 Unable to set cookie

[bmclean]

Describe the bug
After upgrading to Puma 4.3.2, setting a cookie from within a controller action fails to create the cookie.

Example:

  def create
    cookies[:prompt_dismissed] = { value: true }
    redirect_to users_path
  end

Might be similar cause to #2131

[bmclean]

If I comment out line 685 of server.rb setting cookies works correctly.

# headers.reject! { |_k, v| CRLF_REGEX =~ v.to_s }

[ideasasylum]

I encountered a similar problem which Nate tweeted out

In my case, it looks like the cookie path is being set incorrectly to /\n. I just need to figure out where that's being set.

We're on Rails 5.2.4.1

[esb]

Yes, we have a problem here.

Using a standard Rails controller which sets a cookie may result in multiple cookies being set in the header. It's been reported that Rack joins these headers with "\n" so this security fix will totally suppress all the cookie headers being sent.

This has nothing to do with including "\n" within the cookie value, but gets generated as part of the rack environment.

Potentially this breaks all Rails apps that use cookies, so it probably needs to be addressed fairly urgently.

[nateberkopec]

In the meantime, folks can use the workaround mentioned in the CVE and modify the regex to catch only CRLF rather than CR or LF.

[nateberkopec]

Hm. So I literally just copied the regex used in WEBrick, so I'm a little surprised to see that this breaks so many apps. If we have a problem, WEBrick has a problem, which I would've guessed would have been caught already.

I'll have to go back and learn more about the vulnerability. I think we maybe don't need to catch only LFs or only CRs but purely a CR followed at some point later in the same header by an LF? But then perhaps you could put a CR in one header and an LF in another, later header?

Patch will be out tomorrow. Again, in the meantime, either rollback to 4.3.1 if you can be certain you do not have user input in header values or use the workaround here: GHSA-84j7-475p-hp8v with a modified regex (/\r\n/ instead)

[fukayatsu]

https://github.com/fukayatsu/debug-puma-4.3.2-cookie

I created a repo to reproduce this issue.

OK: WEBrick 1.6.0 / ruby 2.7.0 (2019-12-25) [x86_64-darwin19]

$ bin/rails server -u webrick
# => WEBrick 1.6.0 ruby 2.7.0 (2019-12-25) [x86_64-darwin19]
$ open http://localhost:3000/
# => `Set-Cookie` headers shown on devtool.

NG: puma Version 4.3.2 (ruby 2.7.0-p0), codename: Mysterious Traveller

$ bin/rails server -u puma
# => puma Version 4.3.2 (ruby 2.7.0-p0), codename: Mysterious Traveller
$ open http://localhost:3000/
# => `Set-Cookie` headers not shown on devtool.

OK: puma Version 4.3.1 (ruby 2.7.0-p0), codename: Mysterious Traveller

$ git checkout puma-4.3.1
$ bin/rails server -u puma
# => puma Version 4.3.1 (ruby 2.7.0-p0), codename: Mysterious Traveller
$ open http://localhost:3000/
# => `Set-Cookie` headers shown on devtool.

[fukayatsu]

https://twitter.com/nateberkopec/status/1233228543133249537
It looks a little more complicated. Rack is inserting /n in headers, which I didn't expect, because I just copied WEBrick's fix. Maybe hold off on upgrading until I can get a 4.3.3 out tomorrow.

👍

[thomasbalsloev]

We also experience app breakage with this update.
Our login page ceased to work.

Reverted to 4.3.1 makes it work again.

[karol-blaszczyk]

We experiencing the same issue. Login doesn't work. We keep 4.3.1

[gingerlime]

Seeing this also with 3.12.3 :(

[9mm]

Damn.. I just deployed and didn't do a test because it was a minor version bump (my fault), and our entire order flow which depends on cookies/session died for dozens of incoming orders. For me however, it was upgrading to 3.12.3. This is quite bad, it's weird it wasn't fixed already

[gingerlime]

The odd thing for us was that it didn’t actually break logins in staging and live but it did in development ... or maybe the upgrade didn’t take place fully with phased restarts? In any case we reverted the change for now

[summera]

Same issue here after testing the update - can't sign in to app due to cookie issues.

[ideasasylum]

@9mm our order flow depends on cookies too, and logins, and customer support etc — just like everyone else's app. But we all have to take responsibility for the things we deploy. It's no one else's fault when our app breaks—certainly not the puma maintainers

[9mm]

@ideasasylum I understand, and agree. I largely implied that in my post (that it was my fault). I was more speculating why this wasn't a bigger deal (as a whole), as it breaks probably 85-90% of sites. I'm not the maintainer of anything, so I could just be completely off, but that seems like a patch would be released pretty urgently

[nateberkopec]

@9mm I am working on US Central Time. This was reported 19 hours ago at 6pm CST, and for 8 out of those 19 hours I have been asleep. The other 11 hours, I have been working on and thinking about a fix for this bug. You didn't even have to pay me!

[9mm]

@9mm I am working on US Central Time. This was reported 19 hours ago at 6pm CST, and for 8 out of those 19 hours I have been asleep. The other 11 hours, I have been working on and thinking about a fix for this bug. You didn't even have to pay me!

😆Please dont misinterpret anything I've said as expecting anything or being entitled to anything. Thanks for your contributions!

PS. I love your Rails performance book, keep up the awesome work

[nateberkopec]

Fix is on master. 4.3.3 and 3.12.4 plus a new advisory will be out within an hour or so. Thanks for your patience everyone.

[bmclean]

Thank you @nateberkopec! We have 4.3.3 deployed on our staging environment and everything looks good.

[nateberkopec]

4.3.3 and 3.12.4 have been pushed to rubygems.org. In addition to a fix for this issue, I fixed an additional vulnerability in Early Hints. Thanks to @matthewd for helping me with this issue and that vulnerability.

Why was a bug that broke so many applications included in a patch (security) release?

The simple root cause is that Puma did not have test coverage around this line, which ensures that we conform to the Rack spec around header splits. Of course, we do now.

In a Rack-compatible web-server, we're supposed to take a hash that looks like this:

"X-Header": value1\nvalue2

...and turn it into:

"X-Header": value1
"X-Header": value2

It turns out this feature of Rack is particularly widely used to set cookies, which makes sense, because a lot of different parts of the app want to touch the cookie headers. That's why all of your logins broke.

If we had test coverage around the (one!) line that split headers on newlines, 4.3.2 wouldn't have passed that test and I would've eventually figured out what was going wrong.

I copied the fix from WEBrick, but I didn't realize that WEBrick is not a Rack-compatible webserver, and so they can do things a little differently than we do in Puma.

What can be done to prevent something like this in the future?

With security patches especially, it's hard to get good "real-world" testing on a release. With a lot of our performance patches or releases, for example, we try those out on our own applications to make sure they work. With security patches, we have to be kind of careful about testing them because we could leak the vulnerability before we're ready.

Instead, Puma clearly needs more test coverage around Rack compliance. I haven't looked yet, and I don't know how much more is untested, but with just one more test in the right place this would've been caught. Issue #2137 has been created to track this and if you'd like to contribute, we'd love your help.

[ohaddahan]

Thanks @nateberkopec for the awesome patch!

I followed your link headers-spec and I see they mention the entire range under ASCII 37.

I checked and this /[\u0000-\u0025]/ seems to work and cover this requirement hope it helps somehow.

"1\n2\r3 4! 5$6%".scan(/[\u0000-\u0025]/)
[
    [0] "\n",
    [1] "\r",
    [2] " ",
    [3] "!",
    [4] " ",
    [5] "$",
    [6] "%"
]