8296343: CPVE thrown on missing content-length in OCSP response #11917

jnimeh · 2023-01-10T06:02:29Z

This fixes an issue where HTTP responses that do not have an explicit Content-Length are causing an EOFException which unravels into a CertPathValidatorException during validations that involve OCSP checks.

JBS: https://bugs.openjdk.org/browse/JDK-8296343

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-8296343: CPVE thrown on missing content-length in OCSP response

Reviewers

Sean Mullan (@seanjmullan - Reviewer)
Rajan Halade (@rhalade - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11917

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

Using diff file

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

jnimeh · 2023-01-10T06:02:47Z

/issue 8296343

bridgekeeper · 2023-01-10T06:04:23Z

👋 Welcome back jnimeh! 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.

jnimeh · 2023-01-10T06:05:40Z

/label security

openjdk · 2023-01-10T06:07:23Z

@jnimeh This issue is referenced in the PR title - it will now be updated.

openjdk · 2023-01-10T06:07:43Z

@jnimeh
The security label was successfully added.

mlbridge · 2023-01-10T06:11:39Z

Webrevs

mcpowers · 2023-01-10T15:49:26Z

test/jdk/sun/security/provider/certpath/OCSP/OCSPNoContentLength.java

+    static String EE_ALIAS = "endentity";
+
+    // Turn on debugging
+    static final boolean debug = true;


Do you really mean to set debug to true?

The overall output is pretty small even with it on, but I'll switch it off.

You could also use Boolean.getBoolean("test.debug") (or some other property name) so it can be set on the command line when the test is run.

XueleiFan · 2023-01-10T17:30:08Z

src/java.base/share/classes/sun/security/provider/certpath/OCSP.java

-
-            return IOUtils.readExactlyNBytes(con.getInputStream(),
-                    contentLength);
+            return (contentLength == -1) ? con.getInputStream().readAllBytes() :


For the returned OCSP bytes, what if the response code is not OK?

Well, in the case of a 404 what appears to happen is that HttpURLConnection would throw a FileNotFoundException. That ultimately would result in a CPVE if there were no other sources of revocation information (e.g. CRL) for that certificate.

It may be more effective/accuracy to stop read OCSP response bytes if response code is not OK.

Logging the error code and returning with no read and not throwing an exception I believe would still work since the revocation information would be missing. I'm wondering though if this needs to be a separate issue given that we're talking about a different use case, and one that involves the behavior of HttpURLConnection when dealing with different response codes. I'll also check to see if there are existing tests that make CPV checks against URIs that have non-200 response codes.

Hmmm, I was not quite correct about the HttpURLConnection behavior - it's not the 404 that's causing the issue directly, it is indeed the getContentLength when the 404 happens. So forget a separate issue, I will deal with non-200 codes in this PR.

mpdonova · 2023-01-10T15:33:16Z

test/jdk/sun/security/provider/certpath/OCSP/OCSPNoContentLength.java

+        rootOcsp.start();
+
+        // Wait 5 seconds for server ready
+        for (int i = 0; (i < 100 && !rootOcsp.isServerReady()); i++) {


This pattern is repeated over 20 times in the code. Instead of spinning on a boolean, the SimpleOCSPServer class could use a CountdownLatch to signal when it's ready. Then, instead of having an isServerReady() method, it would just have a method e.g., boolean waitForServer(long timeout, TimeUnit unit) which just delegates to CountdownLatch.await(long, TimeUnit).

And to avoid changing 20+ other tests, just mark isServerReady() as deprecated.

mpdonova · 2023-01-10T17:38:25Z

test/jdk/sun/security/provider/certpath/OCSP/OCSPNoContentLength.java

+    static String EE_ALIAS = "endentity";
+
+    // Turn on debugging
+    static final boolean debug = true;


You could also use Boolean.getBoolean("test.debug") (or some other property name) so it can be set on the command line when the test is run.

… tests

mpdonova · 2023-01-12T15:31:30Z

test/jdk/sun/security/provider/certpath/OCSP/OCSPNoContentLength.java

+//        }
+//        if (!rootOcsp.isServerReady()) {
+//            throw new RuntimeException("Server not ready yet");
+//        }


Lines 149-154 can be deleted

I thought I caught all the dead comments, but I guess I missed this one. Good catch, will fix.

seanjmullan

I have reviewed the code changes to OCSP.java and it looks fine. I have not reviewed the test changes though, please find a separate Reviewer for those changes.

openjdk · 2023-01-19T23:11:44Z

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

8296343: CPVE thrown on missing content-length in OCSP response

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

079255e: 8300864: Declare some fields in java.io as final
a56598f: 8299684: (bf) JNI direct buffer functions with large capacity behave unexpectedly
542bfe6: 8300587: (bf) Some covariant overrides are missing @SInCE tags
03a9a88: 8300265: Remove metaprogramming/isSigned.hpp
5a4945c: 8299975: Limit underflow protection CMoveINode in PhaseIdealLoop::do_unroll must also protect type from underflow
f307e8c: 8299795: Relativize locals in interpreter frames
11aadc9: 8244400: MenuItem may cache the size and did not update it when the screen DPI is changed
836198a: 8300591: @SuppressWarnings option "lossy-conversions" missing from jdk.compiler module javadoc
45e4e00: 8300079: SIGSEGV in LibraryCallKit::inline_string_copy due to constant NULL src argument
030b071: 8300207: Add a pre-check for the number of canonical equivalent permutations in j.u.r.Pattern
... and 243 more: https://git.openjdk.org/jdk/compare/7607c07e002cd86cf2a0f44df9933612550ced95...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.

jnimeh · 2023-01-23T17:51:12Z

/integrate

openjdk · 2023-01-23T18:07:40Z

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

86fed79: 8300693: Lower the compile threshold and reduce the iterations of warmup loop in VarHandles tests
4525aa3: 8300867: Fix document issues in java.io
a7f035d: 8300868: Reduce visibility in java.io.SerialCallbackContext
079255e: 8300864: Declare some fields in java.io as final
a56598f: 8299684: (bf) JNI direct buffer functions with large capacity behave unexpectedly
542bfe6: 8300587: (bf) Some covariant overrides are missing @SInCE tags
03a9a88: 8300265: Remove metaprogramming/isSigned.hpp
5a4945c: 8299975: Limit underflow protection CMoveINode in PhaseIdealLoop::do_unroll must also protect type from underflow
f307e8c: 8299795: Relativize locals in interpreter frames
11aadc9: 8244400: MenuItem may cache the size and did not update it when the screen DPI is changed
... and 246 more: https://git.openjdk.org/jdk/compare/7607c07e002cd86cf2a0f44df9933612550ced95...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-23T18:12:33Z

@jnimeh Pushed as commit 1a3cb8c.

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

8296343: CPVE thrown on missing content-length in OCSP response

16a60c8

jnimeh marked this pull request as ready for review January 10, 2023 06:03

openjdk bot added the rfr Pull request is ready for review label Jan 10, 2023

openjdk bot added the security security-dev@openjdk.org label Jan 10, 2023

mcpowers reviewed Jan 10, 2023

View reviewed changes

XueleiFan reviewed Jan 10, 2023

View reviewed changes

mpdonova reviewed Jan 10, 2023

View reviewed changes

jnimeh added 2 commits January 12, 2023 06:36

Moved SimpleOCSPServer to use CountdownLatch for ready state, updated…

beff50a

… tests

Throw exception directly from non 200 HTTP response codes

36a0911

mpdonova reviewed Jan 12, 2023

View reviewed changes

Remove dead commented code

ddcd124

seanjmullan approved these changes Jan 19, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 19, 2023

rhalade approved these changes Jan 23, 2023

View reviewed changes

XueleiFan approved these changes Jan 23, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 23, 2023

openjdk bot closed this Jan 23, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 23, 2023

jnimeh deleted the JDK-8296343 branch January 24, 2023 01:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8296343: CPVE thrown on missing content-length in OCSP response #11917

8296343: CPVE thrown on missing content-length in OCSP response #11917

jnimeh commented Jan 10, 2023 •

edited by openjdk bot

jnimeh commented Jan 10, 2023

bridgekeeper bot commented Jan 10, 2023

jnimeh commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

mlbridge bot commented Jan 10, 2023 •

edited

mcpowers Jan 10, 2023

jnimeh Jan 10, 2023

mpdonova Jan 10, 2023

XueleiFan Jan 10, 2023

jnimeh Jan 10, 2023

XueleiFan Jan 10, 2023

jnimeh Jan 10, 2023

jnimeh Jan 10, 2023

mpdonova Jan 10, 2023

mpdonova Jan 10, 2023

mpdonova Jan 12, 2023

jnimeh Jan 12, 2023

seanjmullan left a comment

openjdk bot commented Jan 19, 2023 •

edited

jnimeh commented Jan 23, 2023

openjdk bot commented Jan 23, 2023

openjdk bot commented Jan 23, 2023

8296343: CPVE thrown on missing content-length in OCSP response #11917

8296343: CPVE thrown on missing content-length in OCSP response #11917

Conversation

jnimeh commented Jan 10, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

jnimeh commented Jan 10, 2023

bridgekeeper bot commented Jan 10, 2023

jnimeh commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

openjdk bot commented Jan 10, 2023

mlbridge bot commented Jan 10, 2023 • edited

Webrevs

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

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

seanjmullan left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 19, 2023 • edited

jnimeh commented Jan 23, 2023

openjdk bot commented Jan 23, 2023

openjdk bot commented Jan 23, 2023

jnimeh commented Jan 10, 2023 •

edited by openjdk bot

mlbridge bot commented Jan 10, 2023 •

edited

openjdk bot commented Jan 19, 2023 •

edited