Content-Length and Transfer-Encoding conflicts

[Tratcher]

Is there an existing issue for this?

• I have searched the existing issues

Describe the bug

https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.2

A sender MUST NOT send a Content-Length header field in any message
that contains a Transfer-Encoding header field.

https://datatracker.ietf.org/doc/html/rfc7230#section-3.3.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 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.

Kestrel follows this today by honoring the Transfer-Encoding: chunked and ignoring the Content-Length header. However, the Content-Length header is still present on the request and apps may still be consuming it.

aspnetcore/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs

Lines 146 to 177 in 94cc679

	if (headers.HasTransferEncoding)
	{
	var transferEncoding = headers.HeaderTransferEncoding;
	var transferCoding = HttpHeaders.GetFinalTransferCoding(transferEncoding);
	// https://tools.ietf.org/html/rfc7230#section-3.3.3
	// If a Transfer-Encoding header field
	// is present in a request and the chunked transfer coding is not
	// the final encoding, the message body length cannot be determined
	// reliably; the server MUST respond with the 400 (Bad Request)
	// status code and then close the connection.
	if (transferCoding != TransferCoding.Chunked)
	{
	KestrelBadHttpRequestException.Throw(RequestRejectionReason.FinalTransferCodingNotChunked, transferEncoding);
	}
	// TODO may push more into the wrapper rather than just calling into the message body
	// NBD for now.
	return new Http1ChunkedEncodingMessageBody(context, keepAlive);
	}
	if (headers.ContentLength.HasValue)
	{
	var contentLength = headers.ContentLength.Value;
	if (contentLength == 0)
	{
	return keepAlive ? MessageBody.ZeroContentLengthKeepAlive : MessageBody.ZeroContentLengthClose;
	}
	return new Http1ContentLengthMessageBody(context, contentLength, keepAlive);
	}

E.g. YARP has to check for this condition too to avoid a smuggling attempt when proxying.
https://github.com/microsoft/reverse-proxy/blob/06f28a36140543512a3fc3e1e9423094c43296ec/src/ReverseProxy/Forwarder/HttpTransformer.cs#L80-L92

App developers are not usually aware of spec nuances like this and write logic that makes decisions based on the Content-Length header when present. This can lead to application level functional and security issues like over/under buffering.

Expected Behavior

Proposal: Kestrel should implement the following spec guideline "A sender MUST remove the received Content-Length field prior to forwarding such a message downstream.", treating the application as the downstream entity. If there is a Transfer-Encoding header then kestrel should remove any Content-Length header to avoid app level confusion.

Since Kestrel avoids modifying the request in most cases, giving the app as much of the original data as possible, we want to preserve the Content-Length header value by moving it to x-Content-Length. This should avoid people consuming the unvalidated value by accident, but it's still available for compat reasons.

YARP could remove its mitigation after this change.

Steps To Reproduce

POST / HTTP/1.1
Content-Length: 500000
Transfer-Encoding: chunked

5
Hello
0

Exceptions (if any)

No response

.NET Version

No response

Anything else?

Recommended for .NET 8.

[FatTigerWang]

I want to try this, can you assign me?

[Tratcher]

Re-opening, we should do the same mitigation for Http.Sys and IIS which today do honor Transfer-Encoding, but continue to flow Content-Length.

[FatTigerWang]

@Tratcher Can you help me find the corresponding file? I'm not very familiar with it.

[Tratcher]

Here:

aspnetcore/src/Servers/HttpSys/src/RequestProcessing/Request.cs

Line 129 in ce2db7e

public long? ContentLength

It kindof does this already by ignoring the header if Transfer-Encoding is set, but that Transfer-Encoding check shouldn't be an exact match, it should be checking for the last value.

aspnetcore/src/Servers/HttpSys/src/RequestProcessing/Request.cs

Lines 136 to 140 in ce2db7e

	var transferEncoding = Headers[HeaderNames.TransferEncoding].ToString();
	if (string.Equals("chunked", transferEncoding.Trim(), StringComparison.OrdinalIgnoreCase))
	{
	_contentBoundaryType = BoundaryType.Chunked;
	}

It should also drop the Content-Length header rather than ignoring it.

This should probably be triggered more eagerly from the constructor so apps don't read the CL header directly before it's dropped.
https://github.com/dotnet/aspnetcore/blob/main/src/Servers/HttpSys/src/RequestProcessing/Request.cs#L36