From a4019cc5608c5220c107b38d129a39bea21063d3 Mon Sep 17 00:00:00 2001
From: David Benoit <dbenoit@redhat.com>
Date: Tue, 8 Oct 2024 07:59:28 -0400
Subject: [PATCH] Backport fix for CVE-2024-24791
---
patches/012-fix-CVE-2024-24791.patch | 353 +++++++++++++++++++++++++++
1 file changed, 353 insertions(+)
create mode 100644 patches/012-fix-CVE-2024-24791.patch
diff --git a/patches/012-fix-CVE-2024-24791.patch b/patches/012-fix-CVE-2024-24791.patch
new file mode 100644
index 0000000000..e08d32ee81
--- /dev/null
+++ b/patches/012-fix-CVE-2024-24791.patch
@@ -0,0 +1,353 @@
+From a42505fbb26a4f301bdd19cd102d345759d7bc65 Mon Sep 17 00:00:00 2001
+From: Damien Neil <dneil@google.com>
+Date: Thu, 6 Jun 2024 12:50:46 -0700
+Subject: [PATCH] [release-branch.go1.21] net/http: send body or close
+ connection on expect-100-continue requests
+
+When sending a request with an "Expect: 100-continue" header,
+we must send the request body before sending any further requests
+on the connection.
+
+When receiving a non-1xx response to an "Expect: 100-continue" request,
+send the request body if the connection isn't being closed after
+processing the response. In other words, if either the request
+or response contains a "Connection: close" header, then skip sending
+the request body (because the connection will not be used for
+further requests), but otherwise send it.
+
+Correct a comment on the server-side Expect: 100-continue handling
+that implied sending the request body is optional. It isn't.
+
+For #67555
+Fixes #68199
+
+Change-Id: Ia2f12091bee697771087f32ac347509ec5922d54
+Reviewed-on: https://go-review.googlesource.com/c/go/+/591255
+LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
+Reviewed-by: Jonathan Amsterdam <jba@google.com>
+(cherry picked from commit cf501e05e138e6911f759a5db786e90b295499b9)
+Reviewed-on: https://go-review.googlesource.com/c/go/+/595096
+Reviewed-by: Joedian Reid <joedian@google.com>
+Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
+---
+ src/net/http/server.go | 25 ++--
+ src/net/http/transport.go | 34 ++++--
+ src/net/http/transport_test.go | 202 ++++++++++++++++++++-------------
+ 3 files changed, 164 insertions(+), 97 deletions(-)
+
+diff --git a/src/net/http/server.go b/src/net/http/server.go
+index c3c3f91d9a..85c04daa95 100644
+--- a/src/net/http/server.go
++++ b/src/net/http/server.go
+@@ -1340,16 +1340,21 @@ func (cw *chunkWriter) writeHeader(p []byte) {
+
+ // If the client wanted a 100-continue but we never sent it to
+ // them (or, more strictly: we never finished reading their
+- // request body), don't reuse this connection because it's now
+- // in an unknown state: we might be sending this response at
+- // the same time the client is now sending its request body
+- // after a timeout. (Some HTTP clients send Expect:
+- // 100-continue but knowing that some servers don't support
+- // it, the clients set a timer and send the body later anyway)
+- // If we haven't seen EOF, we can't skip over the unread body
+- // because we don't know if the next bytes on the wire will be
+- // the body-following-the-timer or the subsequent request.
+- // See Issue 11549.
++ // request body), don't reuse this connection.
++ //
++ // This behavior was first added on the theory that we don't know
++ // if the next bytes on the wire are going to be the remainder of
++ // the request body or the subsequent request (see issue 11549),
++ // but that's not correct: If we keep using the connection,
++ // the client is required to send the request body whether we
++ // asked for it or not.
++ //
++ // We probably do want to skip reusing the connection in most cases,
++ // however. If the client is offering a large request body that we
++ // don't intend to use, then it's better to close the connection
++ // than to read the body. For now, assume that if we're sending
++ // headers, the handler is done reading the body and we should
++ // drop the connection if we haven't seen EOF.
+ if ecr, ok := w.req.Body.(*expectContinueReader); ok && !ecr.sawEOF.Load() {
+ w.closeAfterReply = true
+ }
+diff --git a/src/net/http/transport.go b/src/net/http/transport.go
+index ddcb64815c..e4a2cc91b9 100644
+--- a/src/net/http/transport.go
++++ b/src/net/http/transport.go
+@@ -2302,17 +2302,12 @@ func (pc *persistConn) readResponse(rc requestAndChan, trace *httptrace.ClientTr
+ return
+ }
+ resCode := resp.StatusCode
+- if continueCh != nil {
+- if resCode == 100 {
+- if trace != nil && trace.Got100Continue != nil {
+- trace.Got100Continue()
+- }
+- continueCh <- struct{}{}
+- continueCh = nil
+- } else if resCode >= 200 {
+- close(continueCh)
+- continueCh = nil
++ if continueCh != nil && resCode == StatusContinue {
++ if trace != nil && trace.Got100Continue != nil {
++ trace.Got100Continue()
+ }
++ continueCh <- struct{}{}
++ continueCh = nil
+ }
+ is1xx := 100 <= resCode && resCode <= 199
+ // treat 101 as a terminal status, see issue 26161
+@@ -2335,6 +2330,25 @@ func (pc *persistConn) readResponse(rc requestAndChan, trace *httptrace.ClientTr
+ if resp.isProtocolSwitch() {
+ resp.Body = newReadWriteCloserBody(pc.br, pc.conn)
+ }
++ if continueCh != nil {
++ // We send an "Expect: 100-continue" header, but the server
++ // responded with a terminal status and no 100 Continue.
++ //
++ // If we're going to keep using the connection, we need to send the request body.
++ // Tell writeLoop to skip sending the body if we're going to close the connection,
++ // or to send it otherwise.
++ //
++ // The case where we receive a 101 Switching Protocols response is a bit
++ // ambiguous, since we don't know what protocol we're switching to.
++ // Conceivably, it's one that doesn't need us to send the body.
++ // Given that we'll send the body if ExpectContinueTimeout expires,
++ // be consistent and always send it if we aren't closing the connection.
++ if resp.Close || rc.req.Close {
++ close(continueCh) // don't send the body; the connection will close
++ } else {
++ continueCh <- struct{}{} // send the body
++ }
++ }
+
+ resp.TLS = pc.tlsState
+ return
+diff --git a/src/net/http/transport_test.go b/src/net/http/transport_test.go
+index f4896c5026..c3ace9087c 100644
+--- a/src/net/http/transport_test.go
++++ b/src/net/http/transport_test.go
+@@ -1140,94 +1140,142 @@ func testTransportGzip(t *testing.T, mode testMode) {
+ }
+ }
+
+-// If a request has Expect:100-continue header, the request blocks sending body until the first response.
+-// Premature consumption of the request body should not be occurred.
+-func TestTransportExpect100Continue(t *testing.T) {
+- run(t, testTransportExpect100Continue, []testMode{http1Mode})
++// A transport100Continue test exercises Transport behaviors when sending a
++// request with an Expect: 100-continue header.
++type transport100ContinueTest struct {
++ t *testing.T
++
++ reqdone chan struct{}
++ resp *Response
++ respErr error
++
++ conn net.Conn
++ reader *bufio.Reader
+ }
+-func testTransportExpect100Continue(t *testing.T, mode testMode) {
+- ts := newClientServerTest(t, mode, HandlerFunc(func(rw ResponseWriter, req *Request) {
+- switch req.URL.Path {
+- case "/100":
+- // This endpoint implicitly responds 100 Continue and reads body.
+- if _, err := io.Copy(io.Discard, req.Body); err != nil {
+- t.Error("Failed to read Body", err)
+- }
+- rw.WriteHeader(StatusOK)
+- case "/200":
+- // Go 1.5 adds Connection: close header if the client expect
+- // continue but not entire request body is consumed.
+- rw.WriteHeader(StatusOK)
+- case "/500":
+- rw.WriteHeader(StatusInternalServerError)
+- case "/keepalive":
+- // This hijacked endpoint responds error without Connection:close.
+- _, bufrw, err := rw.(Hijacker).Hijack()
+- if err != nil {
+- log.Fatal(err)
+- }
+- bufrw.WriteString("HTTP/1.1 500 Internal Server Error\r\n")
+- bufrw.WriteString("Content-Length: 0\r\n\r\n")
+- bufrw.Flush()
+- case "/timeout":
+- // This endpoint tries to read body without 100 (Continue) response.
+- // After ExpectContinueTimeout, the reading will be started.
+- conn, bufrw, err := rw.(Hijacker).Hijack()
+- if err != nil {
+- log.Fatal(err)
+- }
+- if _, err := io.CopyN(io.Discard, bufrw, req.ContentLength); err != nil {
+- t.Error("Failed to read Body", err)
+- }
+- bufrw.WriteString("HTTP/1.1 200 OK\r\n\r\n")
+- bufrw.Flush()
+- conn.Close()
+- }
+
+- })).ts
++const transport100ContinueTestBody = "request body"
+
+- tests := []struct {
+- path string
+- body []byte
+- sent int
+- status int
+- }{
+- {path: "/100", body: []byte("hello"), sent: 5, status: 200}, // Got 100 followed by 200, entire body is sent.
+- {path: "/200", body: []byte("hello"), sent: 0, status: 200}, // Got 200 without 100. body isn't sent.
+- {path: "/500", body: []byte("hello"), sent: 0, status: 500}, // Got 500 without 100. body isn't sent.
+- {path: "/keepalive", body: []byte("hello"), sent: 0, status: 500}, // Although without Connection:close, body isn't sent.
+- {path: "/timeout", body: []byte("hello"), sent: 5, status: 200}, // Timeout exceeded and entire body is sent.
++// newTransport100ContinueTest creates a Transport and sends an Expect: 100-continue
++// request on it.
++func newTransport100ContinueTest(t *testing.T, timeout time.Duration) *transport100ContinueTest {
++ ln := newLocalListener(t)
++ defer ln.Close()
++
++ test := &transport100ContinueTest{
++ t: t,
++ reqdone: make(chan struct{}),
+ }
+
+- c := ts.Client()
+- for i, v := range tests {
+- tr := &Transport{
+- ExpectContinueTimeout: 2 * time.Second,
+- }
+- defer tr.CloseIdleConnections()
+- c.Transport = tr
+- body := bytes.NewReader(v.body)
+- req, err := NewRequest("PUT", ts.URL+v.path, body)
+- if err != nil {
+- t.Fatal(err)
+- }
++ tr := &Transport{
++ ExpectContinueTimeout: timeout,
++ }
++ go func() {
++ defer close(test.reqdone)
++ body := strings.NewReader(transport100ContinueTestBody)
++ req, _ := NewRequest("PUT", "http://"+ln.Addr().String(), body)
+ req.Header.Set("Expect", "100-continue")
+- req.ContentLength = int64(len(v.body))
++ req.ContentLength = int64(len(transport100ContinueTestBody))
++ test.resp, test.respErr = tr.RoundTrip(req)
++ test.resp.Body.Close()
++ }()
+
+- resp, err := c.Do(req)
+- if err != nil {
+- t.Fatal(err)
++ c, err := ln.Accept()
++ if err != nil {
++ t.Fatalf("Accept: %v", err)
++ }
++ t.Cleanup(func() {
++ c.Close()
++ })
++ br := bufio.NewReader(c)
++ _, err = ReadRequest(br)
++ if err != nil {
++ t.Fatalf("ReadRequest: %v", err)
++ }
++ test.conn = c
++ test.reader = br
++ t.Cleanup(func() {
++ <-test.reqdone
++ tr.CloseIdleConnections()
++ got, _ := io.ReadAll(test.reader)
++ if len(got) > 0 {
++ t.Fatalf("Transport sent unexpected bytes: %q", got)
+ }
+- resp.Body.Close()
++ })
+
+- sent := len(v.body) - body.Len()
+- if v.status != resp.StatusCode {
+- t.Errorf("test %d: status code should be %d but got %d. (%s)", i, v.status, resp.StatusCode, v.path)
+- }
+- if v.sent != sent {
+- t.Errorf("test %d: sent body should be %d but sent %d. (%s)", i, v.sent, sent, v.path)
++ return test
++}
++
++// respond sends response lines from the server to the transport.
++func (test *transport100ContinueTest) respond(lines ...string) {
++ for _, line := range lines {
++ if _, err := test.conn.Write([]byte(line + "\r\n")); err != nil {
++ test.t.Fatalf("Write: %v", err)
+ }
+ }
++ if _, err := test.conn.Write([]byte("\r\n")); err != nil {
++ test.t.Fatalf("Write: %v", err)
++ }
++}
++
++// wantBodySent ensures the transport has sent the request body to the server.
++func (test *transport100ContinueTest) wantBodySent() {
++ got, err := io.ReadAll(io.LimitReader(test.reader, int64(len(transport100ContinueTestBody))))
++ if err != nil {
++ test.t.Fatalf("unexpected error reading body: %v", err)
++ }
++ if got, want := string(got), transport100ContinueTestBody; got != want {
++ test.t.Fatalf("unexpected body: got %q, want %q", got, want)
++ }
++}
++
++// wantRequestDone ensures the Transport.RoundTrip has completed with the expected status.
++func (test *transport100ContinueTest) wantRequestDone(want int) {
++ <-test.reqdone
++ if test.respErr != nil {
++ test.t.Fatalf("unexpected RoundTrip error: %v", test.respErr)
++ }
++ if got := test.resp.StatusCode; got != want {
++ test.t.Fatalf("unexpected response code: got %v, want %v", got, want)
++ }
++}
++
++func TestTransportExpect100ContinueSent(t *testing.T) {
++ test := newTransport100ContinueTest(t, 1*time.Hour)
++ // Server sends a 100 Continue response, and the client sends the request body.
++ test.respond("HTTP/1.1 100 Continue")
++ test.wantBodySent()
++ test.respond("HTTP/1.1 200", "Content-Length: 0")
++ test.wantRequestDone(200)
++}
++
++func TestTransportExpect100Continue200ResponseNoConnClose(t *testing.T) {
++ test := newTransport100ContinueTest(t, 1*time.Hour)
++ // No 100 Continue response, no Connection: close header.
++ test.respond("HTTP/1.1 200", "Content-Length: 0")
++ test.wantBodySent()
++ test.wantRequestDone(200)
++}
++
++func TestTransportExpect100Continue200ResponseWithConnClose(t *testing.T) {
++ test := newTransport100ContinueTest(t, 1*time.Hour)
++ // No 100 Continue response, Connection: close header set.
++ test.respond("HTTP/1.1 200", "Connection: close", "Content-Length: 0")
++ test.wantRequestDone(200)
++}
++
++func TestTransportExpect100Continue500ResponseNoConnClose(t *testing.T) {
++ test := newTransport100ContinueTest(t, 1*time.Hour)
++ // No 100 Continue response, no Connection: close header.
++ test.respond("HTTP/1.1 500", "Content-Length: 0")
++ test.wantBodySent()
++ test.wantRequestDone(500)
++}
++
++func TestTransportExpect100Continue500ResponseTimeout(t *testing.T) {
++ test := newTransport100ContinueTest(t, 5*time.Millisecond) // short timeout
++ test.wantBodySent() // after timeout
++ test.respond("HTTP/1.1 200", "Content-Length: 0")
++ test.wantRequestDone(200)
+ }
+
+ func TestSOCKS5Proxy(t *testing.T) {
+--
+2.46.0
+