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

vtls: compare and clone ssl configs properly #1917

Closed
wants to merge 2 commits into from

Conversation

mkauf
Copy link
Contributor

@mkauf mkauf commented Sep 24, 2017

Compare these settings in Curl_ssl_config_matches():

  • verifystatus (CURLOPT_SSL_VERIFYSTATUS)
  • sessionid (CURLOPT_SSL_SESSIONID_CACHE)
  • random_file (CURLOPT_RANDOM_FILE)
  • egdsocket (CURLOPT_EGDSOCKET)

Also copy the setting "verifystatus" in Curl_clone_primary_ssl_config(),
and copy the setting "sessionid" unconditionally.

This means that reusing connections that are secured with a client
certificate is now possible, and the statement "TLS session resumption
is disabled when a client certificate is used" in the old advisory at
https://curl.haxx.se/docs/adv_20170419.html is obsolete.


Additional information:

If you (the reviewers) agree, reusing connections that are secured with a client certificate is now officially possible. It was also possible before because of a bug. I think that it's OK to resume the TLS session if all the TLS settings (including the client certificate) are the same.

I have asked the curl security team whether it's OK to post this as a normal pull request, and they agreed.

These old advisories state that TLS session resumption is disabled when a client certificate is used:

... but that's not true. I have tested with curl 7.55.1:

curl -v -k --cert /path/to/cert.pem --key /path/to/key.pem -H "Connection: close" https://localhost/1 https://localhost/2

Output:

* Closing connection 0
...
* SSL re-using session ID
...
* Closing connection 1

Source code analysis:

There's this code in Curl_clone_primary_ssl_config(), vtls.c:

dest->sessionid = (dest->clientcert ? false : source->sessionid);

This sets the "sessionid" flag in the connection object (struct connectdata). But the code that checks whether TLS session resumption is possible does not look at the connection, it looks at the easy handle (struct Curl_easy).

In Curl_ssl_getsessionid():

  if(!SSL_SET_OPTION(primary.sessionid))
    /* session ID re-use is disabled */
    return TRUE;

In ossl_connect_step1():

  /* Check if there's a cached ID we can/should use here! */
  if(SSL_SET_OPTION(primary.sessionid)) {

This pull request cleans things up and allows reusing connections that are secured with a client certificate.

Compare these settings in Curl_ssl_config_matches():
- verifystatus (CURLOPT_SSL_VERIFYSTATUS)
- sessionid (CURLOPT_SSL_SESSIONID_CACHE)
- random_file (CURLOPT_RANDOM_FILE)
- egdsocket (CURLOPT_EGDSOCKET)

Also copy the setting "verifystatus" in Curl_clone_primary_ssl_config(),
and copy the setting "sessionid" unconditionally.

This means that reusing connections that are secured with a client
certificate is now possible, and the statement "TLS session resumption
is disabled when a client certificate is used" in the old advisory at
https://curl.haxx.se/docs/adv_20170419.html is obsolete.
@mkauf mkauf requested review from bagder and jay September 24, 2017 12:55
Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the argument to require the same random file and egdsocket settings? They're used to seed the PRNG so I don't see how they disqualify reuse if set to different values for different connections. (although I suspect both of them are very rarely used so in reality this change will hardly affect any users)

@mkauf
Copy link
Contributor Author

mkauf commented Sep 26, 2017

Interesting point... I think that the security of the SSL encryption depends on these settings. For example, if easy handle A does not use an EGD socket, but easy handle B does (for "extra security"), then A's connection should not be reused for B, because the security properties differ. (the other way round may be OK... it depends on the quality of the EGD)

I don't know whether Curl_ssl_config_matches should compare every ssl setting, or whether it should be written with a focus on connection reuse (maybe the function name should be changed in this case).

If we focus on connection reuse, it's probably also not necessary (or even a bad idea) to compare the sessionid flag.

@mkauf
Copy link
Contributor Author

mkauf commented Sep 28, 2017

If we focus on connection reuse, it's probably also not necessary (or even a bad idea) to compare the sessionid flag.

To address this, I have added a second commit: sessionid is not compared anymore.

@mkauf mkauf added the TLS label Sep 28, 2017
@mkauf mkauf closed this in 9d3dde3 Oct 3, 2017
@mkauf mkauf removed the request for review from jay October 3, 2017 16:12
@mkauf mkauf deleted the vtls_ssl_config_fixes branch October 3, 2017 16:13
@mkauf
Copy link
Contributor Author

mkauf commented Oct 3, 2017

@bagder Thanks a lot for reviewing!

@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 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