openssl: noverify continue if CA_BLOB error #11457

kyled-dell · 2023-07-17T22:11:57Z

See detailed issue description in #11456. This PR preserves independence of the CURLOPT_SSL_VERIFYPEER and CURLOPT_CAINFO_BLOB when using OpenSSL. This ensures certificates will be added to OpenSSL's root certificate chain, even if the user of libcurl is doing something strange with certificates. This behavior was tested with the ossl_custom_verify.c and test_verify.c scripts from #11456.

The major change is the addition of this line which makes these settings interact in libcurl the same was as they did in 7.86.0.

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index c6a1dd218..efd0c1cab 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -3171,6 +3171,7 @@ static CURLcode populate_x509_store(struct Curl_cfilter *cf,
         return result;
       }
       /* Only warn if no certificate verification is required. */
+      result = CURLE_OK;
       infof(data, "error importing CA certificate blob, continuing anyway");
     }
     else {

bagder · 2023-07-17T22:17:23Z

lib/vtls/openssl.c

-          BYTE key_usage[2];
-          DWORD req_size;
-          const unsigned char *encoded_cert;
+  /* Import certificates from the Windows root certificate store if requested.


You report a problem on Ubuntu, but yet the largest chunk of your PR changes code that is Windows-specific. It makes me assume it changes more than you intended?

That large chunk (from my commit 27675b4) is a git revert of a3bcfab which was the easiest way I could see to remove the if(verifypeer){ statement starting on line 3079 that introduced the dependency between CURLOPT_SSL_VERIFYPEER and CURLOPT_CAINFO_BLOB options. There should be no functional change to the Windows certificate handling code, though my change would also revert a small formatting change on line 3006.

Though I didn't consider curl's behavior on Windows if CURLOPT_SSL_VERIFYPEER was set to 0 and a failure occurred while curl was importing certificates from Windows.

Since you removed the if(verifypeer) { for the Windows code that loads native certs as well as for the code you want to change, it certainly changes behavior in a non-intended way.

After examining the code I didn't find that my changes would have any unintended affect on the Windows native certificate import code.

a3bcfab is the latest commit that changed the Windows native certificate import code. This means there haven't been any changes since that commit that would need to be preserved. See the code block at the bottom of this comment for my analysis commands.

Before commit a3bcfab there were 6 if statements contained in the populate_x509_store function (1, 2, 3, 4, 5, 6). The a3bcfab commit moved the first 2 if statements into the if(verifypeer) statement from line 3079 (1, 2). The remaining if statements were not changed (3, 4, 5, 6). However moving these if statements only affected the behavior of the second, making it dependent on the configuration of VERIFYPEER.

For Windows, before a3bcfab, certificates would only be loaded from Windows with the CURLSSLOPT_NATIVE_CA flag set in CURLOPT_SSL_OPTIONS and either VERIFYPEER or VERIFYHOST was set. After the a3bcfab change, certificates from Windows would only be loaded if both the NATIVE_CA flag and VERIFYPEER were set. I believe the a3bcfab commit changes the semantics around the CURLOPT_SSL_VERIFYHOST option, and my change would return to the original semantics. After my changes the first if statement would exactly return to its curl 7.87.0 behavior. Curl's 7.87.0 Windows native CA behavior is identical to curl 7.86.0's behavior, which is the behavior I hope to preserve through this PR. The behavior of Windows' usage of VERIFYPEER with and without my change remains the same. With my change, configuring VERIFYHOST can also be used to enable certificate import from Windows.

The comment on line 3047 states that errors encountered while importing certificates from Windows are ignored. This comment describes the same behavior that my PR is attempting to implement for CAINFO_BLOB. However the Windows native code would not execute if VERIFYPEER is false, so the error handling behavior would not be executed.

My change will remove the second if statement from the verifypeer if statement as it is intends to do. This will restore the independence of the SSL_VERIFYPEER and CAINFO_BLOB settings. This will preseve functionality that identical to curl 7.86.0.

$ git rev-parse --verify HEAD f2aac0d108167bee5d06d6f9a307e0ff5193b816 $ git blame lib/vtls/openssl.c -L 3037,3172 | awk '{print $1}' | sort | uniq 3c16697ebd 3d919440c8 49a6642f01 8fa8df95fb a20daf90e3 a3bcfab4b5 ^ae1912cb0 d288222e80 fa1ae0abcd

bagder · 2023-07-17T22:32:50Z

If this one-liner basically what you want?

diff --git a/lib/vtls/openssl.c b/lib/vtls/openssl.c
index ae33147d0..e8fea0909 100644
--- a/lib/vtls/openssl.c
+++ b/lib/vtls/openssl.c
@@ -3025,11 +3025,11 @@ static CURLcode populate_x509_store(struct Curl_cfilter *cf,
   bool imported_ca_info_blob = false;
 
   if(!store)
     return CURLE_OUT_OF_MEMORY;
 
-  if(verifypeer) {
+  if(verifypeer || data->set.ssl.fsslctx) {
 #if defined(USE_WIN32_CRYPTO)
     /* Import certificates from the Windows root certificate store if
        requested.
        https://stackoverflow.com/questions/9507184/
        https://github.com/d3x0r/SACK/blob/master/src/netlib/ssl_layer.c#L1037

jay · 2023-07-18T18:12:26Z

if(verifypeer || data->set.ssl.fsslctx) {

Though that is the right way to do (probably) what he wants I don't think we should make any change as explained here.

kyled-dell · 2023-07-18T23:10:01Z

If this one-liner basically what you want?

Yes, that would fix our issue but would not support the CAINFO_BLOB.len == 0 use case from #10351. If the proposed change were merged, curl would require either CAINFO_BLOB not being configured or would require a valid certificate to be contained in the CAINFO_BLOB. This would not allow curl to be used with VERIFYPEER == false, a custom TLS callback through CURLOPT_SSL_CTX_FUNCTION, and CAINFO_BLOB.len == 0 configured. Broadly speaking this configuration would have been supported on curl 7.86.0. In my opinion, merging the proposed diff would introduce another change to the semantics of these curl options.

bagder · 2023-07-19T19:21:20Z

Sorry you lost me.

What puzzles me and what I think is the root cause for these problems is your mix of using a custom verification callback method but yet you insisting libcurl to do magic with the certificates. I think you should rather stick to either way; if you use the callback, do not assume that libcurl does magic with the certificates. If you don't ask libcurl to verify the certificates, libcurl has no reason to load them or check them and I think that is valid. It cannot know or presume that your callback wants to piggy-back on that functionality.

kyled-dell · 2023-07-20T20:50:06Z

I think either loading or not loading certificates is reasonable. In either case a few lines should be added to the documentation to highlight the interactions between these settings.

The documentation for CURLOPT_CAINFO_BLOB seems to indicate that the option will cause curl to pass certificates to the TLS backend. My previous sentence describes libcurl's OpenSSL behavior up until version 7.88.0.

If CURLOPT_SSL_VERIFYPEER(3) is zero and you avoid verifying the server's certificate, CURLOPT_CAINFO_BLOB(3) is not needed.

We appear to be attempting to reconcile the answers to 2 questions:

Why would libcurl load certificates that it knows they won't be used?
Why would client code provide a certificate blob if the client code requests that the blob not be used?

I'll expand and provide context for these questions in the following paragraphs.

There are 4 options that have interesting interactions in this space: CURLOPT_SSL_VERIFYPEER, CURLOPT_CAINFO_BLOB, CURLOPT_SSL_CTX_FUNCTION, and CURLOPT_SSL_VERIFYHOST. The code changes in curl 7.87.0 and 7.88.0 have changed the behavior of and interaction between these settings.

There are 2 configurations. The first configuration is from #10351 which wants the ability to provide an empty curl_blob structure to the CURLOPT_CAINFO_BLOB option. The introduction of the populate_x509_store function in curl 7.87.0 broke this usecase. A failure during certificate loading would cause the entire request to fail. To fix this, if CURLOPT_SSL_VERIFYPEER was disabled, loading certificates from the CURLOPT_CAINFO_BLOB option would not be attempted. This makes sense, why waste time loading certificates that won't be used? This change introduced a new dependency between the CURLOPT_SSL_VERIFYPEER and CURLOPT_CAINFO_BLOB options. This new dependency could cause a breaking change for some client applications.

The second configuration worked in curl 7.86.0 and 7.87.0 but broke in 7.88.0. This configuration disabled CURLOPT_SSL_VERIFYPEER, provided certificates through CURLOPT_CAINFO_BLOB, and also used the CURLOPT_SSL_CTX_FUNCTION option for custom verification. Setting CURLOPT_SSL_VERIFYPEER to disabled makes me think that curl would not attempt its own certificate verification, making this the correct value for this setting if CURLOPT_SSL_CTX_FUNCTION is used. However in reality the value of CURLOPT_SSL_VERIFYPEER doesn't matter, the function provided in CURLOPT_SSL_CTX_FUNCTION is called after curl configures its verification and allows the client application to undo any configuration curl previously made. In curl 7.88.0 the certificates from CURLOPT_CAINFO_BLOB were not loaded into OpenSSL despite that being curl's behavior for at least the previous 2 releases. This would mean the client application would need to implement its own certificate loading code to remain compatible with curl 7.88.0 or newer.

From my point of view, if the CURLOPT_CAINFO_BLOB option is provided, curl should attempt to load the certificates into OpenSSL. The client application has explicitly provided curl with certificates that should be used for the request. If a client application did not want verification to be performed with these certificates, why would the application have provided them? If CURLOPT_SSL_VERIFYPEER is disabled, then any errors encountered while loading these certificates should be ignored, since curl does not need the certificates to perform its own validation. However if a certificate import error did occur, the client application would have no way of knowing that the error occurred. If the client application used CURLOPT_SSL_CTX_FUNCTION to configure custom verification, then any combination of these 3 settings would leave the client in a good starting state, with certificates loaded.

A sample of the other vtls backends shows mixed results for the interaction between CURLOPT_CAINFO_BLOB and CURLOPT_SSL_CTX_FUNCTION. BearSSL appears to always load certificates from CURLOPT_CAINFO_BLOB but will ignore errors if CURLOPT_SSL_VERIFYPEER is disabled. Mbedtls and rusttls won't load certificates if CURLOPT_SSL_VERIFYPEER is disabled. Sectransp and schannel don't appear to have a concept of "loading" certificates, and curl only makes use of the certificate blob if verification is enabled.

We already do this for other SSL backends. Bug: curl#11457 (comment) Reported-by: kyled-dell@users.noreply.github.com Closes #xxxx

jay · 2023-07-21T07:34:33Z

BearSSL appears to always load certificates from CURLOPT_CAINFO_BLOB but will ignore errors if CURLOPT_SSL_VERIFYPEER is disabled.

I've submitted #11497 to address that issue. The behavior should be libcurl does not load the CA certificate bundle if peer verification is disabled.

bagder · 2023-07-23T14:56:01Z

if the CURLOPT_CAINFO_BLOB option is provided, curl should attempt to load the certificates into OpenSSL

Why? The CAINFO_BLOB option provides the cacerts in memory. That's an alternative way instead of loading them from disk. This blob is the CA store.

bagder

As discussed, I cannot accept this as-is.

We already do this for other SSL backends. Bug: #11457 (comment) Reported-by: kyled-dell@users.noreply.github.com Closes #11497

If CURLOPT_SSL_VERIFYPEER is disabled and CURLOPT_CAINFO_BLOB is provided, certificates will not be loaded from the BLOB into the TLS backend. This is an important interaction if CURLOPT_SSL_CTX_FUNCTION is used for custom TLS verification.

kyled-dell · 2023-08-02T18:05:42Z

To summarize: our application was using unintended functionality of libcurl that loaded CA certificates provided to the CURLOPT_CAINFO_BLOB option when CURLOPT_SSL_VERIFYPEER was disabled.

Since libcurl's interaction between these 2 settings was unintended I plan to rewrite the code that was using this functionality to load the certificates into OpenSSL itself.

Instead of changing libcurl's behavior, I amended this PR to clarify that if CURLOPT_SSL_VERIFYPEER is disabled, certificates won't be loaded from CURLOPT_CAINFO_BLOB. If you don't have issues with the wording, feel free to accept the PR. Otherwise I will close this PR and associated issue.

bagder · 2023-08-02T22:22:03Z

docs/libcurl/opts/CURLOPT_CAINFO_BLOB.3

-server's certificate, \fICURLOPT_CAINFO_BLOB(3)\fP is not needed.
+If \fICURLOPT_SSL_VERIFYPEER(3)\fP is zero and you avoid verifying the server's
+certificate, any certificates provided by using \fICURLOPT_CAINFO_BLOB(3)\fP
+are not used.


I think this is confusing. Exactly how would libcurl use the certificate when it doesn't verify it? I think this change adds more questions than it answers!

If \fICURLOPT_SSL_VERIFYPEER(3)\fP is zero and you avoid verifying the server's certificate

I think the issue is we have users who are verifying the certificate on their own even though CURLOPT_SSL_VERIFYPEER is zero, so they may misinterpret either version of this. How about "If \fICURLOPT_SSL_VERIFYPEER(3)\fP is zero then the certificates are not loaded."

That seems a very reasonable text to add!

…bled Ref: #11457

…bled Ref: #11457 Closes #11606

bagder · 2023-08-07T08:41:25Z

I added a mention of this in the CURLOPT_SSL_VERIFYPEER.3 documentation. I believe this case is then handled!

We already do this for other SSL backends. Bug: curl#11457 (comment) Reported-by: kyled-dell@users.noreply.github.com Closes curl#11497

…bled Ref: curl#11457 Closes curl#11606

github-actions bot added the TLS label Jul 17, 2023

bagder reviewed Jul 17, 2023

View reviewed changes

jay added a commit to jay/curl that referenced this pull request Jul 21, 2023

bearssl: don't load CA certs when peer verification is disabled

15750d2

We already do this for other SSL backends. Bug: curl#11457 (comment) Reported-by: kyled-dell@users.noreply.github.com Closes #xxxx

jay mentioned this pull request Jul 21, 2023

bearssl: don't load CA certs when peer verification is disabled #11497

Closed

bagder requested changes Jul 23, 2023

View reviewed changes

jay added a commit that referenced this pull request Jul 27, 2023

bearssl: don't load CA certs when peer verification is disabled

691c32f

We already do this for other SSL backends. Bug: #11457 (comment) Reported-by: kyled-dell@users.noreply.github.com Closes #11497

kyled-dell force-pushed the CA_INFO_BLOB branch from 2c83ac1 to 93306b6 Compare August 2, 2023 17:56

github-actions bot added the documentation label Aug 2, 2023

bagder reviewed Aug 2, 2023

View reviewed changes

bagder added a commit that referenced this pull request Aug 7, 2023

CURLOPT_SSL_VERIFYPEER.3: mention it does not load CA certs when disa…

dab605e

…bled Ref: #11457

bagder mentioned this pull request Aug 7, 2023

CURLOPT_SSL_VERIFYPEER.3: does not load CA certs when disabled #11606

Closed

bagder added a commit that referenced this pull request Aug 7, 2023

CURLOPT_SSL_VERIFYPEER.3: mention it does not load CA certs when disa…

90d0e0f

…bled Ref: #11457 Closes #11606

bagder closed this Aug 7, 2023

ptitSeb pushed a commit to wasix-org/curl that referenced this pull request Sep 25, 2023

CURLOPT_SSL_VERIFYPEER.3: mention it does not load CA certs when disa…

99da71a

…bled Ref: curl#11457 Closes curl#11606

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openssl: noverify continue if CA_BLOB error #11457

openssl: noverify continue if CA_BLOB error #11457

kyled-dell commented Jul 17, 2023

bagder Jul 17, 2023

kyled-dell Jul 17, 2023

kyled-dell Jul 17, 2023

bagder Jul 17, 2023

kyled-dell Jul 18, 2023

bagder commented Jul 17, 2023

jay commented Jul 18, 2023

kyled-dell commented Jul 18, 2023

bagder commented Jul 19, 2023

kyled-dell commented Jul 20, 2023

jay commented Jul 21, 2023

bagder commented Jul 23, 2023

bagder left a comment

kyled-dell commented Aug 2, 2023

bagder Aug 2, 2023

jay Aug 3, 2023

bagder Aug 6, 2023

bagder commented Aug 7, 2023

openssl: noverify continue if CA_BLOB error #11457

openssl: noverify continue if CA_BLOB error #11457

Conversation

kyled-dell commented Jul 17, 2023

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

bagder commented Jul 17, 2023

jay commented Jul 18, 2023

kyled-dell commented Jul 18, 2023

bagder commented Jul 19, 2023

kyled-dell commented Jul 20, 2023

jay commented Jul 21, 2023

bagder commented Jul 23, 2023

bagder left a comment

Choose a reason for hiding this comment

kyled-dell commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagder commented Aug 7, 2023