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é <[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]>
> > +#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 :|
