From 9f55620f578ce0981043970ede2b6c3ddcdacd05 Mon Sep 17 00:00:00 2001 From: Miha Zupan Date: Wed, 4 Jun 2025 23:01:05 +0200 Subject: [PATCH] Fix various HTTP header parsing issues --- src/libraries/Fuzzing/.gitignore | 2 ++ .../Fuzzers/HttpHeadersFuzzer.cs | 17 ++++++++- src/libraries/Fuzzing/README.md | 6 ++-- .../Net/Http/Headers/AltSvcHeaderParser.cs | 6 ++-- .../Net/Http/Headers/CookieHeaderParser.cs | 5 ++- .../src/System/Net/Http/HttpRuleParser.cs | 23 +++++++++--- .../Headers/AltSvcHeaderParserTest.cs | 21 ++++++++--- .../Headers/CookieHeaderParserTest.cs | 36 +++++++++++++++++++ .../GenericHeaderParserTest/HostParserTest.cs | 6 ++++ .../tests/UnitTests/HttpRuleParserTest.cs | 4 +++ .../System.Net.Http.Unit.Tests.csproj | 1 + 11 files changed, 111 insertions(+), 16 deletions(-) create mode 100644 src/libraries/System.Net.Http/tests/UnitTests/Headers/CookieHeaderParserTest.cs diff --git a/src/libraries/Fuzzing/.gitignore b/src/libraries/Fuzzing/.gitignore index fb0bb057a399eb..f3c8d6dc86c3b5 100644 --- a/src/libraries/Fuzzing/.gitignore +++ b/src/libraries/Fuzzing/.gitignore @@ -3,3 +3,5 @@ publish deployment inputs crash-* +timeout-* +inputs-* diff --git a/src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/HttpHeadersFuzzer.cs b/src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/HttpHeadersFuzzer.cs index a7cf06395ac08c..b7d4586b91f255 100644 --- a/src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/HttpHeadersFuzzer.cs +++ b/src/libraries/Fuzzing/DotnetFuzzing/Fuzzers/HttpHeadersFuzzer.cs @@ -73,7 +73,22 @@ static void Test(HttpHeaders headers, string name, string value) } catch (FormatException) { } - foreach (var _ in headers) { } + // If values were added with validation, they should not contain CR or LF characters. + foreach ((_, HeaderStringValues values) in headers.NonValidated) + { + foreach (string headerValue in values) + { + Assert.False(headerValue.ContainsAny('\r', '\n')); + } + } + + foreach ((_, IEnumerable values) in headers) + { + foreach (string headerValue in values) + { + Assert.False(headerValue.ContainsAny('\r', '\n')); + } + } } } } diff --git a/src/libraries/Fuzzing/README.md b/src/libraries/Fuzzing/README.md index ff9b55c2f46697..2b7003385ea601 100644 --- a/src/libraries/Fuzzing/README.md +++ b/src/libraries/Fuzzing/README.md @@ -24,7 +24,7 @@ Build the runtime with the desired configuration if you haven't already: ``` > [!TIP] -> The `-rc release` configuration here builds runime in `Release` and libraries in `Debug` mode. +> The `-rc release` configuration here builds runtime in `Release` and libraries in `Debug` mode. > Automated fuzzing runs use a `Checked` runtime + `Debug` libraries configuration by default. > You can use any configuration locally, but `Checked` is recommended when testing changes in `System.Private.CoreLib`. @@ -44,10 +44,10 @@ dotnet build ``` Run `run.bat`, which will create separate directories for each fuzzing target, instrument the relevant assemblies, and generate a helper script for running them locally. -When iterating on changes, remember to rebuild the project again: `dotnet build; .\run.bat`. +When iterating on changes, remember to rebuild the project again. ```cmd -run.bat +dotnet build; .\run.bat ``` Start fuzzing by running the `local-run.bat` script in the folder of the fuzzer you are interested in. diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs index 36838b4d5832de..180a25131c7582 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/AltSvcHeaderParser.cs @@ -259,14 +259,14 @@ private static bool TryReadUnknownPercentEncodedAlpnProtocolName(ReadOnlySpan diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CookieHeaderParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CookieHeaderParser.cs index eb19d0ad07cc78..cc9dcbc7b5321d 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CookieHeaderParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/Headers/CookieHeaderParser.cs @@ -1,6 +1,7 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. +using System.Diagnostics; using System.Diagnostics.CodeAnalysis; namespace System.Net.Http.Headers @@ -18,8 +19,10 @@ private CookieHeaderParser() public override bool TryParseValue(string? value, object? storeValue, ref int index, [NotNullWhen(true)] out object? parsedValue) { + Debug.Assert(index == 0); + // Some headers support empty/null values. This one doesn't. - if (string.IsNullOrEmpty(value) || (index == value.Length)) + if (string.IsNullOrEmpty(value) || HttpRuleParser.ContainsNewLine(value)) { parsedValue = null; return false; diff --git a/src/libraries/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs b/src/libraries/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs index cad5eda382ca00..c64cdc02c8da9a 100644 --- a/src/libraries/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs +++ b/src/libraries/System.Net.Http/src/System/Net/Http/HttpRuleParser.cs @@ -20,6 +20,10 @@ internal static class HttpRuleParser private static readonly SearchValues s_hostDelimiterChars = SearchValues.Create("/ \t\r,"); + // Characters such as '?' or '#' are interpreted as an end of the host part of the URI, so they will not be validated in the same way. + private static readonly SearchValues s_disallowedHostChars = + SearchValues.Create("/\\?#@"); + private const int MaxNestedCount = 5; internal const char CR = (char)13; @@ -193,8 +197,8 @@ internal static HttpParseResult GetQuotedPairLength(string input, int startIndex } // Quoted-char has 2 characters. Check whether there are 2 chars left ('\' + char) - // If so, check whether the character is in the range 0-127. If not, it's an invalid value. - if ((startIndex + 2 > input.Length) || (input[startIndex + 1] > 127)) + // If so, check whether the character is in the range 0-127 and not a new line. Otherwise, it's an invalid value. + if ((startIndex + 2 > input.Length) || (input[startIndex + 1] is > (char)127 or '\r' or '\n')) { return HttpParseResult.InvalidFormat; } @@ -303,8 +307,19 @@ private static HttpParseResult GetExpressionLength(string input, int startIndex, private static bool IsValidHostName(ReadOnlySpan host) { - // Also add user info (u@) to make sure 'host' doesn't include user info. - return Uri.TryCreate($"http://u@{host}/", UriKind.Absolute, out _); + if (host.ContainsAny(s_disallowedHostChars)) + { + return false; + } + + // Using a trailing slash as Uri ignores trailing whitespace otherwise. + if (!Uri.TryCreate($"http://{host}/", UriKind.Absolute, out _)) + { + return false; + } + + Debug.Assert(!ContainsNewLine(host.ToString())); + return true; } } } diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/AltSvcHeaderParserTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/AltSvcHeaderParserTest.cs index 3b32f1933e4c6c..88d395533eaf49 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/AltSvcHeaderParserTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/AltSvcHeaderParserTest.cs @@ -9,14 +9,18 @@ namespace System.Net.Http.Tests { public class AltSvcHeaderParserTest { - [Fact] - public void TryParse_InvalidValueString_ReturnsFalse() + [Theory] + [InlineData("a=")] + [InlineData("%aa=\":123\"")] // Only uppercase hex is allowed + [InlineData("%0A=\":123\"")] // Encoded new line + public void TryParse_InvalidValueString_ReturnsFalse(string value) { HttpHeaderParser parser = AltSvcHeaderParser.Parser; - string invalidInput = "a="; int startIndex = 0; - Assert.False(parser.TryParseValue(invalidInput, null, ref startIndex, out var _)); + Assert.False(parser.TryParseValue(value, null, ref startIndex, out object? parsedValue)); + Assert.Equal(0, startIndex); + Assert.Null(parsedValue); } [Theory] @@ -117,6 +121,15 @@ public static IEnumerable SuccessfulParseData() AltSvcHeaderValue.Clear } }; + + // Encoded protocol name + yield return new object[] + { + "AB%43%44%EF=\":123\"", new[] + { + new AltSvcHeaderValue("ABCD\u00EF", host: null, 123, TimeSpan.FromTicks(AltSvcHeaderParser.DefaultMaxAgeTicks), persist: false) + } + }; } } } diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/CookieHeaderParserTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/CookieHeaderParserTest.cs new file mode 100644 index 00000000000000..ef720fed1052cc --- /dev/null +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/CookieHeaderParserTest.cs @@ -0,0 +1,36 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Net.Http.Headers; + +using Xunit; + +namespace System.Net.Http.Tests +{ + public class CookieHeaderParserTest + { + [Fact] + public void TryParse_ValidValueString_ReturnsTrue() + { + HttpHeaderParser parser = CookieHeaderParser.Parser; + string validInput = "Hello World"; + int startIndex = 0; + + Assert.True(parser.TryParseValue(validInput, null, ref startIndex, out object? parsedValue)); + Assert.Equal(validInput.Length, startIndex); + Assert.Same(validInput, parsedValue); + } + + [Fact] + public void TryParse_InvalidValueString_ReturnsFalse() + { + HttpHeaderParser parser = CookieHeaderParser.Parser; + string invalidInput = "Hello\r\nWorld"; + int startIndex = 0; + + Assert.False(parser.TryParseValue(invalidInput, null, ref startIndex, out object? parsedValue)); + Assert.Equal(0, startIndex); + Assert.Null(parsedValue); + } + } +} diff --git a/src/libraries/System.Net.Http/tests/UnitTests/Headers/GenericHeaderParserTest/HostParserTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/Headers/GenericHeaderParserTest/HostParserTest.cs index c149c59170ebd9..97d3c0bc32f63a 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/Headers/GenericHeaderParserTest/HostParserTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/Headers/GenericHeaderParserTest/HostParserTest.cs @@ -51,9 +51,15 @@ public void TryParse_SetOfInvalidValueStrings_ReturnsFalse() CheckInvalidParsedValue("host host", 0); CheckInvalidParsedValue("host,", 0); CheckInvalidParsedValue("host ,", 0); + CheckInvalidParsedValue("host/", 0); CheckInvalidParsedValue("/", 0); CheckInvalidParsedValue(" , ", 0); CheckInvalidParsedValue(" host\r\n ", 0); + CheckInvalidParsedValue(" host\r\n ", 1); + CheckInvalidParsedValue("host#\r\n", 0); + CheckInvalidParsedValue(" host?\r\n", 0); + CheckInvalidParsedValue("foo:bar@host", 0); + CheckInvalidParsedValue("host\n", 0); } #region Helper methods diff --git a/src/libraries/System.Net.Http/tests/UnitTests/HttpRuleParserTest.cs b/src/libraries/System.Net.Http/tests/UnitTests/HttpRuleParserTest.cs index 8efca2f7ff2887..25920a99392573 100644 --- a/src/libraries/System.Net.Http/tests/UnitTests/HttpRuleParserTest.cs +++ b/src/libraries/System.Net.Http/tests/UnitTests/HttpRuleParserTest.cs @@ -164,6 +164,10 @@ public void GetQuotedPairLength_SetOfInvalidQuotedPairs_AllConsideredInvalid() // only ASCII chars allowed in quoted-pair AssertGetQuotedPairLength("\\\u00FC", 0, 0, HttpParseResult.InvalidFormat); + // New lines are not allowed + AssertGetQuotedPairLength("\\\r", 0, 0, HttpParseResult.InvalidFormat); + AssertGetQuotedPairLength("\\\n", 0, 0, HttpParseResult.InvalidFormat); + // a quoted-pair needs 1 char after '\' AssertGetQuotedPairLength("\\", 0, 0, HttpParseResult.InvalidFormat); } 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 8050e71474c418..40eacbfd8e6e08 100755 --- 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 @@ -267,6 +267,7 @@ +