Skip to content

Backport of fix for CVE-2024-24791 to Go1.19 #220

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

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Backport of fix for CVE-2024-24791 to Go1.19 #220

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
331 changes: 331 additions & 0 deletions patches/011-fix-CVE-2024-24791.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,331 @@
From a5e9aab67d3c4ff3ded35495ae03fa34f12e950b Mon Sep 17 00:00:00 2001
From: Archana Ravindar <aravinda@redhat.com>
Date: Mon, 12 Aug 2024 19:40:21 +0530
Subject: [PATCH] - backport fix for CVE-2024-24791 to Go1.19 -
https://go-review.googlesource.com/c/go/+/595096

---
src/net/http/server.go | 25 ++--
src/net/http/transport.go | 35 ++++--
src/net/http/transport_test.go | 205 ++++++++++++++++++++-------------
3 files changed, 166 insertions(+), 99 deletions(-)

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 87dd412984..fdcc3a9136 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -1344,16 +1344,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.isSet() {
w.closeAfterReply = true
}
diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index e470a6c080..083c907184 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -2288,17 +2288,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
@@ -2322,6 +2317,26 @@ func (pc *persistConn) readResponse(rc requestAndChan, trace *httptrace.ClientTr
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 985d0625dc..63bcf0384b 100644
--- a/src/net/http/transport_test.go
+++ b/src/net/http/transport_test.go
@@ -1150,95 +1150,142 @@ func TestTransportGzip(t *testing.T) {
}
}

-// 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) {
- setParallel(t)
- defer afterTest(t)
+// A transport100Continue test exercises Transport behaviors when sending a
+// request with an Expect: 100-continue header.
+type transport100ContinueTest struct {
+ t *testing.T

- ts := httptest.NewServer(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()
- }
+ reqdone chan struct{}
+ resp *Response
+ respErr error

- }))
- defer ts.Close()
+ conn net.Conn
+ reader *bufio.Reader
+}

- 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.
+const transport100ContinueTestBody = "request body"
+
+// 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)
- }
- req.Header.Set("Expect", "100-continue")
- req.ContentLength = int64(len(v.body))
+ 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(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.39.3