From a55a3027024d4492b897ab697dc41eb98e2ff422 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Tue, 25 Jul 2023 22:19:37 -0400 Subject: [PATCH 1/2] Avoid byte[] allocations in as part of byte[] comparisons - Changes Utility.AreEqual to be non-allocating - Changes SymmetricSignatureProvider.Verify to not allocate a byte[] for a hash - Changes SymmetricSignatureProvider.Verify to use StartsWith for the comparison rather than Utility.AreEqual --- build/dependencies.props | 1 + .../Microsoft.IdentityModel.Tokens.csproj | 1 + .../SymmetricSignatureProvider.cs | 17 +++++++- src/Microsoft.IdentityModel.Tokens/Utility.cs | 41 ++++++++----------- 4 files changed, 35 insertions(+), 25 deletions(-) diff --git a/build/dependencies.props b/build/dependencies.props index 0f2dd00964..2e7daee16a 100644 --- a/build/dependencies.props +++ b/build/dependencies.props @@ -10,6 +10,7 @@ 4.5.0 4.7.2 4.7.2 + 4.5.5 diff --git a/src/Microsoft.IdentityModel.Tokens/Microsoft.IdentityModel.Tokens.csproj b/src/Microsoft.IdentityModel.Tokens/Microsoft.IdentityModel.Tokens.csproj index 22a02c7846..ef494717db 100644 --- a/src/Microsoft.IdentityModel.Tokens/Microsoft.IdentityModel.Tokens.csproj +++ b/src/Microsoft.IdentityModel.Tokens/Microsoft.IdentityModel.Tokens.csproj @@ -43,6 +43,7 @@ + diff --git a/src/Microsoft.IdentityModel.Tokens/SymmetricSignatureProvider.cs b/src/Microsoft.IdentityModel.Tokens/SymmetricSignatureProvider.cs index b7b98bcadb..bc99d33c01 100644 --- a/src/Microsoft.IdentityModel.Tokens/SymmetricSignatureProvider.cs +++ b/src/Microsoft.IdentityModel.Tokens/SymmetricSignatureProvider.cs @@ -3,6 +3,8 @@ using System; using System.Collections.Generic; +using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Security.Cryptography; using Microsoft.IdentityModel.Logging; @@ -289,6 +291,9 @@ public override bool Verify(byte[] input, int inputOffset, int inputLength, byte /// how many bytes to verfiy. /// algorithm passed by AuthenticatedEncryptionProvider. /// true if computed signature matches the signature parameter, false otherwise. +#if NET6_0_OR_GREATER + [SkipLocalsInit] +#endif internal bool Verify(byte[] input, int inputOffset, int inputLength, byte[] signature, int signatureOffset, int signatureLength, string algorithm) { if (input == null || input.Length == 0) @@ -377,7 +382,17 @@ internal bool Verify(byte[] input, int inputOffset, int inputLength, byte[] sign try { keyedHashAlgorithm = GetKeyedHashAlgorithm(GetKeyBytes(Key), Algorithm); - return Utility.AreEqual(signature, keyedHashAlgorithm.ComputeHash(input, inputOffset, inputLength), signatureLength); + + scoped Span hash; +#if NET6_0_OR_GREATER + hash = stackalloc byte[keyedHashAlgorithm.HashSize / 8]; // only known algorithms are used, all of which have a small enough hash size to stackalloc + keyedHashAlgorithm.TryComputeHash(input.AsSpan(inputOffset, inputLength), hash, out int bytesWritten); + Debug.Assert(bytesWritten == hash.Length); +#else + hash = keyedHashAlgorithm.ComputeHash(input, inputOffset, inputLength).AsSpan(); +#endif + + return signature.AsSpan().StartsWith(hash.Slice(0, signatureLength)); } catch { diff --git a/src/Microsoft.IdentityModel.Tokens/Utility.cs b/src/Microsoft.IdentityModel.Tokens/Utility.cs index b9f53cf9b8..70664ea342 100644 --- a/src/Microsoft.IdentityModel.Tokens/Utility.cs +++ b/src/Microsoft.IdentityModel.Tokens/Utility.cs @@ -133,27 +133,25 @@ public static bool IsHttps(Uri uri) /// /// true if the bytes are equal, false otherwise. /// - [MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)] + [MethodImpl(MethodImplOptions.NoInlining)] public static bool AreEqual(byte[] a, byte[] b) { - byte[] s_bytesA = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 }; - byte[] s_bytesB = new byte[] { 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 }; - - int result = 0; - byte[] a1, a2; + ReadOnlySpan a1, a2; if (((a == null) || (b == null)) || (a.Length != b.Length)) { - a1 = s_bytesA; - a2 = s_bytesB; + // Non-allocating. The direct assignment into a ReadOnlySpan causes the C# compiler to emit these as pointers into the assembly's data section. + a1 = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 }; + a2 = new byte[] { 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 }; } else { - a1 = a; - a2 = b; + a1 = a.AsSpan(); + a2 = b.AsSpan(); } + int result = 0; for (int i = 0; i < a1.Length; i++) { result |= a1[i] ^ a2[i]; @@ -176,31 +174,26 @@ public static bool AreEqual(byte[] a, byte[] b) /// /// true if the bytes are equal, false otherwise. /// - [MethodImpl(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)] + [MethodImpl(MethodImplOptions.NoInlining)] internal static bool AreEqual(byte[] a, byte[] b, int length) { - byte[] s_bytesA = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 }; - byte[] s_bytesB = new byte[] { 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 }; - - int result = 0; - int lenToUse = 0; - byte[] a1, a2; + ReadOnlySpan a1, a2; if (((a == null) || (b == null)) || (a.Length < length || b.Length < length)) { - a1 = s_bytesA; - a2 = s_bytesB; - lenToUse = a1.Length; + // Non-allocating. The direct assignment into a ReadOnlySpan causes the C# compiler to emit these as pointers into the assembly's data section. + a1 = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 }; + a2 = new byte[] { 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 }; } else { - a1 = a; - a2 = b; - lenToUse = length; + a1 = a.AsSpan(0, length); + a2 = b.AsSpan(0, length); } - for (int i = 0; i < lenToUse; i++) + int result = 0; + for (int i = 0; i < a1.Length; i++) { result |= a1[i] ^ a2[i]; } From f1b2ef4cc48488d4241f1baf7e2e09c35e2ed294 Mon Sep 17 00:00:00 2001 From: Stephen Toub Date: Wed, 26 Jul 2023 16:38:50 -0400 Subject: [PATCH 2/2] Use AreEqual instead of StartsWith --- .../SymmetricSignatureProvider.cs | 2 +- src/Microsoft.IdentityModel.Tokens/Utility.cs | 23 ++++++++----------- 2 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/Microsoft.IdentityModel.Tokens/SymmetricSignatureProvider.cs b/src/Microsoft.IdentityModel.Tokens/SymmetricSignatureProvider.cs index bc99d33c01..6c70134f37 100644 --- a/src/Microsoft.IdentityModel.Tokens/SymmetricSignatureProvider.cs +++ b/src/Microsoft.IdentityModel.Tokens/SymmetricSignatureProvider.cs @@ -392,7 +392,7 @@ internal bool Verify(byte[] input, int inputOffset, int inputLength, byte[] sign hash = keyedHashAlgorithm.ComputeHash(input, inputOffset, inputLength).AsSpan(); #endif - return signature.AsSpan().StartsWith(hash.Slice(0, signatureLength)); + return Utility.AreEqual(signature, hash, signatureLength); } catch { diff --git a/src/Microsoft.IdentityModel.Tokens/Utility.cs b/src/Microsoft.IdentityModel.Tokens/Utility.cs index 050bbfd610..49e2a3d059 100644 --- a/src/Microsoft.IdentityModel.Tokens/Utility.cs +++ b/src/Microsoft.IdentityModel.Tokens/Utility.cs @@ -161,7 +161,7 @@ public static bool AreEqual(byte[] a, byte[] b) } /// - /// Compares two byte arrays for equality. Hash size is fixed normally it is 32 bytes. + /// Compares two byte spans for equality. Hash size is fixed normally it is 32 bytes. /// The attempt here is to take the same time if an attacker shortens the signature OR changes some of the signed contents. /// /// @@ -170,32 +170,29 @@ public static bool AreEqual(byte[] a, byte[] b) /// /// The other set of bytes to compare with. /// - /// length of array to check + /// length of spans to check /// /// true if the bytes are equal, false otherwise. /// [MethodImpl(MethodImplOptions.NoInlining)] - internal static bool AreEqual(byte[] a, byte[] b, int length) + internal static bool AreEqual(ReadOnlySpan a, ReadOnlySpan b, int length) { - ReadOnlySpan a1, a2; - - if (((a == null) || (b == null)) - || (a.Length < length || b.Length < length)) + if ((a.Length < length || b.Length < length)) { // Non-allocating. The direct assignment into a ReadOnlySpan causes the C# compiler to emit these as pointers into the assembly's data section. - a1 = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 }; - a2 = new byte[] { 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 }; + a = new byte[] { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31 }; + b = new byte[] { 31, 30, 29, 28, 27, 26, 25, 24, 23, 22, 21, 20, 19, 18, 17, 16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1, 0 }; } else { - a1 = a.AsSpan(0, length); - a2 = b.AsSpan(0, length); + a = a.Slice(0, length); + b = b.Slice(0, length); } int result = 0; - for (int i = 0; i < a1.Length; i++) + for (int i = 0; i < a.Length; i++) { - result |= a1[i] ^ a2[i]; + result |= a[i] ^ b[i]; } return result == 0;