openssl: fix potential NULL dereference when loading certs (Windows) by vszakats · Pull Request #20634 · curl/curl

vszakats · 2026-02-19T17:12:31Z

This could happen if the first cert to be loaded missed EKU (Enhanced
Key Usage) data, when using native CA on Windows.

Fix by skipping certs without Enhanced Key Usage data.

Detected by clang-tidy:

lib/vtls/openssl.c:2922:15: warning: Access to field 'cUsageIdentifier'
 results in a dereference of a null pointer (loaded from variable
 'enhkey_usage') [clang-analyzer-core.NullDereference]
 2922 |           if(!enhkey_usage->cUsageIdentifier) {
      |               ^

Refs:
https://learn.microsoft.com/windows/win32/secgloss/e-gly
https://learn.microsoft.com/windows/win32/api/wincrypt/nf-wincrypt-certgetenhancedkeyusage
https://gitlab.winehq.org/wine/wine/-/blob/wine-11.2/dlls/crypt32/cert.c?ref_type=tags#L3061-3164

Assisted-by: Stefan Eissing

https://github.com/curl/curl/pull/20634/files?w=1

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

vszakats · 2026-02-19T18:03:54Z

@aisle-analyzer

aisle-research-bot · 2026-02-19T18:04:00Z

🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.
_{Aisle supplements but does not replace security review.}

Analyzed PR: #20634 at commit 5f619a0

vszakats · 2026-02-19T18:05:45Z

augment review

augmentcode · 2026-02-19T18:09:44Z

🤖 Augment PR Summary

Summary: Hardens Windows system certificate loading for the OpenSSL backend by addressing a potential NULL dereference in Enhanced Key Usage (EKU) handling.

Changes: Gates EKU buffer allocation/fetching on a non-zero req_size returned by CertGetEnhancedKeyUsage(), and skips certificates that report no EKU data to avoid dereferencing a NULL/empty usage list.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 1 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-02-19T18:09:45Z

lib/vtls/openssl.c

        }

+        /* Initial call did not return data. */
+        if(!req_size && !enhkey_usage)


If this path can happen, this continue skips the certificate entirely; per CertGetEnhancedKeyUsage docs, a cert with neither EKU extension nor EKU property is assumed valid for all uses (often surfaced via GetLastError()==CRYPT_E_NOT_FOUND). Can you confirm skipping the cert is intended here vs treating it as “all uses” and proceeding with import?

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

vszakats · 2026-02-19T18:33:51Z

augment review

augmentcode

Review completed. 1 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-02-19T18:35:43Z

lib/vtls/openssl.c

-                found = TRUE;
-                break;
-              }
+        if(enhkey_usage) {  /* We have EKU data */


if(enhkey_usage) is using the pointer value as a proxy for “we have EKU data”, but req_size can be 0 for a cert with no EKU; if enhkey_usage is non-NULL from a previous iteration, the second CertGetEnhancedKeyUsage call is made with req_size == 0 and may fail and incorrectly skip the cert. Consider gating the second call on req_size > 0 (or otherwise decoupling it from prior iterations) so cert acceptance doesn’t depend on whether an earlier cert allocated the buffer.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

vszakats · 2026-02-19T18:45:33Z

augment review

augmentcode

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

icing · 2026-02-20T11:58:17Z

lib/vtls/openssl.c

-                break;
-              }
+        if(req_size) {  /* Cert has EKU data */
+          if(CertGetEnhancedKeyUsage(pContext, 0, enhkey_usage, &req_size)) {


I would just make this if(req_site && CertGetEnhancedKeyUsage(...)) {

Wouldn't that run onto the continue; line skipping the cert in
this case?

MS docs say that a cert without EKU is fit for any use, which is what
augmentcode was quoting to my first attempt which did this.

I think it should then skip to the next cert when no more "EnhancedKeyUsage" is preset in the current certificate.

The documenation at https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certgetenhancedkeyusage is not totally clear, but I suspect that the function never succeeds with a req_size of 0. That would make any change unnecessary.

Yes, docs doesn't say anything about this. This made me check
WINE sources and those seems to be able to return success while
returning req_size == 0. (though I also could have misread it)

My understanding is that this code cycles through all certs,
checks if the current one has EKU data, while keeping around
a single temporary buffer for that data, which is grown as needed
throughout the loop.

For each cert there is a decision to either skip it, or add it if the
EKU is fit. This patch makes it do the latter also if there is no EKU.

Before this patch a no-EKU case made CertGetEnhancedKeyUsage()
retrieve the size twice, while leaving the EKU pointer NULL. Best
case (if CertGetEnhancedKeyUsage() failed) it made these certs
skipped, which also seems wrong, assuming no-EKU means a fit
cert.

So, changing line 2907 to

if(CertGetEnhancedKeyUsage(pContext, 0, NULL, &req_size) && req_size) {

should do the trick?

If we want to skip a cert with no EKU, yes, but the docs says a cert without an EKU
should be accepted for all uses:

"If a certificate has neither an EKU extension nor EKU extended properties, it is assumed to be valid for all uses." via https://learn.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-certgetenhancedkeyusage

Which suggests we should not skip but proceed to X509_STORE_add_cert?

Well, I cannot answer that since I do not know what it really means. What you talk about is a change in how this works. Are the any reported issue with that?

Fixing a potential NULL does not require a change in behaviour, or?

In this case it may have crashed previously. I think it's up to us to define the fixed behavior.
No one reported this, but it's also a rarely used codepath (and a rare case within). It may get
more use with native CA enabled by default, as in the upcoming curl-for-win binaries.

I went with the skip method, but this triggered a Medium severity issue with augment review.

augmentcode

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

Detected by clang-tidy: ``` lib/vtls/openssl.c:2922:15: warning: Access to field 'cUsageIdentifier' results in a dereference of a null pointer (loaded from variable 'enhkey_usage') [clang-analyzer-core.NullDereference] 2922 | if(!enhkey_usage->cUsageIdentifier) { | ^ lib/vtls/openssl.c:2996:3: note: Loop condition is true. Entering loop body 2996 | for(i = 0; i < CURL_ARRAYSIZE(win_stores); ++i) { | ^ lib/vtls/openssl.c:2998:14: note: Calling 'ossl_win_load_store' 2998 | result = ossl_win_load_store(data, win_stores[i], store, &store_added); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/openssl.c:2847:6: note: Assuming 'hStore' is non-null 2847 | if(hStore) { | ^~~~~~ lib/vtls/openssl.c:2847:3: note: Taking true branch 2847 | if(hStore) { | ^ lib/vtls/openssl.c:2852:5: note: 'enhkey_usage' initialized to a null pointer value 2852 | CERT_ENHKEY_USAGE *enhkey_usage = NULL; | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/openssl.c:2860:5: note: Loop condition is true. Entering loop body 2860 | for(;;) { | ^ lib/vtls/openssl.c:2867:10: note: Assuming 'pContext' is non-null 2867 | if(!pContext) | ^~~~~~~~~ lib/vtls/openssl.c:2867:7: note: Taking false branch 2867 | if(!pContext) | ^ lib/vtls/openssl.c:2873:12: note: Assuming the condition is false 2873 | if(!CertGetNameStringA(pContext, CERT_NAME_SIMPLE_DISPLAY_TYPE, 0, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2874 | NULL, cert_name, sizeof(cert_name))) | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/openssl.c:2873:9: note: Taking false branch 2873 | if(!CertGetNameStringA(pContext, CERT_NAME_SIMPLE_DISPLAY_TYPE, 0, | ^ lib/vtls/openssl.c:2877:11: note: Assuming 'data' is null 2877 | infof(data, "SSL: Checking cert \"%s\"", cert_name); | ^ lib/vtls/../curl_trc.h:140:8: note: expanded from macro 'infof' 140 | if(Curl_trc_is_verbose(data)) \ | ^~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/../curl_trc.h:312:4: note: expanded from macro 'Curl_trc_is_verbose' 312 | ((data) && (data)->set.verbose && \ | ^~~~~~ lib/vtls/openssl.c:2877:11: note: Left side of '&&' is false 2877 | infof(data, "SSL: Checking cert \"%s\"", cert_name); | ^ lib/vtls/../curl_trc.h:140:8: note: expanded from macro 'infof' 140 | if(Curl_trc_is_verbose(data)) \ | ^ lib/vtls/../curl_trc.h:312:11: note: expanded from macro 'Curl_trc_is_verbose' 312 | ((data) && (data)->set.verbose && \ | ^ lib/vtls/openssl.c:2877:11: note: Loop condition is false. Exiting loop 2877 | infof(data, "SSL: Checking cert \"%s\"", cert_name); | ^ lib/vtls/../curl_trc.h:139:3: note: expanded from macro 'infof' 139 | do { \ | ^ lib/vtls/openssl.c:2881:10: note: Assuming 'encoded_cert' is non-null 2881 | if(!encoded_cert) | ^~~~~~~~~~~~~ lib/vtls/openssl.c:2881:7: note: Taking false branch 2881 | if(!encoded_cert) | ^ lib/vtls/openssl.c:2885:10: note: Assuming the condition is false 2885 | if(CompareFileTime(&pContext->pCertInfo->NotBefore, &now) > 0 || | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/openssl.c:2885:10: note: Left side of '||' is false lib/vtls/openssl.c:2886:10: note: Assuming the condition is false 2886 | CompareFileTime(&now, &pContext->pCertInfo->NotAfter) > 0) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/openssl.c:2885:7: note: Taking false branch 2885 | if(CompareFileTime(&pContext->pCertInfo->NotBefore, &now) > 0 || | ^ lib/vtls/openssl.c:2890:10: note: Assuming the condition is false 2890 | if(CertGetIntendedKeyUsage(pContext->dwCertEncodingType, | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 2891 | pContext->pCertInfo, | ~~~~~~~~~~~~~~~~~~~~ 2892 | key_usage, sizeof(key_usage))) { | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/openssl.c:2890:7: note: Taking false branch 2890 | if(CertGetIntendedKeyUsage(pContext->dwCertEncodingType, | ^ lib/vtls/openssl.c:2896:15: note: Assuming the condition is false 2896 | else if(GetLastError()) | ^~~~~~~~~~~~~~ lib/vtls/openssl.c:2896:12: note: Taking false branch 2896 | else if(GetLastError()) | ^ lib/vtls/openssl.c:2907:10: note: Assuming the condition is true 2907 | if(CertGetEnhancedKeyUsage(pContext, 0, NULL, &req_size)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/openssl.c:2907:7: note: Taking true branch 2907 | if(CertGetEnhancedKeyUsage(pContext, 0, NULL, &req_size)) { | ^ lib/vtls/openssl.c:2908:12: note: Assuming 'req_size' is 0 2908 | if(req_size && req_size > enhkey_usage_size) { | ^~~~~~~~ lib/vtls/openssl.c:2908:21: note: Left side of '&&' is false 2908 | if(req_size && req_size > enhkey_usage_size) { | ^ lib/vtls/openssl.c:2921:12: note: Assuming the condition is true 2921 | if(CertGetEnhancedKeyUsage(pContext, 0, enhkey_usage, &req_size)) { | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/vtls/openssl.c:2921:9: note: Taking true branch 2921 | if(CertGetEnhancedKeyUsage(pContext, 0, enhkey_usage, &req_size)) { | ^ lib/vtls/openssl.c:2922:15: note: Access to field 'cUsageIdentifier' results in a dereference of a null pointer (loaded from variable 'enhkey_usage') 2922 | if(!enhkey_usage->cUsageIdentifier) { | ^~~~~~~~~~~~ ```

vszakats · 2026-02-23T18:41:23Z

Settled on not importing a cert if the query returned success, but with a zero-length EKU.
To be on the safe side.

vszakats · 2026-02-23T18:58:46Z

@aisle-analyzer augment review

aisle-research-bot · 2026-02-23T18:58:54Z

🔒 Aisle Security Analysis

✅ We scanned this PR and did not find any security vulnerabilities.

_{Aisle supplements but does not replace security review.}

Analyzed PR: #20634 at commit a0e1487

augmentcode

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

vszakats added TLS Windows Windows-specific labels Feb 19, 2026

vszakats requested a review from Copilot February 19, 2026 17:12

Copilot started reviewing on behalf of vszakats February 19, 2026 17:13 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

vszakats requested a review from Copilot February 19, 2026 17:29

Copilot started reviewing on behalf of vszakats February 19, 2026 17:29 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

augmentcode bot reviewed Feb 19, 2026

View reviewed changes

vszakats force-pushed the osslnullptr branch from 5f619a0 to 08c9636 Compare February 19, 2026 18:29

augmentcode bot reviewed Feb 19, 2026

View reviewed changes

vszakats requested review from icing and jay February 20, 2026 11:23

icing requested changes Feb 20, 2026

View reviewed changes

augmentcode bot reviewed Feb 20, 2026

View reviewed changes

vszakats added the on-hold label Feb 20, 2026

vszakats added 5 commits February 23, 2026 19:35

accept first certs if they miss EKU info

a62511f

fixup

5623ea7

comment

c36297a

skip cert if it returned success with zero-length EKU, just in case

a79d6b6

vszakats force-pushed the osslnullptr branch from f63e12f to a79d6b6 Compare February 23, 2026 18:37

vszakats requested a review from icing February 23, 2026 18:50

drop redundant check

a0e1487

augmentcode bot reviewed Feb 23, 2026

View reviewed changes

jay approved these changes Feb 23, 2026

View reviewed changes

vszakats closed this in ba685ad Feb 24, 2026

vszakats deleted the osslnullptr branch February 24, 2026 11:34

Uh oh!

Conversation

vszakats commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

vszakats commented Feb 19, 2026

Uh oh!

aisle-research-bot bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔒 Aisle Security Analysis

Uh oh!

vszakats commented Feb 19, 2026

Uh oh!

augmentcode bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode bot Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

vszakats commented Feb 19, 2026

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode bot Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

vszakats commented Feb 19, 2026

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

icing Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

vszakats Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

icing Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

vszakats Feb 20, 2026 • 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!

icing Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

vszakats Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

icing Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

vszakats Feb 20, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

Uh oh!

vszakats commented Feb 23, 2026

Uh oh!

vszakats commented Feb 23, 2026

Uh oh!

aisle-research-bot bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔒 Aisle Security Analysis

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

vszakats commented Feb 19, 2026 •

edited

Loading

aisle-research-bot bot commented Feb 19, 2026 •

edited

Loading

augmentcode bot commented Feb 19, 2026 •

edited

Loading

vszakats Feb 20, 2026 •

edited

Loading

aisle-research-bot bot commented Feb 23, 2026 •

edited

Loading