SocketsHttpHandler does not enforce request Content-Length correctness

[MihaZupan]

The Content-Length header we send may not match the number of bytes actually sent.

• If the custom content is smaller than Content-Length, this will result in the request waiting on either the client or server to time out. The cause of such a hang can be non-obvious (Timeout when request body is consumed before proxying yarp#1412).
• If the content is larger, it may corrupt the connection for future requests.

SocketsHttpHandler does not currently enforce that the number of bytes sent matches the header. The only exception to that is that HTTP/3 will not allow you to send too much content.

Since this is a protocol violation and non-obvious to diagnose, I suggest that we always throw in case the Content-Length does not match the number of bytes sent. We already track the number of bytes sent (for telemetry purposes), it's just a matter of reacting to a mismatch.

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

The Content-Length header we send may not match the number of bytes actually sent.

• If the custom content is smaller than Content-Length, this will result in the request waiting on either the client or server to time out. The cause of such a hang can be non-obvious (Timeout when request body is consumed before proxying yarp#1412).
• If the content is larger, it may corrupt the connection for future requests.

SocketsHttpHandler does not currently enforce that the number of bytes sent matches the header. The only exception to that is that HTTP/3 will not allow you to send too much content.

Since this is a protocol violation and non-obvious to diagnose, I suggest that we always throw in case the Content-Length does not match the number of bytes sent. We already track the number of bytes sent (for telemetry purposes), it's just a matter of reacting to a mismatch.

Author:	MihaZupan
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[geoffkizer]

You are specifically talking about Content-Length on the request, right?

This does seem like something we could do better about.

[MihaZupan]

You are specifically talking about Content-Length on the request, right?

Yes

[geoffkizer]

I suggest that we always throw in case the Content-Length does not match the number of bytes sent.

Throwing seems reasonable, but the other question is what we send on the wire.

Seems like if the bytes sent < Content-Length, we should cancel the request (i.e. kill the HTTP/1.1 connection, send RST_STREAM for HTTP2, etc).

If bytes sent > Content-Length, things are less clear... the server may have processed the request fully up to Content-Length and reasonably assumed that the request is complete. What should we do on the client at this point? We definitely shouldn't send any more data on the wire past the Content-Length, since this will result in an invalid HTTP/1.1 connection, and for HTTP2 the result is essentially undefined. But should we also fail the request? If so, that means the user would never receive the response from the server, which seems problematic. Should we just ignore any data past Content-Length? This is a bit weird, but may be the best alternative.

[MihaZupan]

send RST_STREAM for HTTP2

Yeah, this sounds like a good thing to do on top of throwing - it may already happen as part of cleanup if we just throw, but worth confirming.

But should we also fail the request?

It indicates that the user's Content-Length calculation logic is wrong, so it seems like something one would want to know about.
It may lead to more failed requests in existing apps, but it only surfaces a preexisting issue that was certainly causing other failures/data corruption/potential request smuggling.

[karelz]

Triage:

• Not enough data - reasonable to throw (and to send RST_STREAM on HTTP/2)
• Too much data - throw and perhaps (TBD) tear down the connection to increase the chance server won't process the request
  • Note: We don't have capability to drain HTTP 1.1 responses at least

[halter73]

With HTTP/1.1, I bet even the first case with not enough data could lead to corruption if the server sends a complete response before attempting to read the entire request body (this would be unusual but certainly not unheard of). If that were to happen, the server could interpret the next request as the end of the previous response body.

Also, tearing down the connection for too much data might be even more effective than it seems at stopping the server from processing the bad HTTP/1.1 request. There's a pretty good chance you can catch this condition before Content-Length bytes are actually sent. And at least with Kestrel, the request body stream will start throwing as soon as a FIN is received even if it buffered a complete request body before then.

I think SocketsHttpHandler should tear down the HTTP/1.1 connection in both cases. That's what Kestrel does.

[karelz]

Triage: Moving to Future as YARP has a workaround. @MihaZupan can you please confirm it is correct?

[samsp-msft]

I was a bit concerned about this as it can be a way to do request stuffing. However as this is the client rather than the server, its not technically a security issue. Being silent about this is likely to cause application issues because there is some form of logic error on the client, and the server is likely to object.

I would suggest we throw in cases where there is this inconsistency. However it probably needs a check on which version of the runtime is targeted/vs run on as it could break older clients that were unaware of them having this problem.

[MihaZupan]

Yarp will likely have a workaround but doesn't have one yet. For the most part, we're safe from sending too much content since Kestrel will only give us Content-Length bytes.

I was a bit concerned about this as it can be a way to do request stuffing. However as this is the client rather than the server, its not technically a security issue.

It's still an issue if you trust the user's input too much. Similar to newlines in header values that we fixed for .NET 6.0.

it could break older clients that were unaware of them having this problem.

How do you mean? If it throws it means they were already broken in some way (e.g. silent data corruption).