8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable #6106

seanjmullan · 2021-10-25T12:53:43Z

The JDK implementation (as supplied by the "SUN" provider) of X509Certificate::getSubjectAlternativeNames and X509Certificate::getIssuerAlternativeNames returns null instead of throwing a CertificateParsingException when the extension is unparseable.

This fix changes the behavior to comply with the specification.

CSR: https://bugs.openjdk.java.net/browse/JDK-8275822

Progress

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

Issue

JDK-8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable

Reviewers

Weijun Wang (@wangweij - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6106

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

Using diff file

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

… throw CertificateParsingException if extension is unparseable

seanjmullan · 2021-10-25T12:53:59Z

/csr

bridgekeeper · 2021-10-25T12:55:07Z

👋 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 · 2021-10-25T12:57:12Z

@seanjmullan this pull request will not be integrated until the CSR request JDK-8275822 for issue JDK-8251468 has been approved.

openjdk · 2021-10-25T13:00:21Z

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

mlbridge · 2021-10-25T13:30:41Z

Webrevs

wangweij

How about other X509Certificate methods that get info of an extension?

wangweij · 2021-10-25T14:15:04Z

test/jdk/java/security/cert/GetSubjectAlternativeNames/NullRFC822Name.java

+ *          CertificateParsingException if extension is unparseable/invalid.
+ */
+public class NullRFC822Name {
+


Adding a comment showing the ASN.1 fields would be a relief for the reader. OpenSSL shows:

// 430:d=4 hl=2 l= 11 cons: SEQUENCE // 432:d=5 hl=2 l= 3 prim: OBJECT :X509v3 Subject Alternative Name // 437:d=5 hl=2 l= 4 prim: OCTET STRING [HEX DUMP]:30028100

wangweij · 2021-10-25T14:17:56Z

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

@@ -1616,6 +1617,14 @@ public int getBasicConstraints() {
        SubjectAlternativeNameExtension subjectAltNameExt =
            getSubjectAlternativeNameExtension();


Does it make sense to let the line above throwing an exception? I see the method is called in several places (X509CertSelector, Builder, etc). What is the correct behavior in those places?

To clarify, do you mean this code in getExtension(ObjectIdentifier) that swallows the exception?:

} catch (IOException ioe) { return null; }

That's probably a little deeper and changing it will have a mass effect. What about at the getIssuerAlternativeNameExtension level?

Unless I am misunderstanding your comment, I don't think this is an issue in practice. The code inside the X509CertImpl.getExtension method only throws an Exception if invalid OIDs or attribute names are passed to the internal get methods of X509CertInfo and CertificateExtensions, which isn't possible when you are passing in known values/attributes. I think this is why the code swallows the exceptions and returns null, but it would be nice to have a comment explaining that.

I was asking if getIssuerAlternativeNameExtension can throw the exception if IAE exists but not parseable.

Ok, I understand your comment now. I'm hesitant to change those methods to throw an exception because to be consistent all the get*Extension() methods should then throw an Exception. That might be the right thing to do, but it is a bigger change and more risky. The code that calls these internal methods is used for building certification paths, and if null is returned, it is as if the certificate did not contain the extension. That might be a more reasonable behavior than throwing an Exception, since it allows the code to find other potential certificates or certification paths. Adding certpath debug can always be used to find out more about why certain certificates were not selected.

wangweij · 2021-10-25T14:19:08Z

src/java.base/share/classes/sun/security/x509/X509CertImpl.java

@@ -1680,6 +1690,14 @@ public int getBasicConstraints() {
        IssuerAlternativeNameExtension issuerAltNameExt =
            getIssuerAlternativeNameExtension();


Same comment as above, but this method seems to be only called once in src.

seanjmullan · 2021-10-25T15:07:00Z

How about other X509Certificate methods that get info of an extension?

Good question. There are 3: getBasicConstraints, getKeyUsage and getExtendedKeyUsage. The first two methods are not specified to throw an Exception, so that seems like a more significant change and I'm not sure it is worth it. Also, if they are marked critical, then an exception will be thrown when the X509Certificate is parsed.

As for getExtendedKeyUsage, it is specified to throw a CertificateParsingException but it looks like it also has the same problem as the altnames, so I will change the impl to comply with the spec.

…s unparseable. - Renamed and enhanced test to check cert with badly encoded extensions. - Added comments to test describing which fields are badly encoded.

openjdk · 2021-10-29T12:38:53Z

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

8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable

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

4c3491b: 8017175: [TESTBUG] javax/swing/JPopupMenu/4634626/bug4634626.java sometimes failed on mac
c0cda1d: 8273026: Slow LoginContext.login() on multi threading application
15fd8a3: 8276102: JDK-8245095 integration reverted JDK-8247980
e89b2c0: 8276086: Increase size of metaspace mappings
24cf480: 8276047: G1: refactor G1CardSetArrayLocker
e922023: 8275909: [JVMCI] c2v_readFieldValue use long instead of jlong for the offset parameter
157e1d5: 8275856: Remove MetaspaceHandleDeallocations debug switch
c9e65f8: 8275440: Remove VirtualSpaceList::is_full()
de93b1d: 8257722: Improve "keytool -printcert -jarfile" output
21da218: 8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior
... and 154 more: https://git.openjdk.java.net/jdk/compare/dcd6e0da245338de68d9dede451e233f4bfaa934...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.

seanjmullan · 2021-10-29T12:41:24Z

/integrate

openjdk · 2021-10-29T12:42:48Z

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

4c3491b: 8017175: [TESTBUG] javax/swing/JPopupMenu/4634626/bug4634626.java sometimes failed on mac
c0cda1d: 8273026: Slow LoginContext.login() on multi threading application
15fd8a3: 8276102: JDK-8245095 integration reverted JDK-8247980
e89b2c0: 8276086: Increase size of metaspace mappings
24cf480: 8276047: G1: refactor G1CardSetArrayLocker
e922023: 8275909: [JVMCI] c2v_readFieldValue use long instead of jlong for the offset parameter
157e1d5: 8275856: Remove MetaspaceHandleDeallocations debug switch
c9e65f8: 8275440: Remove VirtualSpaceList::is_full()
de93b1d: 8257722: Improve "keytool -printcert -jarfile" output
21da218: 8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior
... and 154 more: https://git.openjdk.java.net/jdk/compare/dcd6e0da245338de68d9dede451e233f4bfaa934...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-10-29T12:43:02Z

@seanjmullan Pushed as commit 8cc5950.

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

8251468: X509Certificate.get{Subject,Issuer}AlternativeNames does not…

1b8ba3d

… throw CertificateParsingException if extension is unparseable

openjdk bot changed the title ~~8251468: X509Certificate.get{Subject,Issuer}AlternativeNames does not…~~ 8251468: X509Certificate.get{Subject,Issuer}AlternativeNames does not throw CertificateParsingException if extension is unparseable Oct 25, 2021

openjdk bot added the csr Pull request needs approved CSR before integration label Oct 25, 2021

openjdk bot added the security security-dev@openjdk.org label Oct 25, 2021

Remove trailing whitespace.

e380c45

openjdk bot added the rfr Pull request is ready for review label Oct 25, 2021

wangweij reviewed Oct 25, 2021

View reviewed changes

openjdk bot removed the rfr Pull request is ready for review label Oct 26, 2021

seanjmullan added 2 commits October 26, 2021 15:10

- Modified getExtendedKeyUsage to also throw exception if extension i…

1017203

…s unparseable. - Renamed and enhanced test to check cert with badly encoded extensions. - Added comments to test describing which fields are badly encoded.

Fix NPE.

371a384

wangweij approved these changes Oct 26, 2021

View reviewed changes

openjdk bot added rfr Pull request is ready for review and removed csr Pull request needs approved CSR before integration labels Oct 29, 2021

openjdk bot added the ready Pull request is ready to be integrated label Oct 29, 2021

openjdk bot closed this Oct 29, 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 Oct 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable #6106

8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable #6106

seanjmullan commented Oct 25, 2021 •

edited by openjdk bot

Loading

seanjmullan commented Oct 25, 2021

bridgekeeper bot commented Oct 25, 2021

openjdk bot commented Oct 25, 2021

openjdk bot commented Oct 25, 2021

mlbridge bot commented Oct 25, 2021 •

edited

Loading

wangweij left a comment

wangweij Oct 25, 2021

wangweij Oct 25, 2021

seanjmullan Oct 25, 2021

wangweij Oct 25, 2021

seanjmullan Oct 25, 2021

wangweij Oct 25, 2021 •

edited

Loading

seanjmullan Oct 26, 2021

wangweij Oct 26, 2021

wangweij Oct 25, 2021

seanjmullan commented Oct 25, 2021

openjdk bot commented Oct 29, 2021 •

edited

Loading

seanjmullan commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

		@@ -1616,6 +1617,14 @@ public int getBasicConstraints() {
		SubjectAlternativeNameExtension subjectAltNameExt =
		getSubjectAlternativeNameExtension();

		@@ -1680,6 +1690,14 @@ public int getBasicConstraints() {
		IssuerAlternativeNameExtension issuerAltNameExt =
		getIssuerAlternativeNameExtension();

8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable #6106

8251468: X509Certificate.get{Subject,Issuer}AlternativeNames and getExtendedKeyUsage do not throw CertificateParsingException if extension is unparseable #6106

Conversation

seanjmullan commented Oct 25, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

seanjmullan commented Oct 25, 2021

bridgekeeper bot commented Oct 25, 2021

openjdk bot commented Oct 25, 2021

openjdk bot commented Oct 25, 2021

mlbridge bot commented Oct 25, 2021 • edited Loading

Webrevs

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangweij Oct 25, 2021 • edited Loading

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

seanjmullan commented Oct 25, 2021

openjdk bot commented Oct 29, 2021 • edited Loading

seanjmullan commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

openjdk bot commented Oct 29, 2021

seanjmullan commented Oct 25, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 25, 2021 •

edited

Loading

wangweij Oct 25, 2021 •

edited

Loading

openjdk bot commented Oct 29, 2021 •

edited

Loading