8277246: Check for NonRepudiation as well when validating a TSA certificate #6416

wangweij · 2021-11-16T19:36:11Z

There is no need to check for the KeyUsage extension when validating a TSA certificate.

A test is modified where a TSA cert has a KeyUsage but without the DigitalSignature bit.

Progress

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

Issue

JDK-8277246: Check for NonRepudiation as well when validating a TSA certificate

Reviewers

Xue-Lei Andrew Fan (@XueleiFan - Reviewer) ⚠️ Review applies to a5b3bc8
Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6416

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

Using diff file

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

…icate 8277246: No need to check about KeyUsage when validating a TSA certificate

bridgekeeper · 2021-11-16T19:37:20Z

👋 Welcome back weijun! 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 · 2021-11-16T19:39:03Z

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

mlbridge · 2021-11-16T19:43:49Z

Webrevs

XueleiFan · 2021-11-16T19:46:24Z

src/java.base/share/classes/sun/security/validator/EndEntityChecker.java

-        if (checkKeyUsage(cert, KU_SIGNATURE) == false) {
-            throw new ValidatorException
-                ("KeyUsage does not allow digital signatures",
-                ValidatorException.T_EE_EXTENSIONS, cert);
-        }
-


Could you add a comment here just in case someone else adding the checking back? Otherwise, looks nice to me.

mlbridge · 2021-11-16T20:54:05Z

Mailing list message from Michael StJohns on security-dev:

On 11/16/2021 2:43 PM, Weijun Wang wrote:

I just ran this by one of the PKIX folk and this fix isn't correct.??
Basically, if a certificate has both an Extended Key Usage and a Key
Usage extension? then they both have to be consistent with the actual
purpose and both have to be checked. The former for the specific use
described in the TSA document, and the latter for general use (RFC3280
and 5280).

In this case, checkKeyUsage attempts to find? keyUsageExtension and if
it's not present returns true (e.g. key usage is acceptable).? Otherwise
it checks to see if there's a digitalSignature bit set, and if it's not
set checkKeyUsage returns false.?? The code as written (before the
change) is correct.? Here's the utility method in EndEntityChecker.java

\/\*\*
220 * Utility method checking if bit 'bit' is set in this certificates
221 * key usage extension.
222 * @throws CertificateException if not
223 */
224 private boolean checkKeyUsage(X509Certificate cert, int bit)
225 throws CertificateException {
226 boolean[] keyUsage = cert.getKeyUsage();
227 if (keyUsage == null) {
228 return true;
229 }
230 return (keyUsage.length > bit) && keyUsage[bit];
231 }

It would be acceptable to have a certificate without a
keyUsageExtension, but if the KUE is present, then the digitalSignature
bit needs to be set.

Recommend backing out the change and closing the bug report as mistaken.

Mike

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20211116/8421e5e3/attachment.htm>

wangweij · 2021-11-16T21:02:06Z

Hi Michael. Thanks for the comment. That was also our previous understanding but we are seeing timestamp returning by sigstore.dev (see the rekor timestamp command at https://github.com/sigstore/rekor) whose cert does not have the DigitialSignature bit set.

openjdk · 2021-11-16T21:33:40Z

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

8277246: Check for NonRepudiation as well when validating a TSA certificate

Reviewed-by: xuelei, mullan

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

9f2f46e: 8275037: Test vmTestbase/nsk/sysdict/vm/stress/btree/btree011/btree011.java crashes with memory exhaustion on Windows
2af9e59: 8276139: TestJpsHostName.java not reliable, better to expand HostIdentifierCreate.java test
e9934e1: 8277221: G1: Remove methods without implementations in G1CollectedHeap
9aa30de: 8275317: AArch64: Support some type conversion vectorization in SLP
08f65a5: 8277313: Validate header failed for test/jdk/java/net/httpclient/HeadTest.java
23e5117: 8276559: (httpclient) Consider adding an HttpRequest.Builder.HEAD method to build a HEAD request.
a77d8dd: 8276787: Improve warning messages for -XX:+RecordDynamicDumpInfo
8ed384c: 8276609: Document setting property jdk.serialFilter to an invalid value throws ExceptionInInitializerError
cddc6ce: 8275811: Incorrect instance to dispose
b0a463f: 8169468: NoResizeEventOnDMChangeTest.java fails because FS Window didn't receive all resizes!
... and 31 more: https://git.openjdk.java.net/jdk/compare/176d21d6c525f8fd9592db5b4975308ea0001856...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.

mlbridge · 2021-11-16T23:01:05Z

Mailing list message from Michael StJohns on security-dev:

On 11/16/2021 4:05 PM, Weijun Wang wrote:

The certificate is either incorrect, or weird and correct. I actually
think its incorrect, but let's assume the latter and that either of
these two key purposes can be used. ? ? Change the check to permit
either of the two KUs in a? KeyUsageExtension:

// insert around line 109.

private static final int KU_NON_REPUDIATION = 1;

// replace line 359.

if (checkKeyUsage (cert, KU_SIGNATURE) == false && checkKeyUsage(cert,
KU_NON_REPUDATION) == false) {

From RFC5280:

The digitalSignature bit is asserted when the subject public key
is used for verifying digital signatures, other than signatures on
certificates (bit 5) and CRLs (bit 6), such as those used in an
entity authentication service, a data origin authentication
service, and/or an integrity service.
   The nonRepudiation bit is asserted when the subject public key is
   used to verify digital signatures\, other than signatures on
   certificates $bit 5$ and CRLs $bit 6$\, used to provide a non\-
   repudiation service that protects against the signing entity
   falsely denying some action\.  In the case of later conflict\, a
   reliable third party may determine the authenticity of the signed
   data\.  $Note that recent editions of X\.509 have renamed the
   nonRepudiation bit to contentCommitment\.$

In any event, if you have a KU extension and it includes neither of
these bits, the certificate is invalid for timestamping.? So simply
deleting the check is wrong.

I'll reach out again to my expert and let you know what I find out.

Thanks - Mike

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20211116/33dd02d1/attachment.htm>

wangweij · 2021-11-16T23:25:06Z

Sean found RFC 5280, 4.2.1.12:

   id-kp-timeStamping            OBJECT IDENTIFIER ::= { id-kp 8 }
   -- Binding the hash of an object to a time
   -- Key usage bits that may be consistent: digitalSignature
   -- and/or nonRepudiation

so it seems either is OK. That said, it's just a "may", and to me it reads more like that others are NOT consistent and should be rejected.

wangweij · 2021-11-16T23:34:13Z

I did see an issuer of TSA certs whose own certificate has EKU with id-kp-timeStamping and KU with both DigitialSignature and keyCertsign. This cert should be rejected if it signed a timestamp response.

mlbridge · 2021-11-16T23:41:04Z

Mailing list message from Michael StJohns on security-dev:

On 11/16/2021 5:58 PM, Michael StJohns wrote:

On 11/16/2021 4:05 PM, Weijun Wang wrote:

The certificate is either incorrect, or weird and correct. I actually
think its incorrect, but let's assume the latter and that either of
these two key purposes can be used. ? ? Change the check to permit
either of the two KUs in a KeyUsageExtension:

// insert around line 109.

private static final int KU_NON_REPUDIATION = 1;

// replace line 359.

if (checkKeyUsage (cert, KU_SIGNATURE) == false && checkKeyUsage(cert,
KU_NON_REPUDATION) == false) {

From RFC5280:
The digitalSignature bit is asserted when the subject public key
is used for verifying digital signatures, other than signatures on
certificates (bit 5) and CRLs (bit 6), such as those used in an
entity authentication service, a data origin authentication
service, and/or an integrity service.
   The nonRepudiation bit is asserted when the subject public key is
   used to verify digital signatures\, other than signatures on
   certificates $bit 5$ and CRLs $bit 6$\, used to provide a non\-
   repudiation service that protects against the signing entity
   falsely denying some action\.  In the case of later conflict\, a
   reliable third party may determine the authenticity of the signed
   data\.  $Note that recent editions of X\.509 have renamed the
   nonRepudiation bit to contentCommitment\.$
In any event, if you have a KU extension and it includes neither of
these bits, the certificate is invalid for timestamping.? So simply
deleting the check is wrong.

I'll reach out again to my expert and let you know what I find out.

Thanks - Mike

According to the guy who wrote RFC5280, nonRepudiation (aka
contentCommitment) is a valid alternative for a keyUsage that pairs with
an extended key usage of id-kp-timestamping.? I'd add a check that
requires one or the other or both if a KeyUsage extension exists.

I added a note to the Rekor CA repository, hopefully at the correct
location suggesting they set both bits going forward. This was code they
published only back in May.

Mike

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20211116/166139c0/attachment.htm>

mlbridge · 2021-11-16T23:45:05Z

Mailing list message from Michael StJohns on security-dev:

On 11/16/2021 6:37 PM, Weijun Wang wrote:

Not quite.?? The rule is that if there's both an ExtendedKeyUsage and
KeyUsage extensions, for any given OID in the EKU there has to be at
least one bit in the KeyUsage extenstion that's compatible - there may
be more than one.? If there's an EKU, and no KeyUsage, then only the EKU
needs to have an OID for the key usage purpose - in this case signing a
timestamp.

The cert you cite would be valid for timestamping.

Mike

wangweij · 2021-11-17T00:43:26Z

Really? The TSA is http://timestamp.digicert.com and the cert chain is

CN=DigiCert Timestamp 2021, O="DigiCert, Inc.", C=US
KeyUsage: DigitalSignature
ExtendedKeyUsages: timeStamping

CN=DigiCert SHA2 Assured ID Timestamping CA, OU=www.digicert.com, O=DigiCert Inc, C=US
KeyUsage: DigitalSignature,  Key_CertSign, Crl_Sign
ExtendedKeyUsages: timeStamping

You mean this CA can be used for time stamping as well? I understand that when KU is missing you can find out its usage in EKU (vice versa), but here it's a CA that can sign cert and CRLs. Does it really need to act as the end entity cert of a TSA server?

mlbridge · 2021-11-17T03:30:07Z

Mailing list message from Michael StJohns on security-dev:

On 11/16/2021 7:46 PM, Weijun Wang wrote:

It doesn't need to act as an EE of a TSA server, but with those markings
it can.

Whoever issued these over marked them.?? I think their intent was to say
that this CA chain would issue time stamp issuing certificates, but?
extendedKeyUsage contents are not transitive to the subordinate
certificates so that extension is pretty much extraneous in a CA.? That
said, if you got a timestamp verifiable by the public key in this CA
certificate it would be valid (based on the certificate only).??? The
TSA RFC doesn't actually prohibit having a basicConstraints ca=true
extension.?? If I were verifying a timestamp, I'd probably filter out
any signatures from certificates that are claiming to be CAs, but that's
not strictly according to standards.

If I were issuing this chain, there would be no extendedKeyUsage
extensions in the intermediate certificate(s), and the keyPurpose would
only be keyCertSign or keyCertSign,cRLSign depending.? The EE
certificate would have eku {id-kp-timestamping} and ku {
digitalSignature } as I probably couldn't ensure non-repudiation for the
time stamp (not the data being wrapped by the timestamp which is what
the Rekor chain is trying to claim I think).

Hmm... while I was researching - I found this in RFC5280 - 4.2.1.12
defining extendedKeyUsage oids:

This extension indicates one or more purposes for which the certified
public key may be used, in addition to or in place of the basic
purposes indicated in the key usage extension._In general, this extension will appear only in end entity certificates._ This
extension is defined as follows

and

id\-kp\-timeStamping            OBJECT IDENTIFIER \:\:\= \{ id\-kp 8 \}
\-\- Binding the hash of an object to a time
\-\- Key usage bits that may be consistent\:\_digitalSignature\_
\-\- and\/or\_nonRepudiation\_

I hope that helps.

Mike

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20211116/be8baa79/attachment.htm>

mlbridge · 2021-11-17T05:48:04Z

Mailing list message from Xuelei Fan on security-dev:

On Nov 16, 2021, at 7:28 PM, Michael StJohns <mstjohns at comcast.net<mailto:mstjohns at comcast.net>> wrote:

id-kp-timeStamping OBJECT IDENTIFIER ::= { id-kp 8 }
-- Binding the hash of an object to a time
-- Key usage bits that may be consistent: digitalSignature
-- and/or nonRepudiation

Hm, we may want to check it strictly in this update, by allowing nonRepudiation alternatively.

Xuelei
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20211117/44e73ed8/attachment-0001.htm>

wangweij · 2021-11-17T14:02:25Z

Revert the removal and add check on nonRepudiation as well. And the test still works. :-)

seanjmullan · 2021-11-17T14:16:26Z

Can you change the synopsis of the bug to more accurately reflect the current fix?

wangweij · 2021-11-17T14:24:07Z

Can you change the synopsis of the bug to more accurately reflect the current fix?

Updated. Thanks.

seanjmullan · 2021-11-17T14:37:13Z

Mailing list message from Michael StJohns on security-dev:

On 11/16/2021 7:46 PM, Weijun Wang wrote:

It doesn't need to act as an EE of a TSA server, but with those markings it can.

Whoever issued these over marked them.?? I think their intent was to say that this CA chain would issue time stamp issuing certificates, but? extendedKeyUsage contents are not transitive to the subordinate certificates so that extension is pretty much extraneous in a CA.? That said, if you got a timestamp verifiable by the public key in this CA certificate it would be valid (based on the certificate only).??? The TSA RFC doesn't actually prohibit having a basicConstraints ca=true extension.?? If I were verifying a timestamp, I'd probably filter out any signatures from certificates that are claiming to be CAs, but that's not strictly according to standards.

I agree that having a CA act as a TSA is probably a bad idea. However, I don't think this particular CA is acting as a TSA, just looks like they overmarked it as you say.

In any case, I agree we should allow the KU of a TSA cert to be absent or if present it must contain a non-repudiation and/or digitalSignature bit.

wangweij · 2021-11-17T20:02:29Z

/integrate

openjdk · 2021-11-17T20:04:06Z

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

a907b2b: 8276177: nsk/jvmti/RedefineClasses/StressRedefineWithoutBytecodeCorruption failed with "assert(def_ik->is_being_redefined()) failed: should be being redefined to get here"
b687664: 8277159: Fix java/nio/file/FileStore/Basic.java test by ignoring /run/user/* mount points
8f5a8f7: 8264293: Create implementation for NSAccessibilityMenu protocol peer
9f2f46e: 8275037: Test vmTestbase/nsk/sysdict/vm/stress/btree/btree011/btree011.java crashes with memory exhaustion on Windows
2af9e59: 8276139: TestJpsHostName.java not reliable, better to expand HostIdentifierCreate.java test
e9934e1: 8277221: G1: Remove methods without implementations in G1CollectedHeap
9aa30de: 8275317: AArch64: Support some type conversion vectorization in SLP
08f65a5: 8277313: Validate header failed for test/jdk/java/net/httpclient/HeadTest.java
23e5117: 8276559: (httpclient) Consider adding an HttpRequest.Builder.HEAD method to build a HEAD request.
a77d8dd: 8276787: Improve warning messages for -XX:+RecordDynamicDumpInfo
... and 34 more: https://git.openjdk.java.net/jdk/compare/176d21d6c525f8fd9592db5b4975308ea0001856...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-11-17T20:04:25Z

@wangweij Pushed as commit 262d070.

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

mlbridge · 2021-11-22T15:49:42Z

Mailing list message from Mike StJohns on security-dev:

You?ll be amused to find out that the code that generated the Rekor TS cert has been changed to use digitalSignature as its KU. https://github.com/sigstore/rekor/pull/504/files

I think the change you made is correct, but you probably won?t see a nonRepudiation bit for a while again. Mike

Sent from my iPad

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/security-dev/attachments/20211122/61dc5362/attachment.htm>

wangweij · 2021-11-22T22:01:18Z

Great. Thanks a lot for your suggestion to them. I really appreciate it.

8277246: No need to check about KeyUsage when validating a TSA certif…

e7db147

…icate 8277246: No need to check about KeyUsage when validating a TSA certificate

openjdk bot added the rfr Pull request is ready for review label Nov 16, 2021

openjdk bot added the security security-dev@openjdk.org label Nov 16, 2021

XueleiFan reviewed Nov 16, 2021

View reviewed changes

clarify RFC requirement

a5b3bc8

XueleiFan approved these changes Nov 16, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 16, 2021

check either KU_SIGNATURE or KU_NON_REPUDIATION

2bd3c54

wangweij changed the title ~~8277246: No need to check about KeyUsage when validating a TSA certificate~~ 8277246: Check for NonRepudiation as well when validating a TSA certificate Nov 17, 2021

seanjmullan approved these changes Nov 17, 2021

View reviewed changes

openjdk bot closed this Nov 17, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 17, 2021

wangweij deleted the 8277246 branch November 30, 2021 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8277246: Check for NonRepudiation as well when validating a TSA certificate #6416

8277246: Check for NonRepudiation as well when validating a TSA certificate #6416

wangweij commented Nov 16, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 16, 2021

openjdk bot commented Nov 16, 2021

mlbridge bot commented Nov 16, 2021 •

edited

Loading

XueleiFan Nov 16, 2021

mlbridge bot commented Nov 16, 2021

wangweij commented Nov 16, 2021

openjdk bot commented Nov 16, 2021 •

edited

Loading

mlbridge bot commented Nov 16, 2021

wangweij commented Nov 16, 2021

wangweij commented Nov 16, 2021

mlbridge bot commented Nov 16, 2021

mlbridge bot commented Nov 16, 2021

wangweij commented Nov 17, 2021 •

edited

Loading

mlbridge bot commented Nov 17, 2021

mlbridge bot commented Nov 17, 2021

wangweij commented Nov 17, 2021

seanjmullan commented Nov 17, 2021

wangweij commented Nov 17, 2021

seanjmullan commented Nov 17, 2021

wangweij commented Nov 17, 2021

openjdk bot commented Nov 17, 2021

openjdk bot commented Nov 17, 2021

mlbridge bot commented Nov 22, 2021

wangweij commented Nov 22, 2021

8277246: Check for NonRepudiation as well when validating a TSA certificate #6416

8277246: Check for NonRepudiation as well when validating a TSA certificate #6416

Conversation

wangweij commented Nov 16, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 16, 2021

openjdk bot commented Nov 16, 2021

mlbridge bot commented Nov 16, 2021 • edited Loading

Webrevs

XueleiFan Nov 16, 2021

Choose a reason for hiding this comment

mlbridge bot commented Nov 16, 2021

wangweij commented Nov 16, 2021

openjdk bot commented Nov 16, 2021 • edited Loading

mlbridge bot commented Nov 16, 2021

wangweij commented Nov 16, 2021

wangweij commented Nov 16, 2021

mlbridge bot commented Nov 16, 2021

mlbridge bot commented Nov 16, 2021

wangweij commented Nov 17, 2021 • edited Loading

mlbridge bot commented Nov 17, 2021

mlbridge bot commented Nov 17, 2021

wangweij commented Nov 17, 2021

seanjmullan commented Nov 17, 2021

wangweij commented Nov 17, 2021

seanjmullan commented Nov 17, 2021

wangweij commented Nov 17, 2021

openjdk bot commented Nov 17, 2021

openjdk bot commented Nov 17, 2021

mlbridge bot commented Nov 22, 2021

wangweij commented Nov 22, 2021

wangweij commented Nov 16, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 16, 2021 •

edited

Loading

openjdk bot commented Nov 16, 2021 •

edited

Loading

wangweij commented Nov 17, 2021 •

edited

Loading