Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
8275887: jarsigner prints invalid digest/signature algorithm warnings… 
… if keysize is weak/disabled

Reviewed-by: phh, andrew
Backport-of: 03f8c0fb9363dc1bb07bed1ae0359c029caa0130
  • Loading branch information
Alexey Bakhtin committed Dec 1, 2022
1 parent 7297bdf commit 53620b3
Show file tree
Hide file tree
Showing 9 changed files with 95 additions and 83 deletions.
37 changes: 26 additions & 11 deletions jdk/src/share/classes/sun/security/pkcs/SignerInfo.java
View file
Expand Up @@ -84,10 +84,21 @@ public class SignerInfo implements DerEncoder {
/**
* A map containing the algorithms in this SignerInfo. This is used to
* avoid checking algorithms to see if they are disabled more than once.
* The key is the AlgorithmId of the algorithm, and the value is the name of
* the field or attribute.
* The key is the AlgorithmId of the algorithm, and the value is a record
* containing the name of the field or attribute and whether the key
* should also be checked (ex: if it is a signature algorithm).
*/
private Map<AlgorithmId, String> algorithms = new HashMap<>();
private class AlgorithmInfo {
String field;
boolean checkKey;
private AlgorithmInfo(String f, boolean cK) {
field = f;
checkKey = cK;
}
String field() { return field; }
boolean checkKey() { return checkKey; }
}
private Map<AlgorithmId, AlgorithmInfo> algorithms = new HashMap<>();

public SignerInfo(X500Name issuerName,
BigInteger serial,
Expand Down Expand Up @@ -323,7 +334,8 @@ SignerInfo verify(PKCS7 block, byte[] data)
}

String digestAlgName = digestAlgorithmId.getName();
algorithms.put(digestAlgorithmId, "SignerInfo digestAlgorithm field");
algorithms.put(digestAlgorithmId,
new AlgorithmInfo("SignerInfo digestAlgorithm field", false));

byte[] dataSigned;

Expand Down Expand Up @@ -382,7 +394,8 @@ SignerInfo verify(PKCS7 block, byte[] data)
new AlgorithmId(oid,
digestEncryptionAlgorithmId.getParameters());
algorithms.put(sigAlgId,
"SignerInfo digestEncryptionAlgorithm field");
new AlgorithmInfo(
"SignerInfo digestEncryptionAlgorithm field", true));
} catch (NoSuchAlgorithmException ignore) {
}

Expand Down Expand Up @@ -569,7 +582,8 @@ private void verifyTimestamp(TimestampToken token)
throws NoSuchAlgorithmException, SignatureException {

AlgorithmId digestAlgId = token.getHashAlgorithm();
algorithms.put(digestAlgId, "TimestampToken digestAlgorithm field");
algorithms.put(digestAlgId,
new AlgorithmInfo("TimestampToken digestAlgorithm field", false));

MessageDigest md = MessageDigest.getInstance(digestAlgId.getName());

Expand Down Expand Up @@ -626,18 +640,19 @@ public String toString() {
*/
public static Set<String> verifyAlgorithms(SignerInfo[] infos,
JarConstraintsParameters params, String name) throws SignatureException {
Map<AlgorithmId, String> algorithms = new HashMap<>();
Map<AlgorithmId, AlgorithmInfo> algorithms = new HashMap<>();
for (SignerInfo info : infos) {
algorithms.putAll(info.algorithms);
}

Set<String> enabledAlgorithms = new HashSet<>();
try {
for (Map.Entry<AlgorithmId, String> algorithm : algorithms.entrySet()) {
params.setExtendedExceptionMsg(name, algorithm.getValue());
AlgorithmId algId = algorithm.getKey();
for (Map.Entry<AlgorithmId,AlgorithmInfo> algEntry : algorithms.entrySet()) {
AlgorithmInfo info = algEntry.getValue();
params.setExtendedExceptionMsg(name, info.field());
AlgorithmId algId = algEntry.getKey();
JAR_DISABLED_CHECK.permits(algId.getName(),
algId.getParameters(), params);
algId.getParameters(), params, info.checkKey());
enabledAlgorithms.add(algId.getName());
}
} catch (CertPathValidatorException e) {
Expand Down
31 changes: 3 additions & 28 deletions jdk/src/share/classes/sun/security/provider/certpath/AlgorithmChecker.java
View file
Expand Up @@ -38,7 +38,6 @@
import java.security.AlgorithmParameters;
import java.security.GeneralSecurityException;
import java.security.cert.Certificate;
import java.security.cert.X509CRL;
import java.security.cert.X509Certificate;
import java.security.cert.PKIXCertPathChecker;
import java.security.cert.TrustAnchor;
Expand All @@ -57,7 +56,6 @@
import sun.security.validator.Validator;
import sun.security.x509.AlgorithmId;
import sun.security.x509.X509CertImpl;
import sun.security.x509.X509CRLImpl;

/**
* A {@code PKIXCertPathChecker} implementation to check whether a
Expand Down Expand Up @@ -285,13 +283,13 @@ public void check(Certificate cert,
// Check against local constraints if it is DisabledAlgorithmConstraints
if (constraints instanceof DisabledAlgorithmConstraints) {
((DisabledAlgorithmConstraints)constraints).permits(currSigAlg,
currSigAlgParams, cp);
currSigAlgParams, cp, true);
// DisabledAlgorithmsConstraints does not check primitives, so key
// additional key check.

} else {
// Perform the default constraints checking anyway.
certPathDefaultConstraints.permits(currSigAlg, currSigAlgParams, cp);
certPathDefaultConstraints.permits(currSigAlg, currSigAlgParams, cp, true);
// Call locally set constraints to check key with primitives.
if (!constraints.permits(primitives, currPubKey)) {
throw new CertPathValidatorException(
Expand Down Expand Up @@ -377,29 +375,6 @@ void trySetTrustAnchor(TrustAnchor anchor) {
}
}

/**
* Check the signature algorithm with the specified public key.
*
* @param key the public key to verify the CRL signature
* @param crl the target CRL
* @param variant the Validator variant of the operation. A null value
* passed will set it to Validator.GENERIC.
* @param anchor the trust anchor selected to validate the CRL issuer
*/
static void check(PublicKey key, X509CRL crl, String variant,
TrustAnchor anchor) throws CertPathValidatorException {

X509CRLImpl x509CRLImpl = null;
try {
x509CRLImpl = X509CRLImpl.toImpl(crl);
} catch (CRLException ce) {
throw new CertPathValidatorException(ce);
}

AlgorithmId algorithmId = x509CRLImpl.getSigAlgId();
check(key, algorithmId, variant, anchor);
}

/**
* Check the signature algorithm with the specified public key.
*
Expand All @@ -414,7 +389,7 @@ static void check(PublicKey key, AlgorithmId algorithmId, String variant,

certPathDefaultConstraints.permits(algorithmId.getName(),
algorithmId.getParameters(),
new CertPathConstraintsParameters(key, variant, anchor));
new CertPathConstraintsParameters(key, variant, anchor), true);
}
}

3 changes: 2 additions & 1 deletion jdk/src/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java
View file
Expand Up @@ -689,7 +689,8 @@ static boolean verifyCRL(X509CertImpl certImpl, DistributionPoint point,

// check the crl signature algorithm
try {
AlgorithmChecker.check(prevKey, crl, variant, anchor);
AlgorithmChecker.check(prevKey, crlImpl.getSigAlgId(),
variant, anchor);
} catch (CertPathValidatorException cpve) {
if (debug != null) {
debug.println("CRL signature algorithm check failed: " + cpve);
Expand Down
31 changes: 18 additions & 13 deletions jdk/src/share/classes/sun/security/tools/jarsigner/Main.java
View file
Expand Up @@ -904,9 +904,14 @@ void verifyJar(String jarName)
Calendar c = Calendar.getInstance(
TimeZone.getTimeZone("UTC"),
Locale.getDefault(Locale.Category.FORMAT));
c.setTime(tsTokenInfo.getDate());
Date tsDate = tsTokenInfo.getDate();
c.setTime(tsDate);
JarConstraintsParameters jcp =
new JarConstraintsParameters(chain, si.getTimestamp());
new JarConstraintsParameters(chain, tsDate);
JarConstraintsParameters jcpts =
new JarConstraintsParameters(
tsSi.getCertificateChain(tsToken),
tsDate);
history = String.format(
rb.getString("history.with.ts"),
signer.getSubjectX500Principal(),
Expand All @@ -915,9 +920,9 @@ void verifyJar(String jarName)
verifyWithWeak(key, jcp),
c,
tsSigner.getSubjectX500Principal(),
verifyWithWeak(tsDigestAlg, DIGEST_PRIMITIVE_SET, true, jcp),
verifyWithWeak(tsSigAlg, SIG_PRIMITIVE_SET, true, jcp),
verifyWithWeak(tsKey, jcp));
verifyWithWeak(tsDigestAlg, DIGEST_PRIMITIVE_SET, true, jcpts),
verifyWithWeak(tsSigAlg, SIG_PRIMITIVE_SET, true, jcpts),
verifyWithWeak(tsKey, jcpts));
} else {
JarConstraintsParameters jcp =
new JarConstraintsParameters(chain, null);
Expand Down Expand Up @@ -1267,13 +1272,13 @@ private String verifyWithWeak(String alg, Set<CryptoPrimitive> primitiveSet,
boolean tsa, JarConstraintsParameters jcp) {

try {
DISABLED_CHECK.permits(alg, jcp);
DISABLED_CHECK.permits(alg, jcp, false);
} catch (CertPathValidatorException e) {
disabledAlgFound = true;
return String.format(rb.getString("with.disabled"), alg);
}
try {
LEGACY_CHECK.permits(alg, jcp);
LEGACY_CHECK.permits(alg, jcp, false);
return alg;
} catch (CertPathValidatorException e) {
if (primitiveSet == SIG_PRIMITIVE_SET) {
Expand All @@ -1295,13 +1300,13 @@ private String verifyWithWeak(String alg, Set<CryptoPrimitive> primitiveSet,
private String verifyWithWeak(PublicKey key, JarConstraintsParameters jcp) {
int kLen = KeyUtil.getKeySize(key);
try {
DISABLED_CHECK.permits(key.getAlgorithm(), jcp);
DISABLED_CHECK.permits(key.getAlgorithm(), jcp, true);
} catch (CertPathValidatorException e) {
disabledAlgFound = true;
return String.format(rb.getString("key.bit.disabled"), kLen);
}
try {
LEGACY_CHECK.permits(key.getAlgorithm(), jcp);
LEGACY_CHECK.permits(key.getAlgorithm(), jcp, true);
if (kLen >= 0) {
return String.format(rb.getString("key.bit"), kLen);
} else {
Expand All @@ -1318,9 +1323,9 @@ private void checkWeakSign(String alg, Set<CryptoPrimitive> primitiveSet,
boolean tsa, JarConstraintsParameters jcp) {

try {
DISABLED_CHECK.permits(alg, jcp);
DISABLED_CHECK.permits(alg, jcp, false);
try {
LEGACY_CHECK.permits(alg, jcp);
LEGACY_CHECK.permits(alg, jcp, false);
} catch (CertPathValidatorException e) {
if (primitiveSet == SIG_PRIMITIVE_SET) {
legacyAlg |= 2;
Expand All @@ -1347,9 +1352,9 @@ private void checkWeakSign(String alg, Set<CryptoPrimitive> primitiveSet,

private void checkWeakSign(PrivateKey key, JarConstraintsParameters jcp) {
try {
DISABLED_CHECK.permits(key.getAlgorithm(), jcp);
DISABLED_CHECK.permits(key.getAlgorithm(), jcp, true);
try {
LEGACY_CHECK.permits(key.getAlgorithm(), jcp);
LEGACY_CHECK.permits(key.getAlgorithm(), jcp, true);
} catch (CertPathValidatorException e) {
legacyAlg |= 8;
}
Expand Down
43 changes: 25 additions & 18 deletions jdk/src/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
View file
Expand Up @@ -192,16 +192,16 @@ public final boolean permits(Set<CryptoPrimitive> primitives,
}

public final void permits(String algorithm, AlgorithmParameters ap,
ConstraintsParameters cp) throws CertPathValidatorException {

permits(algorithm, cp);
ConstraintsParameters cp, boolean checkKey)
throws CertPathValidatorException {
permits(algorithm, cp, checkKey);
if (ap != null) {
permits(ap, cp);
}
}

private void permits(AlgorithmParameters ap, ConstraintsParameters cp)
throws CertPathValidatorException {
throws CertPathValidatorException {

switch (ap.getAlgorithm().toUpperCase(Locale.ENGLISH)) {
case "RSASSA-PSS":
Expand All @@ -219,36 +219,38 @@ private void permitsPSSParams(AlgorithmParameters ap,
PSSParameterSpec pssParams =
ap.getParameterSpec(PSSParameterSpec.class);
String digestAlg = pssParams.getDigestAlgorithm();
permits(digestAlg, cp);
permits(digestAlg, cp, false);
AlgorithmParameterSpec mgfParams = pssParams.getMGFParameters();
if (mgfParams instanceof MGF1ParameterSpec) {
String mgfDigestAlg =
((MGF1ParameterSpec)mgfParams).getDigestAlgorithm();
if (!mgfDigestAlg.equalsIgnoreCase(digestAlg)) {
permits(mgfDigestAlg, cp);
permits(mgfDigestAlg, cp, false);
}
}
} catch (InvalidParameterSpecException ipse) {
// ignore
}
}

public final void permits(String algorithm, ConstraintsParameters cp)
throws CertPathValidatorException {
public final void permits(String algorithm, ConstraintsParameters cp,
boolean checkKey) throws CertPathValidatorException {

// Check if named curves in the key are disabled.
for (Key key : cp.getKeys()) {
for (String curve : getNamedCurveFromKey(key)) {
if (!checkAlgorithm(disabledAlgorithms, curve, decomposer)) {
throw new CertPathValidatorException(
if (checkKey) {
// Check if named curves in the key are disabled.
for (Key key : cp.getKeys()) {
for (String curve : getNamedCurveFromKey(key)) {
if (!checkAlgorithm(disabledAlgorithms, curve, decomposer)) {
throw new CertPathValidatorException(
"Algorithm constraints check failed on disabled " +
"algorithm: " + curve,
null, null, -1, BasicReason.ALGORITHM_CONSTRAINED);
}
}
}
}

algorithmConstraints.permits(algorithm, cp);
algorithmConstraints.permits(algorithm, cp, checkKey);
}

private static List<String> getNamedCurveFromKey(Key key) {
Expand Down Expand Up @@ -479,8 +481,8 @@ public boolean permits(String algorithm, AlgorithmParameters aps) {
return true;
}

public void permits(String algorithm, ConstraintsParameters cp)
throws CertPathValidatorException {
public void permits(String algorithm, ConstraintsParameters cp,
boolean checkKey) throws CertPathValidatorException {

if (debug != null) {
debug.println("Constraints.permits(): " + algorithm + ", "
Expand All @@ -494,8 +496,10 @@ public void permits(String algorithm, ConstraintsParameters cp)
algorithms.add(algorithm);
}

for (Key key : cp.getKeys()) {
algorithms.add(key.getAlgorithm());
if (checkKey) {
for (Key key : cp.getKeys()) {
algorithms.add(key.getAlgorithm());
}
}

// Check all applicable constraints
Expand All @@ -505,6 +509,9 @@ public void permits(String algorithm, ConstraintsParameters cp)
continue;
}
for (Constraint constraint : list) {
if (!checkKey && constraint instanceof KeySizeConstraint) {
continue;
}
constraint.permits(cp);
}
}
Expand Down
11 changes: 3 additions & 8 deletions jdk/src/share/classes/sun/security/util/JarConstraintsParameters.java
View file
Expand Up @@ -98,16 +98,11 @@ public JarConstraintsParameters(CodeSigner[] signers) {
this.timestamp = latestTimestamp;
}

public JarConstraintsParameters(List<X509Certificate> chain, Timestamp timestamp) {
public JarConstraintsParameters(List<X509Certificate> chain, Date timestamp) {
this.keys = new HashSet<>();
this.certsIssuedByAnchor = new HashSet<>();
addToCertsAndKeys(chain);
if (timestamp != null) {
addToCertsAndKeys(timestamp.getSignerCertPath());
this.timestamp = timestamp.getTimestamp();
} else {
this.timestamp = null;
}
this.timestamp = timestamp;
}

// extract last certificate and signer's public key from chain
Expand Down Expand Up @@ -178,7 +173,7 @@ public void setExtendedExceptionMsg(String file, String target) {

@Override
public String extendedExceptionMsg() {
return message;
return message == null ? "." : message;
}

@Override
Expand Down
2 changes: 1 addition & 1 deletion jdk/src/share/classes/sun/security/util/ManifestEntryVerifier.java
View file
Expand Up @@ -217,7 +217,7 @@ public CodeSigner[] verify(Hashtable<String, CodeSigner[]> verifiedSigners,
params.setExtendedExceptionMsg(JarFile.MANIFEST_NAME,
name + " entry");
DisabledAlgorithmConstraints.jarConstraints()
.permits(digest.getAlgorithm(), params);
.permits(digest.getAlgorithm(), params, false);
} catch (GeneralSecurityException e) {
if (debug != null) {
debug.println("Digest algorithm is restricted: " + e);
Expand Down
2 changes: 1 addition & 1 deletion jdk/src/share/classes/sun/security/util/SignatureFileVerifier.java
View file
Expand Up @@ -360,7 +360,7 @@ private boolean permittedCheck(String key, String algorithm) {
try {
params.setExtendedExceptionMsg(name + ".SF", key + " attribute");
DisabledAlgorithmConstraints
.jarConstraints().permits(algorithm, params);
.jarConstraints().permits(algorithm, params, false);
} catch (GeneralSecurityException e) {
permittedAlgs.put(algorithm, Boolean.FALSE);
permittedAlgs.put(key.toUpperCase(), Boolean.FALSE);
Expand Down

1 comment on commit 53620b3

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

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

Review

Issues

Please sign in to comment.