Skip to content
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

openssl: fix CURLINFO_SSL_VERIFYRESULT #995

Closed
wants to merge 1 commit into from

Conversation

malhotrag
Copy link
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.

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.
@jay jay added the TLS label Sep 6, 2016
@jay
Copy link
Member

jay commented Sep 6, 2016

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
Copy link
Contributor Author

malhotrag commented Sep 6, 2016

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 closed this in 8e176a7 Sep 6, 2016
@jay
Copy link
Member

jay commented Sep 6, 2016

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants