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

Comments

Projects
None yet
3 participants
@agaloyan

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 SSL/TLS label Oct 4, 2017

@bagder

This comment has been minimized.

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

Artak Galoyan
ssl: Update the current connection SSL params in curl_easy_setopt().
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

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

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

Artak Galoyan
ssl: Update the current connection SSL params in curl_easy_setopt().
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

Artak Galoyan
ssl: Update the current connection SSL params in curl_easy_setopt().
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

This comment has been minimized.

agaloyan commented Oct 6, 2017

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

jay added a commit to jay/curl that referenced this issue Oct 10, 2017

url: Update current connection SSL verify params in setopt
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

Artak Galoyan
ssl: Update the current connection verifystatus in curl_easy_setopt().
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 added a commit that referenced this issue Oct 11, 2017

url: Update current connection SSL verify params in setopt
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

This comment has been minimized.

Member

jay commented Oct 11, 2017

Fix landed in 5505df7.

@jay jay closed this 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.