-
Notifications
You must be signed in to change notification settings - Fork 265
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
SSL_get_certificate() returns wrong certificate #1059
Comments
Have I just run into a similar issue? While testing the reverse proxy function of my server, the last domain cert loaded in the the config is only being used or am I using the library wrong. *ctx =tls_server() ... *cfg=tls_config_new() ...set the the keys ... tls_configure(*ctx,*cfg) free the *cfg, loop adding the proxied domain certs. Then create a socket and call tls_accept_socket. |
I'm not really familiar with libtls interface, but from the description it looks like you try to use multiple certificates for different entities in a single libtls context. From the code it looks like this is implemented by libtls by creating multiple SSL contexts internally and then using first SSL context which matches the server name as provided by the client. This issue, however, is not about about certificates for different entities, but about multiple certificates for the same entity which use different algorithms, such as RSA and ECDSA. This is natively supported by a single SSL context, and mostly works in LibreSSL (with the notable exception described in #1058) - that is, the client sees correct certificate. But the certificate as returned via Summing the above, I doubt it's the same issue. |
You didn't really indicate how you were reproducing this, so I'm not exactly set up to test. Does this fix your issue? diff --git a/lib/libssl/tls13_server.c b/lib/libssl/tls13_server.c
index dfeb1e01663..fad1f3b5f51 100644
--- a/lib/libssl/tls13_server.c
+++ b/lib/libssl/tls13_server.c
@@ -651,6 +651,10 @@ tls13_server_certificate_send(struct tls13_ctx *ctx, CBB *cbb)
}
ctx->hs->tls13.cpk = cpk;
+ /* Set the current certificate to the one we will use so
+ * SSL_get_certificate et al can pick it up.
+ */
+ s->cert->key = cpk;
ctx->hs->our_sigalg = sigalg;
if ((chain = cpk->chain) == NULL) |
To reproduce, I'm using [ssl_stapling.t](github.com/freenginx/nginx-tests](https://github.com/freenginx/nginx-tests/blob/default/ssl_stapling.t) test against freenginx with a workaround for #1058. All tests should pass, including those 3 currently marked as TODO for LibreSSL. Any fix for #1058 would do, so the workaround shouldn't be needed as you've already committed a fix (thanks, btw). With the patch, all tests pass as they should, thanks. Note thought that this does not address similar issue with TLSv1.2 and no OCSP stapling configured. I don't have any automated tests for this though. Reproduction will require calling |
On Tue, Jul 09, 2024 at 10:28:41AM -0700, Maxim Dounin wrote:
To reproduce, I'm using [ssl_stapling.t](github.com/freenginx/nginx-tests](https://github.com/freenginx/nginx-tests/blob/default/ssl_stapling.t) test against freenginx with a workaround for #1058. All tests should pass, including those 3 currently marked as TODO for LibreSSL. Any fix for #1058 would do, so the workaround shouldn't be needed as you've already committed a fix (thanks, btw). With the patch, all tests pass as they should, thanks.
Note thought that this does not address similar issue with TLSv1.2 and no OCSP stapling configured. I don't have any automated tests for this though. Reproduction will require calling `SSL_get_certificate()` on the server side for an established TLSv1.2 connection when the server uses both RSA and ECDSA certificates (and does not use OCSP stapling).
This may fix your TLS1.2 without OCSP stapling issue.
From 6cd01f1ea860a96be688f40369e044eb6202d7aa Mon Sep 17 00:00:00 2001
From: Bob Beck ***@***.***>
Date: Tue, 9 Jul 2024 09:02:55 -0600
Subject: [PATCH] Fix SSL_get_certificate()
Currently the value it looks at is only set when we have ocsp stapling
configured in tls 1.2, so change it to always set the value in tls 1.2
and in tls 1.3 when we know what it is.
---
lib/libssl/t1_lib.c | 51 +++++++++++++++++++++------------------
lib/libssl/tls13_server.c | 1 +
2 files changed, 28 insertions(+), 24 deletions(-)
diff --git a/lib/libssl/t1_lib.c b/lib/libssl/t1_lib.c
index 9680c8d2131..9b596b7e38c 100644
--- a/lib/libssl/t1_lib.c
+++ b/lib/libssl/t1_lib.c
@@ -769,8 +769,7 @@ ssl_check_clienthello_tlsext_late(SSL *s)
* the certificate has changed, and must be called after the cipher
* has been chosen because this may influence which certificate is sent
*/
- if ((s->tlsext_status_type != -1) &&
- s->ctx && s->ctx->tlsext_status_cb) {
+ if ((s->tlsext_status_type != -1) && s->ctx) {
int r;
SSL_CERT_PKEY *certpkey;
certpkey = ssl_get_server_send_pkey(s);
@@ -779,30 +778,34 @@ ssl_check_clienthello_tlsext_late(SSL *s)
s->tlsext_status_expected = 0;
return 1;
}
- /* Set current certificate to one we will use so
- * SSL_get_certificate et al can pick it up.
- */
- s->cert->key = certpkey;
- r = s->ctx->tlsext_status_cb(s,
- s->ctx->tlsext_status_arg);
- switch (r) {
- /* We don't want to send a status request response */
- case SSL_TLSEXT_ERR_NOACK:
- s->tlsext_status_expected = 0;
- break;
- /* status request response should be sent */
- case SSL_TLSEXT_ERR_OK:
- if (s->tlsext_ocsp_resp)
- s->tlsext_status_expected = 1;
- else
+ s->cert->key = certpkey; /* For SSL_get_certificate(). */
+ if (s->ctx->tlsext_status_cb) {
+ r = s->ctx->tlsext_status_cb(s,
+ s->ctx->tlsext_status_arg);
+ switch (r) {
+ /*
+ * We don't want to send a status request
+ * response
+ */
+ case SSL_TLSEXT_ERR_NOACK:
s->tlsext_status_expected = 0;
- break;
- /* something bad happened */
- case SSL_TLSEXT_ERR_ALERT_FATAL:
- ret = SSL_TLSEXT_ERR_ALERT_FATAL;
- al = SSL_AD_INTERNAL_ERROR;
- goto err;
+ break;
+ /* status request response should be sent */
+ case SSL_TLSEXT_ERR_OK:
+ if (s->tlsext_ocsp_resp)
+ s->tlsext_status_expected = 1;
+ else
+ s->tlsext_status_expected = 0;
+ break;
+ /* something bad happened */
+ case SSL_TLSEXT_ERR_ALERT_FATAL:
+ ret = SSL_TLSEXT_ERR_ALERT_FATAL;
+ al = SSL_AD_INTERNAL_ERROR;
+ goto err;
+ }
}
+ else
+ s->tlsext_status_expected = 0;
} else
s->tlsext_status_expected = 0;
diff --git a/lib/libssl/tls13_server.c b/lib/libssl/tls13_server.c
index dfeb1e01663..fc43f9d3404 100644
--- a/lib/libssl/tls13_server.c
+++ b/lib/libssl/tls13_server.c
@@ -651,6 +651,7 @@ tls13_server_certificate_send(struct tls13_ctx *ctx, CBB *cbb)
}
ctx->hs->tls13.cpk = cpk;
+ s->cert->key = cpk; /* For SSL_get_certificate(). */
ctx->hs->our_sigalg = sigalg;
if ((chain = cpk->chain) == NULL)
--
2.45.2
… --
Reply to this email directly or view it on GitHub:
#1059 (comment)
You are receiving this because you commented.
Message ID: ***@***.***>
|
That's not really mine issue: as outlined in the initial description, The TLSv1.2 part of the patch suggested looks wrong though, as it only preserves correct certificate for Just in case, I've used the following patch on top of freenginx for testing:
And a configuration to return
Testing without
Testing with
Hope this helps. |
When testing a workaround for issue #1058, I observed that freenginx OCSP stapling tests still fail even with the workaround in place (again, testing with LibreSSL 3.9.2). Digging further suggests that the culprit is
SSL_get_certificate()
, which returns wrong certificate for TLSv1.3 in configurations with multiple certificates.With TLSv1.2, it works correctly (but only with OCSP stapling actually configured and requested by the client) due to the following code in ssl_check_clienthello_tlsext_late():
There seems to be no equivalent for TLSv1.3.
Please also note that
SSL_get_certificate()
returns wrong certificate in configurations with multiple certificates with TLSv1.2 as well, as long as OCSP stapling is not configured. This is not something freenginx currently use, but I've added a variable just for testing this specific issue - and was surprised it doesn't work properly in simple tests not only with TLSv1.3, but also with TLSv1.2 (but works with TLSv1.2 and OCSP stapling configured). The code quoted above seems to explain the reason.It would be great to get this fixed.
The text was updated successfully, but these errors were encountered: