From: Cory Nelson Date: Fri, 2 Aug 2019 02:59:51 +0000 (-0700) Subject: HPACK decoder fixes (dotnet/corefx#39907) X-Git-Tag: submit/tizen/20210909.063632~11031^2~788 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=2bbb062233b8053b4dfa20ddc945ce7edc539e81;p=platform%2Fupstream%2Fdotnet%2Fruntime.git HPACK decoder fixes (dotnet/corefx#39907) - Fix HPACK decoder failing inconsistently with 0-length header names. Fail fast with same exception message that HTTP1 uses. Resolves dotnet/corefx#39873. - Fix `DynamicTable` returning incorrect values or throwing exceptions when it wraps its ring buffer. Resolves dotnet/corefx#39656. - Moves HPACK encoder methods from `Http2LoopbackConnection` into their own class. - Implement all forms of header encodings in HPACK encoder. - Move a number of files where they belong into the ProductionCode folder in unit test project. Commit migrated from https://github.com/dotnet/corefx/commit/b7e60c9aa17fbe043318afd68af31cd3a6d61a76 --- diff --git a/src/libraries/Common/tests/System/Net/Http/HPackEncoder.cs b/src/libraries/Common/tests/System/Net/Http/HPackEncoder.cs new file mode 100644 index 0000000..5b667e1 --- /dev/null +++ b/src/libraries/Common/tests/System/Net/Http/HPackEncoder.cs @@ -0,0 +1,194 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System.Diagnostics; +using System.Text; + +namespace System.Net.Test.Common +{ + public static class HPackEncoder + { + /// + /// Encodes a dynamic table size update. + /// + /// The new maximum size of the dynamic table. This must be less than or equal to the connection's maximum table size setting, which defaults to 4096 bytes. + /// A span to write the encoded header to. + /// The number of bytes written to . + public static int EncodeDynamicTableSizeUpdate(int newMaximumSize, Span headerBlock) + { + return EncodeInteger(newMaximumSize, 0b00100000, 0b11100000, headerBlock); + } + + /// + /// Encodes a header using an index for both name and value. + /// + /// The header index to encode. + /// A span to write the encoded header to. + /// The number of bytes written to . + public static int EncodeHeader(int headerIndex, Span headerBlock) + { + Debug.Assert(headerIndex > 0); + return EncodeInteger(headerIndex, 0b10000000, 0b10000000, headerBlock); + } + + /// + /// Encodes a header using an indexed name and literal value. + /// + /// An index of a header containing the name for this header. + /// A literal value to encode for this header. + /// A span to write the encoded header to. + /// The number of bytes written to . + public static int EncodeHeader(int nameIdx, string value, HPackFlags flags, Span headerBlock) + { + Debug.Assert(nameIdx > 0); + return EncodeHeaderImpl(nameIdx, null, value, flags, headerBlock); + } + + /// + /// Encodes a header using a literal name and value. + /// + /// A literal name to encode for this header. + /// A literal value to encode for this header. + /// A span to write the encoded header to. + /// The number of bytes written to . + public static int EncodeHeader(string name, string value, HPackFlags flags, Span headerBlock) + { + return EncodeHeaderImpl(0, name, value, flags, headerBlock); + } + + private static int EncodeHeaderImpl(int nameIdx, string name, string value, HPackFlags flags, Span headerBlock) + { + const HPackFlags IndexingMask = HPackFlags.NeverIndexed | HPackFlags.NewIndexed | HPackFlags.WithoutIndexing; + + Debug.Assert((nameIdx != 0) != (name != null), $"Only one of {nameof(nameIdx)} or {nameof(name)} can be used."); + Debug.Assert(name != null || (flags & HPackFlags.HuffmanEncodeName) == 0, "An indexed name can not be huffman encoded."); + + byte prefix, prefixMask; + + switch (flags & IndexingMask) + { + case HPackFlags.WithoutIndexing: + prefix = 0; + prefixMask = 0b11110000; + break; + case HPackFlags.NewIndexed: + prefix = 0b01000000; + prefixMask = 0b11000000; + break; + case HPackFlags.NeverIndexed: + prefix = 0b00010000; + prefixMask = 0b11110000; + break; + default: + throw new Exception("invalid indexing flag"); + } + + int bytesGenerated = EncodeInteger(nameIdx, prefix, prefixMask, headerBlock); + + if (name != null) + { + bytesGenerated += EncodeString(name, headerBlock.Slice(bytesGenerated), (flags & HPackFlags.HuffmanEncodeName) != 0); + } + + bytesGenerated += EncodeString(value, headerBlock.Slice(bytesGenerated), (flags & HPackFlags.HuffmanEncodeValue) != 0); + return bytesGenerated; + } + + private static int EncodeString(string value, Span headerBlock, bool huffmanEncode) + { + byte[] data = Encoding.ASCII.GetBytes(value); + byte prefix; + + if (!huffmanEncode) + { + prefix = 0; + } + else + { + int len = HuffmanEncoder.GetEncodedLength(data); + + byte[] huffmanData = new byte[len]; + HuffmanEncoder.Encode(data, huffmanData); + + data = huffmanData; + prefix = 0x80; + } + + int bytesGenerated = 0; + + bytesGenerated += EncodeInteger(data.Length, prefix, 0x80, headerBlock); + + data.AsSpan().CopyTo(headerBlock.Slice(bytesGenerated)); + bytesGenerated += data.Length; + + return bytesGenerated; + } + + public static int EncodeInteger(int value, byte prefix, byte prefixMask, Span headerBlock) + { + byte prefixLimit = (byte)(~prefixMask); + + if (value < prefixLimit) + { + headerBlock[0] = (byte)(prefix | value); + return 1; + } + + headerBlock[0] = (byte)(prefix | prefixLimit); + int bytesGenerated = 1; + + value -= prefixLimit; + + while (value >= 0x80) + { + headerBlock[bytesGenerated] = (byte)((value & 0x7F) | 0x80); + value >>= 7; + bytesGenerated++; + } + + headerBlock[bytesGenerated] = (byte)value; + bytesGenerated++; + + return bytesGenerated; + } + } + + public enum HPackFlags + { + /// + /// Encodes a header literal without indexing and without huffman encoding. + /// + None = 0, + + /// + /// Applies Huffman encoding to the header's name. + /// + HuffmanEncodeName = 1, + + /// + /// Applies Huffman encoding to the header's value. + /// + HuffmanEncodeValue = 2, + + /// + /// Applies Huffman encoding to both the name and the value of the header. + /// + HuffmanEncode = HuffmanEncodeName | HuffmanEncodeValue, + + /// + /// Encode a literal value without adding a new dynamic index. Intermediaries (such as a proxy) are still allowed to index the value when forwarding the header. + /// + WithoutIndexing = 0, + + /// + /// Encode a literal value to a new dynamic index. + /// + NewIndexed = 4, + + /// + /// Encode a literal value without adding a new dynamic index. Intermediaries (such as a proxy) must not index the value when forwarding the header. + /// + NeverIndexed = 8 + } +} diff --git a/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs b/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs index f5245c6..16c971b 100644 --- a/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs +++ b/src/libraries/Common/tests/System/Net/Http/Http2LoopbackConnection.cs @@ -3,6 +3,7 @@ // See the LICENSE file in the project root for more information. using System.Collections.Generic; +using System.Diagnostics; using System.IO; using System.Net.Http.Functional.Tests; using System.Net.Security; @@ -336,34 +337,6 @@ namespace System.Net.Test.Common return (2, prefixMask + b); } - private static int EncodeInteger(int value, byte prefix, byte prefixMask, Span headerBlock) - { - byte prefixLimit = (byte)(~prefixMask); - - if (value < prefixLimit) - { - headerBlock[0] = (byte)(prefix | value); - return 1; - } - - headerBlock[0] = (byte)(prefix | prefixLimit); - int bytesGenerated = 1; - - value -= prefixLimit; - - while (value >= 0x80) - { - headerBlock[bytesGenerated] = (byte)((value & 0x7F) | 0x80); - value = value >> 7; - bytesGenerated++; - } - - headerBlock[bytesGenerated] = (byte)value; - bytesGenerated++; - - return bytesGenerated; - } - private static (int bytesConsumed, string value) DecodeString(ReadOnlySpan headerBlock) { (int bytesConsumed, int stringLength) = DecodeInteger(headerBlock, 0b01111111); @@ -382,36 +355,6 @@ namespace System.Net.Test.Common } } - private static int EncodeString(string value, Span headerBlock, bool huffmanEncode) - { - byte[] data = Encoding.ASCII.GetBytes(value); - byte prefix; - - if (!huffmanEncode) - { - prefix = 0; - } - else - { - int len = HuffmanEncoder.GetEncodedLength(data); - - byte[] huffmanData = new byte[len]; - HuffmanEncoder.Encode(data, huffmanData); - - data = huffmanData; - prefix = 0x80; - } - - int bytesGenerated = 0; - - bytesGenerated += EncodeInteger(data.Length, prefix, 0x80, headerBlock); - - data.AsSpan().CopyTo(headerBlock.Slice(bytesGenerated)); - bytesGenerated += data.Length; - - return bytesGenerated; - } - private static readonly HttpHeaderData[] s_staticTable = new HttpHeaderData[] { new HttpHeaderData(":authority", ""), @@ -537,20 +480,6 @@ namespace System.Net.Test.Common } } - public static int EncodeHeader(HttpHeaderData headerData, Span headerBlock) - { - // Always encode as literal, no indexing. - int bytesGenerated = EncodeInteger(0, 0, 0b11110000, headerBlock); - bytesGenerated += EncodeString(headerData.Name, headerBlock.Slice(bytesGenerated), headerData.HuffmanEncoded); - bytesGenerated += EncodeString(headerData.Value, headerBlock.Slice(bytesGenerated), headerData.HuffmanEncoded); - return bytesGenerated; - } - - public static int EncodeDynamicTableSizeUpdate(int newMaximumSize, Span headerBlock) - { - return EncodeInteger(newMaximumSize, 0b00100000, 0b11100000, headerBlock); - } - public async Task ReadBodyAsync() { byte[] body = null; @@ -679,14 +608,14 @@ namespace System.Net.Test.Common if (!isTrailingHeader) { string statusCodeString = ((int)statusCode).ToString(); - bytesGenerated += EncodeHeader(new HttpHeaderData(":status", statusCodeString), headerBlock.AsSpan()); + bytesGenerated += HPackEncoder.EncodeHeader(":status", statusCodeString, HPackFlags.None, headerBlock.AsSpan()); } if (headers != null) { foreach (HttpHeaderData headerData in headers) { - bytesGenerated += EncodeHeader(headerData, headerBlock.AsSpan().Slice(bytesGenerated)); + bytesGenerated += HPackEncoder.EncodeHeader(headerData.Name, headerData.Value, headerData.HuffmanEncoded ? HPackFlags.HuffmanEncode : HPackFlags.None, headerBlock.AsSpan(bytesGenerated)); } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs index cfa3faa..b89d936 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/DynamicTable.cs @@ -34,7 +34,15 @@ namespace System.Net.Http.HPack throw new IndexOutOfRangeException(); } - return _buffer[_insertIndex == 0 ? _buffer.Length - 1 : _insertIndex - index - 1]; + index = _insertIndex - index - 1; + + if (index < 0) + { + // _buffer is circular; wrap the index back around. + index += _buffer.Length; + } + + return _buffer[index]; } } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecoder.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecoder.cs index 588a9c31..b9d86db 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecoder.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HPackDecoder.cs @@ -254,6 +254,11 @@ namespace System.Net.Http.HPack if (_integerDecoder.StartDecode((byte)(b & ~HuffmanMask), StringLengthPrefix)) { + if (_integerDecoder.Value == 0) + { + throw new HPackDecodingException(SR.Format(SR.net_http_invalid_response_header_name, "")); + } + OnStringLength(_integerDecoder.Value, nextState: State.HeaderName); } else @@ -265,6 +270,10 @@ namespace System.Net.Http.HPack case State.HeaderNameLengthContinue: if (_integerDecoder.Decode(b)) { + // IntegerDecoder disallows overlong encodings, where an integer is encoded with more bytes than is strictly required. + // 0 should always be represented by a single byte, so we shouldn't need to check for it in the continuation case. + Debug.Assert(_integerDecoder.Value != 0, "A header name length of 0 should never be encoded with a continuation byte."); + OnStringLength(_integerDecoder.Value, nextState: State.HeaderName); } @@ -302,6 +311,10 @@ namespace System.Net.Http.HPack case State.HeaderValueLengthContinue: if (_integerDecoder.Decode(b)) { + // IntegerDecoder disallows overlong encodings where an integer is encoded with more bytes than is strictly required. + // 0 should always be represented by a single byte, so we shouldn't need to check for it in the continuation case. + Debug.Assert(_integerDecoder.Value != 0, "A header value length of 0 should never be encoded with a continuation byte."); + OnStringLength(_integerDecoder.Value, nextState: State.HeaderValue); } diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HeaderField.cs b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HeaderField.cs index 94eadc8..f0f3341 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HeaderField.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HPack/HeaderField.cs @@ -29,6 +29,6 @@ namespace System.Net.Http.HPack public int Length => GetLength(Name.Length, Value.Length); - public static int GetLength(int nameLength, int valueLenth) => nameLength + valueLenth + 32; + public static int GetLength(int nameLength, int valueLenth) => nameLength + valueLenth + RfcOverhead; } } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs index f018c81..cd9554d 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Headers.cs @@ -265,5 +265,23 @@ namespace System.Net.Http.Functional.Tests await Assert.ThrowsAsync(() => client.SendAsync(m)); } } + + [Fact] + public async Task SendAsync_WithZeroLengthHeaderName_Throws() + { + await LoopbackServerFactory.CreateClientAndServerAsync( + async uri => + { + using HttpClient client = CreateHttpClient(); + await Assert.ThrowsAsync(() => client.GetAsync(uri)); + }, + async server => + { + await server.HandleRequestAsync(headers: new[] + { + new HttpHeaderData("", "foo") + }); + }); + } } } diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs index 1f4f67e..f11ace0 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.Http2.cs @@ -480,7 +480,7 @@ namespace System.Net.Http.Functional.Tests private static Frame MakeSimpleContinuationFrame(int streamId, bool endHeaders = false) { Memory headerBlock = new byte[Frame.MaxFrameLength]; - int bytesGenerated = Http2LoopbackConnection.EncodeHeader(new HttpHeaderData("foo", "bar"), headerBlock.Span); + int bytesGenerated = HPackEncoder.EncodeHeader("foo", "bar", HPackFlags.None, headerBlock.Span); return new ContinuationFrame(headerBlock.Slice(0, bytesGenerated), (endHeaders ? FrameFlags.EndHeaders : FrameFlags.None), @@ -3037,7 +3037,7 @@ namespace System.Net.Http.Functional.Tests } byte[] headerData = new byte[16]; - int headersLen = Http2LoopbackConnection.EncodeDynamicTableSizeUpdate(headerTableSize + 1, headerData); + int headersLen = HPackEncoder.EncodeDynamicTableSizeUpdate(headerTableSize + 1, headerData); HeadersFrame frame = new HeadersFrame(headerData.AsMemory(0, headersLen), FrameFlags.EndHeaders | FrameFlags.EndStream, 0, 0, 0, streamId); await con.WriteFrameAsync(frame); diff --git a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj index 8b94518..8bebfc8 100644 --- a/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj +++ b/src/libraries/System.Net.Http/tests/FunctionalTests/System.Net.Http.Functional.Tests.csproj @@ -159,6 +159,9 @@ Common\System\Net\Http\Http2Frames.cs + + Common\System\Net\Http\HPackEncoder.cs + Common\System\Net\Http\Http2LoopbackServer.cs diff --git a/src/libraries/System.Net.Http/tests/UnitTests/HPack/DynamicTableTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/HPack/DynamicTableTest.cs new file mode 100644 index 0000000..de3c2b6 --- /dev/null +++ b/src/libraries/System.Net.Http/tests/UnitTests/HPack/DynamicTableTest.cs @@ -0,0 +1,59 @@ +using System; +using System.Collections.Generic; +using System.Net.Http.HPack; +using System.Reflection; +using System.Text; +using Xunit; + +namespace System.Net.Http.Unit.Tests.HPack +{ + public class DynamicTableTest + { + [Theory] + [InlineData(0)] + [InlineData(1)] + [InlineData(2)] + [InlineData(3)] + public void DynamicTable_WrapsRingBuffer_Success(int targetInsertIndex) + { + FieldInfo insertIndexField = typeof(DynamicTable).GetField("_insertIndex", BindingFlags.NonPublic | BindingFlags.Instance); + var table = new DynamicTable(maxSize: 256); + var insertedHeaders = new Stack(); + + // Insert into dynamic table until its insert index into its ring buffer loops back to 0. + do + { + InsertOne(); + } + while ((int)insertIndexField.GetValue(table) != 0); + + // Finally loop until the insert index reaches the target. + while ((int)insertIndexField.GetValue(table) != targetInsertIndex) + { + InsertOne(); + } + + void InsertOne() + { + byte[] data = Encoding.ASCII.GetBytes($"header-{insertedHeaders.Count}"); + + insertedHeaders.Push(data); + table.Insert(data, data); + } + + // Now check to see that we can retrieve the remaining headers. + // Some headers will have been evacuated from the table during this process, so we don't exhaust the entire insertedHeaders stack. + Assert.True(table.Count > 0); + Assert.True(table.Count < insertedHeaders.Count); + + for (int i = 0; i < table.Count; ++i) + { + HeaderField dynamicField = table[i]; + byte[] expectedData = insertedHeaders.Pop(); + + Assert.True(expectedData.AsSpan().SequenceEqual(dynamicField.Name)); + Assert.True(expectedData.AsSpan().SequenceEqual(dynamicField.Value)); + } + } + } +} diff --git a/src/libraries/System.Net.Http/tests/UnitTests/HPack/HPackDecoderTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/HPack/HPackDecoderTest.cs index 1a4329b..0b6b387 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/HPack/HPackDecoderTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/HPack/HPackDecoderTest.cs @@ -15,6 +15,8 @@ using System.Net.Http.HPack; using Xunit; using System.Buffers; +using HPackEncoder = System.Net.Test.Common.HPackEncoder; + namespace System.Net.Http.Unit.Tests.HPack { public class HPackDecoderTest diff --git a/src/libraries/System.Net.Http/tests/UnitTests/System.Net.Http.Unit.Tests.csproj b/src/libraries/System.Net.Http/tests/UnitTests/System.Net.Http.Unit.Tests.csproj index e5f2d59..d07edd4 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/System.Net.Http.Unit.Tests.csproj +++ b/src/libraries/System.Net.Http/tests/UnitTests/System.Net.Http.Unit.Tests.csproj @@ -28,10 +28,10 @@ ProductionCode\Common\CoreLib\System\IO\StreamHelpers.CopyValidation.cs - Common\System\Net\InternalException.cs + ProductionCode\Common\System\Net\InternalException.cs - Common\System\Net\HttpDateParser.cs + ProductionCode\Common\System\Net\HttpDateParser.cs ProductionCode\Common\System\Net\HttpKnownHeaderNames.cs @@ -46,14 +46,20 @@ ProductionCode\Common\System\Net\Logging\NetEventSource.Common.cs - Common\System\Net\SecurityProtocol.cs + ProductionCode\Common\System\Net\SecurityProtocol.cs - Common\System\Net\UriScheme.cs + ProductionCode\Common\System\Net\UriScheme.cs Common\System\ShouldNotBeInvokedException.cs + + Common\System\Net\Http\HPackEncoder.cs + + + Common\System\Net\Http\HuffmanEncoder.cs + ProductionCode\Common\src\System\Net\Mail\MailAddress.cs @@ -407,6 +413,7 @@ + @@ -433,25 +440,25 @@ ProductionCode\System\Net\Http\WinHttpTraceHelper.cs - Common\Interop\Windows\Interop.Libraries.cs + ProductionCode\Common\Interop\Windows\Interop.Libraries.cs - Common\Interop\Windows\Crypt32\Interop.CertEnumCertificatesInStore.cs + ProductionCode\Common\Interop\Windows\Crypt32\Interop.CertEnumCertificatesInStore.cs - Common\Interop\Windows\Crypt32\Interop.certificates_types.cs + ProductionCode\Common\Interop\Windows\Crypt32\Interop.certificates_types.cs - Common\Interop\Windows\Interop.HRESULT_FROM_WIN32.cs + ProductionCode\Common\Interop\Windows\Interop.HRESULT_FROM_WIN32.cs - Common\Interop\Windows\WinHttp\Interop.SafeWinHttpHandle.cs + ProductionCode\Common\Interop\Windows\WinHttp\Interop.SafeWinHttpHandle.cs - Common\System\Runtime\ExceptionServices\ExceptionStackTrace.cs + ProductionCode\Common\System\Runtime\ExceptionServices\ExceptionStackTrace.cs - Common\Interop\Windows\WinHttp\Interop.winhttp_types.cs + ProductionCode\Common\Interop\Windows\WinHttp\Interop.winhttp_types.cs WinHttpHandler\UnitTests\FakeInterop.cs