8267729: Improve TLS client handshaking

Reviewed-by: ahgross, jnimeh, rhalade

src/java.base/share/classes/sun/security/ssl/ECDHClientKeyExchange.java

@@ -27,6 +27,7 @@ package sun.security.ssl;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.security.CryptoPrimitive;
 import java.security.GeneralSecurityException;
 import java.security.PublicKey;
 import java.security.interfaces.ECPublicKey;
@@ -35,6 +36,7 @@ import java.security.spec.AlgorithmParameterSpec;
 import java.security.spec.ECParameterSpec;
 import java.security.spec.NamedParameterSpec;
 import java.text.MessageFormat;
+import java.util.EnumSet;
 import java.util.Locale;
 import javax.crypto.SecretKey;
 import sun.security.ssl.SSLHandshake.HandshakeMessage;
@@ -317,12 +319,19 @@ final class ECDHClientKeyExchange {
 
             // create the credentials
             try {
-                NamedGroup ng = namedGroup;  // "effectively final" the lambda
+                SSLCredentials sslCredentials =
-                // AlgorithmConstraints are checked internally.
+                        namedGroup.decodeCredentials(cke.encodedPoint);
-                SSLCredentials sslCredentials = namedGroup.decodeCredentials(
+                if (shc.algorithmConstraints != null &&
-                        cke.encodedPoint, shc.algorithmConstraints,
+                        sslCredentials instanceof
-                        s -> shc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
+                            NamedGroupCredentials namedGroupCredentials) {
-                        "ClientKeyExchange " + ng + ": " + s));
+                    if (!shc.algorithmConstraints.permits(
+                            EnumSet.of(CryptoPrimitive.KEY_AGREEMENT),
+                            namedGroupCredentials.getPublicKey())) {
+                        shc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
+                            "ClientKeyExchange for " + namedGroup +
+                            " does not comply with algorithm constraints");
+                    }
+                }
 
                 shc.handshakeCredentials.add(sslCredentials);
             } catch (GeneralSecurityException e) {
@@ -497,12 +506,19 @@ final class ECDHClientKeyExchange {
 
             // create the credentials
             try {
-                NamedGroup ng = namedGroup; // "effectively final" the lambda
+                SSLCredentials sslCredentials =
-                // AlgorithmConstraints are checked internally.
+                        namedGroup.decodeCredentials(cke.encodedPoint);
-                SSLCredentials sslCredentials = namedGroup.decodeCredentials(
+                if (shc.algorithmConstraints != null &&
-                        cke.encodedPoint, shc.algorithmConstraints,
+                        sslCredentials instanceof
-                        s -> shc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
+                                NamedGroupCredentials namedGroupCredentials) {
-                        "ClientKeyExchange " + ng + ": " + s));
+                    if (!shc.algorithmConstraints.permits(
+                            EnumSet.of(CryptoPrimitive.KEY_AGREEMENT),
+                            namedGroupCredentials.getPublicKey())) {
+                        shc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
+                            "ClientKeyExchange for " + namedGroup +
+                            " does not comply with algorithm constraints");
+                    }
+                }
 
                 shc.handshakeCredentials.add(sslCredentials);
             } catch (GeneralSecurityException e) {

src/java.base/share/classes/sun/security/ssl/ECDHServerKeyExchange.java

@@ -27,6 +27,7 @@ package sun.security.ssl;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.security.CryptoPrimitive;
 import java.security.GeneralSecurityException;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.InvalidKeyException;
@@ -37,6 +38,7 @@ import java.security.PublicKey;
 import java.security.Signature;
 import java.security.SignatureException;
 import java.text.MessageFormat;
+import java.util.EnumSet;
 import java.util.Locale;
 import java.util.Map;
 import sun.security.ssl.SSLHandshake.HandshakeMessage;
@@ -214,10 +216,19 @@ final class ECDHServerKeyExchange {
             }
 
             try {
-                sslCredentials = namedGroup.decodeCredentials(
+                sslCredentials =
-                    publicPoint, handshakeContext.algorithmConstraints,
+                        namedGroup.decodeCredentials(publicPoint);
-                     s -> chc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
+                if (handshakeContext.algorithmConstraints != null &&
-                     "ServerKeyExchange " + namedGroup + ": " + (s)));
+                        sslCredentials instanceof
+                                NamedGroupCredentials namedGroupCredentials) {
+                    if (!handshakeContext.algorithmConstraints.permits(
+                            EnumSet.of(CryptoPrimitive.KEY_AGREEMENT),
+                            namedGroupCredentials.getPublicKey())) {
+                        chc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
+                            "ServerKeyExchange for " + namedGroup +
+                            " does not comply with algorithm constraints");
+                    }
+                }
             } catch (GeneralSecurityException ex) {
                 throw chc.conContext.fatal(Alert.UNEXPECTED_MESSAGE,
                         "Cannot decode named group: " +

src/java.base/share/classes/sun/security/ssl/KeyShareExtension.java

@@ -27,6 +27,7 @@ package sun.security.ssl;
 
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.security.CryptoPrimitive;
 import java.security.GeneralSecurityException;
 import java.text.MessageFormat;
 import java.util.Collections;
@@ -349,7 +350,8 @@ final class KeyShareExtension {
                 NamedGroup ng = NamedGroup.valueOf(entry.namedGroupId);
                 if (ng == null || !SupportedGroups.isActivatable(
                         shc.algorithmConstraints, ng)) {
-                    if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) {
+                    if (SSLLogger.isOn &&
+                            SSLLogger.isOn("ssl,handshake")) {
                         SSLLogger.fine(
                                 "Ignore unsupported named group: " +
                                 NamedGroup.nameOf(entry.namedGroupId));
@@ -359,18 +361,35 @@ final class KeyShareExtension {
 
                 try {
                     SSLCredentials kaCred =
-                        ng.decodeCredentials(entry.keyExchange,
+                        ng.decodeCredentials(entry.keyExchange);
-                        shc.algorithmConstraints,
+                    if (shc.algorithmConstraints != null &&
-                        s -> SSLLogger.warning(s));
+                            kaCred instanceof
+                                NamedGroupCredentials namedGroupCredentials) {
+                        if (!shc.algorithmConstraints.permits(
+                                EnumSet.of(CryptoPrimitive.KEY_AGREEMENT),
+                                namedGroupCredentials.getPublicKey())) {
+                            if (SSLLogger.isOn &&
+                                    SSLLogger.isOn("ssl,handshake")) {
+                                SSLLogger.warning(
+                                    "key share entry of " + ng + " does not " +
+                                    " comply with algorithm constraints");
+                            }
+
+                            kaCred = null;
+                        }
+                    }
+
                     if (kaCred != null) {
                         credentials.add(kaCred);
                     }
                 } catch (GeneralSecurityException ex) {
+                    if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) {
                         SSLLogger.warning(
                                 "Cannot decode named group: " +
                                 NamedGroup.nameOf(entry.namedGroupId));
                     }
                 }
+            }
 
             if (!credentials.isEmpty()) {
                 shc.handshakeCredentials.addAll(credentials);
@@ -646,9 +665,20 @@ final class KeyShareExtension {
 
             SSLCredentials credentials = null;
             try {
-                SSLCredentials kaCred = ng.decodeCredentials(
+                SSLCredentials kaCred =
-                    keyShare.keyExchange, chc.algorithmConstraints,
+                        ng.decodeCredentials(keyShare.keyExchange);
-                    s -> chc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, s));
+                if (chc.algorithmConstraints != null &&
+                        kaCred instanceof
+                                NamedGroupCredentials namedGroupCredentials) {
+                    if (!chc.algorithmConstraints.permits(
+                            EnumSet.of(CryptoPrimitive.KEY_AGREEMENT),
+                            namedGroupCredentials.getPublicKey())) {
+                        chc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
+                            "key share entry of " + ng + " does not " +
+                            " comply with algorithm constraints");
+                    }
+                }
+
                 if (kaCred != null) {
                     credentials = kaCred;
                 }

src/java.base/share/classes/sun/security/ssl/NamedGroup.java

@@ -419,12 +419,9 @@ enum NamedGroup {
         return spec.encodePossessionPublicKey(namedGroupPossession);
     }
 
-    SSLCredentials decodeCredentials(byte[] encoded,
+    SSLCredentials decodeCredentials(
-            AlgorithmConstraints constraints,
+            byte[] encoded) throws IOException, GeneralSecurityException {
-            ExceptionSupplier onConstraintFail)
+        return spec.decodeCredentials(this, encoded);
-            throws IOException, GeneralSecurityException {
-        return spec.decodeCredentials(
-                this, encoded, constraints, onConstraintFail);
     }
 
     SSLPossession createPossession(SecureRandom random) {
@@ -436,30 +433,13 @@ enum NamedGroup {
         return spec.createKeyDerivation(hc);
     }
 
-    interface ExceptionSupplier {
-        void apply(String s) throws SSLException;
-    }
-
     // A list of operations related to named groups.
     private interface NamedGroupScheme {
-        default void checkConstraints(PublicKey publicKey,
-                AlgorithmConstraints constraints,
-                ExceptionSupplier onConstraintFail) throws SSLException {
-            if (!constraints.permits(
-                    EnumSet.of(CryptoPrimitive.KEY_AGREEMENT), publicKey)) {
-                onConstraintFail.apply("key share entry does not "
-                        + "comply with algorithm constraints");
-            }
-        }
-
         byte[] encodePossessionPublicKey(
                 NamedGroupPossession namedGroupPossession);
 
-        SSLCredentials decodeCredentials(
+        SSLCredentials decodeCredentials(NamedGroup ng,
-                NamedGroup ng, byte[] encoded,
+                byte[] encoded) throws IOException, GeneralSecurityException;
-                AlgorithmConstraints constraints,
-                ExceptionSupplier onConstraintFail
-            ) throws IOException, GeneralSecurityException;
 
         SSLPossession createPossession(NamedGroup ng, SecureRandom random);
 
@@ -524,13 +504,10 @@ enum NamedGroup {
         }
 
         @Override
-        public SSLCredentials decodeCredentials(NamedGroup ng, byte[] encoded,
+        public SSLCredentials decodeCredentials(NamedGroup ng,
-                    AlgorithmConstraints constraints,
+                byte[] encoded) throws IOException, GeneralSecurityException {
-                    ExceptionSupplier onConstraintFail
-                ) throws IOException, GeneralSecurityException {
             if (scheme != null) {
-                return scheme.decodeCredentials(
+                return scheme.decodeCredentials(ng, encoded);
-                        ng, encoded, constraints, onConstraintFail);
             }
 
             return null;
@@ -567,18 +544,9 @@ enum NamedGroup {
         }
 
         @Override
-        public SSLCredentials decodeCredentials(NamedGroup ng, byte[] encoded,
+        public SSLCredentials decodeCredentials(NamedGroup ng,
-                AlgorithmConstraints constraints,
+                byte[] encoded) throws IOException, GeneralSecurityException {
-                ExceptionSupplier onConstraintFail
+            return DHKeyExchange.DHECredentials.valueOf(ng, encoded);
-            ) throws IOException, GeneralSecurityException {
-
-            DHKeyExchange.DHECredentials result
-                    = DHKeyExchange.DHECredentials.valueOf(ng, encoded);
-
-            checkConstraints(result.getPublicKey(), constraints,
-                    onConstraintFail);
-
-            return result;
         }
 
         @Override
@@ -605,18 +573,9 @@ enum NamedGroup {
         }
 
         @Override
-        public SSLCredentials decodeCredentials(NamedGroup ng, byte[] encoded,
+        public SSLCredentials decodeCredentials(NamedGroup ng,
-                AlgorithmConstraints constraints,
+                byte[] encoded) throws IOException, GeneralSecurityException {
-                ExceptionSupplier onConstraintFail
+            return ECDHKeyExchange.ECDHECredentials.valueOf(ng, encoded);
-            ) throws IOException, GeneralSecurityException {
-
-            ECDHKeyExchange.ECDHECredentials result
-                    = ECDHKeyExchange.ECDHECredentials.valueOf(ng, encoded);
-
-            checkConstraints(result.getPublicKey(), constraints,
-                    onConstraintFail);
-
-            return result;
         }
 
         @Override
@@ -641,18 +600,9 @@ enum NamedGroup {
         }
 
         @Override
-        public SSLCredentials decodeCredentials(NamedGroup ng, byte[] encoded,
+        public SSLCredentials decodeCredentials(NamedGroup ng,
-                AlgorithmConstraints constraints,
+                byte[] encoded) throws IOException, GeneralSecurityException {
-                ExceptionSupplier onConstraintFail
+            return XDHKeyExchange.XDHECredentials.valueOf(ng, encoded);
-            ) throws IOException, GeneralSecurityException {
-
-            XDHKeyExchange.XDHECredentials result
-                    = XDHKeyExchange.XDHECredentials.valueOf(ng, encoded);
-
-            checkConstraints(result.getPublicKey(), constraints,
-                    onConstraintFail);
-
-            return result;
         }
 
         @Override

src/java.base/share/classes/sun/security/ssl/SSLLogger.java

@@ -184,7 +184,7 @@ public final class SSLLogger {
     }
 
     private static void log(Level level, String msg, Object... params) {
-        if (logger.isLoggable(level)) {
+        if (logger != null && logger.isLoggable(level)) {
             if (params == null || params.length == 0) {
                 logger.log(level, msg);
             } else {