8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer #20528

mcpowers · 2024-08-09T17:22:56Z

https://bugs.openjdk.org/browse/JDK-8336665

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer (Bug - P3)

Reviewers

Sean Mullan (@seanjmullan - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/20528/head:pull/20528
$ git checkout pull/20528

Update a local copy of the PR:
$ git checkout pull/20528
$ git pull https://git.openjdk.org/jdk.git pull/20528/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 20528

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/20528.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-08-09T17:23:26Z

👋 Welcome back mpowers! 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 · 2024-08-09T17:38:02Z

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

8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer

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

7af46a6: 8340554: Improve MessageFormat readObject checks
7d5eefa: 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang
d8c3b0f: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
3c14c2b: 8341566: Add Reader.of(CharSequence)
b0ac633: 8342075: HttpClient: improve HTTP/2 flow control checks
85774b7: 8342882: RISC-V: Unify handling of jumps to runtime
2c31c8e: 8339730: Windows regression after removing ObjectMonitor Responsible
f0b130e: 8339296: Record deconstruction pattern in switch fails to compile
e96b4cf: 8342387: C2 SuperWord: refactor and improve compiler/loopopts/superword/TestDependencyOffsets.java
f7a61fc: 8342931: ProblemList failing tests from JDK-8335912
... and 1519 more: https://git.openjdk.org/jdk/compare/1b1dba8082969244effa86ac03c6053b3b0ddc43...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 · 2024-08-09T17:52:44Z

@mcpowers 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 · 2024-08-09T17:56:06Z

Webrevs

mcpowers · 2024-08-09T18:07:31Z

Need to update copyright on X509CRLImpl.java.

seanjmullan · 2024-08-12T19:20:17Z

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

-                X500Name issuerDN = (X500Name) names.get(0).getName();
+                X500Name issuerDN;
+                try {
+                    issuerDN = (X500Name) names.get(0).getName();


Use instanceof X500Name here to see if it is the right type instead of catching the ClassCastException.

Fixed. Good Idea.

seanjmullan · 2024-09-11T13:26:27Z

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

                GeneralNames names = ciExt.getNames();
-                X500Name issuerDN = (X500Name) names.get(0).getName();
+                if (!(names.get(0).getName() instanceof X500Name issuerDN)) {
+                    throw new CRLException("Parsing error: bad X500 issuer");


Suggest a slight rewording of the error message: "Parsing error: issuer is not an X.500 DN"

seanjmullan · 2024-09-24T12:33:54Z

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

+                if (!(names.get(0).getName() instanceof X500Name issuerDN)) {
+                    throw new CRLException("Parsing error: "
+                            + "issuer is not an X.500 DN");
+                }


I checked RFC 5280 and you can have more than one name in the CertificateIssuer field of the CertificateIssuerExtension, see https://www.rfc-editor.org/rfc/rfc5280#section-5.3.3

But for this code, we are only interested in the X500Name, as we subsequently use that to associate the CRL entry with its issuer. So instead, what you should do is loop thru the names until we find an X500Name, and only throw a CRLException if we don't find an X500Name. Let me know if this makes sense.

Does the test need to be modified to test for more than one name? I could go either way.

Are you able to easily create test CRLs with more than one entry? If not, I think the existing test is ok.

I know how to create a CertificateIssuerExtension with multiple names but I haven't connected the dots as to how to add it to a CRL for testing. It would be an interesting exercise but probably not worth the effort.

seanjmullan · 2024-10-18T13:40:38Z

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

+                    }
+                }
+                throw new CRLException("Parsing error: "
+                        + "issuer is not an X.500 DN");


To be more precise, I suggest changing this message to "CertificateIssuer field does not contain an X.500 DN".

seanjmullan · 2024-10-21T17:28:42Z

test/jdk/sun/security/x509/X509CRLImpl/UnexpectedCCE.java

+
+        // "class sun.security.x509.OIDName cannot be cast
+        // to class sun.security.x509.X500Name"
+        byte[] encoded_1 = Base64.getDecoder().decode("""


Could you add some comments before this line and line 59 as to what is in the CRL that makes the format invalid? (Ex: This CRL contains a CertificateIssuerExtension that is not compliant with RFC 5280 because it does not contain a DN)

The CRL is being constructed from a fuzzed data input stream. All I know is that the name in the CertificateIssuerExtension looks like an x509.OIDName in the first test, and in the second test it looks like an x509.X400Address.

I can add these two comments to the test:
"Fuzzed data input stream looks like an x509.OIDName." and
"Fuzzed data input stream looks like an x509.X400Address.".

Yes, I think that would be helpful, but also say that these are in the CertificateIssuerExtension so it is more clear what part of the CRL is being tested for parsing issues.

mcpowers · 2024-10-24T22:00:43Z

/integrate

openjdk · 2024-10-24T23:10:12Z

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

d1540e2: 8342090: Infer::IncorporationBinaryOp::equals can produce side-effects
7af46a6: 8340554: Improve MessageFormat readObject checks
7d5eefa: 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang
d8c3b0f: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
3c14c2b: 8341566: Add Reader.of(CharSequence)
b0ac633: 8342075: HttpClient: improve HTTP/2 flow control checks
85774b7: 8342882: RISC-V: Unify handling of jumps to runtime
2c31c8e: 8339730: Windows regression after removing ObjectMonitor Responsible
f0b130e: 8339296: Record deconstruction pattern in switch fails to compile
e96b4cf: 8342387: C2 SuperWord: refactor and improve compiler/loopopts/superword/TestDependencyOffsets.java
... and 1520 more: https://git.openjdk.org/jdk/compare/1b1dba8082969244effa86ac03c6053b3b0ddc43...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-24T23:10:49Z

@mcpowers Pushed as commit ca1700b.

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

first iteration

72fb3db

mcpowers changed the title ~~CCE in X509CRLImpl$TBSCertList.getCertIssuer~~ 8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer Aug 9, 2024

openjdk bot added the rfr Pull request is ready for review label Aug 9, 2024

openjdk bot added the security security-dev@openjdk.org label Aug 9, 2024

copyright and unnecessary initialization

dd198f2

seanjmullan reviewed Aug 12, 2024

View reviewed changes

comment from Sean

f9b8d0b

seanjmullan reviewed Sep 11, 2024

View reviewed changes

another comment from Sean

6907ae4

seanjmullan reviewed Sep 24, 2024

View reviewed changes

allow more than one name

336f81a

seanjmullan reviewed Oct 18, 2024

View reviewed changes

more precise exception message

91e225a

seanjmullan reviewed Oct 21, 2024

View reviewed changes

more precise comment

007fbf0

seanjmullan approved these changes Oct 24, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 24, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 24, 2024

openjdk bot closed this Oct 24, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Oct 24, 2024

mcpowers deleted the JDK-8336665 branch October 28, 2024 14:28

8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer #20528

8336665: CCE in X509CRLImpl$TBSCertList.getCertIssuer #20528

Uh oh!

Conversation

mcpowers commented Aug 9, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 9, 2024

Uh oh!

openjdk bot commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Aug 9, 2024

Uh oh!

mlbridge bot commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

mcpowers commented Aug 9, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanjmullan Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcpowers commented Oct 24, 2024

Uh oh!

openjdk bot commented Oct 24, 2024

Uh oh!

openjdk bot commented Oct 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

mcpowers commented Aug 9, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Aug 9, 2024 •

edited

Loading

mlbridge bot commented Aug 9, 2024 •

edited

Loading

seanjmullan Sep 24, 2024 •

edited

Loading