SSL_get_certificate() returns wrong certificate

[mdounin]

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():

		/* 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);

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.

[idle-PB]

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.
The reverse proxy works fine but not if you go to to the proxy server then the certs don't match.

[mdounin]

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. The reverse proxy works fine but not if you go to to the proxy server then the certs don't match.

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 SSL_get_certificate() when using TLSv1.3 is wrong, which breaks OCSP stapling in such configurations.

Summing the above, I doubt it's the same issue.

[bob-beck]

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)

[mdounin]

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).

[bob-beck]

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: ***@***.***>

[mdounin]

That's not really mine issue: as outlined in the initial description, SSL_get_certificate() is not currently used by [free]nginx anywhere except for OCSP stapling, and therefore it is not currently possible to trigger the TLSv1.2-specific part of this LibreSSL issue without some additional patches.

The TLSv1.2 part of the patch suggested looks wrong though, as it only preserves correct certificate for SSL_get_certificate() if certificate status extension was requested by the client, which might not be the case. Quick testing with SSL_get_certificate() on an established connections TLSv1.2 confirms this: correct certificate is only returned with openssl s_client ... -status being used as a client, and not without the -status flag.

Just in case, I've used the following patch on top of freenginx for testing:

diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -5482,21 +5482,19 @@ ngx_ssl_get_subject_dn(ngx_connection_t 
 
     s->len = 0;
 
-    cert = SSL_get_peer_certificate(c->ssl->connection);
+    cert = SSL_get_certificate(c->ssl->connection);
     if (cert == NULL) {
         return NGX_OK;
     }
 
     name = X509_get_subject_name(cert);
     if (name == NULL) {
-        X509_free(cert);
         return NGX_ERROR;
     }
 
     bio = BIO_new(BIO_s_mem());
     if (bio == NULL) {
         ngx_ssl_error(NGX_LOG_ALERT, c->log, 0, "BIO_new() failed");
-        X509_free(cert);
         return NGX_ERROR;
     }
 
@@ -5514,14 +5512,12 @@ ngx_ssl_get_subject_dn(ngx_connection_t 
     BIO_read(bio, s->data, s->len);
 
     BIO_free(bio);
-    X509_free(cert);
 
     return NGX_OK;
 
 failed:
 
     BIO_free(bio);
-    X509_free(cert);
 
     return NGX_ERROR;
 }

And a configuration to return $ssl_client_s_dn (which comes from the server certificate with the above patch):

http {
    ssl_certificate ecdsa.crt;
    ssl_certificate_key ecdsa.key;
    ssl_certificate rsa.crt;
    ssl_certificate_key rsa.key;

    ssl_protocols TLSv1.2;

    server {
        listen 8443 ssl;

        location / {
            return 200 "cert: $ssl_client_s_dn\n";
        }
    }
}

Testing without -status, note wrong cert: CN=rsa:

$ (echo 'GET /'; sleep 1) | openssl s_client -quiet -connect 127.0.0.1:8443
Can't use SSL_get_servername
depth=0 CN = ecdsa
verify error:num=18:self signed certificate
verify return:1
depth=0 CN = ecdsa
verify return:1
cert: CN=rsa

Testing with -status, note correct cert: CN=ecdsa:

$ (echo 'GET /'; sleep 1) | openssl s_client -quiet -connect 127.0.0.1:8443 -status
Can't use SSL_get_servername
depth=0 CN = ecdsa
verify error:num=18:self signed certificate
verify return:1
depth=0 CN = ecdsa
verify return:1
cert: CN=ecdsa

Hope this helps.