openssl: fix CURLINFO_SSL_VERIFYRESULT #995

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

@malhotrag
Contributor

CURLINFO_SSL_VERIFYRESULT does not get the certificate verification
result when SSL_connect fails because of a certificate verification
error.

This fix saves the result of SSL_get_verify_result so that it is
returned by CURLINFO_SSL_VERIFYRESULT.

@malhotrag malhotrag openssl: fix CURLINFO_SSL_VERIFYRESULT
CURLINFO_SSL_VERIFYRESULT does not get the certificate verification
result when SSL_connect fails because of a certificate verification
error.

This fix saves the result of SSL_get_verify_result so that it is
returned by CURLINFO_SSL_VERIFYRESULT.
aeecbbb
@jay jay added the SSL/TLS label Sep 6, 2016
@jay
Member
jay commented Sep 6, 2016 edited

I don't see where it's documented that we can get X509_V_ERR stuff from SSL_connect. The only place I see it documented is SSL_get_verify_result which we use already but that early it's just going to return ok I suspect (untested). Can you give more detail and tell us what you see happening that led you to make this change?

@malhotrag
Contributor
malhotrag commented Sep 6, 2016 edited

I am trying to access a https uri with CURLOPT_SSL_VERIFYPEER set to 1. When this option is enabled, curl calls SSL_CTX_set_verify with mode set to SSL_VERIFY_PEER. SSL_VERIFY_PEER causes the SSL_connect call to fail if any certificate verification errors are encountered during the SSL/TLS handshake. The caller is expected to call SSL_get_verify_result to get more details about the verification failure. The various verification failure codes are documented at verify.

I'm simulating various scenarios in which the SSL handshake can fail.

  1. The server certificate is expired. In this scenario, SSL_get_verify_result returns X509_V_ERR_CERT_HAS_EXPIRED.
  2. The server's certificate is issued by an untrusted CA. In this scenario, SSL_get_verify_result returns X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY.

In both of the above scenarios, with existing code, CURLINFO_SSL_VERIFYRESULT returns an incorrect value of 1. This is the default value ssl.certverifyresult is initialized to in ossl_connect_step1. After making the change in this pull request, the correct X509_V_ERR_ value is returned by CURLINFO_SSL_VERIFYRESULT.

Additionally, the existing code where ssl.certverifyresult is initalized (in function servercert) has a comment that alludes to this. The section of code is reproduced below

    lerr = data->set.ssl.certverifyresult =
      SSL_get_verify_result(connssl->handle);

    if(data->set.ssl.certverifyresult != X509_V_OK) {
      if(data->set.ssl.verifypeer) {
        /* We probably never reach this, because SSL_connect() will fail
           and we return earlier if verifypeer is set? */
@jay jay added a commit that closed this pull request Sep 6, 2016
@malhotrag @jay malhotrag + jay openssl: fix CURLINFO_SSL_VERIFYRESULT
CURLINFO_SSL_VERIFYRESULT does not get the certificate verification
result when SSL_connect fails because of a certificate verification
error.

This fix saves the result of SSL_get_verify_result so that it is
returned by CURLINFO_SSL_VERIFYRESULT.

Closes #995
8e176a7
@jay jay closed this in 8e176a7 Sep 6, 2016
@jay
Member
jay commented Sep 6, 2016 edited

Indeed you are correct and thank you for the thorough explanation. I had skimmed your changes and thought you were assigning the SSL_connect return code to ssl.certverifyresult, which I can see now is not what you're doing. I agree this is a bug.

Thank you for your fix. It has landed and you'll notice I modified it slightly because ssl.certverifyresult should not be set to X509_V_OK(0) in the case of SSL_R_CERTIFICATE_VERIFY_FAILED which apparently could happen. We initialize ssl.certverifyresult to 1 by default because that error code is not used and that is the way it should stay in that case.

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