From 46bf97a94a545f9b8a0c3557d88c2b05239026c1 Mon Sep 17 00:00:00 2001 From: William Godbe Date: Fri, 31 May 2019 09:33:19 -0700 Subject: [PATCH] Fix Loop in System.Private.Uri (dotnet/corefx#38076) * Fix Loop in System.Private.Uri * Mark variables in test project as private * Disable test on NetFx Commit migrated from https://github.com/dotnet/corefx/commit/0f2fa63930a26fbb70fbb6601c6d23c9c263d08b --- src/libraries/System.Private.Uri/src/System/Uri.cs | 28 ++++++- .../System.Private.Uri/src/System/UriExt.cs | 27 ++++++- .../tests/FunctionalTests/IriTest.cs | 86 ++++++++++++++++++++++ 3 files changed, 136 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Private.Uri/src/System/Uri.cs b/src/libraries/System.Private.Uri/src/System/Uri.cs index 5a470b0..9de0857 100644 --- a/src/libraries/System.Private.Uri/src/System/Uri.cs +++ b/src/libraries/System.Private.Uri/src/System/Uri.cs @@ -2538,7 +2538,7 @@ namespace System } else { - for (ushort i = 0; i < host.Length; ++i) + for (int i = 0; i < host.Length; ++i) { if ((_info.Offset.Host + i) >= _info.Offset.End || host[i] != _string[_info.Offset.Host + i]) @@ -2655,7 +2655,7 @@ namespace System else { host = CreateHostStringHelper(host, 0, (ushort)host.Length, ref flags, ref _info.ScopeId); - for (ushort i = 0; i < host.Length; ++i) + for (int i = 0; i < host.Length; ++i) { if ((_info.Offset.Host + i) >= _info.Offset.End || host[i] != _string[_info.Offset.Host + i]) { @@ -3404,6 +3404,12 @@ namespace System throw e; } + if (_string.Length > ushort.MaxValue) + { + UriFormatException e = GetException(ParsingError.SizeLimit); + throw e; + } + length = (ushort)_string.Length; // We need to be sure that there isn't a '?' separated from the path by spaces. if (_string == _originalUnicodeString) @@ -3536,6 +3542,12 @@ namespace System throw e; } + if (_string.Length > ushort.MaxValue) + { + UriFormatException e = GetException(ParsingError.SizeLimit); + throw e; + } + length = (ushort)_string.Length; // We need to be sure that there isn't a '#' separated from the query by spaces. if (_string == _originalUnicodeString) @@ -3598,6 +3610,12 @@ namespace System throw e; } + if (_string.Length > ushort.MaxValue) + { + UriFormatException e = GetException(ParsingError.SizeLimit); + throw e; + } + length = (ushort)_string.Length; // we don't need to check _originalUnicodeString == _string because # is last part GetLengthWithoutTrailingSpaces(_string, ref length, idx); @@ -4093,6 +4111,12 @@ namespace System // Normalize user info userInfoString = IriHelper.EscapeUnescapeIri(pString, startInput, start + 1, UriComponents.UserInfo); newHost += userInfoString; + + if (newHost.Length > ushort.MaxValue) + { + err = ParsingError.SizeLimit; + return idx; + } } else { diff --git a/src/libraries/System.Private.Uri/src/System/UriExt.cs b/src/libraries/System.Private.Uri/src/System/UriExt.cs index 6718b57..06c5b18 100644 --- a/src/libraries/System.Private.Uri/src/System/UriExt.cs +++ b/src/libraries/System.Private.Uri/src/System/UriExt.cs @@ -120,8 +120,16 @@ namespace System if (_iriParsing && hasUnicode) { - // In this scenario we need to parse the whole string - EnsureParseRemaining(); + // In this scenario we need to parse the whole string + try + { + EnsureParseRemaining(); + } + catch (UriFormatException ex) + { + e = ex; + return; + } } } else @@ -163,7 +171,15 @@ namespace System if (_iriParsing && hasUnicode) { // In this scenario we need to parse the whole string - EnsureParseRemaining(); + try + { + EnsureParseRemaining(); + } + catch (UriFormatException ex) + { + e = ex; + return; + } } } // will return from here @@ -181,6 +197,11 @@ namespace System // Iri'ze and then normalize relative uris _string = EscapeUnescapeIri(_originalUnicodeString, 0, _originalUnicodeString.Length, (UriComponents)0); + if (_string.Length > ushort.MaxValue) + { + err = ParsingError.SizeLimit; + return; + } } } else diff --git a/src/libraries/System.Private.Uri/tests/FunctionalTests/IriTest.cs b/src/libraries/System.Private.Uri/tests/FunctionalTests/IriTest.cs index d0b6196..b4aeddf 100644 --- a/src/libraries/System.Private.Uri/tests/FunctionalTests/IriTest.cs +++ b/src/libraries/System.Private.Uri/tests/FunctionalTests/IriTest.cs @@ -592,5 +592,91 @@ namespace System.PrivateUri.Tests Assert.Equal(host, uri.Authority); Assert.Equal(scheme + "://" + host + "/", uri.AbsoluteUri); } + + // The behavior here is slightly complicated in order to preserve compat in as many + // cases as possible. There are two limits imposed on the length of URI strings. + // The first, 65519, is specified in the documentation and is one of the first checks + // enforced on a URI. This limit is not enforced after expansion. + private static int InitialLengthLimit = 65519; + + // The second, 65535 (ushort.MaxValue) is only reachable via expansion as a result of + // percent encoding. Exceeding this value used to result in a hang, but now results in + // an exception. + private static int ExpandedLengthLimit = 65535; + + // In order to maximize compat, we have to allow a gap between the two maximum + // values. A URI that starts below 65519 but expands to be in the range [65519,65535) + // would have worked before this change, and so should continue to work despite + // exceeding limit (1). + public static IEnumerable Iri_ExpandingContents_TooLong + { + get + { + // Validate a URI with an initial length less than InitialLengthLimit, and an expanded + // length that is greater than ExpandedLengthLimit. + // The total of len + const parts (15) + expanded unicode (2 * 9) after expansion should be + // just larger than ExpandedLengthLimit. + int len = ExpandedLengthLimit - 15 - (2 * 9) + 1; + yield return new object[] { @"test://" + new string('a', len) + new string('\uD800', 2) + "@8.8.8.8" }; // Userinfo + yield return new object[] { @"test://8.8.8.8?" + new string('a', len) + new string('\uD800', 2) }; // Query + yield return new object[] { @"test://8.8.8.8#" + new string('a', len) + new string('\uD800', 2) }; // Fragment + yield return new object[] { @"test://8.8.8.8/" + new string('a', len) + new string('\uD800', 2) }; // Path + + // Generate a string whose total length is just less than InitialLengthLimit + // but whose content expands to be dramatically larger than ExpandedLengthLimit. + len = InitialLengthLimit - 15; + yield return new object[] { @"test://" + new string('\uD800', len) + "@8.8.8.8" }; // Userinfo + yield return new object[] { @"test://8.8.8.8?" + new string('\uD800', len) }; // Fragment + yield return new object[] { @"test://8.8.8.8#" + new string('\uD800', len) }; // Query + yield return new object[] { @"test://8.8.8.8/" + new string('\uD800', len) }; // Path + + // Test the minimum length URI that will cause an expansion beyond ExpandedLengthLimit. + len = (ExpandedLengthLimit - 15) / 9 + 1; + yield return new object[] { @"test://" + new string('\uD800', len) + "@8.8.8.8" }; // Userinfo + yield return new object[] { @"test://8.8.8.8?" + new string('\uD800', len) }; // Fragment + yield return new object[] { @"test://8.8.8.8#" + new string('\uD800', len) }; // Query + yield return new object[] { @"test://8.8.8.8/" + new string('\uD800', len) }; // Path + } + } + + [Theory] + [MemberData(nameof(Iri_ExpandingContents_TooLong))] + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Disable until the .NET FX CI machines get the latest patches.")] + public static void Iri_ExpandingContents_ThrowsIfTooLong(string input) + { + Assert.Throws(() => { Uri itemUri = new Uri(input); }); + Assert.False(Uri.TryCreate(input, UriKind.Absolute, out Uri itemUri2)); + } + + public static IEnumerable Iri_ExpandingContents_AllowedSize + { + get + { + // Validate a URI with an initial length less than InitialLengthLimit, and an expanded + // length that is greater than InitialLengthLimit but less than ExpandedLengthLimit. + // The total of len + const parts (15) + expanded unicode (2 * 9) after expansion should be + // exactly the ExpandedLengthLimit. + int len = ExpandedLengthLimit - 15 - (2 * 9); + yield return new object[] { @"test://" + new string('a', len) + new string('\uD800', 2) + "@8.8.8.8" }; // Userinfo + yield return new object[] { @"test://8.8.8.8?" + new string('a', len) + new string('\uD800', 2) }; // Query + yield return new object[] { @"test://8.8.8.8#" + new string('a', len) + new string('\uD800', 2) }; // Fragment + yield return new object[] { @"test://8.8.8.8/" + new string('a', len) + new string('\uD800', 2) }; // Path + + // Validate the same behavior, but maximize the amount of expansion. + len = (ExpandedLengthLimit - 15) / 9; + yield return new object[] { @"test://" + new string('\uD800', len) + "@8.8.8.8" }; // Userinfo + yield return new object[] { @"test://8.8.8.8?" + new string('\uD800', len) }; // Fragment + yield return new object[] { @"test://8.8.8.8#" + new string('\uD800', len) }; // Query + yield return new object[] { @"test://8.8.8.8/" + new string('\uD800', len) }; // Path + } + } + + [Theory] + [MemberData(nameof(Iri_ExpandingContents_AllowedSize))] + public static void Iri_ExpandingContents_DoesNotThrowIfSizeAllowed(string input) + { + Uri itemUri = new Uri(input); + Assert.True(Uri.TryCreate(input, UriKind.Absolute, out Uri itemUri2)); + } } } -- 2.7.4