net/http: accepts invalid empty Content-Length header

[kenballus]

What version of Go are you using (go version)?

$ go version
go version devel go1.22-977e23a707 Mon Jul 31 19:10:40 2023 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes.

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

$ 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-977e23a707 Mon Jul 31 19:10:40 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-build2136977550=/tmp/go-build -gno-record-gcc-switches'

What did you do?

1. Started an HTTP server using net/http.
2. Sent it the following request:

GET / HTTP/1.1\r\n
Host: whatever\r\n
Content-Length: \r\n
Test: test\r\n
\r\n

What did you expect to see?

A 400 response informing me that the request is invalid beacuse its Content-Length header does not conform to the grammar rule from the relevant RFCs.
Section 8.6 of RFC 9110 defines the acceptable values in a Content-Length field value as follows:

Content-Length = 1*DIGIT

DIGIT is defined as follows in RFC 5234:

DIGIT = %x30-39

What did you see instead?

A 200 response. The server interpreted the empty Content-Length header as though it were Content-Length: 0. This is potentially of use in a request smuggling exploit chain, because the semantics of an empty CL header are not defined, as far as I am aware.

Suggested fix

AIOHTTP, Apache httpd, Boost::Beast, Gunicorn, H2O, IIS, jetty, lighttpd, NGINX, Node.js, Apache Tomcat, and Tornado all reject requests containing empty CL headers. I suggest that net/http do the same.

[mauri870]

I'm investigating this issue and I'm not sure what is the proper way to handle this. Most places read the Content-Length header from Header.Get (quite a lot of places across net/http), which returns a string. If the string is empty it is assumed that the header was not sent. What is the proper way to differentiate empty header value from it being absent?

[bcmills]

What is the proper way to differentiate empty header value from it being absent?

In general, len(h.Values("Content-Length")) > 0 will tell you whether the header is present.

That said, probably the simplest fix is to have parseContentLength accept a []string instead of a single string.
Then it can be something like:

var laxContentLength = godebug.New("httplaxcontentlength")

func parseContentLength(clHeaders []string) (int64, error) {
	if len(clHeaders) == 0 {
		return -1, nil
	}
	cl := textproto.TrimString(clHeaders[0])
	if cl == "" {
		if laxContentLength.Value() == "1" {
			return -1, nil
		}
		return 0, badStringError(…)
	}
	…
}

Then the call site in fixLength becomes:

	if len(contentLens) > 0 {
		n, err := parseContentLength(contentLens)
		if err != nil {
			return -1, err
		}
		return n, nil
	}

And the call in readTransfer becomes:

		if n, err := parseContentLength(t.Header["Content-Length"]); err != nil {
			return err
		} else {
			t.ContentLength = n
		}

[gopherbot]

Change https://go.dev/cl/517336 mentions this issue: net/http: disallow empty Content-Length header

[neild]

The server interpreted the empty Content-Length header as though it were Content-Length: 0.

I don't think this is correct. parseContentLength treats an empty Content-Length header as though it were missing.

Given that, this doesn't look like a viable request smuggling vector to me. Without a Content-Length, we'll consume the remainder of the connection as the request body.

Is it worth rejecting requests with an empty Content-Length? As with any change, this has the potential to break users depending on the current behavior. Is there enough benefit to justify that risk?

[kenballus]

Without a Content-Length, we'll consume the remainder of the connection as the request body.

This is not the correct behavior either. From RFC 9112:

7. If this is a request message and none of the above are true, then the message body length is zero (no message body is present).
8. Otherwise, this is a response message without a declared message body length, so the message body length is determined by the number of octets received prior to the server closing the connection.

Thus, ignoring the empty Content-Length header should (and does; I tested it) cause net/http to interpret the field as though it were 0 (which is equivalent to ignoring it).

This is a problem because other servers (and proxies...) interpret the empty CL header in exactly the way you described (by consuming the remaining bytes on the line). This discrepancy is a request smuggling vector.

The empty header value should not be allowed at all. It should be rejected when encountered, along with all other malformed CL headers. This is what all of the large web servers do, and this is what net/http should do as well.

[kenballus]

Is it worth rejecting requests with an empty Content-Length? As with any change, this has the potential to break users depending on the current behavior. Is there enough benefit to justify that risk?

I assert that close to zero users will be affected, because any HTTP client generating requests with empty CL headers would be incompatible with the vast majority of the web. (Apache and Nginx both reject all messages containing empty CL headers)

Thus, it seems likely to me that their only use case is request smuggling.

[neild]

Right, I'd forgotten that request and response behave differently when Content-Length is missing.

I'm convinced, rejecting invalid values is the right thing to do.

[gopherbot]

Change https://go.dev/cl/549198 mentions this issue: doc/go1.22: document minor net/http changes