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

[kenballus]

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?

0. 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]

(attn @neild)

[kenballus]

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]

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]

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

[panjf2000]

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.

[kenballus]

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 https://github.com/golang/go/commit/8db131082d08e497fd8e9383d0ff7715e1bef478) 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]

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]

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.