From 56a3b46a97f426c88300164083a5aa18df71d6a4 Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Fri, 10 Jun 2022 13:45:02 -0700 Subject: [PATCH 1/2] Randomize CoseHeaderLabel hash codes --- .../src/System/HashCodeRandomization.cs | 53 +++++++++++++++++++ .../System.Security.Cryptography.Cose.csproj | 6 +++ .../Cryptography/Cose/CoseHeaderLabel.cs | 8 ++- .../tests/CoseHeaderLabelTests.cs | 36 ++++++++----- 4 files changed, 89 insertions(+), 14 deletions(-) create mode 100644 src/libraries/Common/src/System/HashCodeRandomization.cs diff --git a/src/libraries/Common/src/System/HashCodeRandomization.cs b/src/libraries/Common/src/System/HashCodeRandomization.cs new file mode 100644 index 0000000000000..fc5a4c3b0832e --- /dev/null +++ b/src/libraries/Common/src/System/HashCodeRandomization.cs @@ -0,0 +1,53 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace System +{ + // Contains helpers for calculating randomized hash codes of common types. + // These hash codes must not be persisted between AppDomain restarts. + // There's still the potential for two distinct types with the same bit + // pattern (for example, string.Empty and int.Zero) to produce the same + // hash code, since the HashCode seed is global per AppDomain and not per type. + // This shouldn't be a problem for most callers since the number of + // possible types is expected to be small anyway for realistic scenarios. + internal static class HashCodeRandomization + { + public static int GetRandomizedOrdinalHashCode(this string value) + { +#if NETCOREAPP + // In .NET Core, string hash codes are already randomized. + + return value.GetHashCode(); +#else + // Downlevel, we need to perform randomization ourselves. + // This allows "Hello!" and "Hello!\0" to have the same hash + // code, which is acceptable for the scenarios we care about. + // We'll pull out pairs of chars and write 32-bit ints at a time. + + HashCode hashCode = default; + int pair = 0; + for (int i = 0; i < value.Length; i++) + { + int ch = value[i]; + if ((i & 1) == 0) + { + pair = ch << 16; // first member of pair + } + else + { + pair |= ch; // second member of pair + hashCode.Add(pair); // write pair as single unit + pair = 0; + } + } + hashCode.Add(pair); // flush any leftover data (could be 0) + return hashCode.ToHashCode(); +#endif + } + + public static int GetRandomizedHashCode(this int value) + { + return HashCode.Combine(value); + } + } +} diff --git a/src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj b/src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj index 2c66e4920a1e7..e2ef885d16954 100644 --- a/src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj +++ b/src/libraries/System.Security.Cryptography.Cose/src/System.Security.Cryptography.Cose.csproj @@ -13,6 +13,7 @@ + @@ -32,6 +33,11 @@ + + + + + diff --git a/src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseHeaderLabel.cs b/src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseHeaderLabel.cs index e3fd261674f31..1c800f871fcda 100644 --- a/src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseHeaderLabel.cs +++ b/src/libraries/System.Security.Cryptography.Cose/src/System/Security/Cryptography/Cose/CoseHeaderLabel.cs @@ -54,12 +54,16 @@ public bool Equals(CoseHeaderLabel other) public override int GetHashCode() { + // Since this type is used as a key in a dictionary (see CoseHeaderMap) + // and since the label is potentially adversary-provided, we'll need + // to randomize the hash code. + if (LabelAsString != null) { - return LabelAsString.GetHashCode(); + return LabelAsString.GetRandomizedOrdinalHashCode(); } - return LabelAsInt32.GetHashCode(); + return LabelAsInt32.GetRandomizedHashCode(); } public static bool operator ==(CoseHeaderLabel left, CoseHeaderLabel right) => left.Equals(right); diff --git a/src/libraries/System.Security.Cryptography.Cose/tests/CoseHeaderLabelTests.cs b/src/libraries/System.Security.Cryptography.Cose/tests/CoseHeaderLabelTests.cs index a7d65a1d9a521..d4c4b44b29687 100644 --- a/src/libraries/System.Security.Cryptography.Cose/tests/CoseHeaderLabelTests.cs +++ b/src/libraries/System.Security.Cryptography.Cose/tests/CoseHeaderLabelTests.cs @@ -10,23 +10,35 @@ public class CoseHeaderLabelTests [Fact] public void CoseHeaderLabel_GetHashCode() { - CoseHeaderLabel label = default; - Assert.Equal(0, label.GetHashCode()); // There's no need to call GetHashCode on integers as they are their own hashcode. + // First, construct a COSE header of (0) using several different methods. + // They should all have the same hash code, though we don't know what + // the hash code is. - label = new CoseHeaderLabel(); - Assert.Equal(0, label.GetHashCode()); + CoseHeaderLabel label1 = default; + CoseHeaderLabel label2 = new CoseHeaderLabel(); + CoseHeaderLabel label3 = new CoseHeaderLabel(0); - label = new CoseHeaderLabel(0); - Assert.Equal(0, label.GetHashCode()); + int label1HashCode = label1.GetHashCode(); + Assert.Equal(label1HashCode, label2.GetHashCode()); + Assert.Equal(label1HashCode, label3.GetHashCode()); - label = new CoseHeaderLabel(""); - Assert.Equal("".GetHashCode(), label.GetHashCode()); + // Make sure the integer hash code calculation really is randomized. + // Checking 1 & 2 together rather than independently cuts the false + // positive rate down to nearly 1 in 2^64. - label = new CoseHeaderLabel(1); - Assert.Equal(1, label.GetHashCode()); + bool isReturningNormalInt32HashCode = + (new CoseHeaderLabel(1).GetHashCode() == 1) + && (new CoseHeaderLabel(2).GetHashCode() == 2); + Assert.False(isReturningNormalInt32HashCode); - label = new CoseHeaderLabel("content-type"); - Assert.Equal("content-type".GetHashCode(), label.GetHashCode()); + // Make sure the string hash code calculation really is randomized. + // Checking 1 & 2 together rather than independently cuts the false + // positive rate down to nearly 1 in 2^64. + + bool isReturningNormalStringHashCode = + (new CoseHeaderLabel("Hello").GetHashCode() == "Hello".GetHashCode()) + && (new CoseHeaderLabel("World").GetHashCode() == "World".GetHashCode()); + Assert.Equal(PlatformDetection.IsNetCore, isReturningNormalStringHashCode); } [Fact] From 0e6aae274edfed4133c2565e9395a44eae1854cd Mon Sep 17 00:00:00 2001 From: Levi Broderick Date: Mon, 13 Jun 2022 10:57:33 -0700 Subject: [PATCH 2/2] Update comments --- .../src/System/HashCodeRandomization.cs | 27 +++++++++++-------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/libraries/Common/src/System/HashCodeRandomization.cs b/src/libraries/Common/src/System/HashCodeRandomization.cs index fc5a4c3b0832e..16cd6cb577c05 100644 --- a/src/libraries/Common/src/System/HashCodeRandomization.cs +++ b/src/libraries/Common/src/System/HashCodeRandomization.cs @@ -4,12 +4,14 @@ namespace System { // Contains helpers for calculating randomized hash codes of common types. - // These hash codes must not be persisted between AppDomain restarts. - // There's still the potential for two distinct types with the same bit - // pattern (for example, string.Empty and int.Zero) to produce the same - // hash code, since the HashCode seed is global per AppDomain and not per type. - // This shouldn't be a problem for most callers since the number of - // possible types is expected to be small anyway for realistic scenarios. + // Since these hash codes are randomized, callers must not persist them between + // AppDomain restarts. There's still the potential for limited collisions + // if two distinct types have the same bit pattern (e.g., string.Empty and (int)0). + // This should be acceptable because the number of practical collisions is + // limited by the number of distinct types used here, and we expect callers to + // have a small, fixed set of accepted types for any hash-based collection. + // If we really do need to address this in the future, we can use a seed per type + // rather than a global seed for the entire AppDomain. internal static class HashCodeRandomization { public static int GetRandomizedOrdinalHashCode(this string value) @@ -19,10 +21,13 @@ public static int GetRandomizedOrdinalHashCode(this string value) return value.GetHashCode(); #else - // Downlevel, we need to perform randomization ourselves. - // This allows "Hello!" and "Hello!\0" to have the same hash - // code, which is acceptable for the scenarios we care about. - // We'll pull out pairs of chars and write 32-bit ints at a time. + // Downlevel, we need to perform randomization ourselves. There's still + // the potential for limited collisions ("Hello!" and "Hello!\0"), but + // this shouldn't be a problem in practice. If we need to address it, + // we can mix the string length into the accumulator before running the + // string contents through. + // + // We'll pull out pairs of chars and write 32 bits at a time. HashCode hashCode = default; int pair = 0; @@ -40,7 +45,7 @@ public static int GetRandomizedOrdinalHashCode(this string value) pair = 0; } } - hashCode.Add(pair); // flush any leftover data (could be 0) + hashCode.Add(pair); // flush any leftover data (could be 0 or 1 chars) return hashCode.ToHashCode(); #endif }