From fbe4afce032cd058bca7cce590f2049952c7441a Mon Sep 17 00:00:00 2001 From: Kevin Jones Date: Thu, 24 Sep 2026 14:01:37 -0400 Subject: [PATCH] X25519: Handle zero peer keys on downlevel Windows platforms An all-zero peer (public) key should always produce a zero shared secret, which should get rejected during key agreement. Windows normally rejects this during importation time, but that is not enabled because Windows would also eagerly reject off-twist public keys which should work. With this change, when a "zero" public key is imported (either by naturally being zero or reduced to zero) we skip importing it into bcrypt and retain "This was a zero key". During derivation we throw since that would produce a zero shared secret. This keeps Windows consistent with other platforms, where import is not the protected path, but derivation is. For X25519DHCng (ncrypt) its not possible to really gate this when the peer key is zero. However that responsibility falls to the person creating the public key handle, and the handle is external in this case. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../X25519DiffieHellmanCng.Windows.cs | 8 +++ ...5519DiffieHellmanImplementation.Windows.cs | 62 +++++++++++++++--- .../X25519DiffieHellmanBaseTests.cs | 65 +++++++++++++------ .../X25519DiffieHellmanCngTests.Windows.cs | 4 ++ 4 files changed, 112 insertions(+), 27 deletions(-) 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);