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

8259401: Add checking to jarsigner to warn weak algorithms used in signer’s cert chain #2042

Closed
wants to merge 3 commits into from

Conversation

@haimaychao
Copy link
Contributor

@haimaychao haimaychao commented Jan 12, 2021

The jarsigner tool currently provides warning associated with the signer’s cert when it uses weak algorithms, but not for the CA certs. This change is to process the signer’s cert chain to warn if CA certs use weak algorithms.


Progress

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

Issue

  • JDK-8259401: Add checking to jarsigner to warn weak algorithms used in signer’s cert chain

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2042/head:pull/2042
$ git checkout pull/2042

…gner’s cert chain
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 12, 2021

👋 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 openjdk bot added the rfr label Jan 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 12, 2021

Webrevs

@AlanBateman
Copy link
Contributor

@AlanBateman AlanBateman commented Jan 12, 2021

/label remove core-libs

@openjdk openjdk bot removed the core-libs label Jan 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@AlanBateman
The core-libs label was successfully removed.

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Jan 12, 2021

/label remove compiler

@openjdk openjdk bot removed the compiler label Jan 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 12, 2021

@haimaychao
The compiler label was successfully removed.

@@ -1401,6 +1401,35 @@ private void checkWeakSign(PrivateKey key) {
}
}

private String checkWeakKey(PublicKey key) {

This comment has been minimized.

@seanjmullan

seanjmullan Jan 12, 2021
Member

Can this method be static?

This comment has been minimized.

@haimaychao

haimaychao Jan 13, 2021
Author Contributor

static added.

}
}

private String checkWeakAlg(String alg) {

This comment has been minimized.

@seanjmullan

seanjmullan Jan 12, 2021
Member

Can this method be static?

This comment has been minimized.

@haimaychao

haimaychao Jan 13, 2021
Author Contributor

static added.

// weak algorithms are used, and provide warnings as needed.
certStr.append("\n").append(tab)
.append("Signature algorithm: ")
.append(checkWeakAlg(sigalg))

This comment has been minimized.

@seanjmullan

seanjmullan Jan 12, 2021
Member

If the cert is trusted, I don't think we should print a warning if the signature algorithm is weak. Otherwise this will generate false warnings for SHA-1 roots which are not an issue. You should check the key size though. And you can still print the signature algorithm. You may need to move line 1489-1490 before this to first determine if the cert is trusted.

This comment has been minimized.

@haimaychao

haimaychao Jan 13, 2021
Author Contributor

Fixed to not check the signature algorithm for a trusted cert, and updated the test accordingly.


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

// root certificate using SHA1withRSA and 1024-bit key

This comment has been minimized.

@rhalade

rhalade Jan 12, 2021
Member

It will be helpful to have these comments logged as debug messages with System.out

This comment has been minimized.

@haimaychao

haimaychao Jan 13, 2021
Author Contributor

comment messages added.

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Jan 13, 2021

Thanks for your review, Sean and Rajan. I've updated the webrev with your comments.

if (trustedCerts.contains(x509Cert)) {
// If the cert is trusted, only check its key size, but not its
// signature algorithm. This is because warning should not be
// generated for SHA-1 roots which are not an issue.

This comment has been minimized.

@wangweij

wangweij Jan 13, 2021
Contributor

SHA-1 is just a glitch in the long history at this very moment, and thus I think it's inappropriate to mention it in the source code. In my opinion, the general reason we don't check the signature is that we trust its origin anyway and we don't verify the signature at all (do we?). On the other hand, since its key is used to sign other certs, we need to make sure the key size is big enough so that no one else is able to recover the key and use it to sign other certs.

This comment has been minimized.

@seanjmullan

seanjmullan Jan 13, 2021
Member

Yes, I would remove the 2nd sentence that starts with "This is ...". There are plenty of references on the Internet which explain this, so no need to add much detail.

This comment has been minimized.

@haimaychao

haimaychao Jan 13, 2021
Author Contributor

Removed.

.append(rb.getString("COMMA"))
.append(checkWeakKey(key));

certStr.append("\n").append(tab).append("[");

This comment has been minimized.

@wangweij

wangweij Jan 13, 2021
Contributor

It's a little strange to leave the other half of the bracket (certStr.append("]"); on line 1568) outside the if-else block. Can you please move it inside? Of course you will have to duplicate them.

This comment has been minimized.

@haimaychao

haimaychao Jan 13, 2021
Author Contributor

I'd prefer to keep it as is instead of duplicating the code in several places inside if block and else block.

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Jan 13, 2021

Thanks for the review, Max. The comment about trusted cert's sigalg checking has been removed.

@haimaychao haimaychao changed the title 8259401: Add checking to jarsigner to warn weak algorithms used in si… 8259401: Add checking to jarsigner to warn weak algorithms used in signer’s cert chain Jan 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

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

8259401: Add checking to jarsigner to warn weak algorithms used in signer’s cert chain

Reviewed-by: mullan, weijun, rhalade

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

  • ccdf171: 8259377: Shenandoah: Enhance weak reference processing time tracking
  • 916ab4e: 8259283: use new HtmlId and HtmlIds classes
  • 5df2a94: 8212035: merge jdk.test.lib.util.SimpleHttpServer with jaxp.library.SimpleHttpServer
  • 535f2da: 8259486: Replace PreserveExceptionMark with implementation for CautiouslyPreserveExceptionMark
  • ce94512: 8259619: C1: 3-arg StubAssembler::call_RT stack-use condition is incorrect
  • 2e12454: 8259580: Shenandoah: uninitialized label in VerifyThreadGCState
  • 77ca103: 8257236: can't use var with a class named Z
  • 2243a17: 8259485: Document need for short paths when building on Windows
  • 139b6da: 8259372: remove AIX related USE_LIBRARY_BASED_TLS_ONLY and THREAD_LOCAL special handling
  • a483869: 8225045: javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java fails on linux-x64
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/cd73939b794320d92d45e00b220fd953d83183cf...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@seanjmullan, @wangweij, @rhalade) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 13, 2021
@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Jan 13, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jan 13, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@haimaychao
Your change (at version 88d94df) is now ready to be sponsored by a Committer.

@wangweij
Copy link
Contributor

@wangweij wangweij commented Jan 13, 2021

/sponsor

@openjdk
Copy link

@openjdk openjdk bot commented Jan 13, 2021

@wangweij @haimaychao Since your change was applied there have been 44 commits pushed to the master branch:

  • ccdf171: 8259377: Shenandoah: Enhance weak reference processing time tracking
  • 916ab4e: 8259283: use new HtmlId and HtmlIds classes
  • 5df2a94: 8212035: merge jdk.test.lib.util.SimpleHttpServer with jaxp.library.SimpleHttpServer
  • 535f2da: 8259486: Replace PreserveExceptionMark with implementation for CautiouslyPreserveExceptionMark
  • ce94512: 8259619: C1: 3-arg StubAssembler::call_RT stack-use condition is incorrect
  • 2e12454: 8259580: Shenandoah: uninitialized label in VerifyThreadGCState
  • 77ca103: 8257236: can't use var with a class named Z
  • 2243a17: 8259485: Document need for short paths when building on Windows
  • 139b6da: 8259372: remove AIX related USE_LIBRARY_BASED_TLS_ONLY and THREAD_LOCAL special handling
  • a483869: 8225045: javax/swing/JInternalFrame/8146321/JInternalFrameIconTest.java fails on linux-x64
  • ... and 34 more: https://git.openjdk.java.net/jdk/compare/cd73939b794320d92d45e00b220fd953d83183cf...master

Your commit was automatically rebased without conflicts.

Pushed as commit c7e2174.

💡 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
5 participants