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

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Oct 31, 2025 at 03:23:51PM +0400, Marc-André Lureau wrote:
> 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>


> > +#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.

Yep, in practice almost all gnutls _t types are pointers, but
I'll explain that here since it is not obvious to casual
observers.

> 
> 
> > +#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
> > +};
> > +

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|