[libvirt PATCH 0/2] qemu_migration_cookie: Properly fetch cert DN
Jiri Denemark (2): qemu_migration_cookie: Rename ret in qemuDomainExtractTLSSubject qemu_migration_cookie: Properly fetch cert DN src/qemu/qemu_migration_cookie.c | 35 +++++++++++++++++++------------- 1 file changed, 21 insertions(+), 14 deletions(-) -- 2.35.1
We use 'ret' for storing values to be returned from a function. Return values from called functions that are not supposed to be returned further are usually called 'rv' (or 'rc'). Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration_cookie.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_migration_cookie.c b/src/qemu/qemu_migration_cookie.c index ba05a5a07f..76a01781d6 100644 --- a/src/qemu/qemu_migration_cookie.c +++ b/src/qemu/qemu_migration_cookie.c @@ -184,7 +184,7 @@ qemuDomainExtractTLSSubject(const char *certdir) g_autofree char *pemdata = NULL; gnutls_datum_t pemdatum; gnutls_x509_crt_t cert; - int ret; + int rc; size_t subjectlen; certfile = g_strdup_printf("%s/server-cert.pem", certdir); @@ -195,22 +195,22 @@ qemuDomainExtractTLSSubject(const char *certdir) return NULL; } - ret = gnutls_x509_crt_init(&cert); - if (ret < 0) { + rc = gnutls_x509_crt_init(&cert); + if (rc < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot initialize cert object: %s"), - gnutls_strerror(ret)); + gnutls_strerror(rc)); return NULL; } pemdatum.data = (unsigned char *)pemdata; pemdatum.size = strlen(pemdata); - ret = gnutls_x509_crt_import(cert, &pemdatum, GNUTLS_X509_FMT_PEM); - if (ret < 0) { + rc = gnutls_x509_crt_import(cert, &pemdatum, GNUTLS_X509_FMT_PEM); + if (rc < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot load cert data from %s: %s"), - certfile, gnutls_strerror(ret)); + certfile, gnutls_strerror(rc)); return NULL; } -- 2.35.1
On a Thursday in 2022, Jiri Denemark wrote:
We use 'ret' for storing values to be returned from a function. Return values from called functions that are not supposed to be returned further are usually called 'rv' (or 'rc').
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/qemu/qemu_migration_cookie.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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); + 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"), + gnutls_strerror(rc)); + return NULL; + } subject[subjectlen] = '\0'; - return subject; + return g_steal_pointer(&subject); } -- 2.35.1
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@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@redhat.com> Jano
+ gnutls_strerror(rc)); + return NULL; + } subject[subjectlen] = '\0';
- return subject; + return g_steal_pointer(&subject); }
-- 2.35.1
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@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
participants (2)
-
Jiri Denemark -
Ján Tomko