Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 6 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2cf52b3
8243585: AlgorithmChecker::check throws confusing exception when it r…
seanjmullan Oct 13, 2021
2704594
Remove trailing whitespace.
seanjmullan Oct 13, 2021
cf5a4d7
- Changed names of AlgorithmDecomposer.canonicalName and decomposeOne…
seanjmullan Oct 19, 2021
f203112
- Skip digest alg decomposing check for algorithms that don't contain…
seanjmullan Oct 20, 2021
052c28f
Address Max' comments.
seanjmullan Oct 21, 2021
d163b5b
Add setTrustAnchorAndKeys method and change ctor and trySetTrustAncho…
seanjmullan Oct 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
242 changes: 114 additions & 128 deletions src/java.base/share/classes/sun/security/provider/certpath/AlgorithmChecker.java
View file
Open in desktop
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can group fields to final and non-final ones.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

private PublicKey trustedPubKey;
private PublicKey prevPubKey;
private TrustAnchor anchor;

private static final Set<CryptoPrimitive> SIGNATURE_PRIMITIVE_SET =
Expand All @@ -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.
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -152,8 +148,8 @@ public AlgorithmChecker(TrustAnchor 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);
}
Expand All @@ -172,18 +168,14 @@ public AlgorithmChecker(TrustAnchor anchor,
* 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the init() method below, just write prevPubKey = trustedPubKey no matter if trustedPubKey is null or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

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");
Expand All @@ -207,8 +199,8 @@ public void check(Certificate cert,
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;
}

Expand All @@ -233,115 +225,114 @@ public void check(Certificate cert,
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);
}

// Assume all key usage bits are set if key usage is not present
Set<CryptoPrimitive> primitives = KU_PRIMITIVE_SET;

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[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 (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);
}
}

ConstraintsParameters cp =
new CertPathConstraintsParameters(x509Cert, variant,
// Check the signature algorithm and parameters against constraints
CertPathConstraintsParameters 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.

dac.permits(currSigAlg, currSigAlgParams, cp);
} 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",
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;

// 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,
null, null, -1, BasicReason.ALGORITHM_CONSTRAINED);
}
if (keyUsage != null) {
primitives = EnumSet.noneOf(CryptoPrimitive.class);

// 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 (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);
}
}

DSAParams params = ((DSAPublicKey)prevPubKey).getParams();
if (params == null) {
if (!constraints.permits(primitives, currPubKey)) {
throw new CertPathValidatorException(
"Key parameters missing from public key.");
"Algorithm constraints check failed on " +
currPubKey.getAlgorithm() + " key with size of " +
sun.security.util.KeyUtil.getKeySize(currPubKey) +
"bits",
null, null, -1, BasicReason.ALGORITHM_CONSTRAINED);
}
}

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);
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.");
}

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);
}
}
}

Expand All @@ -360,19 +351,14 @@ public void check(Certificate cert,
*/
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.
// been specified.
if (this.trustedPubKey == null) {
if (anchor.getTrustedCert() != null) {
prevPubKey = anchor.getTrustedCert().getPublicKey();
this.trustedPubKey = anchor.getTrustedCert().getPublicKey();
} else {
prevPubKey = anchor.getCAPublicKey();
this.trustedPubKey = anchor.getCAPublicKey();
}
this.prevPubKey = this.trustedPubKey;
this.anchor = anchor;
}
}
Expand Down Expand Up @@ -412,9 +398,9 @@ static void check(PublicKey key, X509CRL crl, String variant,
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));
}
}

8 changes: 4 additions & 4 deletions ...java.base/share/classes/sun/security/provider/certpath/CertPathConstraintsParameters.java
View file
Open in desktop
Expand Up @@ -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,
Expand All @@ -60,8 +60,8 @@ public CertPathConstraintsParameters(X509Certificate cert,
}

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,
Expand Down
4 changes: 2 additions & 2 deletions src/java.base/share/classes/sun/security/provider/certpath/PKIXCertPathValidator.java
View file
Open in desktop
Expand Up @@ -176,8 +176,8 @@ private static PKIXCertPathValidatorResult validate(TrustAnchor anchor,
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));
Expand Down