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.

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

net/http: Chunked request body incorrectly terminated on \r\n\r\n instead of 0\r\n\r\n #64517

Closed
kenballus opened this issue Dec 2, 2023 · 8 comments
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@kenballus
Copy link

Go version

go version devel go1.22-2e6387cbec Fri Dec 1 18:47:51 2023 +0000 linux/amd64

What operating system and processor architecture are you using (go env)?

GO111MODULE=''
GOARCH='amd64'
GOBIN=''
GOCACHE='/root/.cache/go-build'
GOENV='/root/.config/go/env'
GOEXE=''
GOEXPERIMENT=''
GOFLAGS=''
GOHOSTARCH='amd64'
GOHOSTOS='linux'
GOINSECURE=''
GOMODCACHE='/root/go/pkg/mod'
GONOPROXY=''
GONOSUMDB=''
GOOS='linux'
GOPATH='/root/go'
GOPRIVATE=''
GOPROXY='https://proxy.golang.org,direct'
GOROOT='/app/go'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/app/go/pkg/tool/linux_amd64'
GOVCS=''
GOVERSION='devel go1.22-2e6387cbec Fri Dec 1 18:47:51 2023 +0000'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='0'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -m64 -fno-caret-diagnostics -Qunused-arguments -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1615445855=/tmp/go-build -gno-record-gcc-switches'

What did you do?

  1. Compile and run the following program:
package main

import (
    "fmt"
    "net/http"
)

func handle_request(w http.ResponseWriter, req *http.Request) {
    fmt.Fprintf(w, "request received.\n")
}

func main() {
    s := &http.Server{
        Addr: "127.0.0.1:8080",
        Handler: http.HandlerFunc(handle_request),
        MaxHeaderBytes: 1 << 20,
    }

    s.ListenAndServe()
}
  1. Send the following payload to the server (for instance, with nc):
GET / HTTP/1.1\r\n
Host: whatever\r\n
Transfer-Encoding: chunked\r\n
\r\n
\r\n
\r\n

What did you expect to see?

The server should either respond 400 or time out, because the chunked message body is invalid. A chunked message body must be terminated with 0\r\n\r\n. Terminating chunked message bodies on \r\n\r\n alone introduces risk from any gateway that may have interpreted the request framing differently.

What did you see instead?

The server responds 200.

@bcmills
Copy link
Member

bcmills commented Dec 4, 2023

(attn @neild)

@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 4, 2023
@kenballus
Copy link
Author

Turns out that some servers ignore extra CRLFs between chunks. Here's a payload that demonstrates that discrepancy:

POST / HTTP/1.1\r\n
Host: a\r\n
Transfer-Encoding: chunked\r\n
\r\n
1\r\n
X\r\n
\r\n
\r\n                  <-- Go sees this as the end of the request, with body "X"
1\r\n
Y\r\n
0\r\n
\r\n                  <-- Jetty and Waitress see this as the end of the request, with body "XY"

If there were a way to get Go to see a request line where Jetty and Waitress see a chunk size (and extension), then there might be a way to make a request smuggling attack out of this? Not sure if that's possible.

@kenballus
Copy link
Author

The simplest patch is probably this:

diff --git a/src/net/http/internal/chunked.go b/src/net/http/internal/chunked.go
index aad8e5aa09..044a79d053 100644
--- a/src/net/http/internal/chunked.go
+++ b/src/net/http/internal/chunked.go
@@ -263,6 +263,9 @@ type FlushAfterChunkWriter struct {
 }

 func parseHexUint(v []byte) (n uint64, err error) {
+       if len(v) == 0 {
+               return 0, errors.New("invalid empty chunk length")
+       }
        for i, b := range v {
                switch {
                case '0' <= b && b <= '9':

This would just make the connection close after the invalid message, but it wouldn't make the response 400, so it's not an ideal fix on its own.

@gopherbot
Copy link

Change https://go.dev/cl/553835 mentions this issue: net/http: respond with 400 Bad Request for empty hex number of chunk length

@panjf2000 panjf2000 added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 4, 2024
@panjf2000 panjf2000 added this to the Go1.22 milestone Jan 4, 2024
@panjf2000
Copy link
Member

Actually, it would respond with 400 Bad Request when parseHexUint returns a non-error. Therefore, I think your suggestion is sufficient for this kind of case.

@panjf2000 panjf2000 self-assigned this Jan 4, 2024
@kenballus
Copy link
Author

kenballus commented Jan 5, 2024

Actually, it would respond with 400 Bad Request when parseHexUint returns a non-error. Therefore, I think your suggestion is sufficient for this kind of case.

(I'm assuming you meant "error" instead of "non-error" here.)

This isn't true. It only closes the connection; it does not respond 400. To see for yourself, build master (current commit is 8db1310) and run the following simple program:

package main

import (
    "fmt"
    "net/http"
)

func handle_request(w http.ResponseWriter, req *http.Request) {
    fmt.Fprintf(w, "Hello world\n")
}

func main() {
    s := &http.Server{
        Addr: "127.0.0.1:8080",
        Handler: http.HandlerFunc(handle_request),
        MaxHeaderBytes: 1 << 20,
    }

    s.ListenAndServe()
}

Then send it a request with an empty chunk size:

printf 'POST / HTTP/1.1\r\nHost: a\r\nTransfer-Encoding: chunked\r\n\r\n\r\n\r\n' | nc localhost 8080

You should see a valid response:

HTTP/1.1 200 OK
Date: Fri, 05 Jan 2024 00:01:42 GMT
Content-Length: 12
Content-Type: text/plain; charset=utf-8
Connection: close

Hello world

The only difference between the current behavior and the previous behavior is that now the connection is closed after the invalid message body is processed. This should really be a 400; not just a closed connection. I would appreciate it if someone would reopen this issue.

@panjf2000
Copy link
Member

panjf2000 commented Jan 5, 2024

To clarify, you need to do things explicitly with the current fix, something like this:

func echoHandler(w http.ResponseWriter, r *http.Request) {
	body, err := io.ReadAll(r.Body)
	if err != nil {
		http.Error(w, err.Error(), http.StatusBadRequest)
		return
	}

	fmt.Fprintf(w, "Received: %s", body)
}

because it's lazy read for http.Request.Body in Go net/http, which means that it won't read the request body until you do it in http.HandlerFunc, unlike other HTTP frameworks, such as fasthttp that is early read for fasthttp.RequestCtx.PostBody(), which by contrast means that it will read the request body and handle errors before fasthttp.RequestHandler is invoked. Therefore, if you use fasthttp for your example, you will find it works as expected.

On the code level, there is no doubt that we can change the current pattern of net/http to be like fasthttp, but I am afraid that it might break something critical somewhere and hurt the backward compatibility of Go badly. To be frank, I'd say the current fix is balanced or cost-effective rather than perfect.

@kenballus
Copy link
Author

because it's lazy read for http.Request.Body in Go net/http

This is unorthodox, but okay. Thanks for letting me know.

I am afraid that it might break something critical somewhere

I think it's damned if you do, damned if you don't. While changing this behavior would probably break someone's work somewhere, not changing it is probably silently breaking people's expectations now.

To be frank, I'd say the current fix is balanced or cost-effective rather than perfect.

Agreed.

ezz-no pushed a commit to ezz-no/go-ezzno that referenced this issue Feb 18, 2024
…length

Fixes golang#64517

Change-Id: I78b8a6a83301deee05c3ff052a6adcd1f965aef2
Reviewed-on: https://go-review.googlesource.com/c/go/+/553835
Auto-Submit: Damien Neil <dneil@google.com>
Commit-Queue: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Bryan Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Development

No branches or pull requests

4 participants