New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify error codes returned by various TLS backends #2901

Closed
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@sweatybridge
Contributor

sweatybridge commented Aug 20, 2018

Reconciliation of host name validation error

The original issue regarding inconsistent error code returned by different TLS backends during server certificate verification is caused by mishandling of framework return codes on host name validation failure. For openssl, host name is verified by a custom routine verifyhost which returns CURLE_PEER_FAILED_VERIFICATION on failure. For darwinssl, host name is verified automatically by the framework when calling SSLHandle. However, the framework does not return errSSLHostNameMismatch on failure as we might expect. Instead it returns a generic errSSLXCertChainInvalid which is mapped to CURLE_SSL_CACERT by curl. So the first thing I would like to clarify is the intended difference between CURLE_PEER_FAILED_VERIFICATION and CURLE_SSL_CACERT, and whether we can use them interchangeably.

The other inconsistency comes from curl's handling of schannel return code on Windows. Similar to darwinssl, the schannel framework also verifies host name automatically and returns SEC_E_WRONG_PRINCIPAL on validation failure. Although curl correctly catches this error code returned by InitializeSecurityContext, it only prints a different error message instead of mapping it to a return code consistent with darwinssl. We should fix this problem by returning CURLE_SSL_CACERT instead (commit 2e57a07).

Reconciliation of other discrepancies

As we looked deeper into the implementation of schannel, darwinssl, and openssl, we also discovered several other sources of discrepancy. The details of what each discrepancy is and how we think it should be resolved is elaborated in the commit messages of this PR. In general, I think the principle we should follow is to return as specific an error code as possible that’s also semantically consistent with other TLS backends. Since not everyone can be familiar with all backends, I would suggest that every TLS backend should try its best to align the error code with that of openssl. If it's not possible to be consistent, the difference should be documented. For example,

CURLE_SSL_ISSUER_ERROR (83)
	Issuer check failed. (Added in 7.19.0, only returned by curl when compiled with openssl backend).

Test plan

https://badssl.com hosts an extensive set of URLs with intentionally misconfigured SSL certificates. We ran a test suite of curl compiled with different TLS backends against those URLs. The return codes from the different curl versions on different operating systems are consistent.

sweatybridge added some commits Aug 14, 2018

darwinssl: return a single error code on failure to verify peer certi…
…ficate

The reason we want to do this are two fold.
1. The methods SecTrustSetAnchorCertificates, SecTrustSetAnchorCertificatesOnly, and SecTrustEvaluate return error codes from the Security Framework rather than Secure Transport according the Apple's documentation https://developer.apple.com/documentation/security/1396098-sectrustsetanchorcertificates?language=objc. The previous implementation maps return codes from Secure Transport which is wrong.
2. We would like to follow a similar approach as pkp_pin_peer_pubkey for consistency. In that method, error codes returned from intermediate calls are grouped under a single CURLE_SSL_PINNEDPUBKEYNOTMATCH error code.
darwinssl: map client cert validation failure to CURLE_SSL_CERTPROBLEM
Since the CopyCertSubject function is called during both client and server cert verification, we will need to translate the error code returned during client cert verification to maintain consistency with other tls backends.
darwinssl: return CURLE_SSL_CIPHER on failure to set cipher suite
Both openssl and schannel tls backends return this error code when they encounter error setting the cipher suite. We should do the same for darwinssl for consistency. Note that this error code is not used to represent failures during the TLS negotiation process with the server. It is used purely used on the client side.
schannel: use CURLE_SSL_CERTPROBLEM for client cert validation error
Since invalid path to the client certificate is treated as an error for the certificate itself on other tls backends, we should do the same for schannel.
schannel: return CURLE_SSL_CA_BADFILE when bundle size exceeds 1MB
Although this restriction does not seem to exist for other tls backends, we should be consistent with the error code returned when handling the CA bundle imported from the local certificate store.
schannel: remove invalid return code from AcquireCredentialsHandle
The schannel version of the above method does not return SEC_E_WRONG_PRINCIPAL error code according to their documentation https://msdn.microsoft.com/en-us/library/windows/desktop/aa374716(v=vs.85).aspx. I decided to have the full list of possible error codes for easier reference.
schannel: use appropriate return code for memory and host name valida…
…tion error

The IntitializeSecurityContext method returns SEC_E_WRONG_PRINCIPAL on failure to verify the host name of server's certificate. We should return the same error code as other tls backends, i.e. CURLE_SSL_CACERT. In addition, memory error should be mapped to CURLE_OUT_OF_MEMORY. I also decided to use the full list of return codes here for clarity and future reference.
schannel: return CURLE_PEER_FAILED_VERIFICATION on failure to verify …
…host

In openssl and darwinssl implementations of *_connect_step3 methods, CURLE_PEER_FAILED_VERIFICATION is returned when non-memory related error is encountered during any intermediate steps of the host name verification procedure. We should do the same here for consistency.
openssl: return CURLE_PEER_FAILED_VERIFICATION on failure to parse is…
…suer

Failure to extract the issuer name from the server certificate should return a more specific error code like on other TLS backends.
openssl: return CURLE_OUT_OF_MEMORY instead 0 on BIO_new failure
The previous implementation is incorrect on two levels.
1. The return type of the function is CURLcode so we should adhere to that instead of using int literals.
2. 0 is actually mapped to CURLE_OK which means we are wrongly treating a memory error as no error.

@sweatybridge sweatybridge changed the title from Unify SSL error codes returned by curl to Unify error codes returned by various TLS backends Aug 20, 2018

@bagder bagder added the SSL/TLS label Aug 20, 2018

@nickzman

Thanks for your work on this. There are just a few changes that need to be made before I can approve. Remember to keep the style consistent, and do not use symbols or enumerations that did not exist back in Leopard without checking for one of the CURL_BUILD_X preprocessor definitions.

}
else if(ret != noErr) {
CFRelease(array);
return sslerr_to_curlerr(data, ret);

This comment has been minimized.

@nickzman

nickzman Aug 21, 2018

Collaborator

By eliminating sslerr_to_curlerr(), you also eliminated all of the failf() messages that said what went wrong in the process.

This comment has been minimized.

@sweatybridge

sweatybridge Aug 21, 2018

Contributor

In other places where SSLCopyPeerTrust is called, curl simply ignores the error code returned. But I agree with you that printing an error message would help with diagnosis when there's an failure so let me bring back failf.

case -9841:
/* Below is an alias to the deprecated errSSLServerAuthCompleted
which is not defined in Leopard's headers */
case errSSLPeerAuthCompleted:

This comment has been minimized.

@nickzman

nickzman Aug 21, 2018

Collaborator

Please revert this change to the way it was. errSSLPeerAuthCompleted is not defined in Leopard's headers, and our support goes back to Leopard.

@@ -2379,6 +2372,51 @@ darwinssl_connect_step2(struct connectdata *conn, int sockindex)
/* the documentation says we need to call SSLHandshake() again */
return darwinssl_connect_step2(conn, sockindex);
/* Problem with encrypt / decrypt */
case errSSLPeerDecodeError:
failf(data, "Decode failed.");

This comment has been minimized.

@nickzman

nickzman Aug 21, 2018

Collaborator

These error strings do not match the style used elsewhere in this function.

@@ -2389,28 +2427,49 @@ darwinssl_connect_step2(struct connectdata *conn, int sockindex)
case errSSLNoRootCert:
failf(data, "SSL certificate problem: No root certificate");
return CURLE_SSL_CACERT;
case errSSLCertNotYetValid:
failf(data, "SSL certificate problem: The certificate chain had a "
"certificate that is not yet valid.");

This comment has been minimized.

@nickzman

nickzman Aug 21, 2018

Collaborator

Same here - that failf() line mustn't end with a period, since we don't use that style elsewhere.

case errSSLClientCertRequested:
failf(data, "The server has requested a client certificate.");
/* This code should not be returned because
kSSLSessionOptionBreakOnCertRequested is never set */

This comment has been minimized.

@nickzman

nickzman Aug 21, 2018

Collaborator

Then why do we have it here?

This comment has been minimized.

@sweatybridge

sweatybridge Aug 21, 2018

Contributor

I don't know if the implementation might change in the future (either curl starts setting the option or SecureTransport decides to remove the option to always break when server requests cert). Although these circumstances are very unlikely, we might want to log the error just in case.

Alternatively I can change it to a comment if you prefer.

@bagder

This comment has been minimized.

Member

bagder commented Aug 21, 2018

the intended difference between CURLE_PEER_FAILED_VERIFICATION and CURLE_SSL_CACERT

I'm not sure the distinction was ever really clear. The libcurl-errors.3 man page certainly doesn't help much:

  • CURLE_PEER_FAILED_VERIFICATION (51) - The remote server's SSL certificate or SSH md5 fingerprint was deemed not OK.
  • CURLE_SSL_CACERT (60) - Peer certificate cannot be authenticated with known CA certificates.

Given this, I would say that the latter is strictly for checks of the CA, so a mismatched hostname should perhaps use the former. But I also don't think splitting the errors like this is necessary or helps users. I think it is rather detrimental and we should try to use one single error for "not allowed to connect due to failed verification".

sweatybridge added some commits Aug 16, 2018

darwinssl: map additional server cert validation error codes
SSLHandle may return a wide variety of Secure Transport error codes. We are grouping those related to server certificate validation under CURLE_SSL_CACERT. Note that most of these additions should have no effect as they occur way less often than errSSLXCertChainInvalid. In addition, OCSP response error is also mapped to the appropriate code that's consistent with openssl.
darwinssl: add detailed error messages to generic ssl error
I decided to add the full list of Secure Transport error codes here for clarity and future reference. Currently, they are all mapped to CURLE_SSL_CONNECT_ERROR like in the previous implementation. If desired, we can have more specific codes for each error category, such as TLS negotiation, encryption error, etc. With the complete list of return codes, we can also understand future bug reports better since they contain the detailed error message.
darwinssl: remove unused sslerr_to_curlerr function
The translation of error codes is now done inline using a switch case.
x509asn1: return CURLE_PEER_FAILED_VERIFICATION on failure to parse c…
…ertificate

CURLE_PEER_FAILED_VERIFICATION makes more sense because Curl_parseX509 does not allocate memory internally as its first argument is a pointer to the certificate structure. The same error code is also returned by Curl_verifyhost when its call to Curl_parseX509 fails so the change makes error handling more consistent.
schannel: use CURLE_PEER_FAILED_VERIFICATION for general certificate …
…issues

As discussed in the PR, CURLE_PEER_FAILED_VERIFICATION is a more general error code for server certificate issues than CURLE_SSL_CACERT. The latter should be restricted to only those problems with certificate authority's cert.
@sweatybridge

This comment has been minimized.

Contributor

sweatybridge commented Aug 22, 2018

@nickzman I've modified the relevant commits based on your suggestions. The error message style looks consistent now. I've also checked the available enums against https://github.com/phracker/MacOSX-SDKs/blob/master/MacOSX10.5.sdk/System/Library/Frameworks/Security.framework/Versions/A/Headers/SecureTransport.h#L164 to make sure that newer additions are guarded by the appropriate macro.

@bagder I think your explanation makes a lot of sense. I've changed the return code for SEC_E_WRONG_PRINCIPAL to CURLE_PEER_FAILED_VERIFICATION in the last commit since it represents a more general problem than the CA certificate (and more specific than the original CURLE_SSL_CONNECT_ERROR). I think such a design is acceptable as long as it's documented that one code is strictly a subproblem of the other (i.e. CURLE_SSL_CACERT < CURLE_PEER_FAILED_VERIFICATION < CURLE_SSL_CONNECT_ERROR). Sometimes it might even be useful. For example, users of libcurl may want to programmatically handle very specific error cases based on the code returned.

If you agree, I can change the return code for errSSLXCertChainInvalid (darwinssl) and SSL_R_CERTIFICATE_VERIFY_FAILED (openssl) to CURLE_PEER_FAILED_VERIFICATION as well for consistency. But we should beware that such a fix will likely affect many users because those error codes are fairly common. Do you have a plan on how we can manage the risk of this change?

@bagder

This comment has been minimized.

Member

bagder commented Aug 22, 2018

Since openssl is the by far most used SSL backend I think we need to view that's behavior as the default one and the one we should try to align the other backends to. That way we fix more problems than we introduce.

Thinking about these return codes more, I'm pretty much against changing anything with openssl that was CURLE_SSL_CACERT to something else, since I'm pretty sure that's the specific return code more than one application checks for to detect CA problems as that's what the OpenSSL backend has returned for those. Even the curl command line tool does this!

But I find the error code name to be a bit misleading. Maybe we should drop the distinctions and just have a single return code for "failed peer verification"? Something along these lines (although this change alone causes compiler warnings) ?

diff --git a/include/curl/curl.h b/include/curl/curl.h
index 067b34ded..4cd55cc89 100644
--- a/include/curl/curl.h
+++ b/include/curl/curl.h
@@ -515,22 +515,21 @@ typedef enum {
   CURLE_OBSOLETE46,              /* 46 - NOT USED */
   CURLE_TOO_MANY_REDIRECTS,      /* 47 - catch endless re-direct loops */
   CURLE_UNKNOWN_OPTION,          /* 48 - User specified an unknown option */
   CURLE_TELNET_OPTION_SYNTAX,    /* 49 - Malformed telnet option */
   CURLE_OBSOLETE50,              /* 50 - NOT USED */
-  CURLE_PEER_FAILED_VERIFICATION, /* 51 - peer's certificate or fingerprint
-                                     wasn't verified fine */
+  CURLE_OBSOLETE51,              /* 51 - NOT USED from 7.62.0 */
   CURLE_GOT_NOTHING,             /* 52 - when this is a specific error */
   CURLE_SSL_ENGINE_NOTFOUND,     /* 53 - SSL crypto engine not found */
   CURLE_SSL_ENGINE_SETFAILED,    /* 54 - can not set SSL crypto engine as
                                     default */
   CURLE_SEND_ERROR,              /* 55 - failed sending network data */
   CURLE_RECV_ERROR,              /* 56 - failure in receiving network data */
   CURLE_OBSOLETE57,              /* 57 - NOT IN USE */
   CURLE_SSL_CERTPROBLEM,         /* 58 - problem with the local certificate */
   CURLE_SSL_CIPHER,              /* 59 - couldn't use specified cipher */
-  CURLE_SSL_CACERT,              /* 60 - problem with the CA cert (path?) */
+  CURLE_PEER_FAILED_VERIFICATION, /* 60 - not verified okay */
   CURLE_BAD_CONTENT_ENCODING,    /* 61 - Unrecognized/bad encoding */
   CURLE_LDAP_INVALID_URL,        /* 62 - Invalid LDAP URL */
   CURLE_FILESIZE_EXCEEDED,       /* 63 - Maximum file size exceeded */
   CURLE_USE_SSL_FAILED,          /* 64 - Requested FTP SSL level failed */
   CURLE_SEND_FAIL_REWIND,        /* 65 - Sending the data requires a rewind
@@ -582,10 +581,13 @@ typedef enum {
   CURLE_RECURSIVE_API_CALL,      /* 93 - an api function was called from
                                     inside a callback */
   CURL_LAST /* never use! */
 } CURLcode;
 
+/* added in 7.62.0 */
+#define CURLE_SSL_CACERT CURLE_PEER_FAILED_VERIFICATION
+
 #ifndef CURL_NO_OLDIES /* define this to test if your app builds with all
                           the obsolete stuff removed! */
 
 /* Previously obsolete error code re-used in 7.38.0 */
 #define CURLE_OBSOLETE16 CURLE_HTTP2
@sweatybridge

This comment has been minimized.

Contributor

sweatybridge commented Aug 24, 2018

the specific return code more than one application checks for to detect CA problems

Indeed, that was my concern as well. Just beware that changing the error code the other way like you suggested poses the same problem but to a lesser extent since CURLE_PEER_FAILED_VERIFICATION was introduced later and should have less users depending on it.

just have a single return code for "failed peer verification"

That would be the best scenario. I've made the necessary changes to get it to compile. Hopefully it won't break any users. If we want to be safe, I can also move that commit to another PR so that it can be easily reverted if things go south. Otherwise, I think this PR is ready @nickzman.

@nickzman

We're getting closer. Thanks for patching those earlier issues.

case errSSLCrypto:
failf(data, "An underlying cryptographic error was encountered");
break;
#if CURL_BUILD_MAC_10_11

This comment has been minimized.

@nickzman

nickzman Aug 24, 2018

Collaborator

errSSLWeakPeerEphemeralDHKey is available in the iOS 9 & later SDK, so let's open it up to iOS users as well please.

break;
#if CURL_BUILD_MAC_10_11
case errSSLWeakPeerEphemeralDHKey:
failf(data, "Indicates a weak ephemeral dh key");

This comment has been minimized.

@nickzman

nickzman Aug 24, 2018

Collaborator

That should probably be "Diffie-Hellman" or "D-H" and not "dh."

case errSSLPeerUnexpectedMsg:
failf(data, "Peer rejected unexpected message");
break;
#if CURL_BUILD_MAC_10_11

This comment has been minimized.

@nickzman

nickzman Aug 24, 2018

Collaborator

This one's also available in the iOS 9 SDK.

@sweatybridge

This comment has been minimized.

Contributor

sweatybridge commented Aug 24, 2018

PR updated but build failed due to flaky test. Can someone trigger the build again? It passes on my personal CI https://travis-ci.org/sweatybridge/curl

@nickzman

Thanks! I'm fine with these changes, as long as @bagder and @mback2k are fine with them.

@bagder

This comment has been minimized.

Member

bagder commented Aug 28, 2018

Thanks!

I'm fine with the changes, but due to the nature of the changes I prefer to wait until after the release before I merge this PR.

@bagder

bagder approved these changes Aug 28, 2018

@bagder

This comment has been minimized.

Member

bagder commented Sep 6, 2018

Thanks!

@bagder bagder closed this in 84a23a0 Sep 6, 2018

bagder added a commit that referenced this pull request Sep 6, 2018

@mback2k

This comment has been minimized.

Member

mback2k commented Sep 7, 2018

Unfortunately this change breaks lagacy MinGW / Windows XP compatibility, please check the following error message:

vtls/schannel.c: In function 'schannel_connect_step1':
vtls/schannel.c:813:12: error: 'SEC_E_APPLICATION_PROTOCOL_MISMATCH' undeclared (first use in this function)
       case SEC_E_APPLICATION_PROTOCOL_MISMATCH:
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vtls/schannel.c:813:12: note: each undeclared identifier is reported only once for each function it appears in
vtls/schannel.c: In function 'schannel_connect_step2':
vtls/schannel.c:1050:14: error: 'SEC_E_APPLICATION_PROTOCOL_MISMATCH' undeclared (first use in this function)
         case SEC_E_APPLICATION_PROTOCOL_MISMATCH:
              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

moschlar added a commit to moschlar/seafile that referenced this pull request Nov 8, 2018

Drop CURLE_SSL_CACERT from switch statement
The error type is deprecated since curl 7.62.0:
curl/curl#2901
@lietusme

This comment has been minimized.

lietusme commented Nov 14, 2018

As I understand looking briefly - CURLE_SSL_CACERT is deprecated and should no longer be used?

It broke our switch statement because CURLE_SSL_CACERT is same as CURLE_PEER_FAILED_VERIFICATION now.

If its deprecated, it should be documented better, as it now says this:

/* added in 7.62.0 */
#define CURLE_SSL_CACERT CURLE_PEER_FAILED_VERIFICATION
@sweatybridge

This comment has been minimized.

Contributor

sweatybridge commented Nov 19, 2018

Yes, more documentation is better. I can move it to the CURL_NO_OLDIES ifndef and add a bit more context to the comment. Someone might also want to update the documentation page here: https://curl.haxx.se/libcurl/c/libcurl-errors.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment