Impose integer decode limit in HPACK

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 _);
+                }
+            }
+        }
+    }
+}

src/Kestrel.Core/CoreStrings.resx

@@ -1,17 +1,17 @@
 <?xml version="1.0" encoding="utf-8"?>
 <root>
-  <!--
+  <!-- 
-    Microsoft ResX Schema
+    Microsoft ResX Schema 
-
+    
     Version 2.0
-
+    
-    The primary goals of this format is to allow a simple XML format
+    The primary goals of this format is to allow a simple XML format 
-    that is mostly human readable. The generation and parsing of the
+    that is mostly human readable. The generation and parsing of the 
-    various data types are done through the TypeConverter classes
+    various data types are done through the TypeConverter classes 
     associated with the data types.
-
+    
     Example:
-
+    
     ... ado.net/XML headers & schema ...
     <resheader name="resmimetype">text/microsoft-resx</resheader>
     <resheader name="version">2.0</resheader>
@@ -26,36 +26,36 @@
         <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
         <comment>This is a comment</comment>
     </data>
-
+                
-    There are any number of "resheader" rows that contain simple
+    There are any number of "resheader" rows that contain simple 
     name/value pairs.
-
+    
-    Each data row contains a name, and value. The row also contains a
+    Each data row contains a name, and value. The row also contains a 
-    type or mimetype. Type corresponds to a .NET class that support
+    type or mimetype. Type corresponds to a .NET class that support 
-    text/value conversion through the TypeConverter architecture.
+    text/value conversion through the TypeConverter architecture. 
-    Classes that don't support this are serialized and stored with the
+    Classes that don't support this are serialized and stored with the 
     mimetype set.
-
+    
-    The mimetype is used for serialized objects, and tells the
+    The mimetype is used for serialized objects, and tells the 
-    ResXResourceReader how to depersist the object. This is currently not
+    ResXResourceReader how to depersist the object. This is currently not 
     extensible. For a given mimetype the value must be set accordingly:
-
+    
-    Note - application/x-microsoft.net.object.binary.base64 is the format
+    Note - application/x-microsoft.net.object.binary.base64 is the format 
-    that the ResXResourceWriter will generate, however the reader can
+    that the ResXResourceWriter will generate, however the reader can 
     read any of the formats listed below.
-
+    
     mimetype: application/x-microsoft.net.object.binary.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
             : and then encoded with base64 encoding.
-
+    
     mimetype: application/x-microsoft.net.object.soap.base64
-    value   : The object must be serialized with
+    value   : The object must be serialized with 
             : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
             : and then encoded with base64 encoding.
 
     mimetype: application/x-microsoft.net.object.bytearray.base64
-    value   : The object must be serialized into a byte array
+    value   : The object must be serialized into a byte array 
             : using a System.ComponentModel.TypeConverter
             : and then encoded with base64 encoding.
     -->
@@ -581,4 +581,7 @@ For more information on configuring HTTPS see https://go.microsoft.com/fwlink/?l
   <data name="HPackErrorNotEnoughBuffer" xml:space="preserve">
     <value>The given buffer was too small to encode any headers.</value>
   </data>
+  <data name="HPackErrorIntegerTooBig" xml:space="preserve">
+    <value>The decoded integer exceeds the maximum value of Int32.MaxValue.</value>
+  </data>
 </root>

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);
+                        OnStringLength(intResult, nextState: State.HeaderValue);
-                        if (_integerDecoder.Value == 0)
+                        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);
+                        OnStringLength(intResult, nextState: State.HeaderValue);
-                        if (_integerDecoder.Value == 0)
+                        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;
                     }
 

src/Kestrel.Core/Internal/Http2/HPack/IntegerDecoder.cs

@@ -3,40 +3,65 @@
 
 namespace Microsoft.AspNetCore.Server.Kestrel.Core.Internal.Http2.HPack
 {
+    /// <summary>
+    /// The maximum we will decode is Int32.MaxValue, which is also the maximum request header field size.
+    /// </summary>
     public class IntegerDecoder
     {
         private int _i;
         private int _m;
 
-        public int Value { get; private set; }
+        /// <summary>
-
+        /// Callers must ensure higher bits above the prefix are cleared before calling this method.
-        public bool BeginDecode(byte b, int prefixLength)
+        /// </summary>
+        /// <param name="b"></param>
+        /// <param name="prefixLength"></param>
+        /// <param name="result"></param>
+        /// <returns></returns>
+        public bool BeginTryDecode(byte b, int prefixLength, out int result)
         {
             if (b < ((1 << prefixLength) - 1))
             {
-                Value = b;
+                result = 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;
                 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);
     }
 }

src/Kestrel.Core/Properties/CoreStrings.Designer.cs

@@ -2170,6 +2170,20 @@ namespace Microsoft.AspNetCore.Server.Kestrel.Core
         internal static string FormatHPackErrorNotEnoughBuffer()
             => GetString("HPackErrorNotEnoughBuffer");
 
+        /// <summary>
+        /// The decoded integer exceeds the maximum value of Int32.MaxValue.
+        /// </summary>
+        internal static string HPackErrorIntegerTooBig
+        {
+            get => GetString("HPackErrorIntegerTooBig");
+        }
+
+        /// <summary>
+        /// The decoded integer exceeds the maximum value of Int32.MaxValue.
+        /// </summary>
+        internal static string FormatHPackErrorIntegerTooBig()
+            => GetString("HPackErrorIntegerTooBig");
+
         private static string GetString(string name, params string[] formatterNames)
         {
             var value = _resourceManager.GetString(name);

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);
+                }
+            }
+        }
+    }
+}

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<HPackDecodingException>(() => decoder.TryDecode(octets[octets.Length - 1], out intResult));
         }
 
         public static TheoryData<int, int, byte[]> 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<int, byte[]> IntegerData_OverMax
+        {
+            get
+            {
+                var data = new TheoryData<int, byte[]>();
+
+                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;
             }

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<HPackDecodingException>(
+                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)