diff --git a/src/libraries/Common/src/System/Security/Cryptography/X25519DiffieHellmanCng.Windows.cs b/src/libraries/Common/src/System/Security/Cryptography/X25519DiffieHellmanCng.Windows.cs index 4bdeafdbfdac0c..93f2ee749f59fb 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/X25519DiffieHellmanCng.Windows.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/X25519DiffieHellmanCng.Windows.cs @@ -67,6 +67,14 @@ private void DeriveRawSecretAgreementWithPublicKey(ReadOnlySpan otherParty X25519WindowsHelpers.ReducePublicKey(otherPartyPublicKey, reducedPublicKey); + // Older versions of Windows 10 incorrectly produce a shared secret when the peer's public key is zero that + // is itself not a zero shared secret. To be consistent with later versions of Windows and other platforms, + // reject a peer public key that reduces to all-zero during agreement. + if (reducedPublicKey.IndexOfAnyExcept((byte)0) < 0) + { + throw new CryptographicException(); + } + // CNG does not permit cross-provider key agreements. Import the public key in to the same provider // as the current key. CngProvider provider = _key.Provider ?? CngProvider.MicrosoftSoftwareKeyStorageProvider; diff --git a/src/libraries/Common/src/System/Security/Cryptography/X25519DiffieHellmanImplementation.Windows.cs b/src/libraries/Common/src/System/Security/Cryptography/X25519DiffieHellmanImplementation.Windows.cs index 2ce91edc1a06f9..36c58e2d2ae801 100644 --- a/src/libraries/Common/src/System/Security/Cryptography/X25519DiffieHellmanImplementation.Windows.cs +++ b/src/libraries/Common/src/System/Security/Cryptography/X25519DiffieHellmanImplementation.Windows.cs @@ -16,19 +16,32 @@ internal sealed partial class X25519DiffieHellmanImplementation : X25519DiffieHe { private static readonly SafeBCryptAlgorithmHandle? s_algHandle = OpenAlgorithmHandle(); - private readonly SafeBCryptKeyHandle _key; + private readonly SafeBCryptKeyHandle? _key; private readonly bool _hasPrivate; private readonly byte _privatePreservation; private readonly byte[]? _originalPublicKey; - private X25519DiffieHellmanImplementation(SafeBCryptKeyHandle key, bool hasPrivate, byte privatePreservation, byte[]? originalPublicKey = null) + // Older versions of Windows 10 incorrectly produce a shared secret when the peer's public key is zero that + // is itself not a zero shared secret. To be consistent with later versions of Windows and other platforms, + // reject a peer public key that reduces to all-zero during agreement. + [MemberNotNullWhen(false, nameof(_key))] + private bool ReducedZeroPublicKey { get; } + + private X25519DiffieHellmanImplementation( + SafeBCryptKeyHandle? key, + bool hasPrivate, + byte privatePreservation, + byte[]? originalPublicKey = null, + bool reducedZeroPublicKey = false) { _key = key; _hasPrivate = hasPrivate; _privatePreservation = privatePreservation; _originalPublicKey = originalPublicKey; + ReducedZeroPublicKey = reducedZeroPublicKey; Debug.Assert(_hasPrivate || _privatePreservation == 0); Debug.Assert(!_hasPrivate || _originalPublicKey is null); + Debug.Assert(key is null == reducedZeroPublicKey); } [MemberNotNullWhen(true, nameof(s_algHandle))] @@ -41,6 +54,11 @@ protected override void DeriveRawSecretAgreementCore(X25519DiffieHellman otherPa if (otherParty is X25519DiffieHellmanImplementation x25519impl) { + if (x25519impl.ReducedZeroPublicKey) + { + throw new CryptographicException(); + } + DeriveRawSecretAgreementWithKey(x25519impl._key, destination); } else @@ -59,19 +77,28 @@ protected override void DeriveRawSecretAgreementCore(ReadOnlySpan otherPar Debug.Assert(otherPartyPublicKey.Length == PublicKeySizeInBytes); Debug.Assert(destination.Length == SecretAgreementSizeInBytes); ThrowIfPrivateNeeded(); + DeriveRawSecretAgreementWithKey(otherPartyPublicKey, destination); } private void DeriveRawSecretAgreementWithKey(ReadOnlySpan otherPartyPublicKey, Span destination) { - using (SafeBCryptKeyHandle otherPartyKey = ImportPublicKey(otherPartyPublicKey, out _)) + using (SafeBCryptKeyHandle? otherPartyKey = ImportPublicKey(otherPartyPublicKey, out _, out bool reducedZeroPublicKey)) { + if (reducedZeroPublicKey) + { + throw new CryptographicException(); + } + + Debug.Assert(otherPartyKey is not null); + DeriveRawSecretAgreementWithKey(otherPartyKey, destination); } } private void DeriveRawSecretAgreementWithKey(SafeBCryptKeyHandle otherPartyKey, Span destination) { + Debug.Assert(_key is not null); // _key is a private key in this case, can only be null for public keys using (SafeBCryptSecretHandle secret = Interop.BCrypt.BCryptSecretAgreement(_key, otherPartyKey)) { Interop.BCrypt.BCryptDeriveKey( @@ -134,7 +161,7 @@ protected override void Dispose(bool disposing) { if (disposing) { - _key.Dispose(); + _key?.Dispose(); } base.Dispose(disposing); @@ -167,17 +194,20 @@ internal static X25519DiffieHellmanImplementation ImportPrivateKeyImpl(ReadOnlyS internal static X25519DiffieHellmanImplementation ImportPublicKeyImpl(ReadOnlySpan source) { - SafeBCryptKeyHandle key = ImportPublicKey(source, out bool requiredReduction); + SafeBCryptKeyHandle? key = ImportPublicKey(source, out bool requiredReduction, out bool reducedZeroPublicKey); - Debug.Assert(!key.IsInvalid); return new X25519DiffieHellmanImplementation( key, hasPrivate: false, privatePreservation: 0, - requiredReduction ? source.ToArray() : null); + requiredReduction ? source.ToArray() : null, + reducedZeroPublicKey); } - private static SafeBCryptKeyHandle ImportPublicKey(ReadOnlySpan source, out bool requiredReduction) + private static SafeBCryptKeyHandle? ImportPublicKey( + ReadOnlySpan source, + out bool requiredReduction, + out bool reducedZeroPublicKey) { scoped Span reducedPublicKey; @@ -187,6 +217,12 @@ private static SafeBCryptKeyHandle ImportPublicKey(ReadOnlySpan source, ou } requiredReduction = X25519WindowsHelpers.ReducePublicKey(source, reducedPublicKey); + reducedZeroPublicKey = reducedPublicKey.IndexOfAnyExcept((byte)0) < 0; + + if (reducedZeroPublicKey) + { + return null; + } return ImportKey(false, reducedPublicKey, out _); } @@ -197,6 +233,16 @@ private void ExportKey(bool privateKey, Span destination) Interop.BCrypt.KeyBlobType.BCRYPT_ECCPRIVATE_BLOB : Interop.BCrypt.KeyBlobType.BCRYPT_ECCPUBLIC_BLOB; + if (ReducedZeroPublicKey) + { + // If the public key required reduction, then it should have been retained as an original and copied + // before reaching this, so the only zero key that should be known here is a true zero. + Debug.Assert(!privateKey); + Debug.Assert(_originalPublicKey is null); + destination.Clear(); + return; + } + ArraySegment key = Interop.BCrypt.BCryptExportKey(_key, blobType); try diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X25519DiffieHellmanBaseTests.cs b/src/libraries/Common/tests/System/Security/Cryptography/X25519DiffieHellmanBaseTests.cs index ac939e83c58bb1..6aa4ed753c4607 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X25519DiffieHellmanBaseTests.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X25519DiffieHellmanBaseTests.cs @@ -19,6 +19,7 @@ public abstract class X25519DiffieHellmanBaseTests public abstract X25519DiffieHellman ImportPrivateKey(ReadOnlySpan source); public abstract X25519DiffieHellman ImportPublicKey(ReadOnlySpan source); public virtual bool CanRoundTripKeys => true; + protected virtual bool CanRoundTripReducedZeroPublicKeys => true; // SymCrypt, thus SCOSSL, is stricter about keys it is willing to import. These keys fall in to // two buckets. @@ -259,13 +260,29 @@ public void DeriveRawSecretAgreement_CrossImplementation(DeriveSecretAgreementVe AssertExtensions.SequenceEqual(vector.SharedSecret, secretBuffer); } - [ConditionalFact(nameof(IsNotStrictKeyValidatingPlatform))] - public void DeriveRawSecretAgreement_ZeroSharedSecret_Throws() + public static IEnumerable ZeroSharedSecretPublicKeys() { // Wycheproof tcId 64: peer public key is a low-order point on Curve25519. - // This low-order point produces a shared secret that is all zeros. + yield return ["5f9c95bca3508c24b1d0b1559c83ef5b04445cc4581c8e86d8224eddd09f1157", false]; + // Wycheproof tcId 32: peer public key is zero. + yield return ["0000000000000000000000000000000000000000000000000000000000000000", true]; + // Wycheproof tcId 83: peer public key is p, which reduces to zero. + yield return ["edffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff7f", true]; + } + + [ConditionalTheory(nameof(IsNotStrictKeyValidatingPlatform))] + [MemberData(nameof(ZeroSharedSecretPublicKeys))] + public void DeriveRawSecretAgreement_ZeroSharedSecret_Throws( + string peerPublicKeyHex, + bool requiresReducedZeroRoundtrip) + { + if (requiresReducedZeroRoundtrip && !CanRoundTripReducedZeroPublicKeys) + { + return; + } + byte[] privateKey = "387355d995616090503aafad49da01fb3dc3eda962704eaee6b86f9e20c92579".HexToByteArray(); - byte[] peerPublicKey = "5f9c95bca3508c24b1d0b1559c83ef5b04445cc4581c8e86d8224eddd09f1157".HexToByteArray(); + byte[] peerPublicKey = peerPublicKeyHex.HexToByteArray(); using X25519DiffieHellman key = ImportPrivateKey(privateKey); using X25519DiffieHellman peer = ImportPublicKey(peerPublicKey); @@ -273,13 +290,19 @@ public void DeriveRawSecretAgreement_ZeroSharedSecret_Throws() Assert.ThrowsAny(() => key.DeriveRawSecretAgreement(peer)); } - [ConditionalFact(nameof(IsNotStrictKeyValidatingPlatform))] - public void DeriveRawSecretAgreement_ExactBuffers_ZeroSharedSecret_Throws() + [ConditionalTheory(nameof(IsNotStrictKeyValidatingPlatform))] + [MemberData(nameof(ZeroSharedSecretPublicKeys))] + public void DeriveRawSecretAgreement_ExactBuffers_ZeroSharedSecret_Throws( + string peerPublicKeyHex, + bool requiresReducedZeroRoundtrip) { - // Wycheproof tcId 64: peer public key is a low-order point on Curve25519. - // This low-order point produces a shared secret that is all zeros. + if (requiresReducedZeroRoundtrip && !CanRoundTripReducedZeroPublicKeys) + { + return; + } + byte[] privateKey = "387355d995616090503aafad49da01fb3dc3eda962704eaee6b86f9e20c92579".HexToByteArray(); - byte[] peerPublicKey = "5f9c95bca3508c24b1d0b1559c83ef5b04445cc4581c8e86d8224eddd09f1157".HexToByteArray(); + byte[] peerPublicKey = peerPublicKeyHex.HexToByteArray(); using X25519DiffieHellman key = ImportPrivateKey(privateKey); using X25519DiffieHellman peer = ImportPublicKey(peerPublicKey); @@ -288,13 +311,15 @@ public void DeriveRawSecretAgreement_ExactBuffers_ZeroSharedSecret_Throws() () => key.DeriveRawSecretAgreement(peer, new byte[X25519DiffieHellman.SecretAgreementSizeInBytes])); } - [ConditionalFact(nameof(IsNotStrictKeyValidatingPlatform))] - public void DeriveRawSecretAgreement_Bytes_ZeroSharedSecret_Throws() + [ConditionalTheory(nameof(IsNotStrictKeyValidatingPlatform))] + [MemberData(nameof(ZeroSharedSecretPublicKeys))] + public void DeriveRawSecretAgreement_Bytes_ZeroSharedSecret_Throws( + string peerPublicKeyHex, + bool requiresReducedZeroRoundtrip) { - // Wycheproof tcId 64: peer public key is a low-order point on Curve25519. - // This low-order point produces a shared secret that is all zeros. + _ = requiresReducedZeroRoundtrip; byte[] privateKey = "387355d995616090503aafad49da01fb3dc3eda962704eaee6b86f9e20c92579".HexToByteArray(); - byte[] peerPublicKey = "5f9c95bca3508c24b1d0b1559c83ef5b04445cc4581c8e86d8224eddd09f1157".HexToByteArray(); + byte[] peerPublicKey = peerPublicKeyHex.HexToByteArray(); using X25519DiffieHellman key = ImportPrivateKey(privateKey); @@ -304,13 +329,15 @@ public void DeriveRawSecretAgreement_Bytes_ZeroSharedSecret_Throws() AssertExtensions.SequenceEqual(privateKey, key.ExportPrivateKey()); } - [ConditionalFact(nameof(IsNotStrictKeyValidatingPlatform))] - public void DeriveRawSecretAgreement_Bytes_ExactBuffers_ZeroSharedSecret_Throws() + [ConditionalTheory(nameof(IsNotStrictKeyValidatingPlatform))] + [MemberData(nameof(ZeroSharedSecretPublicKeys))] + public void DeriveRawSecretAgreement_Bytes_ExactBuffers_ZeroSharedSecret_Throws( + string peerPublicKeyHex, + bool requiresReducedZeroRoundtrip) { - // Wycheproof tcId 64: peer public key is a low-order point on Curve25519. - // This low-order point produces a shared secret that is all zeros. + _ = requiresReducedZeroRoundtrip; byte[] privateKey = "387355d995616090503aafad49da01fb3dc3eda962704eaee6b86f9e20c92579".HexToByteArray(); - byte[] peerPublicKey = "5f9c95bca3508c24b1d0b1559c83ef5b04445cc4581c8e86d8224eddd09f1157".HexToByteArray(); + byte[] peerPublicKey = peerPublicKeyHex.HexToByteArray(); using X25519DiffieHellman key = ImportPrivateKey(privateKey); diff --git a/src/libraries/Common/tests/System/Security/Cryptography/X25519DiffieHellmanCngTests.Windows.cs b/src/libraries/Common/tests/System/Security/Cryptography/X25519DiffieHellmanCngTests.Windows.cs index 7863ccfa341b7f..524eafd500ca76 100644 --- a/src/libraries/Common/tests/System/Security/Cryptography/X25519DiffieHellmanCngTests.Windows.cs +++ b/src/libraries/Common/tests/System/Security/Cryptography/X25519DiffieHellmanCngTests.Windows.cs @@ -113,6 +113,10 @@ public abstract class X25519DiffieHellmanCngTests : X25519DiffieHellmanBaseTests protected abstract CngExportPolicies ExportPolicy { get; } + // A caller-supplied CngKey has already been imported by the provider. The original public key may have been + // rejected or transformed, so exporting it does not reliably preserve whether it reduced to zero. + protected override bool CanRoundTripReducedZeroPublicKeys => false; + public override X25519DiffieHellman GenerateKey() { using CngKey key = GenerateCngKey(exportPolicy: ExportPolicy);