Added TLS validation check for DH keys

Added further agreement result checks
bcgit · Oct 29, 2016 · 1127131 · 1127131
1 parent 63c5f15
commit 1127131
Show file tree

Hide file tree

Showing 5 changed files with 55 additions and 10 deletions.
diff --git a/core/src/main/java/org/bouncycastle/crypto/agreement/DHAgreement.java b/core/src/main/java/org/bouncycastle/crypto/agreement/DHAgreement.java
@@ -6,11 +6,11 @@
 import org.bouncycastle.crypto.AsymmetricCipherKeyPair;
 import org.bouncycastle.crypto.CipherParameters;
 import org.bouncycastle.crypto.generators.DHKeyPairGenerator;
+import org.bouncycastle.crypto.params.AsymmetricKeyParameter;
 import org.bouncycastle.crypto.params.DHKeyGenerationParameters;
 import org.bouncycastle.crypto.params.DHParameters;
-import org.bouncycastle.crypto.params.DHPublicKeyParameters;
 import org.bouncycastle.crypto.params.DHPrivateKeyParameters;
-import org.bouncycastle.crypto.params.AsymmetricKeyParameter;
+import org.bouncycastle.crypto.params.DHPublicKeyParameters;
 import org.bouncycastle.crypto.params.ParametersWithRandom;
 
 /**
@@ -26,6 +26,8 @@
  */
 public class DHAgreement
 {
+    private static final BigInteger ONE = BigInteger.valueOf(1);
+
     private DHPrivateKeyParameters  key;
     private DHParameters            dhParams;
     private BigInteger              privateValue;
@@ -89,6 +91,12 @@ public BigInteger calculateAgreement(
 
         BigInteger p = dhParams.getP();
 
-        return message.modPow(key.getX(), p).multiply(pub.getY().modPow(privateValue, p)).mod(p);
+        BigInteger result = pub.getY().modPow(privateValue, p);
+        if (result.compareTo(ONE) == 0)
+        {
+            throw new IllegalStateException("Shared key can't be 1");
+        }
+
+        return message.modPow(key.getX(), p).multiply(result).mod(p);
     }
 }
diff --git a/core/src/main/java/org/bouncycastle/crypto/agreement/DHBasicAgreement.java b/core/src/main/java/org/bouncycastle/crypto/agreement/DHBasicAgreement.java
@@ -20,6 +20,8 @@
 public class DHBasicAgreement
     implements BasicAgreement
 {
+    private static final BigInteger ONE = BigInteger.valueOf(1);
+
     private DHPrivateKeyParameters  key;
     private DHParameters            dhParams;
 
@@ -66,6 +68,12 @@ public BigInteger calculateAgreement(
             throw new IllegalArgumentException("Diffie-Hellman public key has wrong parameters.");
         }
 
-        return pub.getY().modPow(key.getX(), dhParams.getP());
+        BigInteger result = pub.getY().modPow(key.getX(), dhParams.getP());
+        if (result.compareTo(ONE) == 0)
+        {
+            throw new IllegalStateException("Shared key can't be 1");
+        }
+
+        return result;
     }
 }
diff --git a/core/src/main/java/org/bouncycastle/crypto/engines/IESEngine.java b/core/src/main/java/org/bouncycastle/crypto/engines/IESEngine.java
@@ -408,6 +408,10 @@ public byte[] processBlock(
                 {
                     throw new InvalidCipherTextException("unable to recover ephemeral public key: " + e.getMessage(), e);
                 }
+                catch (IllegalArgumentException e)
+                {
+                    throw new InvalidCipherTextException("unable to recover ephemeral public key: " + e.getMessage(), e);
+                }
 
                 int encLength = (inLen - bIn.available());
                 this.V = Arrays.copyOfRange(in, inOff, inOff + encLength);

diff --git a/core/src/main/java/org/bouncycastle/crypto/params/DHPublicKeyParameters.java b/core/src/main/java/org/bouncycastle/crypto/params/DHPublicKeyParameters.java
@@ -5,6 +5,9 @@
 public class DHPublicKeyParameters
     extends DHKeyParameters
 {
+    private static final BigInteger ONE = BigInteger.valueOf(1);
+    private static final BigInteger TWO = BigInteger.valueOf(2);
+
     private BigInteger      y;
 
     public DHPublicKeyParameters(
@@ -18,9 +21,14 @@ public DHPublicKeyParameters(
 
     private BigInteger validate(BigInteger y, DHParameters dhParams)
     {
+        if (y == null)
+        {
+            throw new NullPointerException("y value cannot be null");
+        }
+
         if (dhParams.getQ() != null)
         {
-            if (BigInteger.ONE.equals(y.modPow(dhParams.getQ(), dhParams.getP())))
+            if (ONE.equals(y.modPow(dhParams.getQ(), dhParams.getP())))
             {
                 return y;
             }
@@ -29,6 +37,12 @@ private BigInteger validate(BigInteger y, DHParameters dhParams)
         }
         else
         {
+            // TLS check
+            if (y.compareTo(TWO) < 0 || y.compareTo(dhParams.getP().subtract(TWO)) > 0)
+            {
+                throw new IllegalArgumentException("invalid DH public key");
+            }
+
             return y;         // we can't validate without Q.
         }
     }

diff --git a/prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/dh/KeyAgreementSpi.java b/prov/src/main/java/org/bouncycastle/jcajce/provider/asymmetric/dh/KeyAgreementSpi.java
@@ -29,6 +29,9 @@
 public class KeyAgreementSpi
     extends BaseAgreementSpi
 {
+    private static final BigInteger ONE = BigInteger.valueOf(1);
+    private static final BigInteger TWO = BigInteger.valueOf(2);
+
     private BigInteger      x;
     private BigInteger      p;
     private BigInteger      g;
@@ -101,14 +104,22 @@ protected Key engineDoPhase(
             throw new InvalidKeyException("DHPublicKey not for this KeyAgreement!");
         }
 
-        if (lastPhase)
+        BigInteger peerY = ((DHPublicKey)key).getY();
+        if (peerY == null || peerY.compareTo(TWO) < 0
+            || peerY.compareTo(p.subtract(ONE)) >= 0)
         {
-            result = ((DHPublicKey)key).getY().modPow(x, p);
-            return null;
+            throw new InvalidKeyException("Invalid DH PublicKey");
         }
-        else
+
+        result = peerY.modPow(x, p);
+        if (result.compareTo(ONE) == 0)
         {
-            result = ((DHPublicKey)key).getY().modPow(x, p);
+            throw new InvalidKeyException("Shared key can't be 1");
+        }
+
+        if (lastPhase)
+        {
+            return null;
         }
 
         return new BCDHPublicKey(result, pubKey.getParams());