Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Backport fix for CVE-2024-24791 #242

Open
wants to merge 1 commit into
base: go1.20-fips-release
Choose a base branch
from
Open

Backport fix for CVE-2024-24791 #242

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
353 changes: 353 additions & 0 deletions patches/012-fix-CVE-2024-24791.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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

Loading