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

haimaychao · 2022-02-22T22:00:05Z

This fixes jarsigner to enforce checking against algorithm constraint properties so when the signature algorithms parameters use disabled or legacy algorithms, it will emit warnings accordingly. If the algorithm used in parameters is disabled, jarsigner treats the jar as unsigned.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8277474: jarsigner does not check if algorithm parameters are disabled

Reviewers

Sean Mullan (@seanjmullan - Reviewer) ⚠️ Review applies to 70da21b
Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7582/head:pull/7582
$ git checkout pull/7582

Update a local copy of the PR:
$ git checkout pull/7582
$ git pull https://git.openjdk.java.net/jdk pull/7582/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7582

View PR using the GUI difftool:
$ git pr show -t 7582

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7582.diff

haimaychao · 2022-02-22T22:00:22Z

/label remove core-libs

haimaychao · 2022-02-22T22:00:30Z

/label remove compiler

bridgekeeper · 2022-02-22T22:01:46Z

👋 Welcome back hchao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2022-02-22T22:04:40Z

@haimaychao The core-libs label was not set.

openjdk · 2022-02-22T22:04:55Z

@haimaychao The compiler label was not set.

openjdk · 2022-02-22T22:05:46Z

@haimaychao The following labels will be automatically applied to this pull request:

compiler
core-libs
security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

haimaychao · 2022-02-22T22:06:22Z

/label remove core-libs

haimaychao · 2022-02-22T22:06:30Z

/label remove compiler

openjdk · 2022-02-22T22:07:11Z

@haimaychao
The core-libs label was successfully removed.

openjdk · 2022-02-22T22:08:30Z

@haimaychao
The compiler label was successfully removed.

mlbridge · 2022-02-22T22:11:17Z

Webrevs

seanjmullan · 2022-03-02T15:30:22Z

src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java

@@ -30,6 +30,7 @@
 import java.net.URLClassLoader;
 import java.security.cert.CertPathValidatorException;
 import java.security.cert.PKIXBuilderParameters;
+import java.security.spec.PSSParameterSpec;


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

Removed. Thanks for the review.

openjdk · 2022-03-02T15:44:39Z

@haimaychao This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8277474: jarsigner does not check if algorithm parameters are disabled

Reviewed-by: mullan, weijun

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 265 new commits pushed to the master branch:

1581e3f: 8282402: Create a regression test for JDK-4666101
268fa69: 8282511: Use fixed certificate validation date in SSLExampleCert template
c777bb3: 8282619: G1: Fix indentation in G1CollectedHeap::mark_evac_failure_object
080baff: 8282483: Ensure that Utils.getAllInterfaces returns unique instances
57020fd: 8282582: Unused methods in Utils
5c187e3: 8282593: JDK-8281472 breaks 32-bit builds and gtests
7822cbc: 8276711: compiler/codecache/cli tests failing when SegmentedCodeCache used with -Xint
832729b: 6911375: mouseWheel has no effect without vertical scrollbar
0402a28: 8037965: NullPointerException in TextLayout.getBaselineFromGraphic() for JTextComponents
d4d1fbc: 8282484: G1: Predicted old time in log always zero
... and 255 more: https://git.openjdk.java.net/jdk/compare/2f71a6b39ed6bb869b4eb3e81bc1d87f4b3328ff...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

wangweij · 2022-03-02T16:20:53Z

src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java

+                JAR_DISABLED_CHECK.permits(algParams, jcp);
+            } catch (CertPathValidatorException e) {
+                disabledAlgFound = true;
+                return String.format(rb.getString("with.disabled"), algParams);


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.

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.

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.

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.

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 "]".

wangweij

The return values of the 2 updated verifyWithWeak methods need to be fixed.

wangweij · 2022-03-02T16:47:59Z

src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java

+        if (algParams != null) {
+            try {
+                LEGACY_CHECK.permits(algParams, jcp);
+                return alg;


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

wangweij · 2022-03-02T16:50:25Z

test/jdk/sun/security/tools/jarsigner/CheckAlgParams.java

+ * @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


Is this @modules line useful?

No, removed it. Thanks for the review!

wangweij · 2022-03-03T19:35:21Z

src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java

+                        return String.format(rb.getString("with.algparams.disabled"),
+                                "RSASSA-PSS", algParams);
+                    default:
+                        return String.format(rb.getString("with.disabled"), algParams);


There is no need to differentiate RSASSA-PSS and other algorithms. Just use the exact same format as you defined in Resources.java. This makes sure if one day another type of algorithm parameters fail the check, we will see the algorithm name and full description of the parameters as well. If the description is not clear, it's the problem of its toString() implementation.

Removed code that differentiates.

wangweij · 2022-03-03T19:36:05Z

src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java

+                                "RSASSA-PSS", algParams);
+                    default:
+                        return String.format(rb.getString("with.weak"), algParams);
+                }


Same as above.

haimaychao · 2022-03-03T22:59:52Z

/integrate

openjdk · 2022-03-03T23:01:44Z

Going to push as commit fb6b929.
Since your change was applied there have been 265 commits pushed to the master branch:

1581e3f: 8282402: Create a regression test for JDK-4666101
268fa69: 8282511: Use fixed certificate validation date in SSLExampleCert template
c777bb3: 8282619: G1: Fix indentation in G1CollectedHeap::mark_evac_failure_object
080baff: 8282483: Ensure that Utils.getAllInterfaces returns unique instances
57020fd: 8282582: Unused methods in Utils
5c187e3: 8282593: JDK-8281472 breaks 32-bit builds and gtests
7822cbc: 8276711: compiler/codecache/cli tests failing when SegmentedCodeCache used with -Xint
832729b: 6911375: mouseWheel has no effect without vertical scrollbar
0402a28: 8037965: NullPointerException in TextLayout.getBaselineFromGraphic() for JTextComponents
d4d1fbc: 8282484: G1: Predicted old time in log always zero
... and 255 more: https://git.openjdk.java.net/jdk/compare/2f71a6b39ed6bb869b4eb3e81bc1d87f4b3328ff...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-03-03T23:02:02Z

@haimaychao Pushed as commit fb6b929.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8277474: jarsigner does not check if algorithm parameters are disabled

70da21b

openjdk bot added the rfr Pull request is ready for review label Feb 22, 2022

openjdk bot added security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org compiler compiler-dev@openjdk.org labels Feb 22, 2022

openjdk bot removed the core-libs core-libs-dev@openjdk.org label Feb 22, 2022

openjdk bot removed the compiler compiler-dev@openjdk.org label Feb 22, 2022

seanjmullan approved these changes Mar 2, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 2, 2022

wangweij reviewed Mar 2, 2022

View reviewed changes

wangweij suggested changes Mar 2, 2022

View reviewed changes

wangweij reviewed Mar 2, 2022

View reviewed changes

haimaychao added 4 commits March 2, 2022 09:40

Removed unneeded import and updated -verbose output

9ffda80

Updated -verbose output

224bf18

Updated -verbose output

516d8bf

Removed unused string

2a73d1e

wangweij reviewed Mar 3, 2022

View reviewed changes

haimaychao added 2 commits March 3, 2022 13:05

Use algname in output

d2cd7ef

No need to do toUpperCase

74dc704

wangweij approved these changes Mar 3, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 3, 2022

openjdk bot closed this Mar 3, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 3, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

haimaychao commented Feb 22, 2022 •

edited by openjdk bot

haimaychao commented Feb 22, 2022

haimaychao commented Feb 22, 2022

bridgekeeper bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

haimaychao commented Feb 22, 2022

haimaychao commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

mlbridge bot commented Feb 22, 2022 •

edited

seanjmullan Mar 2, 2022

haimaychao Mar 2, 2022

openjdk bot commented Mar 2, 2022 •

edited

wangweij Mar 2, 2022 •

edited

haimaychao Mar 2, 2022

wangweij Mar 2, 2022

wangweij Mar 2, 2022

haimaychao Mar 3, 2022

wangweij left a comment

wangweij Mar 2, 2022

haimaychao Mar 2, 2022

wangweij Mar 2, 2022 •

edited

haimaychao Mar 2, 2022

wangweij Mar 3, 2022

haimaychao Mar 3, 2022

wangweij Mar 3, 2022

haimaychao commented Mar 3, 2022

openjdk bot commented Mar 3, 2022

openjdk bot commented Mar 3, 2022

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

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

Conversation

haimaychao commented Feb 22, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

haimaychao commented Feb 22, 2022

haimaychao commented Feb 22, 2022

bridgekeeper bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

haimaychao commented Feb 22, 2022

haimaychao commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

openjdk bot commented Feb 22, 2022

mlbridge bot commented Feb 22, 2022 • edited

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Mar 2, 2022 • edited

wangweij Mar 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij Mar 2, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haimaychao commented Mar 3, 2022

openjdk bot commented Mar 3, 2022

openjdk bot commented Mar 3, 2022

haimaychao commented Feb 22, 2022 •

edited by openjdk bot

mlbridge bot commented Feb 22, 2022 •

edited

openjdk bot commented Mar 2, 2022 •

edited

wangweij Mar 2, 2022 •

edited

wangweij Mar 2, 2022 •

edited