pkix/est: NullPointerException on HTTP/1.1 Transfer-Encoding chunked during ESTResponse processing

[adippel]

Problem Statement

I am using EST simpleEnroll (bcpkix-jdk18on, 1.72) against an EST Server which answers the request using Transfer-Encoding Chunked. When the response is processed, the following exception occurs:

Exception in thread "main" org.bouncycastle.est.ESTException: Cannot invoke "java.lang.Long.longValue()" because "this.max" is null HTTP Status Code: 0
	at org.bouncycastle.est.ESTService.enroll(ESTService.java:322)
	at org.bouncycastle.est.ESTService.simpleEnroll(ESTService.java:352)
	at com.bbraun.cit.pki.est.Main.main(Main.java:59)
Caused by: java.lang.NullPointerException: Cannot invoke "java.lang.Long.longValue()" because "this.max" is null
	at org.bouncycastle.est.CTEBase64InputStream.pullFromSrc(CTEBase64InputStream.java:43)
	at org.bouncycastle.est.CTEBase64InputStream.read(CTEBase64InputStream.java:104)
	at java.base/java.io.FilterInputStream.read(FilterInputStream.java:82)
	at org.bouncycastle.asn1.ASN1InputStream.readObject(ASN1InputStream.java:189)
	at org.bouncycastle.est.ESTService.handleEnrollResponse(ESTService.java:634)
	at org.bouncycastle.est.ESTService.enroll(ESTService.java:311)
	... 2 more

Problem Analysis

The problematic code starts in ESTResponse:

bc-java/pkix/src/main/java/org/bouncycastle/est/ESTResponse.java

Lines 165 to 168 in c8ff5b1

	if ("base64".equalsIgnoreCase(getHeader("content-transfer-encoding")))
	{
	inputStream = new CTEBase64InputStream(inputStream, getContentLength());
	}

When we have a chunked transfer, getContentLength() returns null since no content length is known before the transfer is complete. So CTEBase64InputStream's constructor receives null as its reading limit max. This leads to the fact that, CTEBase64InputStream#pullFromSrc produces the above mentioned NPE on the following line:

bc-java/pkix/src/main/java/org/bouncycastle/est/CTEBase64InputStream.java

Lines 43 to 46 in c8ff5b1

	if (this.read >= this.max)
	{
	return -1;
	}

Solution

Passing a reading limit is not feasible when dealing with chunked transfers since the Content-Length header is not set. Besides chunked transfers, passing a reading limit is not neccessray at all since all inputStreams which are passed to the constructor return -1 when finished. All CTEBase64InputStream's methods respect this convention and consider the processing to be finished. Hence, removing this reading limit is fine.

I created a patch and tested it against real world servers. Chunked Transfers and non chunked transfers are working.

I am happy to provide a PR.

[dghgit]

Thanks for the report. Yes, please provide a PR.

[adippel]

Thanks for the report. Yes, please provide a PR.

Opened a PR: #1325

Added the bugfix. In addition, I also added a bunch of unit tests as well as a regression test for this issue.

[adippel]

Hey @dghgit, I just discovered another bug in ESTService related to chunked transfer encoding when responses are processed:

bc-java/pkix/src/main/java/org/bouncycastle/est/ESTService.java

Lines 160 to 166 in 8d9afb8

	if (resp.getContentLength() != null && resp.getContentLength() > 0)
	{
	ASN1InputStream ain = new ASN1InputStream(resp.getInputStream());
	SimplePKIResponse spkr = new SimplePKIResponse(ContentInfo.getInstance((ASN1Sequence)ain.readObject()));
	caCerts = spkr.getCertificates();
	crlHolderStore = spkr.getCRLs();
	}

As argued above, checking the content length is not feasible when dealing with chunked transfer encoding. Additionally, there is no need to check the content length since the underlying ESTResponse is, starting with this PR, handling all transfers correctly.

The ESTService has another mehtod which is prone to the exact same error:

bc-java/pkix/src/main/java/org/bouncycastle/est/ESTService.java

Lines 683 to 688 in 8d9afb8

	if (resp.getContentLength() != null && resp.getContentLength() > 0)
	{
	ASN1InputStream ain = new ASN1InputStream(resp.getInputStream());
	ASN1Sequence seq = ASN1Sequence.getInstance(ain.readObject());
	response = new CSRAttributesResponse(CsrAttrs.getInstance(seq));
	}

I removed those lines and I have proper response processing. Shall I add my changes to the existing PR?

[dghgit]

Yes please! Thanks.

[dghgit]

The fix for this is in 1,73, which has now been published.

[adippel]

Thanks for working on this. We will switch to 1.73 in favor of our internal fork :)