Skip to content

Commit

Permalink
8267729: Improve TLS client handshaking
Browse files Browse the repository at this point in the history 
Reviewed-by: mbaesken
Backport-of: 8e4cbf7fd373c5886be1980bed4fa9cd9045f893
  • Loading branch information
@martinuy @MBaesken
martinuy authored and MBaesken committed Oct 12, 2021
1 parent 3e84581 commit a1a52db
Show file tree
Hide file tree
Showing 5 changed files with 107 additions and 94 deletions.
42 changes: 30 additions & 12 deletions src/java.base/share/classes/sun/security/ssl/ECDHClientKeyExchange.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

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;
Expand All @@ -35,6 +36,7 @@
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;
Expand Down Expand Up @@ -317,12 +319,20 @@ public void consume(ConnectionContext context,

// create the credentials
try {
NamedGroup ng = namedGroup; // "effectively final" the lambda
// AlgorithmConstraints are checked internally.
SSLCredentials sslCredentials = namedGroup.decodeCredentials(
cke.encodedPoint, shc.algorithmConstraints,
s -> shc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
"ClientKeyExchange " + ng + ": " + s));
SSLCredentials sslCredentials =
namedGroup.decodeCredentials(cke.encodedPoint);
if (shc.algorithmConstraints != null &&
sslCredentials instanceof NamedGroupCredentials) {
NamedGroupCredentials namedGroupCredentials =
(NamedGroupCredentials) sslCredentials;
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) {
Expand Down Expand Up @@ -499,12 +509,20 @@ public void consume(ConnectionContext context,

// create the credentials
try {
NamedGroup ng = namedGroup; // "effectively final" the lambda
// AlgorithmConstraints are checked internally.
SSLCredentials sslCredentials = namedGroup.decodeCredentials(
cke.encodedPoint, shc.algorithmConstraints,
s -> shc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
"ClientKeyExchange " + ng + ": " + s));
SSLCredentials sslCredentials =
namedGroup.decodeCredentials(cke.encodedPoint);
if (shc.algorithmConstraints != null &&
sslCredentials instanceof NamedGroupCredentials) {
NamedGroupCredentials namedGroupCredentials =
(NamedGroupCredentials) sslCredentials;
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) {
Expand Down
20 changes: 16 additions & 4 deletions src/java.base/share/classes/sun/security/ssl/ECDHServerKeyExchange.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import java.io.IOException;
import java.nio.ByteBuffer;
import java.security.CryptoPrimitive;
import java.security.GeneralSecurityException;
import java.security.InvalidAlgorithmParameterException;
import java.security.InvalidKeyException;
Expand All @@ -37,6 +38,7 @@
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.NamedGroup.NamedGroupSpec;
Expand Down Expand Up @@ -215,10 +217,20 @@ class ECDHServerKeyExchangeMessage extends HandshakeMessage {
}

try {
sslCredentials = namedGroup.decodeCredentials(
publicPoint, handshakeContext.algorithmConstraints,
s -> chc.conContext.fatal(Alert.INSUFFICIENT_SECURITY,
"ServerKeyExchange " + namedGroup + ": " + (s)));
sslCredentials =
namedGroup.decodeCredentials(publicPoint);
if (handshakeContext.algorithmConstraints != null &&
sslCredentials instanceof NamedGroupCredentials) {
NamedGroupCredentials namedGroupCredentials =
(NamedGroupCredentials) sslCredentials;
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: " +
Expand Down
53 changes: 43 additions & 10 deletions src/java.base/share/classes/sun/security/ssl/KeyShareExtension.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@

import java.io.IOException;
import java.nio.ByteBuffer;
import java.security.CryptoPrimitive;
import java.security.GeneralSecurityException;
import java.text.MessageFormat;
import java.util.Arrays;
import java.util.Collections;
import java.util.EnumSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
Expand Down Expand Up @@ -338,7 +340,8 @@ public void consume(ConnectionContext context,
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));
Expand All @@ -348,16 +351,34 @@ public void consume(ConnectionContext context,

try {
SSLCredentials kaCred =
ng.decodeCredentials(entry.keyExchange,
shc.algorithmConstraints,
s -> SSLLogger.warning(s));
ng.decodeCredentials(entry.keyExchange);
if (shc.algorithmConstraints != null &&
kaCred instanceof NamedGroupCredentials) {
NamedGroupCredentials namedGroupCredentials =
(NamedGroupCredentials) kaCred;
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) {
SSLLogger.warning(
"Cannot decode named group: " +
NamedGroup.nameOf(entry.namedGroupId));
if (SSLLogger.isOn && SSLLogger.isOn("ssl,handshake")) {
SSLLogger.warning(
"Cannot decode named group: " +
NamedGroup.nameOf(entry.namedGroupId));
}
}
}

Expand Down Expand Up @@ -637,9 +658,21 @@ public void consume(ConnectionContext context,

SSLCredentials credentials = null;
try {
SSLCredentials kaCred = ng.decodeCredentials(
keyShare.keyExchange, chc.algorithmConstraints,
s -> chc.conContext.fatal(Alert.UNEXPECTED_MESSAGE, s));
SSLCredentials kaCred =
ng.decodeCredentials(keyShare.keyExchange);
if (chc.algorithmConstraints != null &&
kaCred instanceof NamedGroupCredentials) {
NamedGroupCredentials namedGroupCredentials =
(NamedGroupCredentials) kaCred;
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;
}
Expand Down
84 changes: 17 additions & 67 deletions src/java.base/share/classes/sun/security/ssl/NamedGroup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -410,12 +410,9 @@ byte[] encodePossessionPublicKey(
return spec.encodePossessionPublicKey(namedGroupPossession);
}

SSLCredentials decodeCredentials(byte[] encoded,
AlgorithmConstraints constraints,
ExceptionSupplier onConstraintFail)
throws IOException, GeneralSecurityException {
return spec.decodeCredentials(
this, encoded, constraints, onConstraintFail);
SSLCredentials decodeCredentials(
byte[] encoded) throws IOException, GeneralSecurityException {
return spec.decodeCredentials(this, encoded);
}

SSLPossession createPossession(SecureRandom random) {
Expand All @@ -427,30 +424,13 @@ SSLKeyDerivation createKeyDerivation(
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(
NamedGroup ng, byte[] encoded,
AlgorithmConstraints constraints,
ExceptionSupplier onConstraintFail
) throws IOException, GeneralSecurityException;
SSLCredentials decodeCredentials(NamedGroup ng,
byte[] encoded) throws IOException, GeneralSecurityException;

SSLPossession createPossession(NamedGroup ng, SecureRandom random);

Expand Down Expand Up @@ -515,13 +495,10 @@ public byte[] encodePossessionPublicKey(
}

@Override
public SSLCredentials decodeCredentials(NamedGroup ng, byte[] encoded,
AlgorithmConstraints constraints,
ExceptionSupplier onConstraintFail
) throws IOException, GeneralSecurityException {
public SSLCredentials decodeCredentials(NamedGroup ng,
byte[] encoded) throws IOException, GeneralSecurityException {
if (scheme != null) {
return scheme.decodeCredentials(
ng, encoded, constraints, onConstraintFail);
return scheme.decodeCredentials(ng, encoded);
}

return null;
Expand Down Expand Up @@ -558,18 +535,9 @@ public byte[] encodePossessionPublicKey(
}

@Override
public SSLCredentials decodeCredentials(NamedGroup ng, byte[] encoded,
AlgorithmConstraints constraints,
ExceptionSupplier onConstraintFail
) throws IOException, GeneralSecurityException {

DHKeyExchange.DHECredentials result
= DHKeyExchange.DHECredentials.valueOf(ng, encoded);

checkConstraints(result.getPublicKey(), constraints,
onConstraintFail);

return result;
public SSLCredentials decodeCredentials(NamedGroup ng,
byte[] encoded) throws IOException, GeneralSecurityException {
return DHKeyExchange.DHECredentials.valueOf(ng, encoded);
}

@Override
Expand All @@ -596,18 +564,9 @@ public byte[] encodePossessionPublicKey(
}

@Override
public SSLCredentials decodeCredentials(NamedGroup ng, byte[] encoded,
AlgorithmConstraints constraints,
ExceptionSupplier onConstraintFail
) throws IOException, GeneralSecurityException {

ECDHKeyExchange.ECDHECredentials result
= ECDHKeyExchange.ECDHECredentials.valueOf(ng, encoded);

checkConstraints(result.getPublicKey(), constraints,
onConstraintFail);

return result;
public SSLCredentials decodeCredentials(NamedGroup ng,
byte[] encoded) throws IOException, GeneralSecurityException {
return ECDHKeyExchange.ECDHECredentials.valueOf(ng, encoded);
}

@Override
Expand All @@ -632,18 +591,9 @@ public byte[] encodePossessionPublicKey(NamedGroupPossession poss) {
}

@Override
public SSLCredentials decodeCredentials(NamedGroup ng, byte[] encoded,
AlgorithmConstraints constraints,
ExceptionSupplier onConstraintFail
) throws IOException, GeneralSecurityException {

XDHKeyExchange.XDHECredentials result
= XDHKeyExchange.XDHECredentials.valueOf(ng, encoded);

checkConstraints(result.getPublicKey(), constraints,
onConstraintFail);

return result;
public SSLCredentials decodeCredentials(NamedGroup ng,
byte[] encoded) throws IOException, GeneralSecurityException {
return XDHKeyExchange.XDHECredentials.valueOf(ng, encoded);
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion src/java.base/share/classes/sun/security/ssl/SSLLogger.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,7 @@ public static void finest(String msg, Object... params) {
}

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 {
Expand Down

0 comments on commit a1a52db

Please sign in to comment.