From 97bacabec72d0c1450685d895d0f9468658bd506 Mon Sep 17 00:00:00 2001 From: wtgodbe Date: Thu, 16 May 2019 16:02:50 -0700 Subject: [PATCH 1/3] Fix Loop in System.Private.Uri --- src/System.Private.Uri/src/System/Uri.cs | 28 +++++- src/System.Private.Uri/src/System/UriExt.cs | 27 +++++- .../tests/FunctionalTests/IriTest.cs | 85 +++++++++++++++++++ 3 files changed, 135 insertions(+), 5 deletions(-) diff --git a/src/System.Private.Uri/src/System/Uri.cs b/src/System.Private.Uri/src/System/Uri.cs index 5a470b0dca4b..9de08571d8e0 100644 --- a/src/System.Private.Uri/src/System/Uri.cs +++ b/src/System.Private.Uri/src/System/Uri.cs @@ -2538,7 +2538,7 @@ private unsafe void CreateHostString() } 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 @@ private unsafe void GetHostViaCustomSyntax() 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 @@ private unsafe void ParseRemaining() 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 @@ private unsafe void ParseRemaining() 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 @@ private unsafe void ParseRemaining() 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 @@ private unsafe ushort CheckAuthorityHelper(char* pString, ushort idx, ushort len // 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/System.Private.Uri/src/System/UriExt.cs b/src/System.Private.Uri/src/System/UriExt.cs index 6718b5726908..06c5b18fc2b8 100644 --- a/src/System.Private.Uri/src/System/UriExt.cs +++ b/src/System.Private.Uri/src/System/UriExt.cs @@ -120,8 +120,16 @@ private void InitializeUri(ParsingError err, UriKind uriKind, out UriFormatExcep 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 @@ private void InitializeUri(ParsingError err, UriKind uriKind, out UriFormatExcep 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 @@ private void InitializeUri(ParsingError err, UriKind uriKind, out UriFormatExcep // 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/System.Private.Uri/tests/FunctionalTests/IriTest.cs b/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs index 5876419c27d9..75738c63d9c3 100644 --- a/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs +++ b/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs @@ -599,5 +599,90 @@ public void Iri_AllForbiddenDecompositions_NonIdnPropertiesOk(string scheme, str 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. + 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. + 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))] + 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)); + } } } From 83831719f5608cbf6e718e134dea889d0619e0c7 Mon Sep 17 00:00:00 2001 From: wtgodbe Date: Mon, 20 May 2019 13:51:14 -0700 Subject: [PATCH 2/3] Mark variables in test project as private --- src/System.Private.Uri/tests/FunctionalTests/IriTest.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs b/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs index 75738c63d9c3..3d08146b6e3e 100644 --- a/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs +++ b/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs @@ -604,12 +604,12 @@ public void Iri_AllForbiddenDecompositions_NonIdnPropertiesOk(string scheme, str // 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. - static int InitialLengthLimit = 65519; + 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. - static int ExpandedLengthLimit = 65535; + 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) From badad2d375eff6159064ae36ddb8e228736ef896 Mon Sep 17 00:00:00 2001 From: wtgodbe Date: Thu, 30 May 2019 15:02:39 -0700 Subject: [PATCH 3/3] Disable test on NetFx --- src/System.Private.Uri/tests/FunctionalTests/IriTest.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs b/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs index 3d08146b6e3e..af59c908a4b9 100644 --- a/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs +++ b/src/System.Private.Uri/tests/FunctionalTests/IriTest.cs @@ -648,6 +648,7 @@ public static IEnumerable Iri_ExpandingContents_TooLong [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); });