Securely overwrite X-Content-Length for mitigating request smuggling via TransferEncoding & ContentLength headers#65445
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses a potential exception in request smuggling mitigation logic across three ASP.NET Core server implementations (Kestrel, IIS, and HttpSys). When both Transfer-Encoding and Content-Length headers are present in a request (a potential request smuggling attack), the servers move the Content-Length value to an X-Content-Length header for diagnostic purposes. The PR changes from using Add() to TryAdd() to prevent exceptions when a client has explicitly sent an X-Content-Length header.
Changes:
- Modified header handling logic to use
TryAdd()instead ofAdd()for X-Content-Length across Kestrel, IIS, and HttpSys server implementations - Added tests for Kestrel and HttpSys to verify behavior when all three headers (Transfer-Encoding, Content-Length, and X-Content-Length) are present
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Servers/Kestrel/Core/src/Internal/Http/Http1MessageBody.cs | Changed to use TryAdd for X-Content-Length to avoid exception, added comment explaining behavior |
| src/Servers/IIS/IIS/src/Core/IISHttpContext.cs | Changed to use TryAdd for X-Content-Length to avoid exception, added comment explaining behavior |
| src/Servers/HttpSys/src/RequestProcessing/Request.cs | Changed to use TryAdd for X-Content-Length to avoid exception, added comment explaining behavior |
| src/Servers/Kestrel/Core/test/Http1/Http1ConnectionTests.cs | Added test verifying X-Content-Length is preserved when explicitly set by user |
| src/Servers/HttpSys/test/FunctionalTests/RequestHeaderTests.cs | Added test verifying X-Content-Length is preserved when explicitly set by user |
| Assert.Single(requestHeaders["X-Content-Length"]); | ||
| Assert.Equal("123", requestHeaders["X-Content-Length"]); |
There was a problem hiding this comment.
Test behavior concern: This test verifies that when a user explicitly provides X-Content-Length, it's preserved instead of being overwritten with the actual Content-Length. However, this behavior might not align with the security purpose of X-Content-Length, which is to preserve the actual Content-Length for diagnostic purposes during request smuggling detection. If the implementation changes to always overwrite X-Content-Length with the server-observed Content-Length (which would be more secure), this test's assertions would need to be updated accordingly.
| Assert.Single(requestHeaders["X-Content-Length"]); | |
| Assert.Equal("123", requestHeaders["X-Content-Length"]); | |
| Assert.True(requestHeaders.ContainsKey("X-Content-Length")); | |
| Assert.Single(requestHeaders["X-Content-Length"]); |
| IHeaderDictionary headerDictionary = Headers; | ||
| headerDictionary.Add("X-Content-Length", headerDictionary[HeaderNames.ContentLength]); | ||
|
|
||
| // dont overwrite if user explicitly set X-Content-Length |
There was a problem hiding this comment.
Grammar issue: "dont" should be "don't". This comment should match the style used in the other implementations (Http1MessageBody.cs and IISHttpContext.cs).
| // dont overwrite if user explicitly set X-Content-Length | |
| // Don't overwrite if user explicitly set X-Content-Length |
| // if user already passed X-Content-Length, we won't overwrite it | ||
| _ = headerDictionary.TryAdd("X-Content-Length", headerDictionary[HeaderNames.ContentLength]); |
There was a problem hiding this comment.
Security concern: Using TryAdd preserves a user-supplied X-Content-Length header instead of recording the actual Content-Length value that triggered the smuggling detection. This could allow an attacker to hide the true Content-Length value in diagnostics/logs.
Consider using a pattern that always overwrites X-Content-Length with the actual Content-Length value to ensure accurate diagnostics of potential request smuggling attempts. For example:
- Remove existing X-Content-Length if present
- Add the actual Content-Length to X-Content-Length
- Then remove Content-Length
The purpose of X-Content-Length is to preserve the original Content-Length for diagnostic purposes when request smuggling is detected. Preserving an attacker-supplied value defeats this purpose.
| // if user already passed X-Content-Length, we won't overwrite it | |
| _ = headerDictionary.TryAdd("X-Content-Length", headerDictionary[HeaderNames.ContentLength]); | |
| headerDictionary.Remove("X-Content-Length"); | |
| headerDictionary["X-Content-Length"] = headerDictionary[HeaderNames.ContentLength]; |
| // if user already passed X-Content-Length, we won't overwrite it | ||
| _ = RequestHeaders.TryAdd("X-Content-Length", RequestHeaders[HeaderNames.ContentLength]); |
There was a problem hiding this comment.
Security concern: Using TryAdd preserves a user-supplied X-Content-Length header instead of recording the actual Content-Length value that triggered the smuggling detection. This could allow an attacker to hide the true Content-Length value in diagnostics/logs.
Consider using a pattern that always overwrites X-Content-Length with the actual Content-Length value to ensure accurate diagnostics of potential request smuggling attempts. For example:
- Remove existing X-Content-Length if present
- Add the actual Content-Length to X-Content-Length
- Then remove Content-Length
The purpose of X-Content-Length is to preserve the original Content-Length for diagnostic purposes when request smuggling is detected. Preserving an attacker-supplied value defeats this purpose.
| // if user already passed X-Content-Length, we won't overwrite it | |
| _ = RequestHeaders.TryAdd("X-Content-Length", RequestHeaders[HeaderNames.ContentLength]); | |
| var originalContentLength = RequestHeaders[HeaderNames.ContentLength]; | |
| RequestHeaders.Remove("X-Content-Length"); | |
| RequestHeaders["X-Content-Length"] = originalContentLength; |
| // dont overwrite if user explicitly set X-Content-Length | ||
| _ = headerDictionary.TryAdd("X-Content-Length", headerDictionary[HeaderNames.ContentLength]); |
There was a problem hiding this comment.
Security concern: Using TryAdd preserves a user-supplied X-Content-Length header instead of recording the actual Content-Length value that triggered the smuggling detection. This could allow an attacker to hide the true Content-Length value in diagnostics/logs.
Consider using a pattern that always overwrites X-Content-Length with the actual Content-Length value to ensure accurate diagnostics of potential request smuggling attempts. For example:
- Remove existing X-Content-Length if present
- Add the actual Content-Length to X-Content-Length
- Then remove Content-Length
The purpose of X-Content-Length is to preserve the original Content-Length for diagnostic purposes when request smuggling is detected. Preserving an attacker-supplied value defeats this purpose.
| // dont overwrite if user explicitly set X-Content-Length | |
| _ = headerDictionary.TryAdd("X-Content-Length", headerDictionary[HeaderNames.ContentLength]); | |
| // Always record the original Content-Length in X-Content-Length for diagnostics, | |
| // even if the client supplied its own X-Content-Length header. | |
| headerDictionary.Remove("X-Content-Length"); | |
| headerDictionary["X-Content-Length"] = headerDictionary[HeaderNames.ContentLength]; |
| // if user already passed X-Content-Length, we won't overwrite it | ||
| _ = RequestHeaders.TryAdd("X-Content-Length", RequestHeaders[HeaderNames.ContentLength]); |
There was a problem hiding this comment.
Test coverage: The IIS implementation in IISHttpContext.cs was updated with the same logic change (TryAdd for X-Content-Length), but no corresponding test was added for the IIS server. The Kestrel and HttpSys servers both have tests covering the scenario where Transfer-Encoding, Content-Length, and X-Content-Length are all present. Consider adding a similar test for IIS to ensure consistent behavior across all server implementations.
There was a problem hiding this comment.
| Assert.True(_http1Connection.RequestHeaders.ContainsKey("X-Content-Length")); | ||
| Assert.Equal(userXContentLength, _http1Connection.RequestHeaders["X-Content-Length"]); | ||
| Assert.True(_http1Connection.RequestHeaders.ContainsKey(HeaderNames.TransferEncoding)); | ||
| Assert.Equal("chunked", _http1Connection.RequestHeaders[HeaderNames.TransferEncoding]); | ||
| Assert.False(_http1Connection.RequestHeaders.ContainsKey(HeaderNames.ContentLength)); | ||
| } |
There was a problem hiding this comment.
Test behavior concern: This test verifies that when a user explicitly provides X-Content-Length, it's preserved instead of being overwritten with the actual Content-Length. However, this behavior might not align with the security purpose of X-Content-Length, which is to preserve the actual Content-Length for diagnostic purposes during request smuggling detection. If the implementation changes to always overwrite X-Content-Length with the server-observed Content-Length (which would be more secure), this test's assertions would need to be updated accordingly.
| // if user already passed X-Content-Length, we won't overwrite it | ||
| _ = RequestHeaders.TryAdd("X-Content-Length", RequestHeaders[HeaderNames.ContentLength]); |
There was a problem hiding this comment.

Noticed that we are removing
Content-Lengthheader and writing the value toX-Content-Lengthin case of detecting request smuggling attempt viaTransfer-EncodingandContent-Lengthheaders.However, if user has explicitly passed
X-Content-Length, code will throw because we are doing a standarddictionary.Add()ofX-Content-Length.Changed to use
TryAdd()to avoid unintended behavior.