On Thu, Feb 10, 2022 at 17:03:01 +0100, Ján Tomko wrote:
> On a Thursday in 2022, Jiri Denemark wrote:
> >If 1024 was not enough to fit the DN, gnutls_x509_crt_get_dn would store
> >the required size in subjectlen. And since we're not checking the return
> >value of this function, we would happily overwrite some random memory.
> >
> >Signed-off-by: Jiri Denemark <jdenemar@xxxxxxxxxx>
> >---
> > src/qemu/qemu_migration_cookie.c | 21 ++++++++++++++-------
> > 1 file changed, 14 insertions(+), 7 deletions(-)
> >
> >diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c
> >index 76a01781d6..046d588d8a 100644
> >--- a/src/qemu/qemu_migration_cookie.c
> >+++ b/src/qemu/qemu_migration_cookie.c
> >@@ -180,12 +180,12 @@ static char *
> > qemuDomainExtractTLSSubject(const char *certdir)
> > {
> > g_autofree char *certfile = NULL;
> >- char *subject = NULL;
> >+ g_autofree char *subject = NULL;
> > g_autofree char *pemdata = NULL;
> > gnutls_datum_t pemdatum;
> > gnutls_x509_crt_t cert;
> > int rc;
> >- size_t subjectlen;
> >+ size_t subjectlen = 0;
> >
> > certfile = g_strdup_printf("%s/server-cert.pem", certdir);
> >
> >@@ -214,13 +214,20 @@ qemuDomainExtractTLSSubject(const char *certdir)
> > return NULL;
> > }
> >
> >- subjectlen = 1024;
> >- subject = g_new0(char, subjectlen + 1);
> >-
> >- gnutls_x509_crt_get_dn(cert, subject, &subjectlen);
> >+ rc = gnutls_x509_crt_get_dn(cert, NULL, &subjectlen);
>
> Leaving the pre-allocation here would be more consistent with our
> other use of this function (where the original buffer is only 256
> bytes long)
>
> Internally, the function formats the answer into its own buffer
> to figure out its size, so it would also serve as a micro-optimization.
Yeah, makes sense.
>
> >+ if (rc == GNUTLS_E_SHORT_MEMORY_BUFFER) {
> >+ subject = g_new0(char, subjectlen + 1);
> >+ rc = gnutls_x509_crt_get_dn(cert, subject, &subjectlen);
> >+ }
> >+ if (rc != 0) {
> >+ virReportError(VIR_ERR_INTERNAL_ERROR,
> >+ _("cannot get cert distinguished name: %s"),
>
> Please copy the translatable error string from either of the two
> other uses of gnutls_x509_crt_get_dn, to save the translators some work.
Unfortunately, I don't think any of the existing messages would make
sense here.
Jirka
