Re: [PATCH 13/21] crypto: introduce a wrapper around gnutls credentials

["Marc-André Lureau" <marcandre.lureau@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 7013 bytes --]

Hi

On Thu, Oct 30, 2025 at 6:49 PM Daniel P. Berrangé <berrange@redhat.com>
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 <changzihao1@huawei.com>
>   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é <berrange@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


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

[-- Attachment #2: Type: text/html, Size: 8735 bytes --]