net/http: `Content-Length` values are not validated when `Transfer-Encoding: chunked` is present #65505

kenballus · 2024-02-04T03:29:01Z

Go version

go version devel go1.23-b8ac61e6e6 Fri Feb 2 23:14:07 2024 +0000 linux/amd64

Output of `go env` in your module/workspace:

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.23-b8ac61e6e6 Fri Feb 2 23:14:07 2024 +0000'
GCCGO='gccgo'
GOAMD64='v1'
AR='ar'
CC='gcc'
CXX='g++'
CGO_ENABLED='1'
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 -pthread -Wl,--no-gc-sections -fmessage-length=0 -ffile-prefix-map=/tmp/go-build1902103395=/tmp/go-build -gno-record-gcc-switches'

What did you do?

Started a web server using net/http, and sent a request with an invalid Content-Length header, as well as a Transfer-Encoding: chunked header. (e.g. GET / HTTP/1.1\r\nHost: a\r\nContent-Length: blahblahblah\r\nTransfer-Encoding: chunked\r\n\r\n0\r\n\r\n)

What did you see happen?

I received a 200 response, as though the invalid Content-Length value were not sent.

What did you expect to see?

I expected to see a 400 response because the Content-Length value was invalid.

The text was updated successfully, but these errors were encountered:

kenballus · 2024-02-04T03:33:59Z

This discrepancy was found using the HTTP Garden.

bestgopher · 2024-02-04T03:47:43Z

According to https://greenbytes.de/tech/webdav/rfc2616.html#rfc.section.4.4.p.5, if the message does include a non-identity transfer-coding, the Content-Length must be ignored.

panjf2000 · 2024-02-04T04:04:33Z

This is the expected behavior, please check out:

go/src/net/http/transfer.go

Lines 630 to 666 in b8ac61e

    
           func (t *transferReader) parseTransferEncoding() error { 
        
           	raw, present := t.Header["Transfer-Encoding"] 
        
           	if !present { 
        
           		return nil 
        
           	} 
        
           	delete(t.Header, "Transfer-Encoding") 
        
           	// Issue 12785; ignore Transfer-Encoding on HTTP/1.0 requests. 
        
           	if !t.protoAtLeast(1, 1) { 
        
           		return nil 
        
           	} 
        
           	// Like nginx, we only support a single Transfer-Encoding header field, and 
        
           	// only if set to "chunked". This is one of the most security sensitive 
        
           	// surfaces in HTTP/1.1 due to the risk of request smuggling, so we keep it 
        
           	// strict and simple. 
        
           	if len(raw) != 1 { 
        
           		return &unsupportedTEError{fmt.Sprintf("too many transfer encodings: %q", raw)} 
        
           	} 
        
           	if !ascii.EqualFold(raw[0], "chunked") { 
        
           		return &unsupportedTEError{fmt.Sprintf("unsupported transfer encoding: %q", raw[0])} 
        
           	} 
        
           	// RFC 7230 3.3.2 says "A sender MUST NOT send a Content-Length header field 
        
           	// in any message that contains a Transfer-Encoding header field." 
        
           	// 
        
           	// but also: "If a message is received with both a Transfer-Encoding and a 
        
           	// Content-Length header field, the Transfer-Encoding overrides the 
        
           	// Content-Length. Such a message might indicate an attempt to perform 
        
           	// request smuggling (Section 9.5) or response splitting (Section 9.4) and 
        
           	// ought to be handled as an error. A sender MUST remove the received 
        
           	// Content-Length field prior to forwarding such a message downstream." 
        
           	// 
        
           	// Reportedly, these appear in the wild. 
        
           	delete(t.Header, "Content-Length") 
        
           	t.Chunked = true

kenballus · 2024-02-04T04:09:21Z

RFC 9110 is clear that gateway servers must reject messages that contain an invalid Content-Length value:

Likewise, a sender MUST NOT forward a message with a Content-Length header field value that does not match the ABNF above, with one exception: a recipient of a Content-Length header field value consisting of the same decimal value repeated as a comma-separated list (e.g, "Content-Length: 42, 42") MAY either reject the message as invalid or replace that invalid field value with a single instance of the decimal value, since this likely indicates that a duplicate was generated or combined by an upstream message processor.

A gateway server that is implemented on top of net/http has no way of conforming to this rule. Caddy's reverse proxy violates the above rule for this reason.

kenballus · 2024-02-04T04:15:33Z

To be clear, I'm not saying that valid Content-Length values should be interpreted when a Transfer-Encoding header is present. They should definitely be ignored. What I'm saying is that invalid Content-Length headers should invalidate the request regardless of the presence of a Transfer-Encoding header.

This is what nearly all other HTTP implementations do, including AIOHTTP, Apache httpd, Bun, cherrypy, daphne, Deno, fasthttp, Gunicorn, Hyper, Hypercorn, Jetty, Lighttpd, Mongoose, Nginx, Node.js, LiteSpeed, Passenger, Apache Tomcat, Tornado, OpenWrt uhttpd, Unicorn, Uvicorn, WEBrick, HAProxy, Pound, and Varnish.

kenballus · 2024-02-04T04:21:35Z

According to https://greenbytes.de/tech/webdav/rfc2616.html#rfc.section.4.4.p.5, if the message does include a non-identity transfer-coding, the Content-Length must be ignored.

RFC 2616 is obsoleted by RFC 7230, which is itself obsoleted by RFC 9112.

The current version of the text that you're referencing is this:

If a message is received with both a Transfer-Encoding and a Content-Length header field, the Transfer-Encoding overrides the Content-Length. Such a message might indicate an attempt to perform request smuggling (Section 11.2) or response splitting (Section 11.1) and ought to be handled as an error. An intermediary that chooses to forward the message MUST first remove the received Content-Length field and process the Transfer-Encoding (as described below) prior to forwarding the message downstream.

Note that it does not say to ignore the Content-Length header; it instead says that an intermediary that chooses to forward a message containing both Content-Length and Transfer-Encoding must remove the Content-Length header. Since it also says that an intermediary MUST not forward requests with invalid Content-Length headers, my original point stands.

panjf2000 · 2024-02-04T05:00:06Z

It is a bit confusing about the statements of Content-Length across these RFCs, and the statement from the latest RFC 9112 still looks ambiguous to me.

bestgopher · 2024-02-04T07:05:56Z

Note that it does not say to ignore the Content-Length header; it instead says that an intermediary that chooses to forward a message containing both Content-Length and Transfer-Encoding must remove the Content-Length header. Since it also says that an intermediary MUST not forward requests with invalid Content-Length headers, my original point stands.

In my opinion, an intermediary only removes the Content-Length header rather than checks the value of header is valid or not prior to remove it .

panjf2000 · 2024-02-04T07:30:17Z

After reading through the sections of Content-Length in RFC 9110 and 9112, along with browsing the source code in net/http, I find that rejecting requests with invalid Content-Length headers regardless of the presence of a Transfer-Encoding header seems like more rational than ignoring them if that's also how other mainstream HTTP implementations do.

I'll send a CL for this, but I need some comments from @neild on this before we submit it.

gopherbot · 2024-02-04T08:03:56Z

Change https://go.dev/cl/561075 mentions this issue: net/http: reject requests with invalid Content-Length in Header

kenballus · 2024-02-04T16:43:24Z

In my opinion, an intermediary only removes the Content-Length header rather than checks the value of header is valid or not prior to remove it .

I agree that the RFCs could be a little clearer about this, but I think the text really doesn't support your claim.

From RFC 9110, section 8.6:

Likewise, a sender MUST NOT forward a message with a Content-Length header field value that does not match the ABNF above, ...

It's not clear from this text alone whether it's saying that the forwarded version of the message must not contain an invalid Content-Length header, or that messages containing invalid Content-Length headers must not be forwarded in any form.

But the rest of the sentence clears up that ambiguity:

... with one exception: a recipient of a Content-Length header field value consisting of the same decimal value repeated as a comma-separated list (e.g, "Content-Length: 42, 42") MAY either reject the message as invalid or replace that invalid field value with a single instance of the decimal value, since this likely indicates that a duplicate was generated or combined by an upstream message processor.

Given that they're making a point to explicitly say that normalization is acceptable when the Content-Length is a comma-separated list of equal values, this implies that normalization is not acceptable when the Content-Length is otherwise invalid.

This is further supported by RFC 9112, section 6.3:

If a message is received with both a Transfer-Encoding and a Content-Length header field, the Transfer-Encoding overrides the Content-Length. Such a message might indicate an attempt to perform request smuggling (Section 11.2) or response splitting (Section 11.1) and ought to be handled as an error. An intermediary that chooses to forward the message MUST first remove the received Content-Length field and process the Transfer-Encoding (as described below) prior to forwarding the message downstream.

(emphasis mine)
This implies that the intermediary must choose to forward the message before before removing the Content-Length field. Since an intermediary MUST NOT forward a message with an invalid Content-Length header, it must be that invalid Content-Length headers invalidate the message, regardless of the presence of the Transfer-Encoding header.

neild · 2024-02-05T22:14:45Z

RFC 9112 Section 6.1 states:

Early implementations of Transfer-Encoding would occasionally send both a chunked transfer coding for message framing and an estimated Content-Length header field for use by progress bars. This is why Transfer-Encoding is defined as overriding Content-Length, as opposed to them being mutually incompatible.
...
A server MAY reject a request that contains both Content-Length and Transfer-Encoding or process such a request in accordance with the Transfer-Encoding alone. Regardless, the server MUST close the connection after responding to such a request to avoid the potential attacks.

This seems fairly explicit on the valid options when handling a request with both Content-Length and Transfer-Encoding: either reject it outright; or ignore Content-Length, process the message, and close the connection.

kenballus · 2024-02-13T18:31:36Z

On second thought, I think the current behavior is fine. I was misinterpreting the RFC.

According to RFC 9110 and RFC 9112, invalid "Content-Length" headers might involve request smuggling or response splitting, which could also cause security failures. Currently, `net/http` ignores all "Content-Length" headers when there is a "Transfer-Encoding" header and forward the message anyway while other mainstream HTTP implementations such as Apache Tomcat, Nginx, HAProxy, Node.js, Deno, Tornado, etc. reject invalid Content-Length headers regardless of the presence of a "Transfer-Encoding" header and only forward chunked-encoding messages with either valid "Content-Length" headers or no "Content-Length" headers. Fixes #65505 Change-Id: I73af2ee0785137e56c7546a4cce4a5c5c348dbc5 Reviewed-on: https://go-review.googlesource.com/c/go/+/561075 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-by: Damien Neil <dneil@google.com>

According to RFC 9110 and RFC 9112, invalid "Content-Length" headers might involve request smuggling or response splitting, which could also cause security failures. Currently, `net/http` ignores all "Content-Length" headers when there is a "Transfer-Encoding" header and forward the message anyway while other mainstream HTTP implementations such as Apache Tomcat, Nginx, HAProxy, Node.js, Deno, Tornado, etc. reject invalid Content-Length headers regardless of the presence of a "Transfer-Encoding" header and only forward chunked-encoding messages with either valid "Content-Length" headers or no "Content-Length" headers. Fixes golang#65505 Change-Id: I73af2ee0785137e56c7546a4cce4a5c5c348dbc5 Reviewed-on: https://go-review.googlesource.com/c/go/+/561075 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Bryan Mills <bcmills@google.com> Reviewed-by: Damien Neil <dneil@google.com>

panjf2000 self-assigned this Feb 4, 2024

panjf2000 added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Feb 4, 2024

panjf2000 added this to the Go1.23 milestone Feb 4, 2024

panjf2000 added the Security label Feb 4, 2024

kenballus mentioned this issue Feb 5, 2024

Clarification on messages with invalid Content-Length and valid Transfer-Encoding httpwg/http-core#1113

Closed

kenballus closed this as completed Feb 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net/http: `Content-Length` values are not validated when `Transfer-Encoding: chunked` is present #65505

net/http: `Content-Length` values are not validated when `Transfer-Encoding: chunked` is present #65505

kenballus commented Feb 4, 2024

kenballus commented Feb 4, 2024

bestgopher commented Feb 4, 2024

panjf2000 commented Feb 4, 2024

kenballus commented Feb 4, 2024

kenballus commented Feb 4, 2024

kenballus commented Feb 4, 2024

panjf2000 commented Feb 4, 2024 •

edited

bestgopher commented Feb 4, 2024 •

edited

panjf2000 commented Feb 4, 2024 •

edited

gopherbot commented Feb 4, 2024

kenballus commented Feb 4, 2024

neild commented Feb 5, 2024

kenballus commented Feb 13, 2024

net/http: Content-Length values are not validated when Transfer-Encoding: chunked is present #65505

net/http: Content-Length values are not validated when Transfer-Encoding: chunked is present #65505

Comments

kenballus commented Feb 4, 2024

Go version

Output of go env in your module/workspace:

What did you do?

What did you see happen?

What did you expect to see?

kenballus commented Feb 4, 2024

bestgopher commented Feb 4, 2024

panjf2000 commented Feb 4, 2024

kenballus commented Feb 4, 2024

kenballus commented Feb 4, 2024

kenballus commented Feb 4, 2024

panjf2000 commented Feb 4, 2024 • edited

bestgopher commented Feb 4, 2024 • edited

panjf2000 commented Feb 4, 2024 • edited

gopherbot commented Feb 4, 2024

kenballus commented Feb 4, 2024

neild commented Feb 5, 2024

kenballus commented Feb 13, 2024

net/http: `Content-Length` values are not validated when `Transfer-Encoding: chunked` is present #65505

net/http: `Content-Length` values are not validated when `Transfer-Encoding: chunked` is present #65505

Output of `go env` in your module/workspace:

panjf2000 commented Feb 4, 2024 •

edited

bestgopher commented Feb 4, 2024 •

edited

panjf2000 commented Feb 4, 2024 •

edited