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

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

Closed
wants to merge 6 commits into from

Conversation

seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Oct 13, 2021

This fix improves the exception message to better indicate when the key (and not the signature algorithm) is restricted. This change also includes a few other improvements:

  • The constraints checking in AlgorithmChecker.check() has been improved. If the AlgorithmConstraints are an instance of DisabledAlgorithmConstraints, the internal permits methods are always called; otherwise the public permits methods are called. This makes the code easier to understand, and fixes at least one case where duplicate checks were being done.

  • The above change caused some of the exception messages to be slightly different, so some tests that checked the error messages had to be updated to reflect that.

  • AlgorithmDecomposer now stores the decomposed SHA algorithm names in a Map, which fixed a bug where "RSASSA-PSS" was not being restricted properly.


Progress

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

Issue

  • JDK-8243585: AlgorithmChecker::check throws confusing exception when it rejects the signer key

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5928

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 13, 2021

👋 Welcome back mullan! 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 openjdk bot added the rfr label Oct 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 13, 2021

@seanjmullan The following label will be automatically applied to this pull request:

  • security

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

@openjdk openjdk bot added the security label Oct 13, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 13, 2021

Webrevs

Map.of("SHA-1", "SHA1", "SHA-224", "SHA224", "SHA-256", "SHA256",
"SHA-384", "SHA384", "SHA-512", "SHA512", "SHA-512/224",
"SHA512/224", "SHA-512/256", "SHA512/256");

Copy link
Contributor

@wangweij wangweij Oct 19, 2021

Choose a reason for hiding this comment

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

Do you want to support the "SHA" -> "SHA1" mapping?

Copy link
Member Author

@seanjmullan seanjmullan Oct 19, 2021

Choose a reason for hiding this comment

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

These should be standard digest names as specified by the disabled algorithm security property syntax. SHA is an alias.

Copy link
Contributor

@wangweij wangweij Oct 19, 2021

Choose a reason for hiding this comment

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

OK, I saw the default in CANONICAL_NAME.getOrDefault(algorithm, algorithm) and thought non-standard names are also allowed.

public static String hashName(String algorithm) {
return algorithm.replace("-", "");
static String canonicalName(String algorithm) {
return CANONICAL_NAME.getOrDefault(algorithm, algorithm);
}
Copy link
Contributor

@wangweij wangweij Oct 19, 2021

Choose a reason for hiding this comment

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

I'm not sure if canonicalName is good. Normally, we say "SHA-1" is the standard name but this method changes it to "SHA1".

Copy link
Member Author

@seanjmullan seanjmullan Oct 19, 2021

Choose a reason for hiding this comment

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

Right, it's really just about using consistent message digest names so that it can match for example, "SHA-1" and also "SHA1withRSA". I'll change the name to something else.

Copy link
Contributor

@ascarpino ascarpino Oct 19, 2021

Choose a reason for hiding this comment

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

Was the reason for this change that hashName("RSASSA-PSS") was returning an RSASSAPSS?

Copy link
Member Author

@seanjmullan seanjmullan Oct 19, 2021

Choose a reason for hiding this comment

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

Yes.

…HashName

methods.
- Changed other code in AlgorithmDecomposer to use DECOMPOSED_DIGEST_NAMES
Map instead of hardcoding algorithm names.
- Changed AlgorithmChecker.trySetTrustAnchor to set trustedPubKey field so that
constraints on the key algorithm and size are checked in the check() method if
the constraints are an instanceof DisabledAlgorithmConstraints.
}
if (elements.contains("SHA-512") && !elements.contains("SHA512")) {
elements.add("SHA512");
for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
Copy link
Contributor

@ascarpino ascarpino Oct 20, 2021

Choose a reason for hiding this comment

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

If you're going to change this code, you can save me a PR if you surround this by "if (algorithm.contains("SHA") { ... }"
Its a perf change to eliminate the unnecessary map lookups when SHA isn't in the algorithm string

Copy link
Member Author

@seanjmullan seanjmullan Oct 20, 2021

Choose a reason for hiding this comment

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

That's a fine suggestion, although I'll note that your suggested perf improvement also applies to the previous code which did not check the algorithm parameter first to see if it contained SHA.
Also, another small perf imp: I realized below that in the loop, if the first if block gets executed, then the 2nd if block will always be false, so I changed it to an if/else.

Copy link
Contributor

@ascarpino ascarpino Oct 20, 2021

Choose a reason for hiding this comment

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

Yes, I was about to submit a PR to change the previous code, since you changed this code it makes more sense to ask you to do it at the same time.

hasLoop(elements, "SHA-256", "SHA256");
hasLoop(elements, "SHA-384", "SHA384");
hasLoop(elements, "SHA-512", "SHA512");
for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
Copy link
Contributor

@ascarpino ascarpino Oct 20, 2021

Choose a reason for hiding this comment

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

Same "if (algorithm.contains("SHA") { ... }" comment as above

Copy link
Member Author

@seanjmullan seanjmullan Oct 20, 2021

Choose a reason for hiding this comment

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

Ok.

hasLoop(elements, "SHA-384", "SHA384");
hasLoop(elements, "SHA-512", "SHA512");
for (Map.Entry<String, String> e : DECOMPOSED_DIGEST_NAMES.entrySet()) {
hasLoop(elements, e.getKey(), e.getValue());
Copy link
Contributor

@ascarpino ascarpino Oct 20, 2021

Choose a reason for hiding this comment

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

I think it's worth merging the contents of hasLoop() into this for loop. This is the only method calling hasLoop() and the extra method calls are not useful. Your addition DECOMPOSED_DIGEST_NAMES makes a merger a more reasonable solution now than before.

Copy link
Member Author

@seanjmullan seanjmullan Oct 20, 2021

Choose a reason for hiding this comment

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

Ok.

… "SHA".

- Remove hasLoop method and fold code into decomposeName method.
Copy link
Contributor

@ascarpino ascarpino left a comment

looks good to me

@openjdk
Copy link

@openjdk openjdk bot commented Oct 20, 2021

@seanjmullan 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:

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

Reviewed-by: ascarpino

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 120 new commits pushed to the master branch:

  • 3b0ce23: 8274888: Dump "-DReproduce=true" to the test VM command line output
  • d589b66: 8270380: Change the default value of the java.security.manager system property to disallow
  • e39bdc9: 8274714: Incorrect verifier protected access error message
  • 45ce06c: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST
  • 60cb27d: 8275426: PretouchTask num_chunks calculation can overflow
  • cd07b3c: 8257534: misc tests failed with "NoClassDefFoundError: Could not initialize class java.util.concurrent.ThreadLocalRandom"
  • 819d2df: 8274794: Print all owned locks in hs_err file
  • c41ce6d: 8275415: Prepare Leak Profiler for Lilliput
  • 0c3eaea: 8168388: GetMousePositionTest fails with the message "Mouse position should not be null"
  • 09f5235: 8275405: Linking error for classes with lambda template parameters and virtual functions
  • ... and 110 more: https://git.openjdk.java.net/jdk/compare/5ffb5d100f3383f9afaf20c8a659971522153505...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.

@openjdk openjdk bot added the ready label Oct 20, 2021
@@ -172,7 +168,7 @@ 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

@wangweij wangweij Oct 21, 2021

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

@seanjmullan seanjmullan Oct 21, 2021

Choose a reason for hiding this comment

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

Yes.

// specified.
if (prevPubKey == null) {
// been specified.
if (this.prevPubKey == null) {
Copy link
Contributor

@wangweij wangweij Oct 21, 2021

Choose a reason for hiding this comment

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

Maybe it's cleaner to write if (this.trustedPubKey == null) above. Anyway, after init() they are the same.

Copy link
Member Author

@seanjmullan seanjmullan Oct 21, 2021

Choose a reason for hiding this comment

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

Agree.

// specified.
if (prevPubKey == null) {
// been specified.
if (this.prevPubKey == null) {
if (anchor == null) {
Copy link
Contributor

@wangweij wangweij Oct 21, 2021

Choose a reason for hiding this comment

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

This won't happen. Or, you can ignore it.

This makes it possible to call this method in the constructor.

Copy link
Member Author

@seanjmullan seanjmullan Oct 21, 2021

Choose a reason for hiding this comment

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

Ok, will remove. But I will keep this method separate since, unlike the ctor it needs to check if trustedPubKey is null before setting the prevPubKey.

Copy link
Contributor

@wangweij wangweij Oct 21, 2021

Choose a reason for hiding this comment

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

OK, but in the ctor trustedPubKey is also null.

Copy link
Member Author

@seanjmullan seanjmullan Oct 21, 2021

Choose a reason for hiding this comment

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

True, but that's because none of the fields are set yet, so it feels odd for the ctor to check if the field is null when it is always true, even if it is a different method. What if I create a separate method setTrustAnchor(TrustAnchor) which the ctor calls, and then change trySetTrustAnchor to:

        if (this.trustedPubKey == null) {
            setTrustAnchor(anchor);
        }

Copy link
Contributor

@wangweij wangweij Oct 21, 2021

Choose a reason for hiding this comment

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

Sounds good.

@@ -74,7 +74,7 @@
private static final Debug debug = Debug.getInstance("certpath");

private final AlgorithmConstraints constraints;
private final PublicKey trustedPubKey;
private PublicKey trustedPubKey;
private final Date date;
private PublicKey prevPubKey;
private final String variant;
Copy link
Contributor

@wangweij wangweij Oct 21, 2021

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

@seanjmullan seanjmullan Oct 21, 2021

Choose a reason for hiding this comment

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

Ok.

@@ -309,16 +317,6 @@ public void check(Certificate cert,
return;
}
Copy link
Contributor

@wangweij wangweij Oct 21, 2021

Choose a reason for hiding this comment

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

I find it more natural to enclose the block below in a if (prevPubKey != null) block, and you only need to call one prevPubKey = currPubKey at the end.

Copy link
Member Author

@seanjmullan seanjmullan Oct 21, 2021

Choose a reason for hiding this comment

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

Ok.

@seanjmullan
Copy link
Member Author

@seanjmullan seanjmullan commented Oct 21, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

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

  • bef8cf1: 8275714: G1: remove unused variable in G1Policy::transfer_survivors_to_cset
  • af14650: 8275569: Add linux-aarch64 to test-make profiles
  • 0761a4b: 8275688: Suppress warnings on non-serializable non-transient instance fields in DualPivotQuicksort
  • 3cb241a: 8275686: Suppress warnings on non-serializable non-transient instance fields in java.rmi
  • 3b0ce23: 8274888: Dump "-DReproduce=true" to the test VM command line output
  • d589b66: 8270380: Change the default value of the java.security.manager system property to disallow
  • e39bdc9: 8274714: Incorrect verifier protected access error message
  • 45ce06c: 8274779: HttpURLConnection: HttpClient and HttpsClient incorrectly check request method when set to POST
  • 60cb27d: 8275426: PretouchTask num_chunks calculation can overflow
  • cd07b3c: 8257534: misc tests failed with "NoClassDefFoundError: Could not initialize class java.util.concurrent.ThreadLocalRandom"
  • ... and 114 more: https://git.openjdk.java.net/jdk/compare/5ffb5d100f3383f9afaf20c8a659971522153505...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 21, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 21, 2021

@seanjmullan Pushed as commit 49f9d80.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated security
3 participants