Skip to content

Commit

Permalink
TLS: switch off SSL session id when client cert is used
Browse files Browse the repository at this point in the history
CVE-2016-5419
Bug: https://curl.haxx.se/docs/adv_20160803A.html
Reported-by: Bru Rom
Contributions-by: Eric Rescorla and Ray Satiro
  • Loading branch information
bagder committed Aug 2, 2016
1 parent 75dc096 commit 247d890
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 0 deletions.
1 change: 1 addition & 0 deletions lib/url.c
Expand Up @@ -6123,6 +6123,7 @@ static CURLcode create_conn(struct Curl_easy *data,
data->set.ssl.random_file = data->set.str[STRING_SSL_RANDOM_FILE];
data->set.ssl.egdsocket = data->set.str[STRING_SSL_EGDSOCKET];
data->set.ssl.cipher_list = data->set.str[STRING_SSL_CIPHER_LIST];
data->set.ssl.clientcert = data->set.str[STRING_CERT];
#ifdef USE_TLS_SRP
data->set.ssl.username = data->set.str[STRING_TLSAUTH_USERNAME];
data->set.ssl.password = data->set.str[STRING_TLSAUTH_PASSWORD];
Expand Down
1 change: 1 addition & 0 deletions lib/urldata.h
Expand Up @@ -351,6 +351,7 @@ struct ssl_config_data {
char *CAfile; /* certificate to verify peer against */
const char *CRLfile; /* CRL to check certificate revocation */
const char *issuercert;/* optional issuer certificate filename */
char *clientcert;
char *random_file; /* path to file containing "random" data */
char *egdsocket; /* path to file containing the EGD daemon socket */
char *cipher_list; /* list of ciphers to use */
Expand Down
10 changes: 10 additions & 0 deletions lib/vtls/vtls.c
Expand Up @@ -156,6 +156,15 @@ Curl_clone_ssl_config(struct ssl_config_data *source,
else
dest->random_file = NULL;

if(source->clientcert) {
dest->clientcert = strdup(source->clientcert);
if(!dest->clientcert)
return FALSE;
dest->sessionid = FALSE;
}
else
dest->clientcert = NULL;

return TRUE;
}

Expand All @@ -166,6 +175,7 @@ void Curl_free_ssl_config(struct ssl_config_data* sslc)
Curl_safefree(sslc->cipher_list);
Curl_safefree(sslc->egdsocket);
Curl_safefree(sslc->random_file);
Curl_safefree(sslc->clientcert);
}


Expand Down

9 comments on commit 247d890

@iboukris
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, not sure why we must disable TLS resumption altogether, perhaps we could just match the certificate like we do in the other patch for reusing the connection - something like: iboukris@49b17e3

@scantor
Copy link

@scantor scantor commented on 247d890 Aug 3, 2016

Choose a reason for hiding this comment

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

It's also not actually reliable as a check, but I'll guess that the possibility of people attaching certificates dynamically using the SSL_CTX hook with OpenSSL was ruled out of scope. That probably ought to be documented as a loophole if this is viewed as a risk.

@bagder
Copy link
Member Author

@bagder bagder commented on 247d890 Aug 3, 2016

Choose a reason for hiding this comment

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

I'm sure we can make a more clever check, but I really didn't feel like spending a lot of energy on that. I don't think client certificate is a broad use case and session resumptions with session id using client certs is an even smaller user case.

If someone feels like working on improving the session id cache and use to work even with client certs, then by all means please do so. Let's just remember that the different TLS backends have different APIs and conditions so it may take some research.

@scantor
Copy link

@scantor scantor commented on 247d890 Aug 3, 2016

Choose a reason for hiding this comment

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

My point was more that the advisory is saying this is "fixed", but it's only fixed (I think) if you supply a certificate with a file. If you attach it with code using the SSL_CTX callback, I don't think your fix would notice. I may be incorrect about that. So it's more about the advisory making a point of saying this hole was plugged when in fact it's not. I noticed the advisory and did some research today to look at what I had done and the impact of the changes on my code.

In my case, I had code already accounting for this whole situation that dates back a number of years because I manage my own pooling of handles by identity, but I probably should have noted it to the project.

@bagder
Copy link
Member Author

@bagder bagder commented on 247d890 Aug 3, 2016

Choose a reason for hiding this comment

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

I'm pretty sure people using the SSL_CTX callback will understand that they're operating under the hood and will be able to do things libcurl cannot check. But if you have any suggestions on how to improve the docs to make this clearer, then I'll certainly appreciate the help!

@scantor
Copy link

@scantor scantor commented on 247d890 Aug 3, 2016

Choose a reason for hiding this comment

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

I would suggest amending the advisory slightly just to note that it doesn't disable these features when client certificates are used per se, but only when they're supplied using a file. It helps me when people point to advisories and ask me to respond to them since I can point to the language used and explicitly document why I'm not affected.

@bagder
Copy link
Member Author

@bagder bagder commented on 247d890 Aug 4, 2016

Choose a reason for hiding this comment

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

The problem isn't when a client cert is provided by file or not. The problem is if a TLS session is resumed and either the former or the new request is using a client cert since the session id resume logic doesn't take cert status into account and SSL servers are known to not check that for resumed sessions.

@scantor
Copy link

@scantor scantor commented on 247d890 Aug 4, 2016

Choose a reason for hiding this comment

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

I understand the issue, my point was a little muddled there. Your advisory states that you no longer allow this to happen, but in fact you do, if the certificate is not provided by a file. I'm simply saying that it should be explicit about what the fix actually addresses, and as far as I can tell, it applies only to file-based credentials. That would be true of both the session resumption and connection reuse fixes.

@bagder
Copy link
Member Author

@bagder bagder commented on 247d890 Aug 4, 2016

Choose a reason for hiding this comment

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

The advisory text is here: https://github.com/curl/curl-www/blob/master/docs/adv_20160803A.txt please feel free to send a PR or a patch for your suggested update

Please sign in to comment.