[libvirt PATCH 2/2] qemu_migration_cookie: Properly fetch cert DN

[Jiri Denemark]

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 at redhat.com>
> >---
> > 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