8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key

Reviewed-by: ascarpino
This commit is contained in:
Sean Mullan 2021-10-21 17:28:40 +00:00
parent bef8cf1ba1
commit 49f9d8031e
8 changed files with 204 additions and 223 deletions

View file

@ -74,10 +74,10 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
private static final Debug debug = Debug.getInstance("certpath");
private final AlgorithmConstraints constraints;
private final PublicKey trustedPubKey;
private final Date date;
private PublicKey prevPubKey;
private final String variant;
private PublicKey trustedPubKey;
private PublicKey prevPubKey;
private TrustAnchor anchor;
private static final Set<CryptoPrimitive> SIGNATURE_PRIMITIVE_SET =
@ -90,10 +90,6 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
CryptoPrimitive.PUBLIC_KEY_ENCRYPTION,
CryptoPrimitive.KEY_AGREEMENT));
private static final DisabledAlgorithmConstraints
certPathDefaultConstraints =
DisabledAlgorithmConstraints.certPathConstraints();
/**
* Create a new {@code AlgorithmChecker} with the given
* {@code TrustAnchor} and {@code String} variant.
@ -104,7 +100,7 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
* passed will set it to Validator.GENERIC.
*/
public AlgorithmChecker(TrustAnchor anchor, String variant) {
this(anchor, certPathDefaultConstraints, null, variant);
this(anchor, null, null, variant);
}
/**
@ -141,19 +137,11 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
AlgorithmConstraints constraints, Date date, String variant) {
if (anchor != null) {
if (anchor.getTrustedCert() != null) {
this.trustedPubKey = anchor.getTrustedCert().getPublicKey();
} else {
this.trustedPubKey = anchor.getCAPublicKey();
}
this.anchor = anchor;
} else {
this.trustedPubKey = null;
setTrustAnchorAndKeys(anchor);
}
this.prevPubKey = this.trustedPubKey;
this.constraints = (constraints == null ? certPathDefaultConstraints :
constraints);
this.constraints = constraints == null ?
DisabledAlgorithmConstraints.certPathConstraints() : constraints;
this.date = date;
this.variant = (variant == null ? Validator.VAR_GENERIC : variant);
}
@ -172,18 +160,14 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
* passed will set it to Validator.GENERIC.
*/
public AlgorithmChecker(TrustAnchor anchor, Date date, String variant) {
this(anchor, certPathDefaultConstraints, date, variant);
this(anchor, null, date, variant);
}
@Override
public void init(boolean forward) throws CertPathValidatorException {
// Note that this class does not support forward mode.
if (!forward) {
if (trustedPubKey != null) {
prevPubKey = trustedPubKey;
} else {
prevPubKey = null;
}
prevPubKey = trustedPubKey;
} else {
throw new
CertPathValidatorException("forward checking not supported");
@ -207,8 +191,8 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
Collection<String> unresolvedCritExts)
throws CertPathValidatorException {
if (!(cert instanceof X509Certificate) || constraints == null) {
// ignore the check for non-x.509 certificate or null constraints
if (!(cert instanceof X509Certificate)) {
// ignore the check for non-x.509 certificate
return;
}
@ -233,115 +217,114 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
PublicKey currPubKey = cert.getPublicKey();
String currSigAlg = x509Cert.getSigAlgName();
// Check the signature algorithm and parameters against constraints.
if (!constraints.permits(SIGNATURE_PRIMITIVE_SET, currSigAlg,
currSigAlgParams)) {
throw new CertPathValidatorException(
"Algorithm constraints check failed on signature " +
"algorithm: " + currSigAlg, null, null, -1,
BasicReason.ALGORITHM_CONSTRAINED);
}
if (constraints instanceof DisabledAlgorithmConstraints) {
DisabledAlgorithmConstraints dac =
(DisabledAlgorithmConstraints)constraints;
if (prevPubKey != null && prevPubKey == trustedPubKey) {
// check constraints of trusted public key (make sure
// algorithm and size is not restricted)
CertPathConstraintsParameters cp =
new CertPathConstraintsParameters(trustedPubKey, variant,
anchor, date);
dac.permits(trustedPubKey.getAlgorithm(), cp);
}
// Check the signature algorithm and parameters against constraints
CertPathConstraintsParameters cp =
new CertPathConstraintsParameters(x509Cert, variant,
anchor, date);
dac.permits(currSigAlg, currSigAlgParams, cp);
} else {
if (prevPubKey != null) {
if (!constraints.permits(SIGNATURE_PRIMITIVE_SET,
currSigAlg, prevPubKey, currSigAlgParams)) {
throw new CertPathValidatorException(
"Algorithm constraints check failed on " +
currSigAlg + "signature and " +
currPubKey.getAlgorithm() + " key with size of " +
sun.security.util.KeyUtil.getKeySize(currPubKey) +
"bits",
null, null, -1, BasicReason.ALGORITHM_CONSTRAINED);
}
} else {
if (!constraints.permits(SIGNATURE_PRIMITIVE_SET,
currSigAlg, currSigAlgParams)) {
throw new CertPathValidatorException(
"Algorithm constraints check failed on " +
"signature algorithm: " + currSigAlg,
null, null, -1, BasicReason.ALGORITHM_CONSTRAINED);
}
}
// Assume all key usage bits are set if key usage is not present
Set<CryptoPrimitive> primitives = KU_PRIMITIVE_SET;
// Assume all key usage bits are set if key usage is not present
Set<CryptoPrimitive> primitives = KU_PRIMITIVE_SET;
if (keyUsage != null) {
if (keyUsage != null) {
primitives = EnumSet.noneOf(CryptoPrimitive.class);
if (keyUsage[0] || keyUsage[1] || keyUsage[5] || keyUsage[6]) {
// keyUsage[0]: KeyUsage.digitalSignature
// keyUsage[1]: KeyUsage.nonRepudiation
// keyUsage[5]: KeyUsage.keyCertSign
// keyUsage[6]: KeyUsage.cRLSign
primitives.add(CryptoPrimitive.SIGNATURE);
if (keyUsage[0] || keyUsage[1] || keyUsage[5] || keyUsage[6]) {
// keyUsage[0]: KeyUsage.digitalSignature
// keyUsage[1]: KeyUsage.nonRepudiation
// keyUsage[5]: KeyUsage.keyCertSign
// keyUsage[6]: KeyUsage.cRLSign
primitives.add(CryptoPrimitive.SIGNATURE);
}
if (keyUsage[2]) { // KeyUsage.keyEncipherment
primitives.add(CryptoPrimitive.KEY_ENCAPSULATION);
}
if (keyUsage[3]) { // KeyUsage.dataEncipherment
primitives.add(CryptoPrimitive.PUBLIC_KEY_ENCRYPTION);
}
if (keyUsage[4]) { // KeyUsage.keyAgreement
primitives.add(CryptoPrimitive.KEY_AGREEMENT);
}
// KeyUsage.encipherOnly and KeyUsage.decipherOnly are
// undefined in the absence of the keyAgreement bit.
if (primitives.isEmpty()) {
throw new CertPathValidatorException(
"incorrect KeyUsage extension bits",
null, null, -1, PKIXReason.INVALID_KEY_USAGE);
}
}
if (keyUsage[2]) { // KeyUsage.keyEncipherment
primitives.add(CryptoPrimitive.KEY_ENCAPSULATION);
}
if (keyUsage[3]) { // KeyUsage.dataEncipherment
primitives.add(CryptoPrimitive.PUBLIC_KEY_ENCRYPTION);
}
if (keyUsage[4]) { // KeyUsage.keyAgreement
primitives.add(CryptoPrimitive.KEY_AGREEMENT);
}
// KeyUsage.encipherOnly and KeyUsage.decipherOnly are
// undefined in the absence of the keyAgreement bit.
if (primitives.isEmpty()) {
throw new CertPathValidatorException(
"incorrect KeyUsage extension bits",
null, null, -1, PKIXReason.INVALID_KEY_USAGE);
}
}
ConstraintsParameters cp =
new CertPathConstraintsParameters(x509Cert, variant,
anchor, date);
// Check against local constraints if it is DisabledAlgorithmConstraints
if (constraints instanceof DisabledAlgorithmConstraints) {
((DisabledAlgorithmConstraints)constraints).permits(currSigAlg,
currSigAlgParams, cp);
// DisabledAlgorithmsConstraints does not check primitives, so key
// additional key check.
} else {
// Perform the default constraints checking anyway.
certPathDefaultConstraints.permits(currSigAlg, currSigAlgParams, cp);
// Call locally set constraints to check key with primitives.
if (!constraints.permits(primitives, currPubKey)) {
throw new CertPathValidatorException(
"Algorithm constraints check failed on key " +
currPubKey.getAlgorithm() + " with size of " +
sun.security.util.KeyUtil.getKeySize(currPubKey) +
"bits",
null, null, -1, BasicReason.ALGORITHM_CONSTRAINED);
}
}
// If there is no previous key, set one and exit
if (prevPubKey == null) {
prevPubKey = currPubKey;
return;
}
// Check with previous cert for signature algorithm and public key
if (!constraints.permits(
SIGNATURE_PRIMITIVE_SET,
currSigAlg, prevPubKey, currSigAlgParams)) {
throw new CertPathValidatorException(
"Algorithm constraints check failed on " +
"signature algorithm: " + currSigAlg,
currPubKey.getAlgorithm() + " key with size of " +
sun.security.util.KeyUtil.getKeySize(currPubKey) +
"bits",
null, null, -1, BasicReason.ALGORITHM_CONSTRAINED);
}
}
// Inherit key parameters from previous key
if (PKIX.isDSAPublicKeyWithoutParams(currPubKey)) {
// Inherit DSA parameters from previous key
if (!(prevPubKey instanceof DSAPublicKey)) {
throw new CertPathValidatorException("Input key is not " +
"of a appropriate type for inheriting parameters");
}
if (prevPubKey != null) {
// Inherit key parameters from previous key
if (PKIX.isDSAPublicKeyWithoutParams(currPubKey)) {
// Inherit DSA parameters from previous key
if (!(prevPubKey instanceof DSAPublicKey)) {
throw new CertPathValidatorException("Input key is not " +
"of a appropriate type for inheriting parameters");
}
DSAParams params = ((DSAPublicKey)prevPubKey).getParams();
if (params == null) {
throw new CertPathValidatorException(
"Key parameters missing from public key.");
}
DSAParams params = ((DSAPublicKey)prevPubKey).getParams();
if (params == null) {
throw new CertPathValidatorException(
"Key parameters missing from public key.");
}
try {
BigInteger y = ((DSAPublicKey)currPubKey).getY();
KeyFactory kf = KeyFactory.getInstance("DSA");
DSAPublicKeySpec ks = new DSAPublicKeySpec(y, params.getP(),
params.getQ(), params.getG());
currPubKey = kf.generatePublic(ks);
} catch (GeneralSecurityException e) {
throw new CertPathValidatorException("Unable to generate " +
"key with inherited parameters: " + e.getMessage(), e);
try {
BigInteger y = ((DSAPublicKey)currPubKey).getY();
KeyFactory kf = KeyFactory.getInstance("DSA");
DSAPublicKeySpec ks = new DSAPublicKeySpec(y, params.getP(),
params.getQ(), params.getG());
currPubKey = kf.generatePublic(ks);
} catch (GeneralSecurityException e) {
throw new CertPathValidatorException("Unable to generate " +
"key with inherited parameters: " +
e.getMessage(), e);
}
}
}
@ -349,6 +332,20 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
prevPubKey = currPubKey;
}
/**
* Sets the anchor, trustedPubKey and prevPubKey fields based on the
* specified trust anchor.
*/
private void setTrustAnchorAndKeys(TrustAnchor anchor) {
if (anchor.getTrustedCert() != null) {
this.trustedPubKey = anchor.getTrustedCert().getPublicKey();
} else {
this.trustedPubKey = anchor.getCAPublicKey();
}
this.anchor = anchor;
this.prevPubKey = this.trustedPubKey;
}
/**
* Try to set the trust anchor of the checker.
* <p>
@ -359,21 +356,9 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
* certificate
*/
void trySetTrustAnchor(TrustAnchor anchor) {
// Don't bother if the check has started or trust anchor has already
// specified.
if (prevPubKey == null) {
if (anchor == null) {
throw new IllegalArgumentException(
"The trust anchor cannot be null");
}
// Don't bother to change the trustedPubKey.
if (anchor.getTrustedCert() != null) {
prevPubKey = anchor.getTrustedCert().getPublicKey();
} else {
prevPubKey = anchor.getCAPublicKey();
}
this.anchor = anchor;
// Only set if trust anchor has not already been set.
if (this.trustedPubKey == null) {
setTrustAnchorAndKeys(anchor);
}
}
@ -412,9 +397,9 @@ public final class AlgorithmChecker extends PKIXCertPathChecker {
static void check(PublicKey key, AlgorithmId algorithmId, String variant,
TrustAnchor anchor) throws CertPathValidatorException {
certPathDefaultConstraints.permits(algorithmId.getName(),
algorithmId.getParameters(),
new CertPathConstraintsParameters(key, variant, anchor));
DisabledAlgorithmConstraints.certPathConstraints().permits(
algorithmId.getName(), algorithmId.getParameters(),
new CertPathConstraintsParameters(key, variant, anchor, null));
}
}

View file

@ -50,8 +50,8 @@ public class CertPathConstraintsParameters implements ConstraintsParameters {
private final Date date;
// The variant or usage of this certificate
private final String variant;
// The certificate being checked (may be null if a CRL or OCSPResponse is
// being checked)
// The certificate being checked (may be null if a raw public key, a CRL
// or an OCSPResponse is being checked)
private final X509Certificate cert;
public CertPathConstraintsParameters(X509Certificate cert,
@ -60,8 +60,8 @@ public class CertPathConstraintsParameters implements ConstraintsParameters {
}
public CertPathConstraintsParameters(Key key, String variant,
TrustAnchor anchor) {
this(key, variant, anchor, null, null);
TrustAnchor anchor, Date date) {
this(key, variant, anchor, date, null);
}
private CertPathConstraintsParameters(Key key, String variant,

View file

@ -176,8 +176,8 @@ public final class PKIXCertPathValidator extends CertPathValidatorSpi {
List<PKIXCertPathChecker> certPathCheckers = new ArrayList<>();
// add standard checkers that we will be using
certPathCheckers.add(untrustedChecker);
certPathCheckers.add(new AlgorithmChecker(anchor, null,
params.timestamp(), params.variant()));
certPathCheckers.add(new AlgorithmChecker(anchor, params.timestamp(),
params.variant()));
certPathCheckers.add(new KeyChecker(certPathLen,
params.targetCertConstraints()));
certPathCheckers.add(new ConstraintsChecker(certPathLen));

View file

@ -1,5 +1,5 @@
/*
* Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2015, 2021, 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
@ -26,6 +26,8 @@
package sun.security.util;
import java.util.HashSet;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
import java.util.Arrays;
import java.util.Collection;
@ -40,6 +42,14 @@ public class AlgorithmDecomposer {
private static final Pattern PATTERN =
Pattern.compile("with|and|(?<!padd)in", Pattern.CASE_INSENSITIVE);
// A map of standard message digest algorithm names to decomposed names
// so that a constraint can match for example, "SHA-1" and also
// "SHA1withRSA".
private static final Map<String, String> DECOMPOSED_DIGEST_NAMES =
Map.of("SHA-1", "SHA1", "SHA-224", "SHA224", "SHA-256", "SHA256",
"SHA-384", "SHA384", "SHA-512", "SHA512", "SHA-512/224",
"SHA512/224", "SHA-512/256", "SHA512/256");
private static Set<String> decomposeImpl(String algorithm) {
Set<String> elements = new HashSet<>();
@ -93,44 +103,19 @@ public class AlgorithmDecomposer {
// signature algorithm "SHA256withRSA". So we need to check both
// "SHA-256" and "SHA256" to make the right constraint checking.
// handle special name: SHA-1 and SHA1
if (elements.contains("SHA1") && !elements.contains("SHA-1")) {
elements.add("SHA-1");
}
if (elements.contains("SHA-1") && !elements.contains("SHA1")) {
elements.add("SHA1");
// no need to check further if algorithm doesn't contain "SHA"
if (!algorithm.contains("SHA")) {
return elements;
}
// handle special name: SHA-224 and SHA224
if (elements.contains("SHA224") && !elements.contains("SHA-224")) {
elements.add("SHA-224");
}
if (elements.contains("SHA-224") && !elements.contains("SHA224")) {
elements.add("SHA224");
}
// handle special name: SHA-256 and SHA256
if (elements.contains("SHA256") && !elements.contains("SHA-256")) {
elements.add("SHA-256");
}
if (elements.contains("SHA-256") && !elements.contains("SHA256")) {
elements.add("SHA256");
}
// handle special name: SHA-384 and SHA384
if (elements.contains("SHA384") && !elements.contains("SHA-384")) {
elements.add("SHA-384");
}
if (elements.contains("SHA-384") && !elements.contains("SHA384")) {
elements.add("SHA384");
}
// handle special name: SHA-512 and SHA512
if (elements.contains("SHA512") && !elements.contains("SHA-512")) {
elements.add("SHA-512");
}
if (elements.contains("SHA-512") && !elements.contains("SHA512")) {
elements.add("SHA512");
for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
if (elements.contains(e.getValue()) &&
!elements.contains(e.getKey())) {
elements.add(e.getKey());
} else if (elements.contains(e.getKey()) &&
!elements.contains(e.getValue())) {
elements.add(e.getValue());
}
}
return elements;
@ -153,40 +138,44 @@ public class AlgorithmDecomposer {
return Arrays.asList(aliases);
}
private static void hasLoop(Set<String> elements, String find, String replace) {
if (elements.contains(find)) {
if (!elements.contains(replace)) {
elements.add(replace);
}
elements.remove(find);
}
}
/*
* This decomposes a standard name into sub-elements with a consistent
* message digest algorithm name to avoid overly complicated checking.
/**
* Decomposes a standard algorithm name into sub-elements and uses a
* consistent message digest algorithm name to avoid overly complicated
* checking.
*/
public static Set<String> decomposeOneHash(String algorithm) {
static Set<String> decomposeName(String algorithm) {
if (algorithm == null || algorithm.isEmpty()) {
return new HashSet<>();
}
Set<String> elements = decomposeImpl(algorithm);
hasLoop(elements, "SHA-1", "SHA1");
hasLoop(elements, "SHA-224", "SHA224");
hasLoop(elements, "SHA-256", "SHA256");
hasLoop(elements, "SHA-384", "SHA384");
hasLoop(elements, "SHA-512", "SHA512");
// no need to check further if algorithm doesn't contain "SHA"
if (!algorithm.contains("SHA")) {
return elements;
}
for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
if (elements.contains(e.getKey())) {
if (!elements.contains(e.getValue())) {
elements.add(e.getValue());
}
elements.remove(e.getKey());
}
}
return elements;
}
/*
* The provided message digest algorithm name will return a consistent
* naming scheme.
/**
* Decomposes a standard message digest algorithm name into a consistent
* name for matching purposes.
*
* @param algorithm the name to be decomposed
* @return the decomposed name, or the passed in algorithm name if
* it is not a digest algorithm or does not need to be decomposed
*/
public static String hashName(String algorithm) {
return algorithm.replace("-", "");
static String decomposeDigestName(String algorithm) {
return DECOMPOSED_DIGEST_NAMES.getOrDefault(algorithm, algorithm);
}
}

View file

@ -304,10 +304,6 @@ public class DisabledAlgorithmConstraints extends AbstractAlgorithmConstraints {
/**
* Key and Certificate Constraints
*
* The complete disabling of an algorithm is not handled by Constraints or
* Constraint classes. That is addressed with
* permit(Set<CryptoPrimitive>, String, AlgorithmParameters)
*
* When passing a Key to permit(), the boolean return values follow the
* same as the interface class AlgorithmConstraints.permit(). This is to
* maintain compatibility:
@ -318,7 +314,6 @@ public class DisabledAlgorithmConstraints extends AbstractAlgorithmConstraints {
* will be thrown on a failure to better identify why the operation was
* disallowed.
*/
private static class Constraints {
private Map<String, List<Constraint>> constraintsMap = new HashMap<>();
@ -341,9 +336,9 @@ public class DisabledAlgorithmConstraints extends AbstractAlgorithmConstraints {
// Check if constraint is a complete disabling of an
// algorithm or has conditions.
int space = constraintEntry.indexOf(' ');
String algorithm = AlgorithmDecomposer.hashName(
((space > 0 ? constraintEntry.substring(0, space) :
constraintEntry)));
String algorithm = AlgorithmDecomposer.decomposeDigestName(
space > 0 ? constraintEntry.substring(0, space) :
constraintEntry);
List<Constraint> constraintList =
constraintsMap.getOrDefault(
algorithm.toUpperCase(Locale.ENGLISH),
@ -497,7 +492,7 @@ public class DisabledAlgorithmConstraints extends AbstractAlgorithmConstraints {
// Get all signature algorithms to check for constraints
Set<String> algorithms = new HashSet<>();
if (algorithm != null) {
algorithms.addAll(AlgorithmDecomposer.decomposeOneHash(algorithm));
algorithms.addAll(AlgorithmDecomposer.decomposeName(algorithm));
algorithms.add(algorithm);
}