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.

Sign up for GitHub

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

8277474: jarsigner does not check if algorithm parameters are disabled #7582

Closed
wants to merge 7 commits into from
Closed

8277474: jarsigner does not check if algorithm parameters are disabled #7582

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
70da21b
8277474: jarsigner does not check if algorithm parameters are disabled
haimaychao Feb 22, 2022
9ffda80
Removed unneeded import and updated -verbose output
haimaychao Mar 2, 2022
224bf18
Updated -verbose output
haimaychao Mar 3, 2022
516d8bf
Updated -verbose output
haimaychao Mar 3, 2022
2a73d1e
Removed unused string
haimaychao Mar 3, 2022
d2cd7ef
Use algname in output
haimaychao Mar 3, 2022
74dc704
No need to do toUpperCase
haimaychao Mar 3, 2022
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
2 changes: 1 addition & 1 deletion src/java.base/share/classes/sun/security/util/DisabledAlgorithmConstraints.java
View file
Open in desktop
Expand Up @@ -202,7 +202,7 @@ public final void permits(String algorithm, AlgorithmParameters ap,
}
}

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

switch (ap.getAlgorithm().toUpperCase(Locale.ENGLISH)) {
Expand Down
40 changes: 32 additions & 8 deletions src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
View file
Open in desktop
Expand Up @@ -30,6 +30,7 @@
import java.net.URLClassLoader;
import java.security.cert.CertPathValidatorException;
import java.security.cert.PKIXBuilderParameters;
import java.security.spec.PSSParameterSpec;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this import, as this class does not seem to be referenced anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Thanks for the review.

import java.util.*;
import java.util.stream.Collectors;
import java.util.zip.*;
Expand Down Expand Up @@ -1021,6 +1022,8 @@ void verifyJar(String jarName)
si.getDigestAlgorithmId(),
si.getDigestEncryptionAlgorithmId(),
si.getAuthenticatedAttributes() == null);
AlgorithmId encAlgId = si.getDigestEncryptionAlgorithmId();
AlgorithmParameters sigAlgParams = encAlgId.getParameters();
PublicKey key = signer.getPublicKey();
PKCS7 tsToken = si.getTsToken();
if (tsToken != null) {
Expand All @@ -1035,6 +1038,8 @@ void verifyJar(String jarName)
tsSi.getDigestAlgorithmId(),
tsSi.getDigestEncryptionAlgorithmId(),
tsSi.getAuthenticatedAttributes() == null);
AlgorithmId tsEncAlgId = tsSi.getDigestEncryptionAlgorithmId();
AlgorithmParameters tsSigAlgParams = tsEncAlgId.getParameters();
Calendar c = Calendar.getInstance(
TimeZone.getTimeZone("UTC"),
Locale.getDefault(Locale.Category.FORMAT));
Expand All @@ -1049,22 +1054,22 @@ void verifyJar(String jarName)
history = String.format(
rb.getString("history.with.ts"),
signer.getSubjectX500Principal(),
verifyWithWeak(digestAlg, DIGEST_PRIMITIVE_SET, false, jcp),
verifyWithWeak(sigAlg, SIG_PRIMITIVE_SET, false, jcp),
verifyWithWeak(digestAlg, DIGEST_PRIMITIVE_SET, false, jcp, null),
verifyWithWeak(sigAlg, SIG_PRIMITIVE_SET, false, jcp, sigAlgParams),
verifyWithWeak(key, jcp),
c,
tsSigner.getSubjectX500Principal(),
verifyWithWeak(tsDigestAlg, DIGEST_PRIMITIVE_SET, true, jcpts),
verifyWithWeak(tsSigAlg, SIG_PRIMITIVE_SET, true, jcpts),
verifyWithWeak(tsDigestAlg, DIGEST_PRIMITIVE_SET, true, jcpts, null),
verifyWithWeak(tsSigAlg, SIG_PRIMITIVE_SET, true, jcpts, tsSigAlgParams),
verifyWithWeak(tsKey, jcpts));
} else {
JarConstraintsParameters jcp =
new JarConstraintsParameters(chain, null);
history = String.format(
rb.getString("history.without.ts"),
signer.getSubjectX500Principal(),
verifyWithWeak(digestAlg, DIGEST_PRIMITIVE_SET, false, jcp),
verifyWithWeak(sigAlg, SIG_PRIMITIVE_SET, false, jcp),
verifyWithWeak(digestAlg, DIGEST_PRIMITIVE_SET, false, jcp, null),
verifyWithWeak(sigAlg, SIG_PRIMITIVE_SET, false, jcp, sigAlgParams),
verifyWithWeak(key, jcp));
}
} catch (Exception e) {
Expand Down Expand Up @@ -1393,17 +1398,25 @@ private void displayMessagesAndResult(boolean isSigning) {
}

private String verifyWithWeak(String alg, Set<CryptoPrimitive> primitiveSet,
boolean tsa, JarConstraintsParameters jcp) {
boolean tsa, JarConstraintsParameters jcp, AlgorithmParameters algParams) {

try {
JAR_DISABLED_CHECK.permits(alg, jcp, false);
} catch (CertPathValidatorException e) {
disabledAlgFound = true;
return String.format(rb.getString("with.disabled"), alg);
}
if (algParams != null) {
try {
JAR_DISABLED_CHECK.permits(algParams, jcp);
} catch (CertPathValidatorException e) {
disabledAlgFound = true;
return String.format(rb.getString("with.disabled"), algParams);
Copy link
Contributor

@wangweij wangweij Mar 2, 2022 •
edited

Choose a reason for hiding this comment

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

The return value of this method will be shown as the "Signature algorithm" in the output. It's OK to include an optional "weak" (or "disabled") tag, but the core part still must be an algorithm name. Here, the updated code returns the string format of algParams, which is not an algorithm name.

I'm not sure how to fix this nicely. Certainly you want to show the user why it is weak so the weak part should be displayed. A verbose solution could be "RSASSA-PSS using PSSParameterSpec(...SHA-1...) (weak)", but the toString() output of PSSParameterSpec is quite long.

Same comment to the code change below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add "RSSSSA-PSS using “ to the -verbose output as suggested, and keep the remaining output as the parameters for the RSASSA-PSS contain hashAlgorithm and maskGenAlgorithm that could be disabled or weak. At the same time, strip off the saltLength and trailerField display.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does it look like now? Also, you might need to create a mapping in Resources.java because "using" should only be shown when system language is English.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, what if it's another algorithm using another type of parameters? You cannot hardcode "RSASSA-PSS" and take it for granted that there is a "]" inside the string format of the parameter and it's the end of the weak part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made change to add "RSASSA-PSS using” before its parameter output when the signature algorithm is RSASSA-PSS. Also, keep the parameter string without doing further parsing and stripping off based on the "]".

}
}

try {
LEGACY_CHECK.permits(alg, jcp, false);
return alg;
} catch (CertPathValidatorException e) {
if (primitiveSet == SIG_PRIMITIVE_SET) {
legacyAlg |= 2;
Expand All @@ -1419,6 +1432,17 @@ private String verifyWithWeak(String alg, Set<CryptoPrimitive> primitiveSet,
}
return String.format(rb.getString("with.weak"), alg);
}
if (algParams != null) {
try {
LEGACY_CHECK.permits(algParams, jcp);
return alg;
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to return here since it will be returned on line 1445 anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed.

} catch (CertPathValidatorException e) {
legacyAlg |= 2;
legacySigAlg = alg;
return String.format(rb.getString("with.weak"), algParams);
}
}
return alg;
}

private String verifyWithWeak(PublicKey key, JarConstraintsParameters jcp) {
Expand Down
86 changes: 86 additions & 0 deletions test/jdk/sun/security/tools/jarsigner/CheckAlgParams.java
View file
Open in desktop
@@ -0,0 +1,86 @@
/*
* Copyright (c) 2022, 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
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test
* @bug 8277474
* @summary jarsigner -verify should check if the algorithm parameters of
* its signature algorithm use disabled or legacy algorithms
* @library /test/lib
* @modules java.base/sun.security.x509
Copy link
Contributor

@wangweij wangweij Mar 2, 2022 •
edited

Choose a reason for hiding this comment

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

Is this @modules line useful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, removed it. Thanks for the review!

*/

import jdk.test.lib.SecurityTools;
import jdk.test.lib.process.OutputAnalyzer;
import jdk.test.lib.util.JarUtils;

import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;

public class CheckAlgParams {
private static final String JAVA_SECURITY_FILE = "java.security";

public static void main(String[] args) throws Exception{

SecurityTools.keytool("-keystore ks -storepass changeit " +
"-genkeypair -keyalg RSASSA-PSS -alias ca -dname CN=CA " +
"-ext bc:c")
.shouldHaveExitValue(0);

JarUtils.createJarFile(Path.of("a.jar"), Path.of("."), Path.of("ks"));

SecurityTools.jarsigner("-keystore ks -storepass changeit " +
"-signedjar signeda.jar " +
"-verbose" +
" a.jar ca")
.shouldHaveExitValue(0);

Files.writeString(Files.createFile(Paths.get(JAVA_SECURITY_FILE)),
"jdk.jar.disabledAlgorithms=SHA256\n" +
"jdk.security.legacyAlgorithms=\n");

SecurityTools.jarsigner("-verify signeda.jar " +
"-J-Djava.security.properties=" +
JAVA_SECURITY_FILE +
" -keystore ks -storepass changeit -verbose -debug")
.shouldMatch("Digest algorithm: SHA-256.*(disabled)")
.shouldMatch("Signature algorithm: PSSParameterSpec.*hashAlgorithm=SHA-256.*(disabled)")
.shouldContain("The jar will be treated as unsigned")
.shouldHaveExitValue(0);

Files.deleteIfExists(Paths.get(JAVA_SECURITY_FILE));
Files.writeString(Files.createFile(Paths.get(JAVA_SECURITY_FILE)),
"jdk.jar.disabledAlgorithms=\n" +
"jdk.security.legacyAlgorithms=SHA256\n");

SecurityTools.jarsigner("-verify signeda.jar " +
"-J-Djava.security.properties=" +
JAVA_SECURITY_FILE +
" -keystore ks -storepass changeit -verbose -debug")
.shouldMatch("Digest algorithm: SHA-256.*(weak)")
.shouldMatch("Signature algorithm: PSSParameterSpec.*hashAlgorithm=SHA-256.*(weak)")
.shouldNotContain("The jar will be treated as unsigned")
.shouldHaveExitValue(0);
}
}