openssl: noverify continue if CA_BLOB error

[kyled-dell]

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]

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?

[kyled-dell]

That large chunk (from my commit 27675b4e3f86e050efb352157edea659b976dedf) 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.

[kyled-dell]

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.

[bagder]

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.

[kyled-dell]

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]

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]

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]

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]

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]

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:

1. Why would libcurl load certificates that it knows they won't be used?
2. 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.

[jay]

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]

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.

[kyled-dell]

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]

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!

[jay]

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."

[bagder]

That seems a very reasonable text to add!

[bagder]

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