puma: 4.3.2 Unable to set cookie

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.

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.

@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 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

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: https://github.com/puma/puma/security/advisories/GHSA-84j7-475p-hp8v with a modified regex (/\r\n/ instead)

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

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.

We’ve been running 4.3.3 without any problems. Appreciate the quick fix, Nate 👍

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] "%"
]

@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

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

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

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

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

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

@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

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

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

Seeing this also with 3.12.3 😦