Hi On Thu, Oct 30, 2025 at 6:49 PM Daniel P. Berrangé <[email protected]> wrote:
> The gnutls_credentials_set() method has a very suprising API contract > that requires the caller to preserve the passed in credentials pointer > for as long as the gnutls_session_t object is alive. QEMU is failing > to ensure this happens. > > In QEMU the GNUTLS credentials object is owned by the QCryptoTLSCreds > object instance while the GNUTLS session object is owned by the > QCryptoTLSSession object instance. Their lifetimes are not guaranteed > to be the same, though in most common usage the credentials will outlive > the session. This is notably not the case, however, after the VNC server > gained the ability to reload credentials on the fly with: > > commit 1f08e3415120637cad7f540d9ceb4dba3136dbdd > Author: Zihao Chang <[email protected]> > Date: Tue Mar 16 15:58:44 2021 +0800 > > vnc: support reload x509 certificates for vnc > > If that is triggered while a VNC client is in the middle of performing > a TLS handshake, we might hit a use-after-free. > > It is difficult to correct this problem because there's no way to deep- > clone a GNUTLS credentials object, nor is it reference counted. Thus we > introduce a QCryptoTLSCredsBox object whose only purpose is to add > reference counting around the GNUTLS credentials object. > > The DH parameters set against a credentials object also have to be kept > alive for as long as the credentials exist. So the box must also hold > the DH parameters pointer. > > Signed-off-by: Daniel P. Berrangé <[email protected]> > Reviewed-by: Marc-André Lureau <[email protected]> > --- > crypto/meson.build | 5 ++- > crypto/tlscredsbox.c | 101 +++++++++++++++++++++++++++++++++++++++++++ > crypto/tlscredsbox.h | 46 ++++++++++++++++++++ > 3 files changed, 151 insertions(+), 1 deletion(-) > create mode 100644 crypto/tlscredsbox.c > create mode 100644 crypto/tlscredsbox.h > > diff --git a/crypto/meson.build b/crypto/meson.build > index 735635de1f..1fc14b2a04 100644 > --- a/crypto/meson.build > +++ b/crypto/meson.build > @@ -25,7 +25,10 @@ crypto_ss.add(files( > )) > > if gnutls.found() > - crypto_ss.add(files('x509-utils.c')) > + crypto_ss.add(files( > + 'tlscredsbox.c', > + 'x509-utils.c', > + )) > endif > > if nettle.found() > diff --git a/crypto/tlscredsbox.c b/crypto/tlscredsbox.c > new file mode 100644 > index 0000000000..b8d9846af8 > --- /dev/null > +++ b/crypto/tlscredsbox.c > @@ -0,0 +1,101 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-or-later > + * > + * QEMU crypto TLS credential support > + * > + * Copyright (c) 2025 Red Hat, Inc. > + */ > + > +#include "qemu/osdep.h" > +#include "crypto/tlscredsbox.h" > +#include "qemu/atomic.h" > + > + > +static QCryptoTLSCredsBox * > +qcrypto_tls_creds_box_new_impl(int type, bool server) > +{ > + QCryptoTLSCredsBox *credsbox = g_new0(QCryptoTLSCredsBox, 1); > + credsbox->ref = 1; > + credsbox->server = server; > + credsbox->type = type; > + return credsbox; > +} > + > + > +QCryptoTLSCredsBox * > +qcrypto_tls_creds_box_new_server(int type) > +{ > + return qcrypto_tls_creds_box_new_impl(type, true); > +} > + > + > +QCryptoTLSCredsBox * > +qcrypto_tls_creds_box_new_client(int type) > +{ > + return qcrypto_tls_creds_box_new_impl(type, false); > +} > + > +static void qcrypto_tls_creds_box_free(QCryptoTLSCredsBox *credsbox) > +{ > + switch (credsbox->type) { > + case GNUTLS_CRD_CERTIFICATE: > + if (credsbox->data.cert) { > + gnutls_certificate_free_credentials(credsbox->data.cert); > + } > + break; > + case GNUTLS_CRD_PSK: > + if (credsbox->server) { > + if (credsbox->data.pskserver) { > + > gnutls_psk_free_server_credentials(credsbox->data.pskserver); > + } > + } else { > + if (credsbox->data.pskclient) { > + > gnutls_psk_free_client_credentials(credsbox->data.pskclient); > + } > + } > + break; > + case GNUTLS_CRD_ANON: > + if (credsbox->server) { > + if (credsbox->data.anonserver) { > + > gnutls_anon_free_server_credentials(credsbox->data.anonserver); > + } > + } else { > + if (credsbox->data.anonclient) { > + > gnutls_anon_free_client_credentials(credsbox->data.anonclient); > + } > + } > + break; > + default: > + g_assert_not_reached(); > + } > + > + if (credsbox->dh_params) { > + gnutls_dh_params_deinit(credsbox->dh_params); > + } > + > + g_free(credsbox); > +} > + > + > +void qcrypto_tls_creds_box_ref(QCryptoTLSCredsBox *credsbox) > +{ > + uint32_t ref = qatomic_fetch_inc(&credsbox->ref); > + /* Assert waaay before the integer overflows */ > + g_assert(ref < INT_MAX); > +} > + > + > +void qcrypto_tls_creds_box_unref(QCryptoTLSCredsBox *credsbox) > +{ > + if (!credsbox) { > + return; > + } > + > + g_assert(credsbox->ref > 0); > + > + if (qatomic_fetch_dec(&credsbox->ref) == 1) { > + qcrypto_tls_creds_box_free(credsbox); > + } > + > +} > + > diff --git a/crypto/tlscredsbox.h b/crypto/tlscredsbox.h > new file mode 100644 > index 0000000000..5d89335f46 > --- /dev/null > +++ b/crypto/tlscredsbox.h > @@ -0,0 +1,46 @@ > +/* > + * SPDX-License-Identifier: GPL-2.0-or-later > + * > + * QEMU crypto TLS credential support > + * > + * Copyright (c) 2025 Red Hat, Inc. > + */ > + > +#ifndef QCRYPTO_TLSCREDS_BOX_H > +#define QCRYPTO_TLSCREDS_BOX_H > + > +#include "qom/object.h" > + > +#ifdef CONFIG_GNUTLS > +#include <gnutls/gnutls.h> > +#endif > + > +typedef struct QCryptoTLSCredsBox QCryptoTLSCredsBox; > + > +struct QCryptoTLSCredsBox { > + uint32_t ref; > + bool server; > + int type; > + union { > + void *any; > since any is used in code to cast the variant to a void *, it may be worth a comment to say that all fields are expected to be pointers. > +#ifdef CONFIG_GNUTLS > + gnutls_anon_server_credentials_t anonserver; > + gnutls_anon_client_credentials_t anonclient; > + gnutls_psk_server_credentials_t pskserver; > + gnutls_psk_client_credentials_t pskclient; > + gnutls_certificate_credentials_t cert; > +#endif > + } data; > +#ifdef CONFIG_GNUTLS > + gnutls_dh_params_t dh_params; > +#endif > +}; > + > +QCryptoTLSCredsBox *qcrypto_tls_creds_box_new_server(int type); > +QCryptoTLSCredsBox *qcrypto_tls_creds_box_new_client(int type); > +void qcrypto_tls_creds_box_ref(QCryptoTLSCredsBox *credsbox); > +void qcrypto_tls_creds_box_unref(QCryptoTLSCredsBox *credsbox); > + > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSCredsBox, > qcrypto_tls_creds_box_unref); > + > +#endif /* QCRYPTO_TLSCREDS_BOX_H */ > -- > 2.51.1 > >
