7192392: Better validation of client keys

Summary: Also reviewed by Andrew Gross<Andrew.Gross@Oracle.COM>
Reviewed-by: vinnie

src/share/classes/com/sun/crypto/provider/DHKeyAgreement.java

@@ -41,6 +41,8 @@ import javax.crypto.ShortBufferException;
 import javax.crypto.SecretKey;
 import javax.crypto.spec.*;
 
+import sun.security.util.KeyUtil;
+
 /**
  * This class implements the Diffie-Hellman key agreement protocol between
  * any number of parties.
@@ -200,6 +202,9 @@ extends KeyAgreementSpi {
             throw new InvalidKeyException("Incompatible parameters");
         }
 
+        // validate the Diffie-Hellman public key
+        KeyUtil.validate(dhPubKey);
+
         // store the y value
         this.y = dhPubKey.getY();
 

src/share/classes/sun/security/pkcs11/P11KeyAgreement.java

@@ -37,6 +37,7 @@ import javax.crypto.spec.*;
 import static sun.security.pkcs11.TemplateManager.*;
 import sun.security.pkcs11.wrapper.*;
 import static sun.security.pkcs11.wrapper.PKCS11Constants.*;
+import sun.security.util.KeyUtil;
 
 /**
  * KeyAgreement implementation class. This class currently supports
@@ -134,6 +135,10 @@ final class P11KeyAgreement extends KeyAgreementSpi {
         BigInteger p, g, y;
         if (key instanceof DHPublicKey) {
             DHPublicKey dhKey = (DHPublicKey)key;
+
+            // validate the Diffie-Hellman public key
+            KeyUtil.validate(dhKey);
+
             y = dhKey.getY();
             DHParameterSpec params = dhKey.getParams();
             p = params.getP();
@@ -145,6 +150,10 @@ final class P11KeyAgreement extends KeyAgreementSpi {
             try {
                 DHPublicKeySpec spec = kf.engineGetKeySpec(
                         key, DHPublicKeySpec.class);
+
+                // validate the Diffie-Hellman public key
+                KeyUtil.validate(spec);
+
                 y = spec.getY();
                 p = spec.getP();
                 g = spec.getG();

src/share/classes/sun/security/ssl/ClientHandshaker.java

@@ -192,8 +192,12 @@ final class ClientHandshaker extends Handshaker {
                 }
                 break;
             case K_DH_ANON:
-                this.serverKeyExchange(new DH_ServerKeyExchange(
+                try {
+                    this.serverKeyExchange(new DH_ServerKeyExchange(
                                                 input, protocolVersion));
+                } catch (GeneralSecurityException e) {
+                    throwSSLException("Server key", e);
+                }
                 break;
             case K_DHE_DSS:
             case K_DHE_RSA:
@@ -921,7 +925,7 @@ final class ClientHandshaker extends Handshaker {
         case K_DHE_RSA:
         case K_DHE_DSS:
         case K_DH_ANON:
-            preMasterSecret = dh.getAgreedSecret(serverDH);
+            preMasterSecret = dh.getAgreedSecret(serverDH, true);
             break;
         case K_ECDHE_RSA:
         case K_ECDHE_ECDSA:

src/share/classes/sun/security/ssl/DHClientKeyExchange.java

 /*
- * Copyright (c) 1997, 2007, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1997, 2012, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -29,7 +29,7 @@ package sun.security.ssl;
 import java.io.IOException;
 import java.io.PrintStream;
 import java.math.BigInteger;
-
+import javax.net.ssl.SSLHandshakeException;
 
 /*
  * Message used by clients to send their Diffie-Hellman public
@@ -50,7 +50,7 @@ final class DHClientKeyExchange extends HandshakeMessage {
     private byte dh_Yc[];               // 1 to 2^16 -1 bytes
 
     BigInteger getClientPublicKey() {
-        return new BigInteger(1, dh_Yc);
+        return dh_Yc == null ? null : new BigInteger(1, dh_Yc);
     }
 
     /*
@@ -72,7 +72,14 @@ final class DHClientKeyExchange extends HandshakeMessage {
      * but that's what the protocol spec requires.)
      */
     DHClientKeyExchange(HandshakeInStream input) throws IOException {
-        dh_Yc = input.getBytes16();
+        if (input.available() >= 2) {
+            dh_Yc = input.getBytes16();
+        } else {
+            // currently, we don't support cipher suites that requires
+            // implicit public key of client.
+            throw new SSLHandshakeException(
+                    "Unsupported implicit client DiffieHellman public key");
+        }
     }
 
     int messageLength() {
@@ -84,7 +91,9 @@ final class DHClientKeyExchange extends HandshakeMessage {
     }
 
     void send(HandshakeOutStream s) throws IOException {
-        s.putBytes16(dh_Yc);
+        if (dh_Yc != null && dh_Yc.length != 0) {
+            s.putBytes16(dh_Yc);
+        }
     }
 
     void print(PrintStream s) throws IOException {

src/share/classes/sun/security/ssl/DHCrypt.java

 /*
- * Copyright (c) 1996, 2008, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2012, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -28,12 +28,15 @@ package sun.security.ssl;
 
 import java.math.BigInteger;
 import java.security.*;
-
+import java.io.IOException;
+import javax.net.ssl.SSLHandshakeException;
 import javax.crypto.SecretKey;
 import javax.crypto.KeyAgreement;
 import javax.crypto.interfaces.DHPublicKey;
 import javax.crypto.spec.*;
 
+import sun.security.util.KeyUtil;
+
 /**
  * This class implements the Diffie-Hellman key exchange algorithm.
  * D-H means combining your private key with your partners public key to
@@ -54,7 +57,8 @@ import javax.crypto.spec.*;
  *  . if we are server, call DHCrypt(keyLength,random). This generates
  *    an ephemeral keypair of the request length.
  *  . if we are client, call DHCrypt(modulus, base, random). This
- *    generates an ephemeral keypair using the parameters specified by the server.
+ *    generates an ephemeral keypair using the parameters specified by
+ *    the server.
  *  . send parameters and public value to remote peer
  *  . receive peers ephemeral public key
  *  . call getAgreedSecret() to calculate the shared secret
@@ -83,6 +87,9 @@ final class DHCrypt {
     // public component of our key, X = (g ^ x) mod p
     private BigInteger publicValue;             // X (aka y)
 
+    // the times to recove from failure if public key validation
+    private static int MAX_FAILOVER_TIMES = 2;
+
     /**
      * Generate a Diffie-Hellman keypair of the specified size.
      */
@@ -90,9 +97,12 @@ final class DHCrypt {
         try {
             KeyPairGenerator kpg = JsseJce.getKeyPairGenerator("DiffieHellman");
             kpg.initialize(keyLength, random);
-            KeyPair kp = kpg.generateKeyPair();
-            privateKey = kp.getPrivate();
-            DHPublicKeySpec spec = getDHPublicKeySpec(kp.getPublic());
+
+            DHPublicKeySpec spec = generateDHPublicKeySpec(kpg);
+            if (spec == null) {
+                throw new RuntimeException("Could not generate DH keypair");
+            }
+
             publicValue = spec.getY();
             modulus = spec.getP();
             base = spec.getG();
@@ -115,20 +125,25 @@ final class DHCrypt {
             KeyPairGenerator kpg = JsseJce.getKeyPairGenerator("DiffieHellman");
             DHParameterSpec params = new DHParameterSpec(modulus, base);
             kpg.initialize(params, random);
-            KeyPair kp = kpg.generateKeyPair();
-            privateKey = kp.getPrivate();
-            DHPublicKeySpec spec = getDHPublicKeySpec(kp.getPublic());
+
+            DHPublicKeySpec spec = generateDHPublicKeySpec(kpg);
+            if (spec == null) {
+                throw new RuntimeException("Could not generate DH keypair");
+            }
+
             publicValue = spec.getY();
         } catch (GeneralSecurityException e) {
             throw new RuntimeException("Could not generate DH keypair", e);
         }
     }
 
+
     static DHPublicKeySpec getDHPublicKeySpec(PublicKey key) {
         if (key instanceof DHPublicKey) {
             DHPublicKey dhKey = (DHPublicKey)key;
             DHParameterSpec params = dhKey.getParams();
-            return new DHPublicKeySpec(dhKey.getY(), params.getP(), params.getG());
+            return new DHPublicKeySpec(dhKey.getY(),
+                                    params.getP(), params.getG());
         }
         try {
             KeyFactory factory = JsseJce.getKeyFactory("DH");
@@ -166,17 +181,32 @@ final class DHCrypt {
      * <P>It is illegal to call this member function if the private key
      * has not been set (or generated).
      *
-     * @param peerPublicKey the peer's public key.
-     * @returns the secret, which is an unsigned big-endian integer
-     *  the same size as the Diffie-Hellman modulus.
+     * @param  peerPublicKey the peer's public key.
+     * @param  keyIsValidated whether the {@code peerPublicKey} has beed
+     *         validated
+     * @return the secret, which is an unsigned big-endian integer
+     *         the same size as the Diffie-Hellman modulus.
      */
-    SecretKey getAgreedSecret(BigInteger peerPublicValue) {
+    SecretKey getAgreedSecret(BigInteger peerPublicValue,
+            boolean keyIsValidated) throws IOException {
         try {
             KeyFactory kf = JsseJce.getKeyFactory("DiffieHellman");
             DHPublicKeySpec spec =
                         new DHPublicKeySpec(peerPublicValue, modulus, base);
             PublicKey publicKey = kf.generatePublic(spec);
             KeyAgreement ka = JsseJce.getKeyAgreement("DiffieHellman");
+
+            // validate the Diffie-Hellman public key
+            if (!keyIsValidated &&
+                    !KeyUtil.isOracleJCEProvider(ka.getProvider().getName())) {
+                try {
+                    KeyUtil.validate(spec);
+                } catch (InvalidKeyException ike) {
+                    // prefer handshake_failure alert to internal_error alert
+                    throw new SSLHandshakeException(ike.getMessage());
+                }
+            }
+
             ka.init(privateKey);
             ka.doPhase(publicKey, true);
             return ka.generateSecret("TlsPremasterSecret");
@@ -185,4 +215,33 @@ final class DHCrypt {
         }
     }
 
+    // Generate and validate DHPublicKeySpec
+    private DHPublicKeySpec generateDHPublicKeySpec(KeyPairGenerator kpg)
+            throws GeneralSecurityException {
+
+        boolean doExtraValiadtion =
+                    (!KeyUtil.isOracleJCEProvider(kpg.getProvider().getName()));
+        for (int i = 0; i <= MAX_FAILOVER_TIMES; i++) {
+            KeyPair kp = kpg.generateKeyPair();
+            privateKey = kp.getPrivate();
+            DHPublicKeySpec spec = getDHPublicKeySpec(kp.getPublic());
+
+            // validate the Diffie-Hellman public key
+            if (doExtraValiadtion) {
+                try {
+                    KeyUtil.validate(spec);
+                } catch (InvalidKeyException ivke) {
+                    if (i == MAX_FAILOVER_TIMES) {
+                        throw ivke;
+                    }
+                    // otherwise, ignore the exception and try the next one
+                    continue;
+                }
+            }
+
+            return spec;
+        }
+
+        return null;
+    }
 }

src/share/classes/sun/security/ssl/HandshakeMessage.java

 /*
- * Copyright (c) 1996, 2011, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 1996, 2012, Oracle and/or its affiliates. All rights reserved.
  * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
  *
  * This code is free software; you can redistribute it and/or modify it
@@ -41,12 +41,14 @@ import javax.security.auth.x500.X500Principal;
 
 import javax.crypto.KeyGenerator;
 import javax.crypto.SecretKey;
+import javax.crypto.spec.DHPublicKeySpec;
 
 import javax.net.ssl.*;
 
 import sun.security.internal.spec.TlsPrfParameterSpec;
 import sun.security.ssl.CipherSuite.*;
 import static sun.security.ssl.CipherSuite.PRF.*;
+import sun.security.util.KeyUtil;
 
 /**
  * Many data structures are involved in the handshake messages.  These
@@ -702,6 +704,7 @@ class DH_ServerKeyExchange extends ServerKeyExchange
         this.protocolVersion = protocolVersion;
         this.preferableSignatureAlgorithm = null;
 
+        // The DH key has been validated in the constructor of DHCrypt.
         setValues(obj);
         signature = null;
     }
@@ -718,6 +721,7 @@ class DH_ServerKeyExchange extends ServerKeyExchange
 
         this.protocolVersion = protocolVersion;
 
+        // The DH key has been validated in the constructor of DHCrypt.
         setValues(obj);
 
         Signature sig;
@@ -744,7 +748,8 @@ class DH_ServerKeyExchange extends ServerKeyExchange
      * DH_anon key exchange
      */
     DH_ServerKeyExchange(HandshakeInStream input,
-            ProtocolVersion protocolVersion) throws IOException {
+            ProtocolVersion protocolVersion)
+            throws IOException, GeneralSecurityException {
 
         this.protocolVersion = protocolVersion;
         this.preferableSignatureAlgorithm = null;
@@ -752,6 +757,10 @@ class DH_ServerKeyExchange extends ServerKeyExchange
         dh_p = input.getBytes16();
         dh_g = input.getBytes16();
         dh_Ys = input.getBytes16();
+        KeyUtil.validate(new DHPublicKeySpec(new BigInteger(1, dh_Ys),
+                                             new BigInteger(1, dh_p),
+                                             new BigInteger(1, dh_g)));
+
         signature = null;
     }
 
@@ -772,6 +781,9 @@ class DH_ServerKeyExchange extends ServerKeyExchange
         dh_p = input.getBytes16();
         dh_g = input.getBytes16();
         dh_Ys = input.getBytes16();
+        KeyUtil.validate(new DHPublicKeySpec(new BigInteger(1, dh_Ys),
+                                             new BigInteger(1, dh_p),
+                                             new BigInteger(1, dh_g)));
 
         // read the signature and hash algorithm
         if (protocolVersion.v >= ProtocolVersion.TLS12.v) {

src/share/classes/sun/security/ssl/RSAClientKeyExchange.java

@@ -36,7 +36,7 @@ import javax.crypto.spec.*;
 import javax.net.ssl.*;
 
 import sun.security.internal.spec.TlsRsaPremasterSecretParameterSpec;
-import sun.security.util.KeyLength;
+import sun.security.util.KeyUtil;
 
 /**
  * This is the client key exchange message (CLIENT --> SERVER) used with
@@ -193,7 +193,7 @@ final class RSAClientKeyExchange extends HandshakeMessage {
                         "unable to get the plaintext of the premaster secret");
                 }
 
-                int keySize = KeyLength.getKeySize(secretKey);
+                int keySize = KeyUtil.getKeySize(secretKey);
                 if (keySize > 0 && keySize != 384) {       // 384 = 48 * 8
                     if (debug != null && Debug.isOn("handshake")) {
                         System.out.println(

src/share/classes/sun/security/ssl/ServerHandshaker.java

@@ -1365,7 +1365,7 @@ final class ServerHandshaker extends Handshaker {
         if (debug != null && Debug.isOn("handshake")) {
             mesg.print(System.out);
         }
-        return dh.getAgreedSecret(mesg.getClientPublicKey());
+        return dh.getAgreedSecret(mesg.getClientPublicKey(), false);
     }
 
     private SecretKey clientKeyExchange(ECDHClientKeyExchange mesg)

src/share/classes/sun/security/ssl/SignatureAndHashAlgorithm.java

@@ -38,7 +38,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.ArrayList;
 
-import sun.security.util.KeyLength;
+import sun.security.util.KeyUtil;
 
 /**
  * Signature and hash algorithm.
@@ -279,7 +279,7 @@ final class SignatureAndHashAlgorithm {
              * If key size is less than 512, the  digest length should be
              * less than or equal to 20 bytes.
              */
-            int keySize = KeyLength.getKeySize(signingKey);
+            int keySize = KeyUtil.getKeySize(signingKey);
             if (keySize >= 768) {
                 maxDigestLength = HashAlgorithm.SHA512.length;
             } else if ((keySize >= 512) && (keySize < 768)) {

src/share/classes/sun/security/util/DisabledAlgorithmConstraints.java

@@ -440,7 +440,7 @@ public class DisabledAlgorithmConstraints implements AlgorithmConstraints {
 
         // Does this key constraint disable the specified key?
         public boolean disables(Key key) {
-            int size = KeyLength.getKeySize(key);
+            int size = KeyUtil.getKeySize(key);
 
             if (size == 0) {
                 return true;    // we don't allow any key of size 0.

src/share/classes/sun/security/util/KeyLength.java → src/share/classes/sun/security/util/KeyUtil.java

@@ -28,16 +28,22 @@ package sun.security.util;
 import java.security.Key;
 import java.security.PrivilegedAction;
 import java.security.AccessController;
+import java.security.InvalidKeyException;
 import java.security.interfaces.ECKey;
 import java.security.interfaces.RSAKey;
 import java.security.interfaces.DSAKey;
+import java.security.spec.KeySpec;
 import javax.crypto.SecretKey;
 import javax.crypto.interfaces.DHKey;
+import javax.crypto.interfaces.DHPublicKey;
+import javax.crypto.spec.DHParameterSpec;
+import javax.crypto.spec.DHPublicKeySpec;
+import java.math.BigInteger;
 
 /**
- * A utility class to get key length
+ * A utility class to get key length, valiate keys, etc.
  */
-public final class KeyLength {
+public final class KeyUtil {
 
     /**
      * Returns the key size of the given key object in bits.
@@ -46,7 +52,7 @@ public final class KeyLength {
      * @return the key size of the given key object in bits, or -1 if the
      *       key size is not accessible
      */
-    final public static int getKeySize(Key key) {
+    public static final int getKeySize(Key key) {
         int size = -1;
 
         if (key instanceof Length) {
@@ -87,5 +93,112 @@ public final class KeyLength {
 
         return size;
     }
+
+    /**
+     * Returns whether the key is valid or not.
+     * <P>
+     * Note that this method is only apply to DHPublicKey at present.
+     *
+     * @param  publicKey
+     *         the key object, cannot be null
+     *
+     * @throws NullPointerException if {@code publicKey} is null
+     * @throws InvalidKeyException if {@code publicKey} is invalid
+     */
+    public static final void validate(Key key)
+            throws InvalidKeyException {
+        if (key == null) {
+            throw new NullPointerException(
+                "The key to be validated cannot be null");
+        }
+
+        if (key instanceof DHPublicKey) {
+            validateDHPublicKey((DHPublicKey)key);
+        }
+    }
+
+
+    /**
+     * Returns whether the key spec is valid or not.
+     * <P>
+     * Note that this method is only apply to DHPublicKeySpec at present.
+     *
+     * @param  keySpec
+     *         the key spec object, cannot be null
+     *
+     * @throws NullPointerException if {@code keySpec} is null
+     * @throws InvalidKeyException if {@code keySpec} is invalid
+     */
+    public static final void validate(KeySpec keySpec)
+            throws InvalidKeyException {
+        if (keySpec == null) {
+            throw new NullPointerException(
+                "The key spec to be validated cannot be null");
+        }
+
+        if (keySpec instanceof DHPublicKeySpec) {
+            validateDHPublicKey((DHPublicKeySpec)keySpec);
+        }
+    }
+
+    /**
+     * Returns whether the specified provider is Oracle provider or not.
+     * <P>
+     * Note that this method is only apply to SunJCE and SunPKCS11 at present.
+     *
+     * @param  providerName
+     *         the provider name
+     * @return true if, and only if, the provider of the specified
+     *         {@code providerName} is Oracle provider
+     */
+    public static final boolean isOracleJCEProvider(String providerName) {
+        return providerName != null && (providerName.equals("SunJCE") ||
+                                        providerName.startsWith("SunPKCS11"));
+    }
+
+    /**
+     * Returns whether the Diffie-Hellman public key is valid or not.
+     *
+     * Per RFC 2631 and NIST SP800-56A, the following algorithm is used to
+     * validate Diffie-Hellman public keys:
+     * 1. Verify that y lies within the interval [2,p-1]. If it does not,
+     *    the key is invalid.
+     * 2. Compute y^q mod p. If the result == 1, the key is valid.
+     *    Otherwise the key is invalid.
+     */
+    private static void validateDHPublicKey(DHPublicKey publicKey)
+            throws InvalidKeyException {
+        DHParameterSpec paramSpec = publicKey.getParams();
+
+        BigInteger p = paramSpec.getP();
+        BigInteger g = paramSpec.getG();
+        BigInteger y = publicKey.getY();
+
+        validateDHPublicKey(p, g, y);
+    }
+
+    private static void validateDHPublicKey(DHPublicKeySpec publicKeySpec)
+            throws InvalidKeyException {
+        validateDHPublicKey(publicKeySpec.getP(),
+            publicKeySpec.getG(), publicKeySpec.getY());
+    }
+
+    private static void validateDHPublicKey(BigInteger p,
+            BigInteger g, BigInteger y) throws InvalidKeyException {
+
+        // For better interoperability, the interval is limited to [2, p-2].
+        BigInteger leftOpen = BigInteger.ONE;
+        BigInteger rightOpen = p.subtract(BigInteger.ONE);
+        if (y.compareTo(leftOpen) <= 0) {
+            throw new InvalidKeyException(
+                    "Diffie-Hellman public key is too small");
+        }
+        if (y.compareTo(rightOpen) >= 0) {
+            throw new InvalidKeyException(
+                    "Diffie-Hellman public key is too large");
+        }
+
+        // Don't bother to check against the y^q mod p if safe primes are used.
+    }
 }
 

test/sun/security/mscapi/ShortRSAKeyWithinTLS.java

@@ -29,7 +29,7 @@ import javax.net.*;
 import javax.net.ssl.*;
 import java.lang.reflect.*;
 
-import sun.security.util.KeyLength;
+import sun.security.util.KeyUtil;
 
 public class ShortRSAKeyWithinTLS {
 
@@ -175,13 +175,13 @@ public class ShortRSAKeyWithinTLS {
             privateKey = (PrivateKey)ks.getKey(keyAlias, null);
             publicKey = (PublicKey)ks.getCertificate(keyAlias).getPublicKey();
 
-            int privateKeySize = KeyLength.getKeySize(privateKey);
+            int privateKeySize = KeyUtil.getKeySize(privateKey);
             if (privateKeySize != keySize) {
                 throw new Exception("Expected key size is " + keySize +
                         ", but the private key size is " + privateKeySize);
             }
 
-            int publicKeySize = KeyLength.getKeySize(publicKey);
+            int publicKeySize = KeyUtil.getKeySize(publicKey);
             if (publicKeySize != keySize) {
                 throw new Exception("Expected key size is " + keySize +
                         ", but the public key size is " + publicKeySize);