From 9cd239a4b5e42f18634b3dac9a46dcb425eddcde Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Wed, 8 Dec 2021 18:09:03 +0100 Subject: [PATCH 1/4] Enforce HTTP request Content-Length correctness --- .../src/Resources/Strings.resx | 3 ++ .../ContentLengthWriteStream.cs | 21 +++++++- .../Http/SocketsHttpHandler/Http2Stream.cs | 19 ++++++- .../SocketsHttpHandler/Http3RequestStream.cs | 8 +++ .../Http/SocketsHttpHandler/HttpConnection.cs | 3 +- .../FunctionalTests/SocketsHttpHandlerTest.cs | 53 +++++++++++++++++++ 6 files changed, 103 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Net.Http/src/Resources/Strings.resx b/src/libraries/System.Net.Http/src/Resources/Strings.resx index a6972e36a6be5b..7464b6aa14086a 100644 --- a/src/libraries/System.Net.Http/src/Resources/Strings.resx +++ b/src/libraries/System.Net.Http/src/Resources/Strings.resx @@ -309,6 +309,9 @@ The server returned an invalid or unrecognized response. + + Sent {0} request content bytes, but Content-Length promised {1}. + The response ended prematurely. diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthWriteStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthWriteStream.cs index 5b7b6551661972..33add1358ab51a 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthWriteStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/ContentLengthWriteStream.cs @@ -11,14 +11,23 @@ internal sealed partial class HttpConnection : IDisposable { private sealed class ContentLengthWriteStream : HttpContentWriteStream { - public ContentLengthWriteStream(HttpConnection connection) : base(connection) + private readonly long _contentLength; + + public ContentLengthWriteStream(HttpConnection connection, long contentLength) + : base(connection) { + _contentLength = contentLength; } public override void Write(ReadOnlySpan buffer) { BytesWritten += buffer.Length; + if (BytesWritten > _contentLength) + { + throw new HttpRequestException(SR.net_http_content_write_larger_than_content_length); + } + // Have the connection write the data, skipping the buffer. Importantly, this will // force a flush of anything already in the buffer, i.e. any remaining request headers // that are still buffered. @@ -31,6 +40,11 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo { BytesWritten += buffer.Length; + if (BytesWritten > _contentLength) + { + return ValueTask.FromException(new HttpRequestException(SR.net_http_content_write_larger_than_content_length)); + } + // Have the connection write the data, skipping the buffer. Importantly, this will // force a flush of anything already in the buffer, i.e. any remaining request headers // that are still buffered. @@ -41,6 +55,11 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo public override Task FinishAsync(bool async) { + if (BytesWritten != _contentLength) + { + return Task.FromException(new HttpRequestException(SR.Format(SR.net_http_request_content_length_mismatch, BytesWritten, _contentLength))); + } + _connection = null; return Task.CompletedTask; } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs index b5d5ffa8b84698..3f77cd7f88d61e 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http2Stream.cs @@ -195,7 +195,7 @@ public async Task SendRequestBodyAsync(CancellationToken cancellationToken) if (sendRequestContent) { - using var writeStream = new Http2WriteStream(this); + using var writeStream = new Http2WriteStream(this, _request.Content.Headers.ContentLength.GetValueOrDefault(-1)); if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestContentStart(); @@ -214,6 +214,12 @@ public async Task SendRequestBodyAsync(CancellationToken cancellationToken) await vt.ConfigureAwait(false); } + if (writeStream.BytesWritten < writeStream.ContentLength) + { + // The number of bytes we actually sent doesn't match the advertised Content-Length + throw new HttpRequestException(SR.Format(SR.net_http_request_content_length_mismatch, writeStream.BytesWritten, writeStream.ContentLength)); + } + if (HttpTelemetry.Log.IsEnabled()) HttpTelemetry.Log.RequestContentStop(writeStream.BytesWritten); } @@ -1506,10 +1512,14 @@ private sealed class Http2WriteStream : HttpBaseStream public long BytesWritten { get; private set; } - public Http2WriteStream(Http2Stream http2Stream) + public long ContentLength { get; private set; } + + public Http2WriteStream(Http2Stream http2Stream, long contentLength) { Debug.Assert(http2Stream != null); + Debug.Assert(contentLength >= -1); _http2Stream = http2Stream; + ContentLength = contentLength; } protected override void Dispose(bool disposing) @@ -1534,6 +1544,11 @@ public override ValueTask WriteAsync(ReadOnlyMemory buffer, CancellationTo { BytesWritten += buffer.Length; + if ((ulong)BytesWritten > (ulong)ContentLength) // If ContentLength == -1, this will always be false + { + return ValueTask.FromException(new HttpRequestException(SR.net_http_content_write_larger_than_content_length)); + } + Http2Stream? http2Stream = _http2Stream; if (http2Stream == null) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs index e14302ef51f216..3a0e674609a5af 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs @@ -384,6 +384,14 @@ private async Task SendContentAsync(HttpContent content, CancellationToken cance await content.CopyToAsync(writeStream, null, cancellationToken).ConfigureAwait(false); } + if (_requestContentLengthRemaining > 0) + { + // The number of bytes we actually sent doesn't match the advertised Content-Length + long contentLength = content.Headers.ContentLength.GetValueOrDefault(); + long sent = contentLength - _requestContentLengthRemaining; + throw new HttpRequestException(SR.Format(SR.net_http_request_content_length_mismatch, sent, contentLength)); + } + // Set to 0 to recognize that the whole request body has been sent and therefore there's no need to abort write side in case of a premature disposal. _requestContentLengthRemaining = 0; diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs index d75ed2779f0428..12b6b4bf3c55a0 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnection.cs @@ -860,10 +860,11 @@ private bool MapSendException(Exception exception, CancellationToken cancellatio private HttpContentWriteStream CreateRequestContentStream(HttpRequestMessage request) { + Debug.Assert(request.Content is not null); bool requestTransferEncodingChunked = request.HasHeaders && request.Headers.TransferEncodingChunked == true; HttpContentWriteStream requestContentStream = requestTransferEncodingChunked ? (HttpContentWriteStream) new ChunkedEncodingWriteStream(this) : - new ContentLengthWriteStream(this); + new ContentLengthWriteStream(this, request.Content.Headers.ContentLength.GetValueOrDefault()); return requestContentStream; } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 14f444b1935a60..f8dd52f8fe3865 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -3365,4 +3365,57 @@ public sealed class SocketsHttpHandler_RequestValidationTest_Sync : SocketsHttpH { protected override bool TestAsync => false; } + + public abstract class SocketsHttpHandler_RequestContentLengthMismatchTest : HttpClientHandlerTestBase + { + public SocketsHttpHandler_RequestContentLengthMismatchTest(ITestOutputHelper output) : base(output) { } + + [Theory] + [InlineData(0, 1)] + [InlineData(1, 0)] + [InlineData(1, 2)] + [InlineData(2, 1)] + public async Task ContentLength_DoesNotMatchRequestContentLength_Throws(int contentLength, int bytesSent) + { + await LoopbackServerFactory.CreateClientAndServerAsync(async uri => + { + using var client = CreateHttpClient(); + + var content = new ByteArrayContent(new byte[bytesSent]); + content.Headers.ContentLength = contentLength; + + Exception ex = await Assert.ThrowsAsync(() => client.PostAsync(uri, content)); + Assert.Contains("Content-Length", ex.Message); + }, + async server => + { + try + { + await server.HandleRequestAsync(); + } + catch { } + }); + } + } + + public sealed class SocketsHttpHandler_RequestContentLengthMismatchTest_Http11 : SocketsHttpHandler_RequestContentLengthMismatchTest + { + public SocketsHttpHandler_RequestContentLengthMismatchTest_Http11(ITestOutputHelper output) : base(output) { } + protected override Version UseVersion => HttpVersion.Version11; + } + + [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.SupportsAlpn))] + public sealed class SocketsHttpHandler_RequestContentLengthMismatchTest_Http2 : SocketsHttpHandler_RequestContentLengthMismatchTest + { + public SocketsHttpHandler_RequestContentLengthMismatchTest_Http2(ITestOutputHelper output) : base(output) { } + protected override Version UseVersion => HttpVersion.Version20; + } + + [ConditionalClass(typeof(HttpClientHandlerTestBase), nameof(IsMsQuicSupported))] + public sealed class SocketsHttpHandler_RequestContentLengthMismatchTest_Http3 : SocketsHttpHandler_RequestContentLengthMismatchTest + { + public SocketsHttpHandler_RequestContentLengthMismatchTest_Http3(ITestOutputHelper output) : base(output) { } + protected override Version UseVersion => HttpVersion.Version30; + protected override QuicImplementationProvider UseQuicImplementationProvider => QuicImplementationProviders.MsQuic; + } } From f75a004b3e308dc8f2b11b9bfa34b14790097f08 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Wed, 8 Dec 2021 20:02:01 +0100 Subject: [PATCH 2/4] Skip test on Browser --- .../tests/FunctionalTests/SocketsHttpHandlerTest.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index f8dd52f8fe3865..9a68b27d773321 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -3366,6 +3366,7 @@ public sealed class SocketsHttpHandler_RequestValidationTest_Sync : SocketsHttpH protected override bool TestAsync => false; } + [ConditionalClass(typeof(PlatformDetection), nameof(PlatformDetection.IsNotBrowser))] public abstract class SocketsHttpHandler_RequestContentLengthMismatchTest : HttpClientHandlerTestBase { public SocketsHttpHandler_RequestContentLengthMismatchTest(ITestOutputHelper output) : base(output) { } From 0217e34cfa204784a51762111ddac60109f0f141 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Wed, 8 Dec 2021 20:10:09 +0100 Subject: [PATCH 3/4] Test that subsequent HTTP/1.1 requests can still succeed on a new connection --- .../tests/FunctionalTests/SocketsHttpHandlerTest.cs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs index 9a68b27d773321..bd52121a4797b8 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/SocketsHttpHandlerTest.cs @@ -3387,6 +3387,11 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => Exception ex = await Assert.ThrowsAsync(() => client.PostAsync(uri, content)); Assert.Contains("Content-Length", ex.Message); + + if (UseVersion.Major == 1) + { + await client.GetStringAsync(uri).WaitAsync(TestHelper.PassingTestTimeout); + } }, async server => { @@ -3395,6 +3400,13 @@ await LoopbackServerFactory.CreateClientAndServerAsync(async uri => await server.HandleRequestAsync(); } catch { } + + // On HTTP/1.x, an exception being thrown while sending the request content will result in the connection being closed. + // This test is ensuring that a subsequent request can succeed on a new connection. + if (UseVersion.Major == 1) + { + await server.HandleRequestAsync().WaitAsync(TestHelper.PassingTestTimeout); + } }); } } From eb8d21e3f77a4ce983d35fe5de707a484e15c2e2 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Thu, 9 Dec 2021 18:05:23 +0100 Subject: [PATCH 4/4] Be consistent about exception nesting --- .../System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs index 3a0e674609a5af..357e768e80319e 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/Http3RequestStream.cs @@ -422,8 +422,7 @@ private async ValueTask WriteRequestContentAsync(ReadOnlyMemory buffer, Ca if (buffer.Length > _requestContentLengthRemaining) { - string msg = SR.net_http_content_write_larger_than_content_length; - throw new IOException(msg, new HttpRequestException(msg)); + throw new HttpRequestException(SR.net_http_content_write_larger_than_content_length); } _requestContentLengthRemaining -= buffer.Length;