From 6a9e81b705c04b7488e49103c51ee209065c83e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 6 Oct 2025 19:46:21 +0100 Subject: [PATCH 01/13] crypto: propagate Error object on premature termination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The way that premature termination was handled in TLS connections was changed to handle an ordering problem during graceful shutdown in the migration code. Unfortunately one of the codepaths returned -1 to indicate an error condition, but failed to set the 'errp' parameter. This broke error handling in the qio_channel_tls_handshake function, as the QTask callback would no longer see that an error was raised. As a result, the client will go on to try to use the already closed TLS connection, resulting in misleading errors. This was evidenced in the I/O test 233 which showed changes such as -qemu-nbd: Certificate does not match the hostname localhost +qemu-nbd: Failed to read initial magic: Unable to read from socket: Connection reset by peer Fixes: 7e0c22d585581b8083ffdeb332ea497218665daf Acked-by: Peter Xu Signed-off-by: Daniel P. Berrangé --- crypto/tlssession.c | 8 +++++--- io/channel-tls.c | 13 +++++++------ 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/crypto/tlssession.c b/crypto/tlssession.c index ac38c2121d..8c0bf457ad 100644 --- a/crypto/tlssession.c +++ b/crypto/tlssession.c @@ -569,8 +569,6 @@ qcrypto_tls_session_read(QCryptoTLSSession *session, if (ret < 0) { if (ret == GNUTLS_E_AGAIN) { return QCRYPTO_TLS_SESSION_ERR_BLOCK; - } else if (ret == GNUTLS_E_PREMATURE_TERMINATION) { - return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION; } else { if (session->rerr) { error_propagate(errp, session->rerr); @@ -580,7 +578,11 @@ qcrypto_tls_session_read(QCryptoTLSSession *session, "Cannot read from TLS channel: %s", gnutls_strerror(ret)); } - return -1; + if (ret == GNUTLS_E_PREMATURE_TERMINATION) { + return QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION; + } else { + return -1; + } } } diff --git a/io/channel-tls.c b/io/channel-tls.c index 1fbed4be0c..70fad38d18 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -368,6 +368,7 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc, int flags, Error **errp) { + ERRP_GUARD(); QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc); size_t i; ssize_t got = 0; @@ -384,13 +385,13 @@ static ssize_t qio_channel_tls_readv(QIOChannel *ioc, } else { return QIO_CHANNEL_ERR_BLOCK; } - } else if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION) { - if (qio_channel_tls_allow_premature_termination(tioc, flags)) { - ret = 0; - } else { - return -1; - } } else if (ret < 0) { + if (ret == QCRYPTO_TLS_SESSION_PREMATURE_TERMINATION && + qio_channel_tls_allow_premature_termination(tioc, flags)) { + error_free(*errp); + *errp = NULL; + return got; + } return -1; } got += ret; From 7249713b4f3a0c0ae5e83015f9889d1c3d75218d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 1 Aug 2025 17:39:58 +0100 Subject: [PATCH 02/13] qom: use ERRP_GUARD in user_creatable_complete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit With error_propagate, the stack trace from any error_abort/fatal usage will start from the error_propagate() call, which is largely useless. Using ERRP_GUARD ensures the stack trace starts from the origin that reported the error. Reviewed-by: Paolo Bonzini Reviewed-by: Markus Armbruster Signed-off-by: Daniel P. Berrangé --- qom/object_interfaces.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c index 1ffea1a728..415cbee8c5 100644 --- a/qom/object_interfaces.c +++ b/qom/object_interfaces.c @@ -24,13 +24,12 @@ bool user_creatable_complete(UserCreatable *uc, Error **errp) { UserCreatableClass *ucc = USER_CREATABLE_GET_CLASS(uc); - Error *err = NULL; + ERRP_GUARD(); if (ucc->complete) { - ucc->complete(uc, &err); - error_propagate(errp, err); + ucc->complete(uc, errp); } - return !err; + return !*errp; } bool user_creatable_can_be_deleted(UserCreatable *uc) From e60eff91ef347361411c5173c829a8a94acf8795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Thu, 3 Mar 2022 16:34:51 +0000 Subject: [PATCH 03/13] tests: use macros for registering char tests for sockets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The test-char.c has a couple of helper macros for registering tests that need to be repeated for both IP and UNIX sockets. One test case was not using the macro though. Reviewed-by: Peter Maydell Signed-off-by: Daniel P. Berrangé --- tests/unit/test-char.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/unit/test-char.c b/tests/unit/test-char.c index f30a39f61f..e156b17329 100644 --- a/tests/unit/test-char.c +++ b/tests/unit/test-char.c @@ -1934,7 +1934,9 @@ int main(int argc, char **argv) g_test_add_data_func("/char/socket/server/mainloop-fdpass/" # name, \ &server3 ##name, char_socket_server_test); \ g_test_add_data_func("/char/socket/server/wait-conn-fdpass/" # name, \ - &server4 ##name, char_socket_server_test) + &server4 ##name, char_socket_server_test); \ + g_test_add_data_func("/char/socket/server/two-clients/" # name, \ + addr, char_socket_server_two_clients_test) #define SOCKET_CLIENT_TEST(name, addr) \ static CharSocketClientTestConfig client1 ## name = \ @@ -1974,14 +1976,10 @@ int main(int argc, char **argv) if (has_ipv4) { SOCKET_SERVER_TEST(tcp, &tcpaddr); SOCKET_CLIENT_TEST(tcp, &tcpaddr); - g_test_add_data_func("/char/socket/server/two-clients/tcp", &tcpaddr, - char_socket_server_two_clients_test); } #ifndef WIN32 SOCKET_SERVER_TEST(unix, &unixaddr); SOCKET_CLIENT_TEST(unix, &unixaddr); - g_test_add_data_func("/char/socket/server/two-clients/unix", &unixaddr, - char_socket_server_two_clients_test); #endif g_test_add_func("/char/udp", char_udp_test); From 2c147611cf568eb1cd7dc8bf4479b272bad3b9d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 3 Oct 2025 14:22:12 +0100 Subject: [PATCH 04/13] io: release active GSource in TLS channel finalizer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit While code is supposed to call qio_channel_close() before releasing the last reference on an QIOChannel, this is not guaranteed. QIOChannelFile and QIOChannelSocket both cleanup resources in their finalizer if the close operation was missed. This ensures the TLS channel will do the same failsafe cleanup. Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- io/channel-tls.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/io/channel-tls.c b/io/channel-tls.c index 70fad38d18..ce041795c1 100644 --- a/io/channel-tls.c +++ b/io/channel-tls.c @@ -342,6 +342,16 @@ static void qio_channel_tls_finalize(Object *obj) { QIOChannelTLS *ioc = QIO_CHANNEL_TLS(obj); + if (ioc->hs_ioc_tag) { + trace_qio_channel_tls_handshake_cancel(ioc); + g_clear_handle_id(&ioc->hs_ioc_tag, g_source_remove); + } + + if (ioc->bye_ioc_tag) { + trace_qio_channel_tls_bye_cancel(ioc); + g_clear_handle_id(&ioc->bye_ioc_tag, g_source_remove); + } + object_unref(OBJECT(ioc->master)); qcrypto_tls_session_free(ioc->session); } From 322c3c4f3abee616a18b3bfe563ec29dd67eae63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 30 Sep 2025 11:58:35 +0100 Subject: [PATCH 05/13] io: move websock resource release to close method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The QIOChannelWebsock object releases all its resources in the finalize callback. This is later than desired, as callers expect to be able to call qio_channel_close() to fully close a channel and release resources related to I/O. The logic in the finalize method is at most a failsafe to handle cases where a consumer forgets to call qio_channel_close. This adds equivalent logic to the close method to release the resources, using g_clear_handle_id/g_clear_pointer to be robust against repeated invocations. The finalize method is tweaked so that the GSource is removed before releasing the underlying channel. Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- io/channel-websock.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/io/channel-websock.c b/io/channel-websock.c index 0a8c5c4712..a50a160e18 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -922,13 +922,13 @@ static void qio_channel_websock_finalize(Object *obj) buffer_free(&ioc->encinput); buffer_free(&ioc->encoutput); buffer_free(&ioc->rawinput); - object_unref(OBJECT(ioc->master)); if (ioc->io_tag) { g_source_remove(ioc->io_tag); } if (ioc->io_err) { error_free(ioc->io_err); } + object_unref(OBJECT(ioc->master)); } @@ -1218,6 +1218,15 @@ static int qio_channel_websock_close(QIOChannel *ioc, QIOChannelWebsock *wioc = QIO_CHANNEL_WEBSOCK(ioc); trace_qio_channel_websock_close(ioc); + buffer_free(&wioc->encinput); + buffer_free(&wioc->encoutput); + buffer_free(&wioc->rawinput); + if (wioc->io_tag) { + g_clear_handle_id(&wioc->io_tag, g_source_remove); + } + if (wioc->io_err) { + g_clear_pointer(&wioc->io_err, error_free); + } return qio_channel_close(wioc->master, errp); } From b7a1f2ca45c7865b9e98e02ae605a65fc9458ae9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 30 Sep 2025 12:03:15 +0100 Subject: [PATCH 06/13] io: fix use after free in websocket handshake code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the QIOChannelWebsock object is freed while it is waiting to complete a handshake, a GSource is leaked. This can lead to the callback firing later on and triggering a use-after-free in the use of the channel. This was observed in the VNC server with the following trace from valgrind: ==2523108== Invalid read of size 4 ==2523108== at 0x4054A24: vnc_disconnect_start (vnc.c:1296) ==2523108== by 0x4054A24: vnc_client_error (vnc.c:1392) ==2523108== by 0x4068A09: vncws_handshake_done (vnc-ws.c:105) ==2523108== by 0x44863B4: qio_task_complete (task.c:197) ==2523108== by 0x448343D: qio_channel_websock_handshake_io (channel-websock.c:588) ==2523108== by 0x6EDB862: UnknownInlinedFun (gmain.c:3398) ==2523108== by 0x6EDB862: g_main_context_dispatch_unlocked.lto_priv.0 (gmain.c:4249) ==2523108== by 0x6EDBAE4: g_main_context_dispatch (gmain.c:4237) ==2523108== by 0x45EC79F: glib_pollfds_poll (main-loop.c:287) ==2523108== by 0x45EC79F: os_host_main_loop_wait (main-loop.c:310) ==2523108== by 0x45EC79F: main_loop_wait (main-loop.c:589) ==2523108== by 0x423A56D: qemu_main_loop (runstate.c:835) ==2523108== by 0x454F300: qemu_default_main (main.c:37) ==2523108== by 0x73D6574: (below main) (libc_start_call_main.h:58) ==2523108== Address 0x57a6e0dc is 28 bytes inside a block of size 103,608 free'd ==2523108== at 0x5F2FE43: free (vg_replace_malloc.c:989) ==2523108== by 0x6EDC444: g_free (gmem.c:208) ==2523108== by 0x4053F23: vnc_update_client (vnc.c:1153) ==2523108== by 0x4053F23: vnc_refresh (vnc.c:3225) ==2523108== by 0x4042881: dpy_refresh (console.c:880) ==2523108== by 0x4042881: gui_update (console.c:90) ==2523108== by 0x45EFA1B: timerlist_run_timers.part.0 (qemu-timer.c:562) ==2523108== by 0x45EFC8F: timerlist_run_timers (qemu-timer.c:495) ==2523108== by 0x45EFC8F: qemu_clock_run_timers (qemu-timer.c:576) ==2523108== by 0x45EFC8F: qemu_clock_run_all_timers (qemu-timer.c:663) ==2523108== by 0x45EC765: main_loop_wait (main-loop.c:600) ==2523108== by 0x423A56D: qemu_main_loop (runstate.c:835) ==2523108== by 0x454F300: qemu_default_main (main.c:37) ==2523108== by 0x73D6574: (below main) (libc_start_call_main.h:58) ==2523108== Block was alloc'd at ==2523108== at 0x5F343F3: calloc (vg_replace_malloc.c:1675) ==2523108== by 0x6EE2F81: g_malloc0 (gmem.c:133) ==2523108== by 0x4057DA3: vnc_connect (vnc.c:3245) ==2523108== by 0x448591B: qio_net_listener_channel_func (net-listener.c:54) ==2523108== by 0x6EDB862: UnknownInlinedFun (gmain.c:3398) ==2523108== by 0x6EDB862: g_main_context_dispatch_unlocked.lto_priv.0 (gmain.c:4249) ==2523108== by 0x6EDBAE4: g_main_context_dispatch (gmain.c:4237) ==2523108== by 0x45EC79F: glib_pollfds_poll (main-loop.c:287) ==2523108== by 0x45EC79F: os_host_main_loop_wait (main-loop.c:310) ==2523108== by 0x45EC79F: main_loop_wait (main-loop.c:589) ==2523108== by 0x423A56D: qemu_main_loop (runstate.c:835) ==2523108== by 0x454F300: qemu_default_main (main.c:37) ==2523108== by 0x73D6574: (below main) (libc_start_call_main.h:58) ==2523108== The above can be reproduced by launching QEMU with $ qemu-system-x86_64 -vnc localhost:0,websocket=5700 and then repeatedly running: for i in {1..100}; do (echo -n "GET / HTTP/1.1" && sleep 0.05) | nc -w 1 localhost 5700 & done CVE-2025-11234 Reported-by: Grant Millar | Cylo Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- include/io/channel-websock.h | 3 ++- io/channel-websock.c | 22 ++++++++++++++++------ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/include/io/channel-websock.h b/include/io/channel-websock.h index e180827c57..6700cf8946 100644 --- a/include/io/channel-websock.h +++ b/include/io/channel-websock.h @@ -61,7 +61,8 @@ struct QIOChannelWebsock { size_t payload_remain; size_t pong_remain; QIOChannelWebsockMask mask; - guint io_tag; + guint hs_io_tag; /* tracking handshake task */ + guint io_tag; /* tracking watch task */ Error *io_err; gboolean io_eof; uint8_t opcode; diff --git a/io/channel-websock.c b/io/channel-websock.c index a50a160e18..cb4dafdebb 100644 --- a/io/channel-websock.c +++ b/io/channel-websock.c @@ -545,6 +545,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc, trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err)); qio_task_set_error(task, err); qio_task_complete(task); + wioc->hs_io_tag = 0; return FALSE; } @@ -560,6 +561,7 @@ static gboolean qio_channel_websock_handshake_send(QIOChannel *ioc, trace_qio_channel_websock_handshake_complete(ioc); qio_task_complete(task); } + wioc->hs_io_tag = 0; return FALSE; } trace_qio_channel_websock_handshake_pending(ioc, G_IO_OUT); @@ -586,6 +588,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc, trace_qio_channel_websock_handshake_fail(ioc, error_get_pretty(err)); qio_task_set_error(task, err); qio_task_complete(task); + wioc->hs_io_tag = 0; return FALSE; } if (ret == 0) { @@ -597,7 +600,7 @@ static gboolean qio_channel_websock_handshake_io(QIOChannel *ioc, error_propagate(&wioc->io_err, err); trace_qio_channel_websock_handshake_reply(ioc); - qio_channel_add_watch( + wioc->hs_io_tag = qio_channel_add_watch( wioc->master, G_IO_OUT, qio_channel_websock_handshake_send, @@ -907,11 +910,12 @@ void qio_channel_websock_handshake(QIOChannelWebsock *ioc, trace_qio_channel_websock_handshake_start(ioc); trace_qio_channel_websock_handshake_pending(ioc, G_IO_IN); - qio_channel_add_watch(ioc->master, - G_IO_IN, - qio_channel_websock_handshake_io, - task, - NULL); + ioc->hs_io_tag = qio_channel_add_watch( + ioc->master, + G_IO_IN, + qio_channel_websock_handshake_io, + task, + NULL); } @@ -922,6 +926,9 @@ static void qio_channel_websock_finalize(Object *obj) buffer_free(&ioc->encinput); buffer_free(&ioc->encoutput); buffer_free(&ioc->rawinput); + if (ioc->hs_io_tag) { + g_source_remove(ioc->hs_io_tag); + } if (ioc->io_tag) { g_source_remove(ioc->io_tag); } @@ -1221,6 +1228,9 @@ static int qio_channel_websock_close(QIOChannel *ioc, buffer_free(&wioc->encinput); buffer_free(&wioc->encoutput); buffer_free(&wioc->rawinput); + if (wioc->hs_io_tag) { + g_clear_handle_id(&wioc->hs_io_tag, g_source_remove); + } if (wioc->io_tag) { g_clear_handle_id(&wioc->io_tag, g_source_remove); } From 75216f239f663f66ea0358a6fc2354d39909c1e3 Mon Sep 17 00:00:00 2001 From: Henry Kleynhans Date: Wed, 22 Dec 2021 15:06:00 +0000 Subject: [PATCH 07/13] crypto: only verify CA certs in chain of trust MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The CA file provided to qemu may contain CA certificates which do not form part of the chain of trust for the specific certificate we are sanity checking. This patch changes the sanity checking from validating every CA certificate to only checking the CA certificates which are part of the chain of trust (issuer chain). Other certificates are ignored. Reviewed-by: Daniel P. Berrangé Reviewed-by: Eric Blake Signed-off-by: Henry Kleynhans [DB: changed 'int' to 'bool' in 'checking_issuer' variable] Signed-off-by: Daniel P. Berrangé --- crypto/tlscredsx509.c | 57 ++++++++++++++++++++++++--- tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index cd1f504471..3df2a33b0b 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -315,6 +315,51 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds, return 0; } +static int +qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, + gnutls_x509_crt_t cert, + gnutls_x509_crt_t *cacerts, + unsigned int ncacerts, + const char *cacertFile, + bool isServer, + bool isCA, + Error **errp) +{ + gnutls_x509_crt_t *cert_to_check = &cert; + bool checking_issuer = true; + int retval = 0; + + while (checking_issuer) { + checking_issuer = false; + + if (gnutls_x509_crt_check_issuer(*cert_to_check, + *cert_to_check)) { + /* + * The cert is self-signed indicating we have + * reached the root of trust. + */ + return qcrypto_tls_creds_check_cert( + creds, *cert_to_check, cacertFile, + isServer, isCA, errp); + } + for (int i = 0; i < ncacerts; i++) { + if (gnutls_x509_crt_check_issuer(*cert_to_check, + cacerts[i])) { + retval = qcrypto_tls_creds_check_cert( + creds, cacerts[i], cacertFile, + isServer, isCA, errp); + if (retval < 0) { + return retval; + } + cert_to_check = &cacerts[i]; + checking_issuer = true; + break; + } + } + } + + return -1; +} static int qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert, @@ -499,12 +544,12 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, goto cleanup; } - for (i = 0; i < ncacerts; i++) { - if (qcrypto_tls_creds_check_cert(creds, - cacerts[i], cacertFile, - isServer, true, errp) < 0) { - goto cleanup; - } + if (cert && + qcrypto_tls_creds_check_authority_chain(creds, cert, + cacerts, ncacerts, + cacertFile, isServer, + true, errp) < 0) { + goto cleanup; } if (cert && ncacerts && diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c index 3c25d75ca1..a7ea5f422d 100644 --- a/tests/unit/test-crypto-tlscredsx509.c +++ b/tests/unit/test-crypto-tlscredsx509.c @@ -589,6 +589,12 @@ int main(int argc, char **argv) true, true, GNUTLS_KEY_KEY_CERT_SIGN, false, false, NULL, NULL, 0, 0); + TLS_CERT_REQ(cacertlevel1creq_invalid, cacertrootreq, + "UK", "qemu level 1c invalid", NULL, NULL, NULL, NULL, + true, true, true, + true, true, GNUTLS_KEY_KEY_CERT_SIGN, + false, false, NULL, NULL, + 360, 400); TLS_CERT_REQ(cacertlevel2areq, cacertlevel1areq, "UK", "qemu level 2a", NULL, NULL, NULL, NULL, true, true, true, @@ -617,16 +623,32 @@ int main(int argc, char **argv) cacertlevel2areq.crt, }; + test_tls_write_cert_chain(WORKDIR "cacertchain-ctx.pem", certchain, G_N_ELEMENTS(certchain)); + gnutls_x509_crt_t certchain_with_invalid[] = { + cacertrootreq.crt, + cacertlevel1areq.crt, + cacertlevel1breq.crt, + cacertlevel1creq_invalid.crt, + cacertlevel2areq.crt, + }; + + test_tls_write_cert_chain(WORKDIR "cacertchain-with-invalid-ctx.pem", + certchain_with_invalid, + G_N_ELEMENTS(certchain_with_invalid)); + TLS_TEST_REG(chain1, true, WORKDIR "cacertchain-ctx.pem", servercertlevel3areq.filename, false); TLS_TEST_REG(chain2, false, WORKDIR "cacertchain-ctx.pem", clientcertlevel2breq.filename, false); + TLS_TEST_REG(certchainwithexpiredcert, false, + WORKDIR "cacertchain-with-invalid-ctx.pem", + clientcertlevel2breq.filename, false); /* Some missing certs - first two are fatal, the last * is ok @@ -640,7 +662,6 @@ int main(int argc, char **argv) TLS_TEST_REG(missingclient, false, cacert1req.filename, "clientcertdoesnotexist.pem", false); - ret = g_test_run(); test_tls_discard_cert(&cacertreq); @@ -694,10 +715,12 @@ int main(int argc, char **argv) test_tls_discard_cert(&cacertrootreq); test_tls_discard_cert(&cacertlevel1areq); test_tls_discard_cert(&cacertlevel1breq); + test_tls_discard_cert(&cacertlevel1creq_invalid); test_tls_discard_cert(&cacertlevel2areq); test_tls_discard_cert(&servercertlevel3areq); test_tls_discard_cert(&clientcertlevel2breq); unlink(WORKDIR "cacertchain-ctx.pem"); + unlink(WORKDIR "cacertchain-with-invalid-ctx.pem"); test_tls_cleanup(KEYFILE); rmdir(WORKDIR); From 068e0379af39e5f02fc3e47f7638af194695c990 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 11 Jul 2025 13:30:07 +0100 Subject: [PATCH 08/13] crypto: remove extraneous pointer usage in gnutls certs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 'gnutls_x509_crt_t' type is already a pointer, not a struct, so the extra level of pointer indirection is not needed. Reviewed-by: Philippe Mathieu-Daudé Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- crypto/tlscredsx509.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index 3df2a33b0b..4169ad9a75 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -325,25 +325,25 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, bool isCA, Error **errp) { - gnutls_x509_crt_t *cert_to_check = &cert; + gnutls_x509_crt_t cert_to_check = cert; bool checking_issuer = true; int retval = 0; while (checking_issuer) { checking_issuer = false; - if (gnutls_x509_crt_check_issuer(*cert_to_check, - *cert_to_check)) { + if (gnutls_x509_crt_check_issuer(cert_to_check, + cert_to_check)) { /* * The cert is self-signed indicating we have * reached the root of trust. */ return qcrypto_tls_creds_check_cert( - creds, *cert_to_check, cacertFile, + creds, cert_to_check, cacertFile, isServer, isCA, errp); } for (int i = 0; i < ncacerts; i++) { - if (gnutls_x509_crt_check_issuer(*cert_to_check, + if (gnutls_x509_crt_check_issuer(cert_to_check, cacerts[i])) { retval = qcrypto_tls_creds_check_cert( creds, cacerts[i], cacertFile, @@ -351,7 +351,7 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, if (retval < 0) { return retval; } - cert_to_check = &cacerts[i]; + cert_to_check = cacerts[i]; checking_issuer = true; break; } From d37677602a76557f9096ce35eaf25296c7da636c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Mon, 20 Oct 2025 15:04:39 +0100 Subject: [PATCH 09/13] crypto: validate an error is reported in test expected fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There was a bug where TLS x509 credentials validation failed to fill out the Error object. Validate this in the failure scenarios. Reviewed-by: Eric Blake Reviewed-by: Philippe Mathieu-Daudé Signed-off-by: Daniel P. Berrangé --- tests/unit/test-crypto-tlscredsx509.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c index a7ea5f422d..85f51aee1b 100644 --- a/tests/unit/test-crypto-tlscredsx509.c +++ b/tests/unit/test-crypto-tlscredsx509.c @@ -73,6 +73,7 @@ static void test_tls_creds(const void *opaque) struct QCryptoTLSCredsTestData *data = (struct QCryptoTLSCredsTestData *)opaque; QCryptoTLSCreds *creds; + Error *err = NULL; #define CERT_DIR "tests/test-crypto-tlscredsx509-certs/" g_mkdir_with_parents(CERT_DIR, 0700); @@ -111,10 +112,12 @@ static void test_tls_creds(const void *opaque) QCRYPTO_TLS_CREDS_ENDPOINT_SERVER : QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT), CERT_DIR, - data->expectFail ? NULL : &error_abort); + data->expectFail ? &err : &error_abort); if (data->expectFail) { g_assert(creds == NULL); + g_assert(err != NULL); + error_free(err); } else { g_assert(creds != NULL); } From c677657e99e080a174052074ae77ff37dbe91335 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Wed, 9 Jul 2025 08:36:22 +0100 Subject: [PATCH 10/13] crypto: fix error reporting in cert chain checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The loop that checks the CA certificate chain can fail to report an error message if one of the certs in the chain has an issuer that is not present in the chain. In this case, the outer loop 'while (checking_issuer)' will terminate after failing to find the issuer, and no error message will be reported. Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- crypto/tlscredsx509.c | 33 ++++++++++++++++++--------- tests/unit/test-crypto-tlscredsx509.c | 12 ++++++++++ 2 files changed, 34 insertions(+), 11 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index 4169ad9a75..2eccd71b3c 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -326,11 +326,11 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, Error **errp) { gnutls_x509_crt_t cert_to_check = cert; - bool checking_issuer = true; int retval = 0; + gnutls_datum_t dn = {}; - while (checking_issuer) { - checking_issuer = false; + for (;;) { + gnutls_x509_crt_t cert_issuer = NULL; if (gnutls_x509_crt_check_issuer(cert_to_check, cert_to_check)) { @@ -345,19 +345,30 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, for (int i = 0; i < ncacerts; i++) { if (gnutls_x509_crt_check_issuer(cert_to_check, cacerts[i])) { - retval = qcrypto_tls_creds_check_cert( - creds, cacerts[i], cacertFile, - isServer, isCA, errp); - if (retval < 0) { - return retval; - } - cert_to_check = cacerts[i]; - checking_issuer = true; + cert_issuer = cacerts[i]; break; } } + if (!cert_issuer) { + break; + } + + if (qcrypto_tls_creds_check_cert(creds, cert_issuer, cacertFile, + isServer, isCA, errp) < 0) { + return -1; + } + + cert_to_check = cert_issuer; } + retval = gnutls_x509_crt_get_dn2(cert_to_check, &dn); + if (retval < 0) { + error_setg(errp, "Unable to fetch cert DN: %s", + gnutls_strerror(retval)); + return -1; + } + error_setg(errp, "Cert '%s' has no issuer in CA chain", dn.data); + g_free(dn.data); return -1; } diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c index 85f51aee1b..7c5df32bcc 100644 --- a/tests/unit/test-crypto-tlscredsx509.c +++ b/tests/unit/test-crypto-tlscredsx509.c @@ -643,6 +643,15 @@ int main(int argc, char **argv) certchain_with_invalid, G_N_ELEMENTS(certchain_with_invalid)); + gnutls_x509_crt_t certchain_incomplete[] = { + cacertrootreq.crt, + cacertlevel2areq.crt, + }; + + test_tls_write_cert_chain(WORKDIR "cacertchain-incomplete-ctx.pem", + certchain_incomplete, + G_N_ELEMENTS(certchain_incomplete)); + TLS_TEST_REG(chain1, true, WORKDIR "cacertchain-ctx.pem", servercertlevel3areq.filename, false); @@ -652,6 +661,9 @@ int main(int argc, char **argv) TLS_TEST_REG(certchainwithexpiredcert, false, WORKDIR "cacertchain-with-invalid-ctx.pem", clientcertlevel2breq.filename, false); + TLS_TEST_REG(chainincomplete, true, + WORKDIR "cacertchain-incomplete-ctx.pem", + servercertlevel3areq.filename, true); /* Some missing certs - first two are fatal, the last * is ok From 87d6282e53a2db1915ea715cf9ee0634f6fb1c70 Mon Sep 17 00:00:00 2001 From: matoro Date: Mon, 13 Feb 2023 13:00:49 -0500 Subject: [PATCH 11/13] crypto: allow client/server cert chains MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The existing implementation assumes that client/server certificates are single individual certificates. If using publicly-issued certificates, or internal CAs that use an intermediate issuer, this is unlikely to be the case, and they will instead be certificate chains. While this can be worked around by moving the intermediate certificates to the CA certificate, which DOES currently support multiple certificates, this instead allows the issued certificate chains to be used as-is, without requiring the overhead of shuffling certificates around. Corresponding libvirt change is available here: https://gitlab.com/libvirt/libvirt/-/merge_requests/222 Reviewed-by: Daniel P. Berrangé Reviewed-by: Eric Blake Signed-off-by: matoro [DB: adapted for code conflicts with multi-CA patch] Signed-off-by: Daniel P. Berrangé --- crypto/tlscredsx509.c | 157 ++++++++++++-------------- tests/unit/test-crypto-tlscredsx509.c | 77 +++++++++++++ 2 files changed, 147 insertions(+), 87 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index 2eccd71b3c..86fdfce886 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -317,7 +317,8 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds, static int qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, - gnutls_x509_crt_t cert, + gnutls_x509_crt_t *certs, + unsigned int ncerts, gnutls_x509_crt_t *cacerts, unsigned int ncacerts, const char *cacertFile, @@ -325,9 +326,32 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, bool isCA, Error **errp) { - gnutls_x509_crt_t cert_to_check = cert; + gnutls_x509_crt_t cert_to_check = certs[ncerts - 1]; int retval = 0; - gnutls_datum_t dn = {}; + gnutls_datum_t dn = {}, dnissuer = {}; + + for (int i = 0; i < (ncerts - 1); i++) { + if (!gnutls_x509_crt_check_issuer(certs[i], certs[i + 1])) { + retval = gnutls_x509_crt_get_dn2(certs[i], &dn); + if (retval < 0) { + error_setg(errp, "Unable to fetch cert DN: %s", + gnutls_strerror(retval)); + return -1; + } + retval = gnutls_x509_crt_get_dn2(certs[i + 1], &dnissuer); + if (retval < 0) { + g_free(dn.data); + error_setg(errp, "Unable to fetch cert DN: %s", + gnutls_strerror(retval)); + return -1; + } + error_setg(errp, "Cert '%s' does not match issuer of cert '%s'", + dnissuer.data, dn.data); + g_free(dn.data); + g_free(dnissuer.data); + return -1; + } + } for (;;) { gnutls_x509_crt_t cert_issuer = NULL; @@ -373,7 +397,8 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, } static int -qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert, +qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t *certs, + size_t ncerts, const char *certFile, gnutls_x509_crt_t *cacerts, size_t ncacerts, @@ -383,7 +408,7 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert, { unsigned int status; - if (gnutls_x509_crt_list_verify(&cert, 1, + if (gnutls_x509_crt_list_verify(certs, ncerts, cacerts, ncacerts, NULL, 0, 0, &status) < 0) { @@ -425,66 +450,14 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert, } -static gnutls_x509_crt_t -qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds, - const char *certFile, - bool isServer, - Error **errp) -{ - gnutls_datum_t data; - gnutls_x509_crt_t cert = NULL; - g_autofree char *buf = NULL; - gsize buflen; - GError *gerr = NULL; - int ret = -1; - int err; - - trace_qcrypto_tls_creds_x509_load_cert(creds, isServer, certFile); - - err = gnutls_x509_crt_init(&cert); - if (err < 0) { - error_setg(errp, "Unable to initialize certificate: %s", - gnutls_strerror(err)); - goto cleanup; - } - - if (!g_file_get_contents(certFile, &buf, &buflen, &gerr)) { - error_setg(errp, "Cannot load CA cert list %s: %s", - certFile, gerr->message); - g_error_free(gerr); - goto cleanup; - } - - data.data = (unsigned char *)buf; - data.size = strlen(buf); - - err = gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM); - if (err < 0) { - error_setg(errp, isServer ? - "Unable to import server certificate %s: %s" : - "Unable to import client certificate %s: %s", - certFile, - gnutls_strerror(err)); - goto cleanup; - } - - ret = 0; - - cleanup: - if (ret != 0) { - gnutls_x509_crt_deinit(cert); - cert = NULL; - } - return cert; -} - - static int -qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds, - const char *certFile, - gnutls_x509_crt_t **certs, - unsigned int *ncerts, - Error **errp) +qcrypto_tls_creds_load_cert_list(QCryptoTLSCredsX509 *creds, + const char *certFile, + gnutls_x509_crt_t **certs, + unsigned int *ncerts, + bool isServer, + bool isCA, + Error **errp) { gnutls_datum_t data; g_autofree char *buf = NULL; @@ -507,7 +480,9 @@ qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds, if (gnutls_x509_crt_list_import2(certs, ncerts, &data, GNUTLS_X509_FMT_PEM, 0) < 0) { error_setg(errp, - "Unable to import CA certificate list %s", + isCA ? "Unable to import CA certificate list %s" : + (isServer ? "Unable to import server certificate %s" : + "Unable to import client certificate %s"), certFile); return -1; } @@ -523,7 +498,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, const char *certFile, Error **errp) { - gnutls_x509_crt_t cert = NULL; + gnutls_x509_crt_t *certs = NULL; + unsigned int ncerts = 0; gnutls_x509_crt_t *cacerts = NULL; unsigned int ncacerts = 0; size_t i; @@ -531,41 +507,48 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, if (certFile && access(certFile, R_OK) == 0) { - cert = qcrypto_tls_creds_load_cert(creds, - certFile, isServer, - errp); - if (!cert) { + if (qcrypto_tls_creds_load_cert_list(creds, + certFile, + &certs, + &ncerts, + isServer, + false, + errp) < 0) { goto cleanup; } } if (access(cacertFile, R_OK) == 0) { - if (qcrypto_tls_creds_load_ca_cert_list(creds, - cacertFile, - &cacerts, - &ncacerts, - errp) < 0) { + if (qcrypto_tls_creds_load_cert_list(creds, + cacertFile, + &cacerts, + &ncacerts, + isServer, + true, + errp) < 0) { goto cleanup; } } - if (cert && - qcrypto_tls_creds_check_cert(creds, - cert, certFile, isServer, - false, errp) < 0) { - goto cleanup; + for (i = 0; i < ncerts; i++) { + if (qcrypto_tls_creds_check_cert(creds, + certs[i], certFile, + isServer, i != 0, errp) < 0) { + goto cleanup; + } } - if (cert && - qcrypto_tls_creds_check_authority_chain(creds, cert, + if (ncerts && + qcrypto_tls_creds_check_authority_chain(creds, + certs, ncerts, cacerts, ncacerts, cacertFile, isServer, true, errp) < 0) { goto cleanup; } - if (cert && ncacerts && - qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts, - ncacerts, cacertFile, + if (ncerts && ncacerts && + qcrypto_tls_creds_check_cert_pair(certs, ncerts, certFile, + cacerts, ncacerts, cacertFile, isServer, errp) < 0) { goto cleanup; } @@ -573,8 +556,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, ret = 0; cleanup: - if (cert) { - gnutls_x509_crt_deinit(cert); + for (i = 0; i < ncerts; i++) { + gnutls_x509_crt_deinit(certs[i]); } for (i = 0; i < ncacerts; i++) { gnutls_x509_crt_deinit(cacerts[i]); diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c index 7c5df32bcc..96ad4e741b 100644 --- a/tests/unit/test-crypto-tlscredsx509.c +++ b/tests/unit/test-crypto-tlscredsx509.c @@ -580,6 +580,12 @@ int main(int argc, char **argv) true, true, GNUTLS_KEY_KEY_CERT_SIGN, false, false, NULL, NULL, 0, 0); + TLS_ROOT_REQ(someotherrootreq, + "UK", "some other random CA", NULL, NULL, NULL, NULL, + true, true, true, + true, true, GNUTLS_KEY_KEY_CERT_SIGN, + false, false, NULL, NULL, + 0, 0); TLS_CERT_REQ(cacertlevel1areq, cacertrootreq, "UK", "qemu level 1a", NULL, NULL, NULL, NULL, true, true, true, @@ -626,6 +632,32 @@ int main(int argc, char **argv) cacertlevel2areq.crt, }; + gnutls_x509_crt_t cabundle[] = { + someotherrootreq.crt, + cacertrootreq.crt, + }; + + gnutls_x509_crt_t servercertchain[] = { + servercertlevel3areq.crt, + cacertlevel2areq.crt, + cacertlevel1areq.crt, + }; + + gnutls_x509_crt_t servercertchain_incomplete[] = { + servercertlevel3areq.crt, + cacertlevel2areq.crt, + }; + + gnutls_x509_crt_t servercertchain_unsorted[] = { + servercertlevel3areq.crt, + cacertlevel1areq.crt, + cacertlevel2areq.crt, + }; + + gnutls_x509_crt_t clientcertchain[] = { + clientcertlevel2breq.crt, + cacertlevel1breq.crt, + }; test_tls_write_cert_chain(WORKDIR "cacertchain-ctx.pem", certchain, @@ -665,6 +697,46 @@ int main(int argc, char **argv) WORKDIR "cacertchain-incomplete-ctx.pem", servercertlevel3areq.filename, true); + test_tls_write_cert_chain(WORKDIR "servercertchain-ctx.pem", + servercertchain, + G_N_ELEMENTS(servercertchain)); + + TLS_TEST_REG(serverchain, true, + cacertrootreq.filename, + WORKDIR "servercertchain-ctx.pem", false); + + test_tls_write_cert_chain(WORKDIR "cabundle-ctx.pem", + cabundle, + G_N_ELEMENTS(cabundle)); + + TLS_TEST_REG(multiplecaswithchain, true, + WORKDIR "cabundle-ctx.pem", + WORKDIR "servercertchain-ctx.pem", false); + + test_tls_write_cert_chain(WORKDIR "servercertchain_incomplete-ctx.pem", + servercertchain_incomplete, + G_N_ELEMENTS(servercertchain_incomplete)); + + TLS_TEST_REG(incompleteserverchain, true, + cacertrootreq.filename, + WORKDIR "servercertchain_incomplete-ctx.pem", true); + + test_tls_write_cert_chain(WORKDIR "servercertchain_unsorted-ctx.pem", + servercertchain_unsorted, + G_N_ELEMENTS(servercertchain_unsorted)); + + TLS_TEST_REG(unsortedserverchain, true, + cacertrootreq.filename, + WORKDIR "servercertchain_unsorted-ctx.pem", true); + + test_tls_write_cert_chain(WORKDIR "clientcertchain-ctx.pem", + clientcertchain, + G_N_ELEMENTS(clientcertchain)); + + TLS_TEST_REG(clientchain, false, + cacertrootreq.filename, + WORKDIR "clientcertchain-ctx.pem", false); + /* Some missing certs - first two are fatal, the last * is ok */ @@ -734,8 +806,13 @@ int main(int argc, char **argv) test_tls_discard_cert(&cacertlevel2areq); test_tls_discard_cert(&servercertlevel3areq); test_tls_discard_cert(&clientcertlevel2breq); + test_tls_discard_cert(&someotherrootreq); unlink(WORKDIR "cacertchain-ctx.pem"); unlink(WORKDIR "cacertchain-with-invalid-ctx.pem"); + unlink(WORKDIR "servercertchain-ctx.pem"); + unlink(WORKDIR "servercertchain_incomplete-ctx.pem"); + unlink(WORKDIR "servercertchain_unsorted-ctx.pem"); + unlink(WORKDIR "clientcertchain-ctx.pem"); test_tls_cleanup(KEYFILE); rmdir(WORKDIR); From 3995fc238e0599e0417ba958ffc5c7a609e82a7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Tue, 1 Jul 2025 18:33:21 +0100 Subject: [PATCH 12/13] crypto: stop requiring "key encipherment" usage in x509 certs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This usage flag was deprecated by RFC8813, such that it is forbidden to be present for certs using ECDSA/ECDH algorithms, and in TLS 1.3 is conceptually obsolete. As such many valid certs will no longer have this key usage flag set, and QEMU should not be rejecting them, as this prevents use of otherwise valid & desirable algorithms. Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- crypto/tlscredsx509.c | 10 +------- docs/system/tls.rst | 13 +++------- tests/unit/crypto-tls-x509-helpers.h | 6 ++--- tests/unit/test-crypto-tlscredsx509.c | 36 +++++++++++++-------------- tests/unit/test-crypto-tlssession.c | 14 +++++------ tests/unit/test-io-channel-tls.c | 4 +-- 6 files changed, 34 insertions(+), 49 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index 86fdfce886..db2b74bafa 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -144,7 +144,7 @@ qcrypto_tls_creds_check_cert_key_usage(QCryptoTLSCredsX509 *creds, if (status < 0) { if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) { usage = isCA ? GNUTLS_KEY_KEY_CERT_SIGN : - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT; + GNUTLS_KEY_DIGITAL_SIGNATURE; } else { error_setg(errp, "Unable to query certificate %s key usage: %s", @@ -171,14 +171,6 @@ qcrypto_tls_creds_check_cert_key_usage(QCryptoTLSCredsX509 *creds, return -1; } } - if (!(usage & GNUTLS_KEY_KEY_ENCIPHERMENT)) { - if (critical) { - error_setg(errp, - "Certificate %s usage does not permit key " - "encipherment", certFile); - return -1; - } - } } return 0; diff --git a/docs/system/tls.rst b/docs/system/tls.rst index e284c82801..a4f6781d62 100644 --- a/docs/system/tls.rst +++ b/docs/system/tls.rst @@ -118,7 +118,6 @@ information for each server, and use it to issue server certificates. ip_address = 2620:0:cafe::87 ip_address = 2001:24::92 tls_www_server - encryption_key signing_key EOF # certtool --generate-privkey > server-hostNNN-key.pem @@ -134,9 +133,8 @@ the subject alt name extension data. The ``tls_www_server`` keyword is the key purpose extension to indicate this certificate is intended for usage in a web server. Although QEMU network services are not in fact HTTP servers (except for VNC websockets), setting this key purpose is -still recommended. The ``encryption_key`` and ``signing_key`` keyword is -the key usage extension to indicate this certificate is intended for -usage in the data session. +still recommended. The ``signing_key`` keyword is the key usage extension +to indicate this certificate is intended for usage in the data session. The ``server-hostNNN-key.pem`` and ``server-hostNNN-cert.pem`` files should now be securely copied to the server for which they were @@ -171,7 +169,6 @@ certificates. organization = Name of your organization cn = hostNNN.foo.example.com tls_www_client - encryption_key signing_key EOF # certtool --generate-privkey > client-hostNNN-key.pem @@ -187,9 +184,8 @@ the ``dns_name`` and ``ip_address`` fields are not included. The ``tls_www_client`` keyword is the key purpose extension to indicate this certificate is intended for usage in a web client. Although QEMU network clients are not in fact HTTP clients, setting this key purpose is still -recommended. The ``encryption_key`` and ``signing_key`` keyword is the -key usage extension to indicate this certificate is intended for usage -in the data session. +recommended. The ``signing_key`` keyword is the key usage extension to +indicate this certificate is intended for usage in the data session. The ``client-hostNNN-key.pem`` and ``client-hostNNN-cert.pem`` files should now be securely copied to the client for which they were @@ -222,7 +218,6 @@ client and server instructions in one. ip_address = 2001:24::92 tls_www_server tls_www_client - encryption_key signing_key EOF # certtool --generate-privkey > both-hostNNN-key.pem diff --git a/tests/unit/crypto-tls-x509-helpers.h b/tests/unit/crypto-tls-x509-helpers.h index 2a0f7c04fd..7e9a508ad6 100644 --- a/tests/unit/crypto-tls-x509-helpers.h +++ b/tests/unit/crypto-tls-x509-helpers.h @@ -148,8 +148,7 @@ void test_tls_cleanup(const char *keyfile); .basicConstraintsIsCA = false, \ .keyUsageEnable = true, \ .keyUsageCritical = true, \ - .keyUsageValue = \ - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, \ + .keyUsageValue = GNUTLS_KEY_DIGITAL_SIGNATURE, \ .keyPurposeEnable = true, \ .keyPurposeCritical = true, \ .keyPurposeOID1 = GNUTLS_KP_TLS_WWW_CLIENT, \ @@ -168,8 +167,7 @@ void test_tls_cleanup(const char *keyfile); .basicConstraintsIsCA = false, \ .keyUsageEnable = true, \ .keyUsageCritical = true, \ - .keyUsageValue = \ - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, \ + .keyUsageValue = GNUTLS_KEY_DIGITAL_SIGNATURE, \ .keyPurposeEnable = true, \ .keyPurposeCritical = true, \ .keyPurposeOID1 = GNUTLS_KP_TLS_WWW_SERVER, \ diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c index 96ad4e741b..a5f21728d4 100644 --- a/tests/unit/test-crypto-tlscredsx509.c +++ b/tests/unit/test-crypto-tlscredsx509.c @@ -169,14 +169,14 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); TLS_CERT_REQ(clientcertreq, cacertreq, "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL, 0, 0); @@ -199,7 +199,7 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); @@ -214,7 +214,7 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); @@ -229,7 +229,7 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); @@ -253,7 +253,7 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); /* no-basic */ @@ -267,7 +267,7 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); /* Key usage:dig-sig:critical */ @@ -281,7 +281,7 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); @@ -306,7 +306,7 @@ int main(int argc, char **argv) "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT | + GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_CERT_SIGN, false, false, NULL, NULL, 0, 0); @@ -409,7 +409,7 @@ int main(int argc, char **argv) "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT | + GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_CERT_SIGN, false, false, NULL, NULL, 0, 0); @@ -511,21 +511,21 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); TLS_CERT_REQ(servercertexp1req, cacertreq, "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, -1); TLS_CERT_REQ(clientcertexp1req, cacertreq, "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL, 0, -1); @@ -549,21 +549,21 @@ int main(int argc, char **argv) "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); TLS_CERT_REQ(servercertnew1req, cacertreq, "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 1, 2); TLS_CERT_REQ(clientcertnew1req, cacertreq, "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL, 1, 2); @@ -614,14 +614,14 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); TLS_CERT_REQ(clientcertlevel2breq, cacertlevel1breq, "UK", "qemu client level 2b", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL, 0, 0); diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c index 61311cbe6e..d0baf3b304 100644 --- a/tests/unit/test-crypto-tlssession.c +++ b/tests/unit/test-crypto-tlssession.c @@ -472,14 +472,14 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); TLS_CERT_REQ(clientcertreq, cacertreq, "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL, 0, 0); @@ -487,7 +487,7 @@ int main(int argc, char **argv) "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL, 0, 0); @@ -506,7 +506,7 @@ int main(int argc, char **argv) "192.168.122.1", "fec0::dead:beaf", true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); /* This intentionally doesn't replicate */ @@ -515,7 +515,7 @@ int main(int argc, char **argv) "192.168.122.1", "fec0::dead:beaf", true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); @@ -619,14 +619,14 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); TLS_CERT_REQ(clientcertlevel2breq, cacertlevel1breq, "UK", "qemu client level 2b", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL, 0, 0); diff --git a/tests/unit/test-io-channel-tls.c b/tests/unit/test-io-channel-tls.c index 6f282ad45d..4e4034af67 100644 --- a/tests/unit/test-io-channel-tls.c +++ b/tests/unit/test-io-channel-tls.c @@ -302,14 +302,14 @@ int main(int argc, char **argv) "UK", "qemu.org", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL, 0, 0); TLS_CERT_REQ(clientcertreq, cacertreq, "UK", "qemu", NULL, NULL, NULL, NULL, true, true, false, true, true, - GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, + GNUTLS_KEY_DIGITAL_SIGNATURE, true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL, 0, 0); From 3b3257b00fd256b8704db13373f4fa9c8bc40342 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20P=2E=20Berrang=C3=A9?= Date: Fri, 11 Jul 2025 13:21:34 +0100 Subject: [PATCH 13/13] crypto: switch to newer gnutls API for distinguished name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new API automatically allocates the right amount of memory to hold the distinguished name, avoiding the need to loop and realloc. Reviewed-by: Eric Blake Signed-off-by: Daniel P. Berrangé --- crypto/tlssession.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/crypto/tlssession.c b/crypto/tlssession.c index 8c0bf457ad..92fe4f0380 100644 --- a/crypto/tlssession.c +++ b/crypto/tlssession.c @@ -409,20 +409,14 @@ qcrypto_tls_session_check_certificate(QCryptoTLSSession *session, } if (i == 0) { - size_t dnameSize = 1024; - session->peername = g_malloc(dnameSize); - requery: - ret = gnutls_x509_crt_get_dn(cert, session->peername, &dnameSize); + gnutls_datum_t dname = {}; + ret = gnutls_x509_crt_get_dn2(cert, &dname); if (ret < 0) { - if (ret == GNUTLS_E_SHORT_MEMORY_BUFFER) { - session->peername = g_realloc(session->peername, - dnameSize); - goto requery; - } error_setg(errp, "Cannot get client distinguished name: %s", gnutls_strerror(ret)); goto error; } + session->peername = (char *)g_steal_pointer(&dname.data); if (session->authzid) { bool allow;