Dirkjan Bussink wrote: > Hello Eric, > > > On 11 Mar 2021, at 04:02, Eric Wong wrote: > > > > Thanks for reaching out. Fwiw, I prefer if everything were made > > public right away, but I'll leave it up to you if you're not > > comfortable with it. > > > > I don't know much about security, anwyays; and don't like > > classifying bugs (or classifying anything)... > > We reached out privately first out of care and to follow best practices > around coordinated disclosure, in case you considered this a security > vulnerability. We have no objections to moving this to the public mailing > list. We are viewing this patch as a proactive hardening against race > conditions more so than a vulnerability. OK, I'm adding unicorn-public@yhbt.net to the Cc: of this mail. Personally, I prefer everything be reported publicly ASAP. There's a constant threat of power/network failures from disasters and such that could cause messages to be delayed too long or indefinitely. > We are also reaching out to a private Rails Security coordination channel > that we’re a part of to raise awareness of this behavior so other Unicorn > users in this group can look for similar issues in their code. OK. > To move this discussion to the public list, would you prefer that you move > this thread publicly, or that we resend the message or forward it to the > public mailing list? Attached are the initial two (previously private) emails in this thread, so no need to resend. They have the correct message/rfc822 MIME type set so they should be readable from most MUAs and mail archives. > > Ouch, so the hijack check we had in HttpParser_clear didn't fire... > > Yes, to our understanding it only would fire if explicitly set and that > doesn’t happen here. > > >> While we understand and appreciate that Unicorn is not a multi-threaded web > >> server, it feels like using the same `Hash` object between requests > >> introduces the chance that a dependency like an external gem may use threads > >> and thus potentially leak information between requests. > > > > Yes, there's likely problems in trying to use threads with a > > codebase that was only intended to be single-threaded. And > > using Thread.current[...] here wouldn't have made a. difference. > > For us it was also the difference between “requests are handled single threaded” > vs “you can’t use threads for anything else either.” We were totally aware of the > first, but the latter seems more accidental and has a much broader impact. Agreed. > > I worry some endpoints out there will suffer performance > > degradation. Expensive endpoints probably won't notice, > > but maybe the fastest ones will... > > That is a good point, but I think in practice most users do enough in most > requests that the trade off is totally worth it. At least for us it definitely > is. Maybe it would be an option to make the sharing somehow opt-in instead of > default behavior? So that by default the safe behavior is used, but for those > that want to, they can opt into the sharing if they know their app is safe > enough to work with that. I'm not in favor of new options since they add support costs and increase the learning/maintenance curve. What I've been thinking about is bumping the major version to 6.0 Although our internals are technically not supported stable API, there may be odd stuff out there similar to OobGC that uses instance_variable_get or similar things to reach into internals. Added with the fact our internals haven't changed in many years; I'm inclined to believe there are other OobGC-like things out there that can break. Also, with 6.0; users who completely avoid Threads can keep using 5.x, while others can use 6.x > >> For the sake of transparency to our users, we plan on publishing a public > >> post next week on how this was part of the larger series of bugs that had a > >> security impact at GitHub. We've also attached a suggested patch that removes > >> the environment sharing, which is what we're running right now to reduce the > >> risk of this ever happening again. > > > > Did you measure performance degradations in any endpoints you have? > > We did measure and there were zero noticeable performance degradations. That’s > also because all our requests do a bunch of work and are not direct Rack apps, > and use stuff like Rails or Sinatra. Those all usually wrap the `env` hash > anyway with their own per request object and there’s a lot of other allocations > happening anyway. > > In microbenchmarks we could see a difference, but even there, at least for us, > we’d gladly pay the performance price for the safety if we’d have any endpoints > where it would be measurable. OK. Fwiw, there's still some stones left unturned that could recover the lost speed if somebody _really_ cares for it(*) (*) String#clear on response header buffer, swapping Ragel for a faster HTTP parser, ... > All in all, I think here that a safe default would be helpful for users, as > mentioned earlier, and that maybe for those cases where the latest performance > bit matters, the existing behavior could be opted into. Whether this option is > worth it from a maintenance standpoint is something you’re better able to answer > than we are though. It's probably OK and I'm inclined to accept your patch for 6.0. At this point, I'm more worried about potential breakage of some 3rd-party OobGC-like thing that reaches deep into our internals. Btw, did you consider replacing the @request HttpRequest object entirely instead of the env and buf elements? I suppose that's more allocations, still; but could've been a smaller change. > > I'll see about finding a less-noisy/overloaded system to run > > benchmarks against... > > > > I noticed some of the OobGC tests in t/ were failing (patch below), > > but few users use the that version of OobGC. > > I wasn’t easily able to run the entire suite, but only parts of it which is why > I didn’t have a complete fix there. I can add this to the patch then. Oops, was that the integration tests in t/* ? They can run separately via "make test-integration" (I never trusted some Ruby behaviors to remain stable, so I started writing tests in Bourne shell). I have nothing to add to the rest of the mail unicorn-public readers: see attachments