[PATCH 0/2] MR1940: Cryptnet ocsp caching

[Paul Gofman (＠gofman)]

OCSP support was introduced in the latest development cycle. In improves revocation check performance in general, although the caching is currently performed by CRL revocation check only. Also, even that result is not used before performing OCSP check which goes first, effectively forcing OCSP check to be always performed when a certificate has OCSP info. That leads to performance regression in some cases.

I believe once we validated revocation either way it is safe to assume the cached result valid until NextUpdate regardless of the method (that what the first patch does). Then, we can also cache the result of OCSP response the same way we do for CRL (second patch).

[PATCH 1/2] cryptnet: Check cached revocation status in verify_cert_revocation().

[Paul Gofman]

From: Paul Gofman

---
 dlls/cryptnet/cryptnet_main.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/dlls/cryptnet/cryptnet_main.c b/dlls/cryptnet/cryptnet_main.c
index 19de1ed2d8e..01ded96d88f 100644
--- a/dlls/cryptnet/cryptnet_main.c
+++ b/dlls/cryptnet/cryptnet_main.c
@@ -1696,9 +1696,6 @@ static DWORD verify_cert_revocation_from_dist_points_ext(const CRYPT_DATA_BLOB *
         return CRYPT_E_REVOCATION_OFFLINE;
     }

-    if (find_cached_revocation_status(&cert->pCertInfo->SerialNumber, time, status))
-        return status->dwError;
-
     if (!CRYPT_GetUrlFromCRLDistPointsExt(value, NULL, &url_array_size, NULL, NULL))
         return GetLastError();

@@ -2146,6 +2143,12 @@ static DWORD verify_cert_revocation(const CERT_CONTEXT *cert, FILETIME *pTime,
     DWORD error = ERROR_SUCCESS;
     PCERT_EXTENSION ext;

+    if (find_cached_revocation_status(&cert->pCertInfo->SerialNumber, pTime, pRevStatus))
+    {
+        if (pRevStatus->dwError == ERROR_SUCCESS || pRevStatus->dwError == CRYPT_E_REVOKED)
+            return pRevStatus->dwError;
+    }
+
     if ((ext = CertFindExtension(szOID_AUTHORITY_INFO_ACCESS, cert->pCertInfo->cExtension, cert->pCertInfo->rgExtension)))
     {
         error = verify_cert_revocation_from_aia_ext(&ext->Value, cert, pTime, dwFlags, pRevPara, pRevStatus);

[PATCH 2/2] cryptnet: Also cache revocation status when using OCSP.

[Paul Gofman]

From: Paul Gofman

---
 dlls/cryptnet/cryptnet_main.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/dlls/cryptnet/cryptnet_main.c b/dlls/cryptnet/cryptnet_main.c
index 01ded96d88f..516bbad9ca4 100644
--- a/dlls/cryptnet/cryptnet_main.c
+++ b/dlls/cryptnet/cryptnet_main.c
@@ -1895,11 +1895,12 @@ static BOOL match_cert_id(const OCSP_CERT_ID *id, const CERT_INFO *cert, const C
 }

static DWORD check_ocsp_response_info(const CERT_INFO *cert, const CERT_INFO *issuer,
-                                      const CRYPT_OBJID_BLOB *blob, DWORD *status)
+                                      const CRYPT_OBJID_BLOB *blob, DWORD *status, FILETIME *next_update)
 {
     OCSP_BASIC_RESPONSE_INFO *info;
     DWORD size, i;

+    memset(next_update, 0, sizeof(*next_update));
     if (!CryptDecodeObjectEx(X509_ASN_ENCODING, OCSP_BASIC_RESPONSE, blob->pbData, blob->cbData,
                              CRYPT_DECODE_ALLOC_FLAG, NULL, &info, &size)) return GetLastError();

@@ -1907,7 +1908,11 @@ static DWORD check_ocsp_response_info(const CERT_INFO *cert, const CERT_INFO *is
     for (i = 0; i < info->cResponseEntry; i++)
     {
         OCSP_BASIC_RESPONSE_ENTRY *entry = &info->rgResponseEntry[i];
-        if (match_cert_id(&entry->CertId, cert, issuer)) *status = map_ocsp_status(entry->dwCertStatus);
+        if (match_cert_id(&entry->CertId, cert, issuer))
+        {
+            *status = map_ocsp_status(entry->dwCertStatus);
+            *next_update = entry->NextUpdate;
+        }
     }

LocalFree(info);
@@ -1920,6 +1925,7 @@ static DWORD verify_signed_ocsp_response_info(const CERT_INFO *cert, const CERT_
     OCSP_BASIC_SIGNED_RESPONSE_INFO *info;
     DWORD size, error, status = CRYPT_E_REVOCATION_OFFLINE;
     CRYPT_ALGORITHM_IDENTIFIER *alg;
+    FILETIME next_update;
     CRYPT_BIT_BLOB *sig;
     HCRYPTPROV prov = 0;
     HCRYPTHASH hash = 0;
@@ -1929,7 +1935,7 @@ static DWORD verify_signed_ocsp_response_info(const CERT_INFO *cert, const CERT_
     if (!CryptDecodeObjectEx(X509_ASN_ENCODING, OCSP_BASIC_SIGNED_RESPONSE, blob->pbData, blob->cbData,
                              CRYPT_DECODE_ALLOC_FLAG, NULL, &info, &size)) return GetLastError();

-    if ((error = check_ocsp_response_info(cert, issuer, &info->ToBeSigned, &status))) goto done;
+    if ((error = check_ocsp_response_info(cert, issuer, &info->ToBeSigned, &status, &next_update))) goto done;

alg = &info->SignatureInfo.SignatureAlgorithm;
     if (!alg->pszObjId || !(algid = CertOIDToAlgId(alg->pszObjId)))
@@ -1958,6 +1964,16 @@ static DWORD verify_signed_ocsp_response_info(const CERT_INFO *cert, const CERT_
     else error = ERROR_SUCCESS;

done:
+    if (next_update.dwLowDateTime || next_update.dwHighDateTime)
+    {
+        CERT_REVOCATION_STATUS rev_status;
+
+        memset(&rev_status, 0, sizeof(rev_status));
+        rev_status.cbSize = sizeof(rev_status);
+        rev_status.dwError = status;
+        cache_revocation_status(&cert->SerialNumber, &next_update, &rev_status);
+    }
+
     CryptDestroyKey(key);
     CryptDestroyHash(hash);
     CryptReleaseContext(prov, 0);

[Hans Leidekker (＠hans)]

I would expect the OCSP response to be read from the wininet cache on subsequent requests. Is that not the case here? Caching the CRL check makes sense because CRLs can become very large and parsing them takes a long time. That's not the case with OCSP responses.

[Paul Gofman (＠gofman)]

For some reason it is probably not (I am not immediately fully sure but I think it redownloads all the time). It might be something to recheck and potentially an issue on its own, but there is at least one legitimate  case when wininet caching won't help. That is, when the request fails to get response for whatever reason (outdated ocsp link in certificate which happens; other reasons why request fails, like currently we have a bug when the request is wrongly built for http://ocsp2.globalsign.com/rootr3 where it doesn't handle /rootr3 path in URI and omits it; possibly other cases). Currently in such cases, even if we successfully fallback to CRL and cached the success, we will repeat OCSP part first on each revocation verify request. And that is quite lengthy process.

So it seems to me that just checking valid status is something we need to do regardless to avoid lengthy process in case of http(s) requests failures (that is, the first patch). Then, the second patch is indeed should not be saving that much provided wininet cache works, but at the same time it looks simple and unifies the revocation caching. Maybe it still makes sense to have it? Also, we are probably on the mercy of the server here, what if, e. g., it is specifies no-cache while response itself has NextUpdate in the future?

[Hans Leidekker (＠hans)]

I'd rather figure out why the response isn't read from wininet cache first. Optimizing corner cases like outdated links or servers specifying no-cache may be a good reason to do this, if there's evidence that Windows does it.

[Paul Gofman]

On 1/12/23 04:47, Hans Leidekker (@hans) wrote:
> I'd rather figure out why the response isn't read from wininet cache first. Optimizing corner cases like outdated links or servers specifying no-cache may be a good reason to do this, if there's evidence that Windows does it.

I looked into the case which inspired this a bit more, and wininet 
caches are actually working when http request is successful. Yet I think 
there are things to mention:

1. cache max age (as returned by globalsign.com Web server) is 3600sec. 
While OCSP (and CRL) validity period it reports inside the response is a 
few days.

2. The whole OCSP check process, even with caches used, takes good 
100-150ms.

Do you know any specific reasons to prefer Web response caching and 
avoid revokation specific caching? Those things look functionally 
unrelated to me in the first place: http response vailidity time can be 
wrong both ways, isn't it just correct to rely on the protocol specific 
validity indication, and also use already existing revocation check 
caching to avoid all that entirely: extra requests each hour instead of 
once a few days, shorten that 100ms when response is cached, cover the 
corner cases like wrong ocsp url?

[Hans Leidekker (＠hans)]

On Thu Jan 12 16:43:06 2023 +0000, **** wrote:
> Paul Gofman replied on the mailing list:
> On 1/12/23 04:47, Hans Leidekker (@hans) wrote:
> > I'd rather figure out why the response isn't read from wininet cache
> first. Optimizing corner cases like outdated links or servers specifying
> no-cache may be a good reason to do this, if there's evidence that
> Windows does it.
> >
> I looked into the case which inspired this a bit more, and wininet 
> caches are actually working when http request is successful. Yet I think 
> there are things to mention:
> 1. cache max age (as returned by globalsign.com Web server) is 3600sec. 
> While OCSP (and CRL) validity period it reports inside the response is a 
> few days.
> 2. The whole OCSP check process, even with caches used, takes good 
> 100-150ms.
> Do you know any specific reasons to prefer Web response caching and 
> avoid revokation specific caching? Those things look functionally 
> unrelated to me in the first place: http response vailidity time can be 
> wrong both ways, isn't it just correct to rely on the protocol specific 
> validity indication, and also use already existing revocation check 
> caching to avoid all that entirely: extra requests each hour instead of 
> once a few days, shorten that 100ms when response is cached, cover the 
> corner cases like wrong ocsp url?

It's not that I prefer web caching. I wanted to be convinced that OCSP status caching is needed even when the response is already cached by wininet, since you said that it might not be cached at all. I think you have demonstrated that it is needed, thanks.

[PATCH 0/2] MR1940: Cryptnet ocsp caching - approved

[Hans Leidekker (＠hans)]

This merge request was approved by Hans Leidekker.