aspnetcore
aspnetcore copied to clipboard
dotnet
Content-Length and Transfer-Encoding conflicts
Is there an existing issue for this?
- [X] 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.
https://github.com/dotnet/aspnetcore/blob/94cc679bce4967568fd458433ecda3b3fb0578b6/src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs#L146-L177
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.
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.
@Tratcher Can you help me find the corresponding file? I'm not very familiar with it.
Here: https://github.com/dotnet/aspnetcore/blob/ce2db7ea0b161fc5eb35710fca6feeafeeac37bc/src/Servers/HttpSys/src/RequestProcessing/Request.cs#L129 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. https://github.com/dotnet/aspnetcore/blob/ce2db7ea0b161fc5eb35710fca6feeafeeac37bc/src/Servers/HttpSys/src/RequestProcessing/Request.cs#L136-L140 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
If Transfer-Encoding has multiple values, is chunked always at the end? Otherwise I should check all the values.
chunked is required to be the last entry. Http.Sys enforces this and rejects the request if it's not.
IIS also needs work: https://github.com/dotnet/aspnetcore/blob/01177230911caab93dff106e4f0d973a06f51bf3/src/Servers/IIS/IIS/src/Core/IISHttpContext.cs#L261-L262
That exact match should be checking for the last value instead.
If there's also a Content-Length it should be dropped/renamed.