From d090d7a261e4ca3a2f46ba3f8259bd67f2668abe Mon Sep 17 00:00:00 2001 From: Shaun Cummings Date: Tue, 28 Nov 2023 11:34:07 -0700 Subject: [PATCH] + (Core) Fixed an issue that prevented datetime values from being passed through the login page URL redirection. (Fixes #5615) --- Rock.Blocks/Security/AccountEntry.cs | 2 +- Rock.Blocks/Security/Login.cs | 2 +- .../ExtensionMethods/StringExtensions.cs | 166 +++++++++++++++++- .../ExtensionMethods/StringExtensionsTests.cs | 35 ++++ .../PhoneNumberIdentification.ascx.cs | 3 +- 5 files changed, 203 insertions(+), 5 deletions(-) diff --git a/Rock.Blocks/Security/AccountEntry.cs b/Rock.Blocks/Security/AccountEntry.cs index 3f9951ae024..7c70e8cd612 100644 --- a/Rock.Blocks/Security/AccountEntry.cs +++ b/Rock.Blocks/Security/AccountEntry.cs @@ -1082,7 +1082,7 @@ private string GetSafeDecodedUrl( string url ) // Remove the http and https schemes before checking if URL contains XSS objects. if ( decodedUrl.Replace( "https://", string.Empty ) .Replace( "http://", string.Empty ) - .HasXssObjects() ) + .RedirectUrlContainsXss() ) { return null; } diff --git a/Rock.Blocks/Security/Login.cs b/Rock.Blocks/Security/Login.cs index a30af0e69d6..7250fb2b01f 100644 --- a/Rock.Blocks/Security/Login.cs +++ b/Rock.Blocks/Security/Login.cs @@ -1293,7 +1293,7 @@ private string GetSafeDecodedUrl( string url ) // Remove the http and https schemes before checking if URL contains XSS objects. if ( decodedUrl.Replace( "https://", string.Empty ) .Replace( "http://", string.Empty ) - .HasXssObjects() ) + .RedirectUrlContainsXss() ) { return null; } diff --git a/Rock.Common/ExtensionMethods/StringExtensions.cs b/Rock.Common/ExtensionMethods/StringExtensions.cs index c6076233950..b70d426f21a 100644 --- a/Rock.Common/ExtensionMethods/StringExtensions.cs +++ b/Rock.Common/ExtensionMethods/StringExtensions.cs @@ -181,6 +181,8 @@ public static string HmacSha256Hash( this string str, string keyString ) /// Accepts an encoded string and returns an encoded string /// /// + [RockObsolete( "1.16.1" )] + [Obsolete( "This method is no longer suitable. Consider using RedirectUrlContainsXss." )] public static string ScrubEncodedStringForXSSObjects( this string encodedString ) { var decodedString = encodedString.GetFullyUrlDecodedValue(); @@ -200,6 +202,8 @@ public static string ScrubEncodedStringForXSSObjects( this string encodedString /// /// true if has XSS objects; otherwise, false. /// + [RockObsolete( "1.16.1" )] + [Obsolete( "This method is no longer suitable. Consider using RedirectUrlContainsXss." )] public static bool HasXssObjects( this string decodedString ) { // Characters used by DOM Objects; javascript, document, window and URLs @@ -213,13 +217,65 @@ public static bool HasXssObjects( this string decodedString ) return false; } + /// + /// Checks a value intended to be a redirect URL for XSS injection methods. + /// + /// The value intended to be used for a redirect URL. + /// + /// true if [redirectUrl contains XSS injection methods]; otherwise, false. + /// + public static bool RedirectUrlContainsXss( this string redirectUrl ) + { + if ( string.IsNullOrWhiteSpace( redirectUrl ) ) + { + return false; // Early exit, nothing to check. + } + + // These are characters which we will not ever allow in a URL query string value. We used to block colon (:), but that + // has a valid use in DateTime values. + char[] badCharacters = new char[] { '<', '>', '*' }; + + // Ensure we URL and HTML decode all characters, even if they are double/triple/etc encoded. + var decodedString = redirectUrl.GetFullyUrlDecodedValue().GetFullyHtmlDecodedValue(); + + // Check for bad characters. + if ( decodedString.IndexOfAny( badCharacters ) >= 0 ) + { + return true; + } + + // Some special whitespace characters are ignored by the browser when parsing script. Remove all whitespace from the + // string and make it lower case for easier comparison. + decodedString = Regex.Replace( decodedString, @"\s+", "" ).ToLower(); + + // These are strings that should never be allowed in a URL query string value. + string[] blockedStrings = new string[] { "javascript:" }; + + // Check for bad strings. + foreach ( string blockedString in blockedStrings ) + { + if ( decodedString.Contains( blockedString ) ) + { + return true; + } + } + + // Everything looks okay. + return false; + } + /// /// Gets a fully URL-decoded string (or returns string.Empty if it cannot be decoded within 10 attempts). /// - /// + /// The encoded string. /// public static string GetFullyUrlDecodedValue( this string encodedString ) { + if ( string.IsNullOrWhiteSpace( encodedString) ) + { + return encodedString; + } + int loopCount = 0; var decodedString = encodedString; var testString = WebUtility.UrlDecode( encodedString ); @@ -238,6 +294,114 @@ public static string GetFullyUrlDecodedValue( this string encodedString ) return decodedString; } + /// + /// Gets a fully HTML-decoded string (or returns string.Empty if it cannot be decoded within 10 attempts). + /// + /// The encoded string. + /// + public static string GetFullyHtmlDecodedValue( this string encodedString ) + { + if ( string.IsNullOrWhiteSpace( encodedString ) ) + { + return encodedString; + } + + int loopCount = 0; + var decodedString = encodedString; + var testString = HtmlDecodeCharactersWithoutSeparators( encodedString ); + while ( testString != decodedString ) + { + loopCount++; + if ( loopCount >= 10 ) + { + return string.Empty; + } + + decodedString = testString; + testString = HtmlDecodeCharactersWithoutSeparators( testString ); + } + + return decodedString; + } + + /// + /// This method inserts missing semi-colon separators into HTML-encoded character strings that use hex or decimal character + /// references before HTML decoding them. Browsers will render these strings without the separators, but + /// will not. + /// + /// The encoded string. + /// + private static string HtmlDecodeCharactersWithoutSeparators( this string encodedString ) + { + if ( string.IsNullOrWhiteSpace( encodedString ) ) + { + return encodedString; + } + + // Hex encoded HTML characters should follow the format "�" (for decimal) or � (for hex). If we don't have + // an ampersand together with a pound symbol, we can just use the base method. + if ( !encodedString.Contains( "&#" ) ) + { + return WebUtility.HtmlDecode( encodedString ); + } + + // This loop will shift each segment of the string from encodedString to correctedEncodedString, adding any missing + // semi-colons after each decimal or hex encoded character. + var correctedEncodedString = string.Empty; + while ( encodedString.Contains( "&#" ) ) + { + var elementBoundary = encodedString.IndexOf( "&#" ); + + // Start by putting everything before the next &# into the corrected string and removing it from the original. + correctedEncodedString += encodedString.Substring( 0, elementBoundary ) + "&#"; + elementBoundary += 2; + encodedString = encodedString.Substring( elementBoundary, encodedString.Length - elementBoundary ); + + // If the next character in the string is an "x", move it to the corrected string. + var nextChar = encodedString.FirstOrDefault(); + bool useHex = false; + if ( nextChar == 'x' ) + { + useHex = true; + correctedEncodedString += nextChar; + encodedString = encodedString.Substring( 1, encodedString.Length - 1 ); + nextChar = encodedString.FirstOrDefault(); + } + + // Keep moving any digits to the corrected string. There's technically no limit on padding zeros. + var isDigit = useHex ? nextChar.IsHexDigit() : char.IsDigit( nextChar ); + while ( isDigit ) + { + correctedEncodedString += nextChar; + encodedString = encodedString.Substring( 1, encodedString.Length - 1 ); + nextChar = encodedString.FirstOrDefault(); + isDigit = useHex ? nextChar.IsHexDigit() : char.IsDigit( nextChar ); + } + + // Add missing semi-colons. + if ( nextChar != ';' ) + { + correctedEncodedString += ";"; + } + } + + // Re-append any trailing characters left over in the original string. + correctedEncodedString += encodedString; + + return WebUtility.HtmlDecode( correctedEncodedString ); + } + + /// + /// Checks a digit to see if it's a valid hexadecimal character (0-9 or A-F). Permits lowercase. + /// + /// The character. + /// + private static bool IsHexDigit( this char c ) + { + char[] nonNumericHexCharacters = new char[] { 'A', 'B', 'C', 'D', 'E', 'F', 'a', 'b', 'c', 'd', 'e', 'f' }; + return char.IsDigit( c ) || nonNumericHexCharacters.Contains( c ); + } + /// /// Joins and array of strings using the provided separator. /// diff --git a/Rock.Tests.UnitTests/Rock/Utility/ExtensionMethods/StringExtensionsTests.cs b/Rock.Tests.UnitTests/Rock/Utility/ExtensionMethods/StringExtensionsTests.cs index 24ef9d708f3..9c18b29f9b3 100644 --- a/Rock.Tests.UnitTests/Rock/Utility/ExtensionMethods/StringExtensionsTests.cs +++ b/Rock.Tests.UnitTests/Rock/Utility/ExtensionMethods/StringExtensionsTests.cs @@ -234,5 +234,40 @@ public void AsNumeric_NumbersOnly() } #endregion + + #region RedirectUrlContainsXss + + [DataRow( "page/1" )] + [DataRow( "test test" )] + [DataRow( "Occurrence=2023-09-28T09:00:00" )] // Valid date input. + [DataRow( "Occurrence%253d2023-09-28T09%25253a00%25253a00" )] // Valid date input, partially double and triple encoded. + [DataTestMethod] + public void RedirectUrlContainsXss_ValidInput( string input ) + { + var output = input.RedirectUrlContainsXss(); + Assert.That.AreEqual( output, false ); + + } + + [DataRow( "