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

CURLOPT_SSLCERT_BLOB is not checked when trying to reuse a connection #5617

Closed
ngg opened this issue Jun 26, 2020 · 9 comments
Closed

CURLOPT_SSLCERT_BLOB is not checked when trying to reuse a connection #5617

ngg opened this issue Jun 26, 2020 · 9 comments
Labels

Comments

@ngg
Copy link
Contributor

@ngg ngg commented Jun 26, 2020

CURLOPT_SSLCERT_BLOB was added in 7.71.0
In vtls.c the function Curl_ssl_config_matches does not check it for equality when determining if there is a good connection to re-use.
If there are multiple connections after each other to the same host with different client certificates, the first one will be used.
CURLOPT_SSLCERT was checked in this function (data->clientcert), so I think the blob version should be checked as well.

In my current use-case, I first connect to a server without client certificates and later I switch to using them, and it fails because of this issue.

@ngg
Copy link
Contributor Author

@ngg ngg commented Jun 26, 2020

I just realized that the blob is not part of struct ssl_primary_config, so it cannot be easily checked there, so maybe it can be checked in the ConnectionExists function after each Curl_ssl_config_matches invocation, but in that case I also think it should be checked in Curl_ssl_getsessionid. It might be better to move the blob itself to the primary part.

@bagder bagder added the SSL/TLS label Jun 26, 2020
@bagder
Copy link
Member

@bagder bagder commented Jun 26, 2020

@gvollant, want to take a look at this?

@ngg
Copy link
Contributor Author

@ngg ngg commented Jun 26, 2020

As a really UGLY workaround, I managed to make this work without modifying curl by providing the certificate chain with CURLOPT_SSLCERT_BLOB and also providing the hash of the certificate to CURLOPT_SSLCERT. This only works because in vtls/openssl.c the CURLOPT_SSLCERT_BLOB takes priority so the file is not used (which is not even existing, as it is only a hash), but the filename is checked in the reuse logic.
I didn't try other SSL backends, and the docs does not explicitly say what happens if both the blob and the file is provided, it might be worth specifying the exact behavior in that strange case.

@dfandrich
Copy link
Collaborator

@dfandrich dfandrich commented Jun 26, 2020

@bagder
Copy link
Member

@bagder bagder commented Jun 26, 2020

Me too, but since this is a brand new feature in 7.71.0 there shouldn't be any released software having had the chance to get this messed up yet. Possibly this bug is reason enough for a patch release soon?

@ngg
Copy link
Contributor Author

@ngg ngg commented Jun 26, 2020

I'm really sorry for not reporting this privately, I didn't think it has serious security implications, as this doesn't leak any new information to servers (the certificate was already used in a previous connection) and certainly does not leak any private information (which would be the case if this bug happens with passwords for example).

Because this bug is already public (to those checking this Github issue), I think the best way to treat it would be to create a patch release soon to publicly disclose the issue to every potentially affected user. (I don't think there are many software that already uses this feature)

@bagder
Copy link
Member

@bagder bagder commented Jun 26, 2020

I let had this issue rumble in my head for an hour and I'm now convinced this is reason enough for a patch release. Let's aim at fixing this ASAP and doing a 7.71.1 on Wednesday, July 1st. I'll email the list about it too. We have a few other regressions as well that will be fine to get patched sooner rather than later.

bagder added a commit that referenced this issue Jun 26, 2020
Reported-by: Gergely Nagy
Fixes #5617
@bagder
Copy link
Member

@bagder bagder commented Jun 26, 2020

First take at a fix submitted in #5619. Please give it a look.

@gvollant
Copy link
Contributor

@gvollant gvollant commented Jun 26, 2020

I'll look

bagder added a commit that referenced this issue Jun 27, 2020
Reported-by: Gergely Nagy
Fixes #5617
Closes #5619
bagder added a commit that referenced this issue Jun 28, 2020
Reported-by: Gergely Nagy
Fixes #5617
Closes #5619
@bagder bagder closed this in 9bfe665 Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

4 participants
You can’t perform that action at this time.