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
base: master
from

Conversation

Projects
None yet
2 participants
@mkauf
Contributor

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.

vtls: compare and clone ssl configs properly
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 Sep 24, 2017

@bagder

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

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Sep 26, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Sep 28, 2017

Contributor

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.

Contributor

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 SSL/TLS label Sep 28, 2017

@bagder

bagder approved these changes Oct 3, 2017

@mkauf mkauf closed this in 9d3dde3 Oct 3, 2017

@mkauf mkauf removed the request for review from jay Oct 3, 2017

@mkauf mkauf deleted the mkauf:vtls_ssl_config_fixes branch Oct 3, 2017

@mkauf

This comment has been minimized.

Show comment
Hide comment
@mkauf

mkauf Oct 3, 2017

Contributor

@bagder Thanks a lot for reviewing!

Contributor

mkauf commented Oct 3, 2017

@bagder Thanks a lot for reviewing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment