From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-3.5 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from mail-ej1-x635.google.com (mail-ej1-x635.google.com [IPv6:2a00:1450:4864:20::635]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by dcvr.yhbt.net (Postfix) with ESMTPS id 743761F9FE for ; Fri, 12 Mar 2021 12:24:41 +0000 (UTC) Received: by mail-ej1-x635.google.com with SMTP id jt13so53056027ejb.0 for ; Fri, 12 Mar 2021 04:24:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=google; h=from:message-id:mime-version:subject:date:in-reply-to:cc:to :references; bh=OY5nUYTfrjMQHiy9RfnUhblQCeOAB/bpAAWITt+7ddE=; b=YaGqS7iw4b0OM0wrl6RtvK3Dm18WDVORTBdAdVCYvSXbNIIegxl+QtMkOfu84jsJ+u mMDTYIZcTqrRAvkkSfMqmcBrwJdpKaW6mdDpIgZ7yHkCGP3EA/MbbtIKKpsWGtrKUcVl p/h+7Fz7zBy4bfHQEKuVWkxfAoH5liw8t6Nq3jYKVPPNtOFu7ZaVuIMT4UY8eKV6dDQl cGfX/bj8tm+X9u1hxIdQj40GvUhsDJRMJCw73fLrRCMV9ZtQYKmjc7n6U/KZ4cuxBsGO xVnRoxu2+F4vTWFblDcrB3PaR5+3zby+LQxTMqsOV7MceqvSI39LW5q6yKid/rqo7Xms zx+w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=OY5nUYTfrjMQHiy9RfnUhblQCeOAB/bpAAWITt+7ddE=; b=UFs2nwpenP2zIvJk0kvWosk2uTdtYlsE5TLDYysfAUQx8eXVr03v2JTbSjkWzhbbpI liaD2qETjbDGHW2Aa30jAz6B+a3Rc+iHPRh7rkKtNBKpIBG9PF5Acma62dDuP19tgPia DVUUUSToSWM+mPXwKj+Qo5RFZMlmqR8EqtcPlT8k0ucCI2dqWuXTBGNkxf3iAFVFpJjT 26TYOn8uNnWk4Iv9bYy8RH7Wre3Ji1powXkzOfFx6Cg8i/PW/R/gq4EdzFWNe8qTAGUa ztkchTm0fKIMMuYQQ5S+gXLx3PiyxL0llRY7L4BMbeMcOwRpZivohbn47kM2WpLXwmVi gDZg== X-Gm-Message-State: AOAM533qKU2N5P0egcvigCl9x3imgxS7fdjVDfMgYZgzHEBfQ5pko0zC NPKvX1pv3m8ygzWS0lDc0w88LG3KyGF5xQ== X-Google-Smtp-Source: ABdhPJy602SmDt/nOVpokwM1K8j0dupBQ0oh9Vufk5RChwZsUy8/OqS6JhM0oxl90ZIznQExm/Q+GA== X-Received: by 2002:a17:906:3295:: with SMTP id 21mr8176273ejw.88.1615551877659; Fri, 12 Mar 2021 04:24:37 -0800 (PST) Received: from dbussink.archimedes.bussink.me (178-85-12-131.dynamic.upc.nl. [178.85.12.131]) by smtp.gmail.com with ESMTPSA id p19sm2884566edr.57.2021.03.12.04.24.36 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 12 Mar 2021 04:24:36 -0800 (PST) From: Dirkjan Bussink Message-Id: <66A68DD8-83EF-4C7A-80E8-3F1F7AB31670@github.com> Content-Type: multipart/mixed; boundary="Apple-Mail=_2A064DCC-B3E0-4F15-BE93-244E86F804B4" Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.120.23.2.4\)) Subject: Re: Potential Unicorn vulnerability Date: Fri, 12 Mar 2021 13:24:36 +0100 In-Reply-To: <20210312120007.GA24539@dcvr> Cc: John Crepezzi , Kevin Sawicki , unicorn-public@yhbt.net To: Eric Wong References: <20210311030250.GA1266@dcvr> <7F6FD017-7802-4871-88A3-1E03D26D967C@github.com> <20210312094129.GA14538@dcvr> <382B893C-A07C-4705-950E-6D1CA766D998@github.com> <20210312120007.GA24539@dcvr> X-Mailer: Apple Mail (2.3608.120.23.2.4) List-Id: --Apple-Mail=_2A064DCC-B3E0-4F15-BE93-244E86F804B4 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi Eric, > On 12 Mar 2021, at 13:00, Eric Wong wrote: >=20 > I was going to say I didn't have a preference and the > current approach was fine... >=20 > However, I just now realized now that clobbering+replacing all > of @request is required. >=20 > 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) >=20 > 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=E2=80=99s a good point. I don=E2=80=99t 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=E2=80=99d = 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=E2=80=99s still a bunch of logic for Rack = hijack around then which might not be needed at that point, but I=E2=80=99m not = entirely sure how that would look like. Cheers, Dirkjan --Apple-Mail=_2A064DCC-B3E0-4F15-BE93-244E86F804B4 Content-Disposition: attachment; filename=0001-Allocate-a-new-request-for-each-client.patch Content-Type: application/octet-stream; x-unix-mode=0644; name="0001-Allocate-a-new-request-for-each-client.patch" Content-Transfer-Encoding: quoted-printable =46rom=2058c4c153ba6c007284771c0899798a14df28c7c3=20Mon=20Sep=2017=20= 00:00:00=202001=0AFrom:=20Dirkjan=20Bussink=20=0A= Date:=20Fri,=2012=20Mar=202021=2013:22:25=20+0100=0ASubject:=20[PATCH]=20= Allocate=20a=20new=20request=20for=20each=20client=0A=0AThis=20removes=20= the=20reuse=20of=20the=20parser=20between=20requests.=20Reusing=20these=20= is=0Arisky=20in=20the=20context=20of=20running=20any=20other=20threads=20= within=20the=20unicorn=0Aprocess,=20also=20for=20threads=20that=20run=20= background=20tasks.=0A=0AIf=20any=20other=20thread=20accidentally=20= grabs=20hold=20of=20the=20request=20it=20can=20modify=0Athings=20for=20= the=20next=20request=20in=20flight.=0A=0AThe=20downside=20here=20is=20= that=20we=20allocate=20more=20for=20each=20request,=20but=20that=20is=0A= worth=20the=20trade=20off=20here=20and=20the=20security=20risk=20we=20= otherwise=20would=20carry=0Ato=20leaking=20wrong=20and=20incorrect=20= data.=0A---=0A=20lib/unicorn/http_server.rb=20|=202=20+-=0A=201=20file=20= changed,=201=20insertion(+),=201=20deletion(-)=0A=0Adiff=20--git=20= a/lib/unicorn/http_server.rb=20b/lib/unicorn/http_server.rb=0Aindex=20= c0f14ba..22f067f=20100644=0A---=20a/lib/unicorn/http_server.rb=0A+++=20= b/lib/unicorn/http_server.rb=0A@@=20-69,7=20+69,6=20@@=20class=20= Unicorn::HttpServer=0A=20=20=20#=20incoming=20requests=20on=20the=20= socket.=0A=20=20=20def=20initialize(app,=20options=20=3D=20{})=0A=20=20=20= =20=20@app=20=3D=20app=0A-=20=20=20=20@request=20=3D=20= Unicorn::HttpRequest.new=0A=20=20=20=20=20@reexec_pid=20=3D=200=0A=20=20=20= =20=20@default_middleware=20=3D=20true=0A=20=20=20=20=20options=20=3D=20= options.dup=0A@@=20-621,6=20+620,7=20@@=20def=20= e100_response_write(client,=20env)=0A=20=20=20#=20once=20a=20client=20is=20= accepted,=20it=20is=20processed=20in=20its=20entirety=20here=0A=20=20=20= #=20in=203=20easy=20steps:=20read=20request,=20call=20app,=20write=20app=20= response=0A=20=20=20def=20process_client(client)=0A+=20=20=20=20@request=20= =3D=20Unicorn::HttpRequest.new=0A=20=20=20=20=20env=20=3D=20= @request.read(client)=0A=20=0A=20=20=20=20=20if=20early_hints=0A--=20=0A= 2.30.1=0A=0A= --Apple-Mail=_2A064DCC-B3E0-4F15-BE93-244E86F804B4--