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

[Ján Tomko]

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.

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

Reviewed-by: Ján Tomko <jtomko at redhat.com>

Jano

>+                       gnutls_strerror(rc));
>+        return NULL;
>+    }
>     subject[subjectlen] = '\0';
>
>-    return subject;
>+    return g_steal_pointer(&subject);
> }
>
>
>-- 
>2.35.1
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220210/2f8e6226/attachment-0001.sig>