[ocsp stapling] reject incorrect http response if the status code is not 2xx

[cdlliuy]

Draft a possible fix for #89907

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Draft a possible fix for #89907

Author:	cdlliuy
Assignees:	-
Labels:	area-System.Net.Http, community-contribution
Milestone:	-

[ghost]

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Draft a possible fix for #89907

Author:	cdlliuy
Assignees:	-
Labels:	area-System.Security, community-contribution
Milestone:	-

[vcsjones]

If the server really is indeed stapling a nonsense OCSP then I think a more appropriate fix would be to make sure that the stapled response is a well formed OCSP response, valid for the certificate, etc.

This fix as written will still staple incorrect responses if happens to respond with non-OCSP content but with an HTTP 200 status code.

Also, returning null for >= HTTP 300 will break following redirects.

A more appropriate fix might be that in

runtime/src/native/libs/System.Security.Cryptography.Native/pal_ssl.c

Line 1273 in 44b0543

if (SSL_set_tlsext_status_ocsp_resp(ssl, copy, len) != 1)

We might consider parsing the content and, if it isn’t a parseable OCSP, then skipping the stapling. That still doesn’t address the problem where a wrong OCSP response might be stapled.

[wfurt]

we may still bail for anything > 400 @vcsjones ? The body is likely some kind of error message anyway.

[vcsjones]

still bail for anything > 400 @vcsjones ?

That seems reasonable, but not a complete fix. I would guess that there is greater than zero CAs out there that return an error result with an HTTP 200, or someone has a proxy, captive portal, etc. that intercepts HTTP requests and may return HTTP 200 pages.

[bartonjs]

There should be a path when we download it where we crack the payload to extract the notAfter value. That path should be able to be enhanced to say not just "I didn't get a date from it" (it's optional), but also "it's not a valid response". I kinda thought it was already checking that, but apparently not. (Leaving a note in case someone has the time to dig before I do)

[bartonjs]

• runtime/src/libraries/System.Net.Security/src/System/Net/Security/SslStreamCertificateContext.Linux.cs

  Lines 226 to 233 in f43b0c1

  	ret = await System.Net.Http.X509ResourceClient.DownloadAssetAsync(url, TimeSpan.MaxValue).ConfigureAwait(false);
  	if (ret is not null)
  	{
  	if (!Interop.Crypto.X509DecodeOcspToExpiration(ret, ocspRequest, subject, issuer, out DateTimeOffset expiration))
  	{
  	continue;
  	}

  calls X509DecodeOcspToExpiration, and ignores the response if it fails

• runtime/src/libraries/Common/src/Interop/Unix/System.Security.Cryptography.Native/Interop.OCSP.cs

  Line 35 in f43b0c1

  internal static unsafe bool X509DecodeOcspToExpiration(

  calls CryptoNative_X509DecodeOcspToExpiration, and fails if that call returns 0.

• runtime/src/native/libs/System.Security.Cryptography.Native/pal_x509.c

  Lines 1305 to 1319 in f447532

  	int32_t CryptoNative_X509DecodeOcspToExpiration(const uint8_t* buf, int32_t len, OCSP_REQUEST* req, X509* subject, X509* issuer, int64_t* expiration)
  	{
  	ERR_clear_error();
  	if (buf == NULL || len == 0)
  	{
  	return 0;
  	}
  	OCSP_RESPONSE* resp = d2i_OCSP_RESPONSE(NULL, &buf, len);
  	if (resp == NULL)
  	{
  	return 0;
  	}

  calls d2i_OCSP_RESPONSE, and returns 0 if that fails (or things about it look wrong).

So, I don't think the proposed fix would even help, since it should be hitting the same "we ignored this" case that should already be happening.

If a bad response got in there, it sort of feels like some very wonky thing happened, like that array got overwritten?

CryptoNative_X509DecodeOcspToExpiration looks like it might pass if there was a legitimate response plus some dangling garbage, which probably isn't what was intended, but also doesn't sound like the problem that was experienced.

[cdlliuy]

yeah, I agree the original fix won't work (and also break the redirect flow by typo. Checking status code>=400 is better than current)

So the current idea is to validate the OCSP content, and cache the correct format only. Is my understanding correct?
will "retrying the connection to ocsp responder" help here in case it is an intermittent failure.

Then, for the failed case, i.e. a bad response status code or bad content, does it still "return null" for DownloadOcsp request?
When "returning null", what will happen in client side? will client side fall back to client-driven ocsp? or just fail the https connection request?

[bartonjs]

Since the proposed fix is not correct (it's invalidating the redirect handler code immediately below it), and doesn't feel like the right shape at all (an HTTP 200 with an error message is still "not a valid OCSP payload"), I'm going ahead and closing this PR. We'll continue discussion/investigation on the issue instead of the PR.