From 08142d712d0b6da147d39678d4eda90f0c14dd94 Mon Sep 17 00:00:00 2001 From: Stephen Halter Date: Mon, 27 Apr 2020 16:11:59 -0700 Subject: [PATCH] Don't count start line toward header size limit --- .../Core/src/Internal/Http/Http1Connection.cs | 8 +- .../Kestrel/Core/test/Http1ConnectionTests.cs | 95 ++++++++++++------- 2 files changed, 65 insertions(+), 38 deletions(-) diff --git a/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs b/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs index 7432903bdbe8..145568af7218 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http/Http1Connection.cs @@ -41,7 +41,7 @@ internal partial class Http1Connection : HttpProtocol, IRequestProcessor private string _parsedRawTarget = null; private Uri _parsedAbsoluteRequestTarget; - private int _remainingRequestHeadersBytesAllowed; + private long _remainingRequestHeadersBytesAllowed; public Http1Connection(HttpConnectionContext context) { @@ -213,6 +213,8 @@ public bool TakeMessageHeaders(ref SequenceReader reader, bool trailers) return TrimAndTakeMessageHeaders(ref reader, trailers); } + var alreadyConsumed = reader.Consumed; + try { var result = _parser.ParseHeaders(new Http1ParsingHandler(this, trailers), ref reader); @@ -225,7 +227,7 @@ public bool TakeMessageHeaders(ref SequenceReader reader, bool trailers) } finally { - _remainingRequestHeadersBytesAllowed -= (int)reader.Consumed; + _remainingRequestHeadersBytesAllowed -= reader.Consumed - alreadyConsumed; } bool TrimAndTakeMessageHeaders(ref SequenceReader reader, bool trailers) @@ -248,7 +250,7 @@ bool TrimAndTakeMessageHeaders(ref SequenceReader reader, bool trailers) } finally { - _remainingRequestHeadersBytesAllowed -= (int)reader.Consumed; + _remainingRequestHeadersBytesAllowed -= trimmedReader.Consumed; } } } diff --git a/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs b/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs index 666aa419d4ed..d5d476d15e82 100644 --- a/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs +++ b/src/Servers/Kestrel/Core/test/Http1ConnectionTests.cs @@ -127,9 +127,7 @@ public async Task TakeMessageHeadersThrowsWhenHeadersExceedTotalSizeLimit() await _application.Output.WriteAsync(Encoding.ASCII.GetBytes($"{headerLine}\r\n")); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => TakeMessageHeaders(readableBuffer, trailers: false, out _consumed, out _examined)); -#pragma warning restore CS0618 // Type or member is obsolete + var exception = Assert.ThrowsAny(() => TakeMessageHeaders(readableBuffer, trailers: false, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); Assert.Equal(CoreStrings.BadRequest_HeadersExceedMaxTotalSize, exception.Message); @@ -145,15 +143,60 @@ public async Task TakeMessageHeadersThrowsWhenHeadersExceedCountLimit() await _application.Output.WriteAsync(Encoding.ASCII.GetBytes($"{headerLines}\r\n")); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => TakeMessageHeaders(readableBuffer, trailers: false, out _consumed, out _examined)); -#pragma warning restore CS0618 // Type or member is obsolete + var exception = Assert.ThrowsAny(() => TakeMessageHeaders(readableBuffer, trailers: false, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); Assert.Equal(CoreStrings.BadRequest_TooManyHeaders, exception.Message); Assert.Equal(StatusCodes.Status431RequestHeaderFieldsTooLarge, exception.StatusCode); } + [Fact] + public async Task TakeMessageHeadersDoesNotCountAlreadyConsumedBytesTowardsSizeLimit() + { + const string startLine = "GET / HTTP/1.1\r\n"; + + // This doesn't actually need to be larger than the start line to cause the regression, + // but doing so gives us a nice HeadersExceedMaxTotalSize error rather than an invalid slice + // when we do see the regression. + const string headerLine = "Header: makethislargerthanthestartline\r\n"; + + _serviceContext.ServerOptions.Limits.MaxRequestHeadersTotalSize = headerLine.Length; + _http1Connection.Reset(); + + // Don't send header initially because the regression is only caught if TakeMessageHeaders + // is called multiple times. The first call overcounted the header bytes consumed, and the + // subsequent calls overslice the buffer due to the overcounting. + await _application.Output.WriteAsync(Encoding.ASCII.GetBytes($"{startLine}")); + var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; + + SequencePosition TakeStartLineAndMessageHeaders() + { + var reader = new SequenceReader(readableBuffer); + Assert.True(_http1Connection.TakeStartLine(ref reader)); + Assert.False(_http1Connection.TakeMessageHeaders(ref reader, trailers: false)); + return reader.Position; + } + + _transport.Input.AdvanceTo(TakeStartLineAndMessageHeaders()); + + Assert.Equal(0, _http1Connection.RequestHeaders.Count); + + await _application.Output.WriteAsync(Encoding.ASCII.GetBytes($"{headerLine}\r\n")); + readableBuffer = (await _transport.Input.ReadAsync()).Buffer; + + SequencePosition TakeMessageHeaders() + { + var reader = new SequenceReader(readableBuffer); + Assert.True(_http1Connection.TakeMessageHeaders(ref reader, trailers: false)); + return reader.Position; + } + + _transport.Input.AdvanceTo(TakeMessageHeaders()); + + Assert.Equal(1, _http1Connection.RequestHeaders.Count); + Assert.Equal("makethislargerthanthestartline", _http1Connection.RequestHeaders["Header"]); + } + [Fact] public void ResetResetsScheme() { @@ -442,9 +485,7 @@ public async Task TakeStartLineThrowsWhenTooLong() await _application.Output.WriteAsync(requestLineBytes); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => TakeStartLine(readableBuffer, out _consumed, out _examined)); -#pragma warning restore CS0618 // Type or member is obsolete + var exception = Assert.ThrowsAny(() => TakeStartLine(readableBuffer, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); Assert.Equal(CoreStrings.BadRequest_RequestLineTooLong, exception.Message); @@ -458,9 +499,7 @@ public async Task TakeStartLineThrowsOnEncodedNullCharInTarget(string target) await _application.Output.WriteAsync(Encoding.ASCII.GetBytes($"GET {target} HTTP/1.1\r\n")); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => -#pragma warning restore CS0618 // Type or member is obsolete + var exception = Assert.ThrowsAny(() => TakeStartLine(readableBuffer, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); @@ -474,9 +513,7 @@ public async Task TakeStartLineThrowsOnNullCharInTarget(string target) await _application.Output.WriteAsync(Encoding.ASCII.GetBytes($"GET {target} HTTP/1.1\r\n")); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => -#pragma warning restore CS0618 // Type or member is obsolete + var exception = Assert.ThrowsAny(() => TakeStartLine(readableBuffer, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); @@ -492,9 +529,7 @@ public async Task TakeStartLineThrowsOnNullCharInMethod(string method) await _application.Output.WriteAsync(Encoding.ASCII.GetBytes(requestLine)); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => -#pragma warning restore CS0618 // Type or member is obsolete + var exception = Assert.ThrowsAny(() => TakeStartLine(readableBuffer, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); @@ -510,9 +545,7 @@ public async Task TakeStartLineThrowsOnNullCharInQueryString(string queryString) await _application.Output.WriteAsync(Encoding.ASCII.GetBytes($"GET {target} HTTP/1.1\r\n")); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => -#pragma warning restore CS0618 // Type or member is obsolete + var exception = Assert.ThrowsAny(() => TakeStartLine(readableBuffer, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); @@ -528,9 +561,7 @@ public async Task TakeStartLineThrowsWhenRequestTargetIsInvalid(string method, s await _application.Output.WriteAsync(Encoding.ASCII.GetBytes(requestLine)); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => -#pragma warning restore CS0618 // Type or member is obsolete + var exception = Assert.ThrowsAny(() => TakeStartLine(readableBuffer, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); @@ -548,7 +579,7 @@ public async Task TakeStartLineThrowsWhenMethodNotAllowed(string requestLine, in #pragma warning disable CS0618 // Type or member is obsolete var exception = Assert.Throws(() => #pragma warning restore CS0618 // Type or member is obsolete - TakeStartLine(readableBuffer, out _consumed, out _examined)); + TakeStartLine(readableBuffer, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); Assert.Equal(405, exception.StatusCode); @@ -792,10 +823,8 @@ public async Task ExceptionDetailNotIncludedWhenLogLevelInformationNotEnabled() await _application.Output.WriteAsync(Encoding.ASCII.GetBytes($"GET /%00 HTTP/1.1\r\n")); var readableBuffer = (await _transport.Input.ReadAsync()).Buffer; -#pragma warning disable CS0618 // Type or member is obsolete - var exception = Assert.Throws(() => -#pragma warning restore CS0618 // Type or member is obsolete - TakeStartLine(readableBuffer, out _consumed, out _examined)); + var exception = Assert.ThrowsAny(() => + TakeStartLine(readableBuffer, out _consumed, out _examined)); _transport.Input.AdvanceTo(_consumed, _examined); Assert.Equal(CoreStrings.FormatBadRequest_InvalidRequestTarget_Detail(string.Empty), exception.Message); @@ -954,9 +983,7 @@ public void BadRequestFor10BadHostHeaderFormat() { _http1Connection.HttpVersion = "HTTP/1.0"; _http1Connection.RequestHeaders[HeaderNames.Host] = "a=b"; -#pragma warning disable CS0618 // Type or member is obsolete - var ex = Assert.Throws(() => _http1Connection.EnsureHostHeaderExists()); -#pragma warning restore CS0618 // Type or member is obsolete + var ex = Assert.ThrowsAny(() => _http1Connection.EnsureHostHeaderExists()); Assert.Equal(CoreStrings.FormatBadRequest_InvalidHostHeader_Detail("a=b"), ex.Message); } @@ -965,9 +992,7 @@ public void BadRequestFor11BadHostHeaderFormat() { _http1Connection.HttpVersion = "HTTP/1.1"; _http1Connection.RequestHeaders[HeaderNames.Host] = "a=b"; -#pragma warning disable CS0618 // Type or member is obsolete - var ex = Assert.Throws(() => _http1Connection.EnsureHostHeaderExists()); -#pragma warning restore CS0618 // Type or member is obsolete + var ex = Assert.ThrowsAny(() => _http1Connection.EnsureHostHeaderExists()); Assert.Equal(CoreStrings.FormatBadRequest_InvalidHostHeader_Detail("a=b"), ex.Message); }