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

Backward compatibility issue with CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST options. #1941

Closed
agaloyan opened this issue Oct 3, 2017 · 6 comments
Labels

Comments

@agaloyan
Copy link

agaloyan commented Oct 3, 2017

Starting from "proxy: Support HTTPS proxy and SOCKS+HTTP(s)" (cb4e2be) change it is not possible anymore to change
CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST options values for the current connection.

Because SSL configuration options were moved to connectdata structure which is created at connecting phase, they are not affected by curl_easy_setopt function anymore for current connection.

The old behavior was very helpful in the following two use cases:

  1. Interactive application after receiving an unknown (or Self-Signed) certificate shows it on the console and ask user to reject or accept it.
  2. Non-interactive application implements some sort of Trust On First Use, for self-signed certificates. When such certificate received the verify callback checks local storage for known certificates. If there is no any previously saved certificate for the host then it saves the new received certificate and accepts connection. In further connections it will accept self-signed certificates for the same host only if it's public key matches previously stored certificates.

In both cases both options (CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST) set to TRUE before connection initialization.
During SSL negotation phase a verify callback (set by SSL_CTX_set_verify function) does all appropriate checks and if it decided to accept connection then it switches both CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST to FALSE to allow curl/openssl establish connection.
This was perfectly working in curl 7.50.3 and older versions... But now curl_easy_setopt updates only Easy_curl structure but not connectdata which is used during current connection.

We are using libcurl and openssl 1.0.1/1.0.2 almost 4 years in our projects and this is first time when we stucked on update.

I suggest to update data->easy_conn->ssl_config fields in curl_easy_setopt functions along with data->set.ssl.primary, i.e.

in url.c: ~2137 for case CURLOPT_SSL_VERIFYPEER: after the following lines:

data->set.ssl.primary.verifypeer = (0 != va_arg(param, long)) ? TRUE : FALSE;

update the current connection appropriate SSL variables:

if (data->easy_conn) {
   data->easy_conn->ssl_config.verifypeer = data->set.ssl.primary.verifypeer;
}

... similar code need to be applied for CURLOPT_PROXY_SSL_VERIFYPEER, CURLOPT_SSL_VERIFYHOST and CURLOPT_PROXY_SSL_VERIFYHOST cases.

operating system

win32, lin32 and lin64.

@bagder bagder added the TLS label Oct 4, 2017
@bagder
Copy link
Member

bagder commented Oct 4, 2017

Thank you for this detailed report.

I'm inclined to agree with you that this is a regression and not quite intended. I certainly did not consider this properly when I reviewed and accept that patch set.

Are you able and interested in providing a PR for this? Since you seem to have decent way to test/reproduce it and all.

agaloyan pushed a commit to agaloyan/curl that referenced this issue Oct 5, 2017
Now VERIFYHOST and VERIFYPEER options change during active connection
updates the current connection's (i.e.'connectdata' structure)
appropriate ssl_config (and ssl_proxy_config) structures variables,
making these options effective for ongoing connection.

This functionality was available before and was broken by the
following change:
"proxy: Support HTTPS proxy and SOCKS+HTTP(s)"
CommitId: cb4e2be.

bug: curl#1941
@agaloyan
Copy link
Author

agaloyan commented Oct 5, 2017

Hi Daniel,
Sure, I'm glad to help, but my PR (#1951) got some problems which I do not understand.
Can you have a look please?

@bagder
Copy link
Member

bagder commented Oct 5, 2017

I see no problems? The CI builds/tests are still ongoing so they're yellow circles. They should end up green checkmarks when everything is fine, or red crosses if some builds fail. Let's monitor what happens and see what to do. The CI tests can take a few hours to complete.

@agaloyan
Copy link
Author

agaloyan commented Oct 5, 2017

I understand, but it seems that Travis CI build 6473.4 already failed, and no OSX builders were allocated for awhile.

agaloyan pushed a commit to agaloyan/curl that referenced this issue Oct 6, 2017
Now VERIFYHOST and VERIFYPEER options change during active connection
updates the current connection's (i.e.'connectdata' structure)
appropriate ssl_config (and ssl_proxy_config) structures variables,
making these options effective for ongoing connection.

This functionality was available before and was broken by the
following change:
"proxy: Support HTTPS proxy and SOCKS+HTTP(s)"
CommitId: cb4e2be.

bug: curl#1941
agaloyan pushed a commit to agaloyan/curl that referenced this issue Oct 6, 2017
Now VERIFYHOST and VERIFYPEER options change during active connection
updates the current connection's (i.e.'connectdata' structure)
appropriate ssl_config (and ssl_proxy_config) structures variables,
making these options effective for ongoing connection.

This functionality was available before and was broken by the
following change:
"proxy: Support HTTPS proxy and SOCKS+HTTP(s)"
CommitId: cb4e2be.

bug: curl#1941
@agaloyan
Copy link
Author

agaloyan commented Oct 6, 2017

Fixed code style issues...
can't assign reviewers...

jay pushed a commit to jay/curl that referenced this issue Oct 10, 2017
Now VERIFYHOST and VERIFYPEER options change during active connection
updates the current connection's (i.e.'connectdata' structure)
appropriate ssl_config (and ssl_proxy_config) structures variables,
making these options effective for ongoing connection.

This functionality was available before and was broken by the
following change:
"proxy: Support HTTPS proxy and SOCKS+HTTP(s)"
CommitId: cb4e2be.

Bug: curl#1941

Closes curl#1951
agaloyan pushed a commit to agaloyan/curl that referenced this issue Oct 10, 2017
Updating current connection verifystatus on CURLOPT_SSL_VERIFYSTATUS
option change.

This is an amendment to:
"ssl: Update the current connection SSL params in curl_easy_setopt()."

bug: curl#1941
jay pushed a commit that referenced this issue Oct 11, 2017
Now VERIFYHOST, VERIFYPEER and VERIFYSTATUS options change during active
connection updates the current connection's (i.e.'connectdata'
structure) appropriate ssl_config (and ssl_proxy_config) structures
variables, making these options effective for ongoing connection.

This functionality was available before and was broken by the
following change:
"proxy: Support HTTPS proxy and SOCKS+HTTP(s)"
CommitId: cb4e2be.

Bug: #1941

Closes #1951
@jay
Copy link
Member

jay commented Oct 11, 2017

Fix landed in 5505df7.

@jay jay closed this as completed Oct 11, 2017
@lock lock bot locked as resolved and limited conversation to collaborators May 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

3 participants