From: Eric Wong <normalperson@yhbt.net> To: Dirkjan Bussink <dbussink@github.com> Cc: John Crepezzi <seejohnrun@github.com>, Kevin Sawicki <kevinsawicki@github.com>, unicorn-public@yhbt.net Subject: Re: Potential Unicorn vulnerability Date: Fri, 12 Mar 2021 09:41:29 +0000 [thread overview] Message-ID: <20210312094129.GA14538@dcvr> (raw) In-Reply-To: <7F6FD017-7802-4871-88A3-1E03D26D967C@github.com> [-- Attachment #1: Type: text/plain, Size: 6397 bytes --] Dirkjan Bussink <dbussink@github.com> wrote: > Hello Eric, > > > On 11 Mar 2021, at 04:02, Eric Wong <normalperson@yhbt.net> 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). <snip> I have nothing to add to the rest of the mail unicorn-public readers: see attachments [-- Attachment #2: 0001-potential-unicorn-vulnerability.eml --] [-- Type: message/rfc822, Size: 12105 bytes --] [-- Attachment #2.1.1: Type: text/plain, Size: 2813 bytes --] Hello Eric, We're reaching out privately first on what we think could be classified as a security issue in Unicorn. Since there may be other similarly impacted users, this is out of an abundance of caution before sending it to the public Unicorn mailing list. The issue at hand is that the environment sharing and reuse between requests in Unicorn, combined with other non thread safe code, resulted in a security vulnerability where a very small number of user sessions tracked through cookies got misrouted. For this reason, we logged everyone out of GitHub, see also https://github.blog/2021-03-08-github-security-update-a-bug-related-to-handling-of-authenticated-sessions/. The unsafe background thread code we had ended up triggering an exception at rare times and the exception tracking logic was using a deferred block executed through a Ruby Proc to gather information, including things from the request at the time the logic started. That thread captured something from the cookie jar among other things, and the following code in Rack memoized that into the (shared) environment. From https://github.com/rack/rack/blob/2.1.4/lib/rack/request.rb#L219-L229 def cookies hash = fetch_header(RACK_REQUEST_COOKIE_HASH) do |k| set_header(k, {}) end string = get_header HTTP_COOKIE return hash if string == get_header(RACK_REQUEST_COOKIE_STRING) hash.replace Utils.parse_cookies_header string set_header(RACK_REQUEST_COOKIE_STRING, string) hash end Because of the environment sharing, the memoization ended up overriding what would be memoized (with the RACK_REQUEST_COOKIE_STRING key) for the new request and because a specific session cookie was bumped then with a new timeout on each request, the wrong session cookie was serialized. 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. 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. We hope you're open to collaborating on a fix prior to any public detailed disclosure of how this request environment sharing could lead to security issues, if you feel that is desired. I’ve added John & Kevin here on the CC since they’ve also worked on this and that way we have some better timezone spread on our side if needed. Cheers, Dirkjan Bussink [-- Attachment #2.1.2: 0001-Drop-reuse-of-Ruby-level-objects.patch --] [-- Type: application/octet-stream, Size: 4067 bytes --] From 44aa6b056e1b24d42ab3efba738b38f4cd54a068 Mon Sep 17 00:00:00 2001 From: Dirkjan Bussink <d.bussink@gmail.com> Date: Mon, 8 Mar 2021 09:51:09 +0100 Subject: [PATCH] Drop reuse of Ruby level objects Remove the reuse of environment and the buffer as Ruby level objects. Reusing these is very risky in the context of running any other threads within the unicorn process, also for threads that run background tasks. This lead to a significant security incident where the problems would not have happened if there was no reuse of the `env` object. From a safety perspective, this also removes reuse of the buffer object as it's also a Ruby object and could be grabbed and retained outside of the http parsing logic. The downside here is that we allocate two extra objects on each request, but that is worth the trade off here and the security risk we otherwise would carry to leaking wrong and incorrect data. --- ext/unicorn_http/extconf.rb | 1 - ext/unicorn_http/unicorn_http.rl | 30 +++--------------------------- test/unit/test_http_parser.rb | 3 +++ 3 files changed, 6 insertions(+), 28 deletions(-) diff --git a/ext/unicorn_http/extconf.rb b/ext/unicorn_http/extconf.rb index 95514bc..7e4775c 100644 --- a/ext/unicorn_http/extconf.rb +++ b/ext/unicorn_http/extconf.rb @@ -10,7 +10,6 @@ have_macro("SIZEOF_SIZE_T", "ruby.h") or check_sizeof("size_t", "sys/types.h") have_macro("SIZEOF_LONG", "ruby.h") or check_sizeof("long", "sys/types.h") have_func("rb_str_set_len", "ruby.h") or abort 'Ruby 1.9.3+ required' -have_func("rb_hash_clear", "ruby.h") # Ruby 2.0+ have_func("gmtime_r", "time.h") message('checking if String#-@ (str_uminus) dedupes... ') diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl index 21e09d6..9904d85 100644 --- a/ext/unicorn_http/unicorn_http.rl +++ b/ext/unicorn_http/unicorn_http.rl @@ -65,18 +65,6 @@ struct http_parser { static ID id_set_backtrace, id_is_chunked_p; static VALUE cHttpParser; -#ifdef HAVE_RB_HASH_CLEAR /* Ruby >= 2.0 */ -# define my_hash_clear(h) (void)rb_hash_clear(h) -#else /* !HAVE_RB_HASH_CLEAR - Ruby <= 1.9.3 */ - -static ID id_clear; - -static void my_hash_clear(VALUE h) -{ - rb_funcall(h, id_clear, 0); -} -#endif /* HAVE_RB_HASH_CLEAR */ - static void finalize_header(struct http_parser *hp); static void parser_raise(VALUE klass, const char *msg) @@ -445,6 +433,8 @@ static void http_parser_init(struct http_parser *hp) hp->cont = Qfalse; /* zero on MRI, should be optimized away by above */ %% write init; hp->cs = cs; + hp->buf = rb_str_new(NULL, 0); + hp->env = rb_hash_new(); } /** exec **/ @@ -628,8 +618,6 @@ static VALUE HttpParser_init(VALUE self) struct http_parser *hp = data_get(self); http_parser_init(hp); - hp->buf = rb_str_new(NULL, 0); - hp->env = rb_hash_new(); return self; } @@ -643,16 +631,7 @@ static VALUE HttpParser_init(VALUE self) */ static VALUE HttpParser_clear(VALUE self) { - struct http_parser *hp = data_get(self); - - /* we can't safely reuse .buf and .env if hijacked */ - if (HP_FL_TEST(hp, HIJACK)) - return HttpParser_init(self); - - http_parser_init(hp); - my_hash_clear(hp->env); - - return self; + return HttpParser_init(self); } static void advance_str(VALUE str, off_t nr) @@ -1025,9 +1004,6 @@ void Init_unicorn_http(void) id_set_backtrace = rb_intern("set_backtrace"); init_unicorn_httpdate(); -#ifndef HAVE_RB_HASH_CLEAR - id_clear = rb_intern("clear"); -#endif id_is_chunked_p = rb_intern("is_chunked?"); } #undef SET_GLOBAL diff --git a/test/unit/test_http_parser.rb b/test/unit/test_http_parser.rb index 697af44..d3f9ae7 100644 --- a/test/unit/test_http_parser.rb +++ b/test/unit/test_http_parser.rb @@ -33,6 +33,9 @@ def test_parse_simple parser.clear req.clear + req = parser.env + http = parser.buf + http << "G" assert_nil parser.parse assert_equal "G", http -- 2.30.1 [-- Attachment #3: 0002-re-potential-unicorn-vulnerability.eml --] [-- Type: message/rfc822, Size: 5635 bytes --] From: Eric Wong <normalperson@yhbt.net> To: Dirkjan Bussink <dbussink@github.com> Cc: John Crepezzi <seejohnrun@github.com>, Kevin Sawicki <kevinsawicki@github.com> Subject: Re: Potential Unicorn vulnerability Date: Thu, 11 Mar 2021 03:02:50 +0000 Message-ID: <20210311030250.GA1266@dcvr> Dirkjan Bussink <dbussink@github.com> wrote: > Hello Eric, > > We're reaching out privately first on what we think could be classified as a > security issue in Unicorn. Since there may be other similarly impacted users, > this is out of an abundance of caution before sending it to the public > Unicorn mailing list. 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)... <snip> > That thread captured something from the cookie jar among other things, and > the following code in Rack memoized that into the (shared) environment. Ouch, so the hijack check we had in HttpParser_clear didn't fire... <snip> > 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. I worry some endpoints out there will suffer performance degradation. Expensive endpoints probably won't notice, but maybe the fastest ones will... `buf' is particularly worrying to me since it's a 16384-byte allocation for a socket read on headers that could amount to lots of GC pressure and hurt locality all around. env['rack.input'] may also hold onto `buf', too, since input is lazily-consumed (though it may be possible to workaround that...). Losing `env' is probably less worrying since keys are all fstrings in modern Rubies. Though losing the backing store (and not having rb_hash_new_with_size in the C API) would mean more rehashing with many headers. The simple Rack apps I worked on back in-the-day which benefitted from these object-reuse optimizations are unlikely to be upgraded to newer versions of unicorn. Of course, there may be other similar Rack apps out there that depend on these... > 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? 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. Also, t/t0200-rack-hijack.sh would go away. > We hope you're open to collaborating on a fix prior to any public detailed > disclosure of how this request environment sharing could lead to security > issues, if you feel that is desired. Sure, as long as everything is done via plain-text email so I won't need working graphics drivers or modern hardware :> Along the same lines, would you be OK with this entire mail thread (including all mail headers) being eventually published in the public mailbox at http://ou63pmih66umazou.onion/unicorn-public/ and https://yhbt.net/unicorn-public/ ? > I’ve added John & Kevin here on the CC since they’ve also worked on this and > that way we have some better timezone spread on our side if needed. OK, I'm around/awake at pretty random times. Aforementioned OomGC change: -------8<------- diff --git a/lib/unicorn/oob_gc.rb b/lib/unicorn/oob_gc.rb index 3b2f488..91a8e51 100644 --- a/lib/unicorn/oob_gc.rb +++ b/lib/unicorn/oob_gc.rb @@ -60,7 +60,6 @@ def self.new(app, interval = 5, path = %r{\A/}) self.const_set :OOBGC_INTERVAL, interval ObjectSpace.each_object(Unicorn::HttpServer) do |s| s.extend(self) - self.const_set :OOBGC_ENV, s.instance_variable_get(:@request).env end app # pretend to be Rack middleware since it was in the past end @@ -68,9 +67,10 @@ def self.new(app, interval = 5, path = %r{\A/}) #:stopdoc: def process_client(client) super(client) # Unicorn::HttpServer#process_client - if OOBGC_PATH =~ OOBGC_ENV['PATH_INFO'] && ((@@nr -= 1) <= 0) + env = instance_variable_get(:@request).env + if OOBGC_PATH =~ env['PATH_INFO'] && ((@@nr -= 1) <= 0) @@nr = OOBGC_INTERVAL - OOBGC_ENV.clear + env.clear disabled = GC.enable GC.start GC.disable if disabled
next parent reply other threads:[~2021-03-12 9:41 UTC|newest] Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <F6712BF3-A4DD-41EE-8252-B9799B35E618@github.com> [not found] ` <20210311030250.GA1266@dcvr> [not found] ` <7F6FD017-7802-4871-88A3-1E03D26D967C@github.com> 2021-03-12 9:41 ` Eric Wong [this message] 2021-03-12 11:14 ` Dirkjan Bussink 2021-03-12 12:00 ` Eric Wong 2021-03-12 12:24 ` Dirkjan Bussink 2021-03-13 2:26 ` Eric Wong 2021-03-13 2:31 ` [PATCH] http_request: drop unnecessary #clear call Eric Wong 2021-03-16 10:15 ` Potential Unicorn vulnerability Dirkjan Bussink 2021-03-16 10:31 ` Eric Wong 2021-03-17 8:03 ` Dirkjan Bussink 2021-03-17 8:47 ` Eric Wong 2021-03-19 13:55 ` Dirkjan Bussink
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: https://yhbt.net/unicorn/ * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20210312094129.GA14538@dcvr \ --to=normalperson@yhbt.net \ --cc=dbussink@github.com \ --cc=kevinsawicki@github.com \ --cc=seejohnrun@github.com \ --cc=unicorn-public@yhbt.net \ --subject='Re: Potential Unicorn vulnerability' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: ../../unicorn.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).