Skip to content

Commit

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

Reviewed-by: mdoerr
Backport-of: 03f8c0fb9363dc1bb07bed1ae0359c029caa0130
  • Loading branch information
@GoeLin
GoeLin committed Sep 16, 2022
1 parent 6f2bbc4 commit 6bc6980
Show file tree
Hide file tree
Showing 9 changed files with 90 additions and 87 deletions.
28 changes: 17 additions & 11 deletions src/java.base/share/classes/sun/security/pkcs/SignerInfo.java
View file
Expand Up @@ -76,10 +76,12 @@ 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 record AlgorithmInfo(String field, boolean checkKey) {}
private Map<AlgorithmId, AlgorithmInfo> algorithms = new HashMap<>();

public SignerInfo(X500Name issuerName,
BigInteger serial,
Expand Down Expand Up @@ -350,7 +352,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 @@ -428,7 +431,8 @@ SignerInfo verify(PKCS7 block, byte[] data)
new AlgorithmId(ObjectIdentifier.of(oid),
digestEncryptionAlgorithmId.getParameters());
algorithms.put(sigAlgId,
"SignerInfo digestEncryptionAlgorithm field");
new AlgorithmInfo(
"SignerInfo digestEncryptionAlgorithm field", true));
}

X509Certificate cert = getCertificate(block);
Expand Down Expand Up @@ -685,7 +689,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 @@ -742,18 +747,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 (var 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 src/java.base/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 src/java.base/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java
View file
Expand Up @@ -691,7 +691,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
43 changes: 25 additions & 18 deletions src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
View file
Expand Up @@ -199,16 +199,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 @@ -226,36 +226,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 (!cachedCheckAlgorithm(curve)) {
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 (!cachedCheckAlgorithm(curve)) {
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 @@ -493,8 +495,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 @@ -508,8 +510,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 @@ -519,6 +523,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 src/java.base/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 src/java.base/share/classes/sun/security/util/ManifestEntryVerifier.java
View file
Expand Up @@ -295,7 +295,7 @@ private boolean checkConstraints(String algorithm,
params.setExtendedExceptionMsg(JarFile.MANIFEST_NAME,
name + " entry");
DisabledAlgorithmConstraints.jarConstraints()
.permits(algorithm, params);
.permits(algorithm, params, false);
return true;
} catch (GeneralSecurityException e) {
if (debug != null) {
Expand Down
2 changes: 1 addition & 1 deletion src/java.base/share/classes/sun/security/util/SignatureFileVerifier.java
View file
Expand Up @@ -383,7 +383,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

0 comments on commit 6bc6980

Please sign in to comment.