Checks on DH peer public key

4 files changed, 106 insertions, 3 deletions

diff --git a/crypto/src/crypto/agreement/DHAgreement.cs b/crypto/src/crypto/agreement/DHAgreement.cs
index b5af104f9..e988c0d53 100644
--- a/crypto/src/crypto/agreement/DHAgreement.cs
+++ b/crypto/src/crypto/agreement/DHAgreement.cs
@@ -85,7 +85,11 @@ namespace Org.BouncyCastle.Crypto.Agreement
 
             BigInteger p = dhParams.P;
 
-            BigInteger result = pub.Y.ModPow(privateValue, p);
+            BigInteger peerY = pub.Y;
+            if (peerY == null || peerY.CompareTo(BigInteger.One) <= 0 || peerY.CompareTo(p.Subtract(BigInteger.One)) >= 0)
+                throw new ArgumentException("Diffie-Hellman public key is weak");
+
+            BigInteger result = peerY.ModPow(privateValue, p);
             if (result.Equals(BigInteger.One))
                 throw new InvalidOperationException("Shared key can't be 1");
 
diff --git a/crypto/src/crypto/agreement/DHBasicAgreement.cs b/crypto/src/crypto/agreement/DHBasicAgreement.cs
index d6f017e32..6c3fe6595 100644
--- a/crypto/src/crypto/agreement/DHBasicAgreement.cs
+++ b/crypto/src/crypto/agreement/DHBasicAgreement.cs
@@ -56,7 +56,13 @@ namespace Org.BouncyCastle.Crypto.Agreement
             if (!pub.Parameters.Equals(dhParams))
                 throw new ArgumentException("Diffie-Hellman public key has wrong parameters.");
 
-            BigInteger result = pub.Y.ModPow(key.X, dhParams.P);
+            BigInteger p = dhParams.P;
+
+            BigInteger peerY = pub.Y;
+            if (peerY == null || peerY.CompareTo(BigInteger.One) <= 0 || peerY.CompareTo(p.Subtract(BigInteger.One)) >= 0)
+                throw new ArgumentException("Diffie-Hellman public key is weak");
+
+            BigInteger result = peerY.ModPow(key.X, p);
             if (result.Equals(BigInteger.One))
                 throw new InvalidOperationException("Shared key can't be 1");
 
diff --git a/crypto/src/crypto/parameters/DHPublicKeyParameters.cs b/crypto/src/crypto/parameters/DHPublicKeyParameters.cs
index 1704c47dc..e7aeeff19 100644
--- a/crypto/src/crypto/parameters/DHPublicKeyParameters.cs
+++ b/crypto/src/crypto/parameters/DHPublicKeyParameters.cs
@@ -46,7 +46,7 @@ namespace Org.BouncyCastle.Crypto.Parameters
             this.y = Validate(y, parameters);
         }
 
-        public BigInteger Y
+        public virtual BigInteger Y
         {
             get { return y; }
         }
diff --git a/crypto/test/src/test/DHTest.cs b/crypto/test/src/test/DHTest.cs
index 18a0d84d6..fca487008 100644
--- a/crypto/test/src/test/DHTest.cs
+++ b/crypto/test/src/test/DHTest.cs
@@ -633,6 +633,98 @@ namespace Org.BouncyCastle.Tests
             }
         }
 
+        internal static readonly string Message = "Hello";
+
+        internal static readonly SecureRandom rand = new SecureRandom();
+
+        public static DHParameters Ike2048()
+        {
+            BigInteger p = new BigInteger(
+                "ffffffffffffffffc90fdaa22168c234c4c6628b80dc1cd129024e088a67cc74"
+                    + "020bbea63b139b22514a08798e3404ddef9519b3cd3a431b302b0a6df25f1437"
+                    + "4fe1356d6d51c245e485b576625e7ec6f44c42e9a637ed6b0bff5cb6f406b7ed"
+                    + "ee386bfb5a899fa5ae9f24117c4b1fe649286651ece45b3dc2007cb8a163bf05"
+                    + "98da48361c55d39a69163fa8fd24cf5f83655d23dca3ad961c62f356208552bb"
+                    + "9ed529077096966d670c354e4abc9804f1746c08ca18217c32905e462e36ce3b"
+                    + "e39e772c180e86039b2783a2ec07a28fb5c55df06f4c52c9de2bcbf695581718"
+                    + "3995497cea956ae515d2261898fa051015728e5a8aacaa68ffffffffffffffff", 16);
+            BigInteger g = new BigInteger("2");
+            return new DHParameters(p, g);
+        }
+
+        internal class DHWeakPubKey
+            : DHPublicKeyParameters
+        {
+            private readonly BigInteger weakY;
+
+            public DHWeakPubKey(BigInteger weakY, DHParameters parameters)
+			    : base(BigInteger.Two, parameters)
+            {
+                this.weakY = weakY;
+            }
+
+            public override BigInteger Y
+            {
+                get { return weakY; }
+            }
+        }
+
+        /**
+         * Tests whether a provider accepts invalid public keys that result in predictable shared secrets.
+         * This test is based on RFC 2785, Section 4 and NIST SP 800-56A,
+         * If an attacker can modify both public keys in an ephemeral-ephemeral key agreement scheme then
+         * it may be possible to coerce both parties into computing the same predictable shared key.
+         * <p/>
+         * Note: the test is quite whimsical. If the prime p is not a safe prime then the provider itself
+         * cannot prevent all small-subgroup attacks because of the missing parameter q in the
+         * Diffie-Hellman parameters. Implementations must add additional countermeasures such as the ones
+         * proposed in RFC 2785.
+         */
+        [Test]
+        public void TestSubgroupConfinement()
+        {
+            DHParameters parameters = Ike2048();
+            BigInteger p = parameters.P, g = parameters.G;
+
+            IAsymmetricCipherKeyPairGenerator keyGen = GeneratorUtilities.GetKeyPairGenerator("DH");
+
+            //keyGen.initialize(params);
+            keyGen.Init(new DHKeyGenerationParameters(new SecureRandom(), parameters));
+
+            AsymmetricCipherKeyPair kp = keyGen.GenerateKeyPair();
+            AsymmetricKeyParameter priv = kp.Private;
+
+            IBasicAgreement ka = AgreementUtilities.GetBasicAgreement("DH");
+
+            BigInteger[] weakPublicKeys = { BigInteger.Zero, BigInteger.One, p.Subtract(BigInteger.One), p,
+                p.Add(BigInteger.One), BigInteger.One.Negate() };
+
+            foreach (BigInteger weakKey in weakPublicKeys)
+            {
+                try
+                {
+                    new DHPublicKeyParameters(weakKey, parameters);
+                    Fail("Generated weak public key");
+                }
+                catch (ArgumentException ex)
+                {
+                    IsTrue("wrong message (constructor)", Platform.StartsWith(ex.Message, "invalid DH public key"));
+                }
+
+                ka.Init(priv);
+
+                try
+                {
+                    ka.CalculateAgreement(new DHWeakPubKey(weakKey, parameters));
+                    Fail("Generated secrets with weak public key");
+                }
+                catch (ArgumentException ex)
+                {
+                    IsTrue("wrong message (CalculateAgreement)", "Diffie-Hellman public key is weak".Equals(ex.Message));
+                }
+            }
+        }
+
         public override void PerformTest()
         {
             TestEnc();
@@ -640,6 +732,7 @@ namespace Org.BouncyCastle.Tests
             TestECDH();
             TestECDHC();
             TestExceptions();
+            TestSubgroupConfinement();
         }
 
         public static void Main(