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

Assertion incorrectly triggering in Curl_ssl_getsessionid() #8472

Closed
jimbeveridge opened this issue Feb 18, 2022 · 5 comments
Closed

Assertion incorrectly triggering in Curl_ssl_getsessionid() #8472

jimbeveridge opened this issue Feb 18, 2022 · 5 comments
Labels

Comments

@jimbeveridge
Copy link

@jimbeveridge jimbeveridge commented Feb 18, 2022

I did this

Built a Debug version of curl using gcc (Ubuntu 9.3.0-17ubuntu1~20.04) 9.3.0.

Passed the -D ENABLE_DEBUG=1 flag to CMake to enable assertions in libcurl.

Modified docs/examples/https.c to set CURLOPT_SSL_SESSIONID_CACHE to zero, as follows:

int main(void)
{
  CURL *curl;
  CURLcode res;

  curl_global_init(CURL_GLOBAL_DEFAULT);

  curl = curl_easy_init();
  if(curl) {

    curl_easy_setopt(curl, CURLOPT_URL, "https://example.com/");

    // Added this line
    curl_easy_setopt(curl, CURLOPT_SSL_SESSIONID_CACHE, 0L);

Built https.c with this command:

gcc https.c -I ../../include/ -L ../../build/Debug/lib -l:libcurl-d.so

When this code ran, I received this error:

a.out: /home/jim/repos/curl/lib/vtls/vtls.c:424: Curl_ssl_getsessionid: Assertion `((CURLPROXY_HTTPS == conn->http_proxy.proxytype && ssl_connection_complete != conn->proxy_ssl[conn->sock[1] == -1 ? 0 : 1].state) ? data->set.proxy_ssl.primary.sessionid : data->set.ssl.primary.sessionid)' failed.
Aborted

Reviewing vtls.c line 424, it is asserting on the value of sessionid, which is correctly set to zero because of the call to curl_easy_setopt(curl, CURLOPT_SSL_SESSIONID_CACHE, 0L);. The zero is interpreted as a boolean false and the assertion fails.

It appears that Line 424 should be removed entirely because it's handled properly on line 426.

I expected the following

I expected the sample to run correctly and print the content from example.com.

curl/libcurl version

Using commit 161cbc502ee0bdbda052d6da17d24fa7835f83e7, but this problem goes back to at least curl-7.79.1 .

operating system

Linux mynode 5.10.16.3-microsoft-standard-WSL2 #1 SMP Fri Apr 2 22:23:49 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

@bagder bagder added the TLS label Feb 18, 2022
@bagder
Copy link
Member

@bagder bagder commented Feb 18, 2022

Thanks. That assert is clearly wrong there...

bagder added a commit that referenced this issue Feb 18, 2022
As assert that required an non-zero session id cache just before the
run-time check that handles it correctly is an incorrect assert.

Reported-by: Jim Beveridge
Fixes #8472
@bagder
Copy link
Member

@bagder bagder commented Feb 18, 2022

The assert was originally added in 04b4ee5 but was touched at a few later occasions.

@jimbeveridge
Copy link
Author

@jimbeveridge jimbeveridge commented Feb 18, 2022

Does the equivalent DEBUGASSERT() in Curl_ssl_addsessionid() have the same problem? There's a prior check for if(!data->state.session), but that doesn't appear to be sufficient.

@bagder
Copy link
Member

@bagder bagder commented Feb 18, 2022

I can't spot any code path where set->ssl.primary.sessionid is FALSE and it can reach that ASSERT and we haven't had any reports of it either. Can you see how it could trigger wrongly?

jay added a commit to jay/curl that referenced this issue Feb 20, 2022
Ideally, Curl_ssl_getsessionid should not be called unless sessionid
caching is enabled. There is a debug assertion in the function to help
ensure that. Therefore, the pattern in all vtls is basically:

  if(primary.sessionid) {lock(); Curl_ssl_getsessionid(...); unlock();}

There was one instance in openssl.c where sessionid was not checked
beforehand and this change fixes that.

Prior to this change an assertion would occur in openssl debug builds
during connection stage if session caching was disabled.

Reported-by: Jim Beveridge

Fixes curl#8472
Closes #xxxx
@jay
Copy link
Member

@jay jay commented Feb 21, 2022

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants