Re: Potential Unicorn vulnerability

Re: Potential Unicorn vulnerability

[Eric Wong]

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

Re: Potential Unicorn vulnerability

[Dirkjan Bussink]

Hi Eric,

> On 12 Mar 2021, at 10:41, Eric Wong <normalperson@yhbt.net> wrote:
> 
> 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

That sounds like a good plan then. Once there’s a new version we can
bump that on our side to remove the manual patch then.

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

Ah, that’s a very good point. I think that would also have been a valid
approach but it does indeed add more allocations. If that approach would
be preferred, I think it can also be changed to that?

I don’t really have a strong preference on which approach to take here,
do you? 

> Oops, was that the integration tests in t/* ?

Nope, looks like some platform behavior changes (tried on MacOS first).
I was able to get the tests running and working on Debian Buster this
morning before I sent a new version of the patch and they are all passing
there for me locally.

Cheers,

Dirkjan

Re: Potential Unicorn vulnerability

[Eric Wong]

Dirkjan Bussink <dbussink@github.com> wrote:
> Hi Eric,
> 
> > On 12 Mar 2021, at 10:41, Eric Wong <normalperson@yhbt.net> wrote:
> > 
> > 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
> 
> That sounds like a good plan then. Once there’s a new version we can
> bump that on our side to remove the manual patch then.

OK.  I think it's safe to wait a few days for more comments
before releasing in case there's more last-minute revelations
(see below :x)

> > 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.
> 
> Ah, that’s a very good point. I think that would also have been a valid
> approach but it does indeed add more allocations. If that approach would
> be preferred, I think it can also be changed to that?
> 
> I don’t really have a strong preference on which approach to take here,
> do you?

I was going to say I didn't have a preference and the
current approach was fine...

However, I just now realized now that clobbering+replacing all
of @request is required.

That's because env['rack.input'] is (Stream|Tee)Input,
and that is lazily consumed and those objects keep state in
@request (as the historically-named @parser)

If we're to make env safe to be shipped off to another thread,
then @request still needs to stick around to maintain state
of env['rack.input'] until it's all consumed.

It probably doesn't affect most apps out there that just decode
forms via HTTP POST; but the streamed rack.input is something
that's critical for projects that feed unicorn with PUTs via
"curl -T"

> > Oops, was that the integration tests in t/* ?
> 
> Nope, looks like some platform behavior changes (tried on MacOS first).
> I was able to get the tests running and working on Debian Buster this
> morning before I sent a new version of the patch and they are all passing
> there for me locally.

Ah, no idea about MacOS or any proprietary OS; I've never
considered them supported.  But yeah, it should work on any
GNU/Linux and Free-as-in-speech *BSDs

Re: Potential Unicorn vulnerability

[Dirkjan Bussink]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

Hi Eric,

> On 12 Mar 2021, at 13:00, Eric Wong <normalperson@yhbt.net> wrote:
> 
> I was going to say I didn't have a preference and the
> current approach was fine...
> 
> However, I just now realized now that clobbering+replacing all
> of @request is required.
> 
> That's because env['rack.input'] is (Stream|Tee)Input,
> and that is lazily consumed and those objects keep state in
> @request (as the historically-named @parser)
> 
> If we're to make env safe to be shipped off to another thread,
> then @request still needs to stick around to maintain state
> of env['rack.input'] until it's all consumed.


Ah yeah, that’s a good point. I don’t think this affects us right
now so the existing patch still keeps us safe, but it would break this
case then indeed.

> It probably doesn't affect most apps out there that just decode
> forms via HTTP POST; but the streamed rack.input is something
> that's critical for projects that feed unicorn with PUTs via
> "curl -T"

Ah yeah. So do you think that on top of the current patch we’d need
something like the attached patch (which moves the @request allocation),
or would only the latter patch be needed then?

In the latter case there’s still a bunch of logic for Rack hijack around
then which might not be needed at that point, but I’m not entirely sure
how that would look like.

Cheers,

Dirkjan


[-- Attachment #2: 0001-Allocate-a-new-request-for-each-client.patch --]
[-- Type: application/octet-stream, Size: 1579 bytes --]

From 58c4c153ba6c007284771c0899798a14df28c7c3 Mon Sep 17 00:00:00 2001
From: Dirkjan Bussink <d.bussink@gmail.com>
Date: Fri, 12 Mar 2021 13:22:25 +0100
Subject: [PATCH] Allocate a new request for each client

This removes the reuse of the parser between requests. Reusing these is
risky in the context of running any other threads within the unicorn
process, also for threads that run background tasks.

If any other thread accidentally grabs hold of the request it can modify
things for the next request in flight.

The downside here is that we allocate more for each request, but that is
worth the trade off here and the security risk we otherwise would carry
to leaking wrong and incorrect data.
---
 lib/unicorn/http_server.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index c0f14ba..22f067f 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -69,7 +69,6 @@ class Unicorn::HttpServer
   # incoming requests on the socket.
   def initialize(app, options = {})
     @app = app
-    @request = Unicorn::HttpRequest.new
     @reexec_pid = 0
     @default_middleware = true
     options = options.dup
@@ -621,6 +620,7 @@ def e100_response_write(client, env)
   # once a client is accepted, it is processed in its entirety here
   # in 3 easy steps: read request, call app, write app response
   def process_client(client)
+    @request = Unicorn::HttpRequest.new
     env = @request.read(client)
 
     if early_hints
-- 
2.30.1

Re: Potential Unicorn vulnerability

[Eric Wong]

Dirkjan Bussink <dbussink@github.com> wrote:
> Ah yeah. So do you think that on top of the current patch we’d need
> something like the attached patch (which moves the @request allocation),
> or would only the latter patch be needed then?

Not really, aside from the OobGC change and hijack test removal.

Anyways, I've squashed the test removal, OobGC adjustment, and
your 2nd patch together as commit c917ac526df214611ec33c21de2b070452ec8434
and pushed it out as the "v6-wip" branch.

> In the latter case there’s still a bunch of logic for Rack hijack around
> then which might not be needed at that point, but I’m not entirely sure
> how that would look like.

Yes, though there are also some other HTTP servers that use the
parser.  I prefer to minimize changes to the ext code at this
point given the relative lack of C/Ragel-knowledgeable users
compared to Ruby-knowledgeable

[PATCH] http_request: drop unnecessary #clear call

[Eric Wong]

Also noticed #clear is unnecessary, now

------------8<-----------
Subject: [PATCH] http_request: drop unnecessary #clear call

Since we allocate a new request object for each request,
the #clear call is now unnecessary
---
 lib/unicorn/http_request.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/lib/unicorn/http_request.rb b/lib/unicorn/http_request.rb
index 6ca4592..e3ad592 100644
--- a/lib/unicorn/http_request.rb
+++ b/lib/unicorn/http_request.rb
@@ -62,7 +62,6 @@ def self.check_client_connection=(bool)
   # This does minimal exception trapping and it is up to the caller
   # to handle any socket errors (e.g. user aborted upload).
   def read(socket)
-    clear
     e = env
 
     # From https://www.ietf.org/rfc/rfc3875:

Re: Potential Unicorn vulnerability

[Dirkjan Bussink]

Hello Eric,

> On 13 Mar 2021, at 03:26, Eric Wong <normalperson@yhbt.net> wrote:
> 
> Anyways, I've squashed the test removal, OobGC adjustment, and
> your 2nd patch together as commit c917ac526df214611ec33c21de2b070452ec8434
> and pushed it out as the "v6-wip" branch.

We’ve updated to the v6-wip branch on our side and everything looks good with
that branch as well in production. Performance etc. also all looks healthy
as well.

Cheers,

Dirkjan

Re: Potential Unicorn vulnerability

[Eric Wong]

Dirkjan Bussink <dbussink@github.com> wrote:
> We’ve updated to the v6-wip branch on our side and everything looks good with
> that branch as well in production. Performance etc. also all looks healthy
> as well.

Thanks for the update, will release in a some hours when more
awake and able to form coherent release notes

Re: Potential Unicorn vulnerability

[Dirkjan Bussink]

Hi Eric,

> On 16 Mar 2021, at 11:31, Eric Wong <normalperson@yhbt.net> wrote:
> 
> Thanks for the update, will release in a some hours when more
> awake and able to form coherent release notes

Thanks for the release! Would you mind if we link to the release announcement
from our upcoming in depth post on what happened? 

Cheers,

Dirkjan

Re: Potential Unicorn vulnerability

[Eric Wong]

Dirkjan Bussink <dbussink@github.com> wrote:
> Thanks for the release! Would you mind if we link to the release announcement
> from our upcoming in depth post on what happened? 

Sure, I don't see why not.  Though (as you might've figured) I'm
not a fan of marketing or putting a positive spin on things at all.
I can't dictate what others say, but I'm learning to get worse at
marketing over time :>

Re: Potential Unicorn vulnerability

[Dirkjan Bussink]

Hi Eric.

> On 17 Mar 2021, at 09:47, Eric Wong <normalperson@yhbt.net> wrote:
> 
> Sure, I don't see why not.  Though (as you might've figured) I'm
> not a fan of marketing or putting a positive spin on things at all.
> I can't dictate what others say, but I'm learning to get worse at
> marketing over time :>

FYI, we posted the results of our investigation at
https://github.blog/2021-03-18-how-we-found-and-fixed-a-rare-race-condition-in-our-session-handling/.

We tried to stay factual there but I can’t judge of course how
anyone reads it.

Cheers,

Dirkjan