diff --git a/benchmarks/Kestrel.Performance/IntegerDecoderBenchmark.cs b/benchmarks/Kestrel.Performance/IntegerDecoderBenchmark.cs new file mode 100644 index 0000000000..6fc22390c0 --- /dev/null +++ b/benchmarks/Kestrel.Performance/IntegerDecoderBenchmark.cs @@ -0,0 +1,41 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using BenchmarkDotNet.Attributes; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack; + +namespace Microsoft.AspNetCore.Server.Kestrel.Performance +{ + public class IntegerDecoderBenchmark + { + private const int Iterations = 50_000; + + private int _prefixLength = 5; // Arbitrary prefix length + private byte _singleByte = 0x1e; // 30 + private byte[] _multiByte = new byte[] { 0x1f, 0xe0, 0xff, 0xff, 0xff, 0x03}; // int32.MaxValue + private IntegerDecoder _integerDecoder = new IntegerDecoder(); + + [Benchmark(Baseline = true, OperationsPerInvoke = Iterations)] + public void DecodeSingleByteInteger() + { + for (var i = 0; i < Iterations; i++) + { + _integerDecoder.BeginTryDecode(_singleByte, _prefixLength, out _); + } + } + + [Benchmark(OperationsPerInvoke = Iterations)] + public void DecodeMultiByteInteger() + { + for (var i = 0; i < Iterations; i++) + { + _integerDecoder.BeginTryDecode(_multiByte[0], _prefixLength, out _); + + for (var j = 1; j < _multiByte.Length; j++) + { + _integerDecoder.TryDecode(_multiByte[j], out _); + } + } + } + } +} diff --git a/src/Kestrel.Core/CoreStrings.resx b/src/Kestrel.Core/CoreStrings.resx index 5a04c01d0c..e3b66fe44a 100644 --- a/src/Kestrel.Core/CoreStrings.resx +++ b/src/Kestrel.Core/CoreStrings.resx @@ -1,17 +1,17 @@  - @@ -581,4 +581,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l The given buffer was too small to encode any headers. + + The decoded integer exceeds the maximum value of Int32.MaxValue. + \ No newline at end of file diff --git a/src/Kestrel.Core/Internal/Http2/HPack/HPackDecoder.cs b/src/Kestrel.Core/Internal/Http2/HPack/HPackDecoder.cs index e88a4e077b..3decfb4c08 100644 --- a/src/Kestrel.Core/Internal/Http2/HPack/HPackDecoder.cs +++ b/src/Kestrel.Core/Internal/Http2/HPack/HPackDecoder.cs @@ -132,6 +132,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack private void OnByte(byte b, IHttpHeadersHandler handler) { + int intResult; switch (_state) { case State.Ready: @@ -140,9 +141,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack _headersObserved = true; var val = b & ~IndexedHeaderFieldMask; - if (_integerDecoder.BeginDecode((byte)val, IndexedHeaderFieldPrefix)) + if (_integerDecoder.BeginTryDecode((byte)val, IndexedHeaderFieldPrefix, out intResult)) { - OnIndexedHeaderField(_integerDecoder.Value, handler); + OnIndexedHeaderField(intResult, handler); } else { @@ -159,9 +160,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack { _state = State.HeaderNameLength; } - else if (_integerDecoder.BeginDecode((byte)val, LiteralHeaderFieldWithIncrementalIndexingPrefix)) + else if (_integerDecoder.BeginTryDecode((byte)val, LiteralHeaderFieldWithIncrementalIndexingPrefix, out intResult)) { - OnIndexedHeaderName(_integerDecoder.Value); + OnIndexedHeaderName(intResult); } else { @@ -178,9 +179,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack { _state = State.HeaderNameLength; } - else if (_integerDecoder.BeginDecode((byte)val, LiteralHeaderFieldWithoutIndexingPrefix)) + else if (_integerDecoder.BeginTryDecode((byte)val, LiteralHeaderFieldWithoutIndexingPrefix, out intResult)) { - OnIndexedHeaderName(_integerDecoder.Value); + OnIndexedHeaderName(intResult); } else { @@ -197,9 +198,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack { _state = State.HeaderNameLength; } - else if (_integerDecoder.BeginDecode((byte)val, LiteralHeaderFieldNeverIndexedPrefix)) + else if (_integerDecoder.BeginTryDecode((byte)val, LiteralHeaderFieldNeverIndexedPrefix, out intResult)) { - OnIndexedHeaderName(_integerDecoder.Value); + OnIndexedHeaderName(intResult); } else { @@ -217,9 +218,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack throw new HPackDecodingException(CoreStrings.HPackErrorDynamicTableSizeUpdateNotAtBeginningOfHeaderBlock); } - if (_integerDecoder.BeginDecode((byte)(b & ~DynamicTableSizeUpdateMask), DynamicTableSizeUpdatePrefix)) + if (_integerDecoder.BeginTryDecode((byte)(b & ~DynamicTableSizeUpdateMask), DynamicTableSizeUpdatePrefix, out intResult)) { - SetDynamicHeaderTableSize(_integerDecoder.Value); + SetDynamicHeaderTableSize(intResult); } else { @@ -234,25 +235,25 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack break; case State.HeaderFieldIndex: - if (_integerDecoder.Decode(b)) + if (_integerDecoder.TryDecode(b, out intResult)) { - OnIndexedHeaderField(_integerDecoder.Value, handler); + OnIndexedHeaderField(intResult, handler); } break; case State.HeaderNameIndex: - if (_integerDecoder.Decode(b)) + if (_integerDecoder.TryDecode(b, out intResult)) { - OnIndexedHeaderName(_integerDecoder.Value); + OnIndexedHeaderName(intResult); } break; case State.HeaderNameLength: _huffman = (b & HuffmanMask) != 0; - if (_integerDecoder.BeginDecode((byte)(b & ~HuffmanMask), StringLengthPrefix)) + if (_integerDecoder.BeginTryDecode((byte)(b & ~HuffmanMask), StringLengthPrefix, out intResult)) { - OnStringLength(_integerDecoder.Value, nextState: State.HeaderName); + OnStringLength(intResult, nextState: State.HeaderName); } else { @@ -261,9 +262,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack break; case State.HeaderNameLengthContinue: - if (_integerDecoder.Decode(b)) + if (_integerDecoder.TryDecode(b, out intResult)) { - OnStringLength(_integerDecoder.Value, nextState: State.HeaderName); + OnStringLength(intResult, nextState: State.HeaderName); } break; @@ -279,10 +280,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack case State.HeaderValueLength: _huffman = (b & HuffmanMask) != 0; - if (_integerDecoder.BeginDecode((byte)(b & ~HuffmanMask), StringLengthPrefix)) + if (_integerDecoder.BeginTryDecode((byte)(b & ~HuffmanMask), StringLengthPrefix, out intResult)) { - OnStringLength(_integerDecoder.Value, nextState: State.HeaderValue); - if (_integerDecoder.Value == 0) + OnStringLength(intResult, nextState: State.HeaderValue); + if (intResult == 0) { ProcessHeaderValue(handler); } @@ -294,10 +295,10 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack break; case State.HeaderValueLengthContinue: - if (_integerDecoder.Decode(b)) + if (_integerDecoder.TryDecode(b, out intResult)) { - OnStringLength(_integerDecoder.Value, nextState: State.HeaderValue); - if (_integerDecoder.Value == 0) + OnStringLength(intResult, nextState: State.HeaderValue); + if (intResult == 0) { ProcessHeaderValue(handler); } @@ -314,9 +315,9 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack break; case State.DynamicTableSizeUpdate: - if (_integerDecoder.Decode(b)) + if (_integerDecoder.TryDecode(b, out intResult)) { - SetDynamicHeaderTableSize(_integerDecoder.Value); + SetDynamicHeaderTableSize(intResult); _state = State.Ready; } diff --git a/src/Kestrel.Core/Internal/Http2/HPack/IntegerDecoder.cs b/src/Kestrel.Core/Internal/Http2/HPack/IntegerDecoder.cs index 5bc051a9a3..4805993b6f 100644 --- a/src/Kestrel.Core/Internal/Http2/HPack/IntegerDecoder.cs +++ b/src/Kestrel.Core/Internal/Http2/HPack/IntegerDecoder.cs @@ -3,40 +3,65 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack { + /// + /// The maximum we will decode is Int32.MaxValue, which is also the maximum request header field size. + /// public class IntegerDecoder { private int _i; private int _m; - public int Value { get; private set; } - - public bool BeginDecode(byte b, int prefixLength) + /// + /// Callers must ensure higher bits above the prefix are cleared before calling this method. + /// + /// + /// + /// + /// + public bool BeginTryDecode(byte b, int prefixLength, out int result) { if (b < ((1 << prefixLength) - 1)) { - Value = b; - return true; - } - else - { - _i = b; - _m = 0; - return false; - } - } - - public bool Decode(byte b) - { - _i = _i + (b & 127) * (1 << _m); - _m = _m + 7; - - if ((b & 128) != 128) - { - Value = _i; + result = b; return true; } + _i = b; + _m = 0; + result = 0; return false; } + + public bool TryDecode(byte b, out int result) + { + var m = _m; // Enregister + var i = _i + ((b & 0x7f) << m); // Enregister + + if ((b & 0x80) == 0) + { + // Int32.MaxValue only needs a maximum of 5 bytes to represent and the last byte cannot have any value set larger than 0x7 + if ((m > 21 && b > 0x7) || i < 0) + { + ThrowIntegerTooBigException(); + } + + result = i; + return true; + } + else if (m > 21) + { + // Int32.MaxValue only needs a maximum of 5 bytes to represent + ThrowIntegerTooBigException(); + } + + _m = m + 7; + _i = i; + + result = 0; + return false; + } + + public static void ThrowIntegerTooBigException() + => throw new HPackDecodingException(CoreStrings.HPackErrorIntegerTooBig); } } diff --git a/src/Kestrel.Core/Properties/CoreStrings.Designer.cs b/src/Kestrel.Core/Properties/CoreStrings.Designer.cs index 4717a70da3..51388a5b88 100644 --- a/src/Kestrel.Core/Properties/CoreStrings.Designer.cs +++ b/src/Kestrel.Core/Properties/CoreStrings.Designer.cs @@ -2170,6 +2170,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core internal static string FormatHPackErrorNotEnoughBuffer() => GetString("HPackErrorNotEnoughBuffer"); + /// + /// The decoded integer exceeds the maximum value of Int32.MaxValue. + /// + internal static string HPackErrorIntegerTooBig + { + get => GetString("HPackErrorIntegerTooBig"); + } + + /// + /// The decoded integer exceeds the maximum value of Int32.MaxValue. + /// + internal static string FormatHPackErrorIntegerTooBig() + => GetString("HPackErrorIntegerTooBig"); + private static string GetString(string name, params string[] formatterNames) { var value = _resourceManager.GetString(name); diff --git a/test/Kestrel.Core.Tests/HPackIntegerTests.cs b/test/Kestrel.Core.Tests/HPackIntegerTests.cs new file mode 100644 index 0000000000..06cd800a7f --- /dev/null +++ b/test/Kestrel.Core.Tests/HPackIntegerTests.cs @@ -0,0 +1,39 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +using System.Linq; +using Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack; +using Xunit; + +namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests +{ + public class HPackIntegerTests + { + [Fact] + public void IntegerEncoderDecoderRoundtrips() + { + var decoder = new IntegerDecoder(); + var range = 1 << 8; + + foreach (var i in Enumerable.Range(0, range).Concat(Enumerable.Range(int.MaxValue - range + 1, range))) + { + for (int n = 1; n <= 8; n++) + { + var integerBytes = new byte[6]; + Assert.True(IntegerEncoder.Encode(i, n, integerBytes, out var length)); + + var decodeResult = decoder.BeginTryDecode(integerBytes[0], n, out var intResult); + + for (int j = 1; j < length; j++) + { + Assert.False(decodeResult); + decodeResult = decoder.TryDecode(integerBytes[j], out intResult); + } + + Assert.True(decodeResult); + Assert.Equal(i, intResult); + } + } + } + } +} diff --git a/test/Kestrel.Core.Tests/IntegerDecoderTests.cs b/test/Kestrel.Core.Tests/IntegerDecoderTests.cs index b89b0f84a3..fdebb56922 100644 --- a/test/Kestrel.Core.Tests/IntegerDecoderTests.cs +++ b/test/Kestrel.Core.Tests/IntegerDecoderTests.cs @@ -13,7 +13,7 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests public void IntegerDecode(int i, int prefixLength, byte[] octets) { var decoder = new IntegerDecoder(); - var result = decoder.BeginDecode(octets[0], prefixLength); + var result = decoder.BeginTryDecode(octets[0], prefixLength, out var intResult); if (octets.Length == 1) { @@ -25,13 +25,28 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests for (; j < octets.Length - 1; j++) { - Assert.False(decoder.Decode(octets[j])); + Assert.False(decoder.TryDecode(octets[j], out intResult)); } - Assert.True(decoder.Decode(octets[j])); + Assert.True(decoder.TryDecode(octets[j], out intResult)); } - Assert.Equal(i, decoder.Value); + Assert.Equal(i, intResult); + } + + [Theory] + [MemberData(nameof(IntegerData_OverMax))] + public void IntegerDecode_Throws_IfMaxExceeded(int prefixLength, byte[] octets) + { + var decoder = new IntegerDecoder(); + var result = decoder.BeginTryDecode(octets[0], prefixLength, out var intResult); + + for (var j = 1; j < octets.Length - 1; j++) + { + Assert.False(decoder.TryDecode(octets[j], out intResult)); + } + + Assert.Throws(() => decoder.TryDecode(octets[octets.Length - 1], out intResult)); } public static TheoryData IntegerData @@ -43,6 +58,26 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests data.Add(10, 5, new byte[] { 10 }); data.Add(1337, 5, new byte[] { 0x1f, 0x9a, 0x0a }); data.Add(42, 8, new byte[] { 42 }); + data.Add(7, 3, new byte[] { 0x7, 0x0 }); + data.Add(int.MaxValue, 1, new byte[] { 0x01, 0xfe, 0xff, 0xff, 0xff, 0x07 }); + data.Add(int.MaxValue, 8, new byte[] { 0xff, 0x80, 0xfe, 0xff, 0xff, 0x07 }); + + return data; + } + } + + public static TheoryData IntegerData_OverMax + { + get + { + var data = new TheoryData(); + + data.Add(1, new byte[] { 0x01, 0xff, 0xff, 0xff, 0xff, 0x07 }); // Int32.MaxValue + 1 + data.Add(1, new byte[] { 0x01, 0xff, 0xff, 0xff, 0xff, 0x08 }); // MSB exceeds maximum + data.Add(1, new byte[] { 0x01, 0xff, 0xff, 0xff, 0xff, 0x80 }); // Undefined since continuation bit set + data.Add(8, new byte[] { 0xff, 0x81, 0xfe, 0xff, 0xff, 0x07 }); // Int32.MaxValue + 1 + data.Add(8, new byte[] { 0xff, 0x81, 0xfe, 0xff, 0xff, 0x08 }); // MSB exceeds maximum + data.Add(8, new byte[] { 0xff, 0x81, 0xfe, 0xff, 0xff, 0x80 }); // Undefined since continuation bit set return data; } diff --git a/test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs b/test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs index ef2be68ae4..c0ec666f62 100644 --- a/test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs +++ b/test/Kestrel.InMemory.FunctionalTests/Http2/Http2ConnectionTests.cs @@ -1527,6 +1527,34 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core.Tests expectedErrorMessage: CoreStrings.HPackErrorIncompleteHeaderBlock); } + [Fact] + public async Task HEADERS_Received_IntegerOverLimit_ConnectionError() + { + await InitializeConnectionAsync(_noopApplication); + + var outputWriter = _pair.Application.Output; + var frame = new Http2Frame(); + + frame.PrepareHeaders(Http2HeadersFrameFlags.END_HEADERS, 1); + frame.PayloadLength = 7; + var payload = new byte[] + { + // Set up an incomplete Literal Header Field w/ Incremental Indexing frame, + 0x00, + // with an name of size that's greater than int.MaxValue + 0x7f, 0x80, 0x80, 0x80, 0x80, 0x7f + }; + + Http2FrameWriter.WriteHeader(frame, outputWriter); + await SendAsync(payload); + + await WaitForConnectionErrorAsync( + ignoreNonGoAwayFrames: false, + expectedLastStreamId: 1, + expectedErrorCode: Http2ErrorCode.COMPRESSION_ERROR, + expectedErrorMessage: CoreStrings.HPackErrorIntegerTooBig); + } + [Theory] [MemberData(nameof(IllegalTrailerData))] public async Task HEADERS_Received_WithTrailers_ContainsIllegalTrailer_ConnectionError(byte[] trailers, string expectedErrorMessage)