9:19 a.m.
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
9:19 a.m.
New subject: [libvirt PATCH 1/2] qemu_migration_cookie: Rename ret in qemuDomainExtractTLSSubject
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(a)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
9:55 a.m.
New subject: [libvirt PATCH 1/2] qemu_migration_cookie: Rename ret in qemuDomainExtractTLSSubject
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(a)redhat.com>
---
src/qemu/qemu_migration_cookie.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
9:19 a.m.
New subject: [libvirt PATCH 2/2] qemu_migration_cookie: Properly fetch cert DN
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(a)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
10:03 a.m.
New subject: [libvirt PATCH 2/2] qemu_migration_cookie: Properly fetch cert DN
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(a)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(a)redhat.com>
Jano
+ gnutls_strerror(rc));
+ return NULL;
+ }
subject[subjectlen] = '\0';
- return subject;
+ return g_steal_pointer(&subject);
}
--
2.35.1
6:27 a.m.
New subject: [libvirt PATCH 2/2] qemu_migration_cookie: Properly fetch cert DN
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(a)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
1015
days inactive
1016
days old
5 comments
2 participants
participants (2)
-
Jiri Denemark -
Ján Tomko