10:28 a.m.
In certain cases we tried to clear stuff which isn't secure and in other
cases we clear the pointer but then pass the secret on the commandline.
Remove the security theatre.
Additionally all other instances which pass secret via RPC can be
theoreticlaly removed as the secret is copied to/from and non-sanitized
RPC buffer.
We'd have to clear all RPC buffers though for this to be "properly"
handled and not just security theater.
Peter Krempa (11):
virCryptoEncryptDataAESgnutls: Don't secure erase gnutls_datum_t
structs
virCryptoEncryptDataAESgnutls: Properly initialize data structures
virCryptoEncryptDataAESgnutls: Restructure control flow
virStorageBackendISCSISetAuth: Don't bother securely erasing password
virStorageBackendISCSISetAuth: Use g_strndup to '\0' terminate data
virStorageBackendISCSISetAuth: Refactor cleanup
libxlMakeNetworkDiskSrc: Don't bother with secure erase of secrets
libxlMakeNetworkDiskSrc: Refactor cleanup
virStorageBackendRBDOpenRADOSConn: Don't log the RBD key
datatypes: Register autoptr cleanup for virSecret
virSecretGetSecretString: Refactor cleanup
src/datatypes.h | 1 +
src/libxl/libxl_conf.c | 24 +++++-----------
src/storage/storage_backend_iscsi.c | 22 +++++----------
src/storage/storage_backend_rbd.c | 24 ++++++++++++----
src/util/vircrypto.c | 43 ++++++++++-------------------
src/util/virsecret.c | 19 ++++---------
6 files changed, 53 insertions(+), 80 deletions(-)
--
2.38.1
10:28 a.m.
New subject: [PATCH 01/11] virCryptoEncryptDataAESgnutls: Don't secure erase gnutls_datum_t structs
'gnutls_datum_t' simply holds pointers to the encryption key and it's
length. There's absolutely no point in securely erasing that.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/vircrypto.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 828e822d8e..1bddb333dc 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -164,8 +164,6 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t
gnutls_enc_alg,
/* Encrypt the data and free the memory for cipher operations */
rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen);
gnutls_cipher_deinit(handle);
- virSecureErase(&enc_key, sizeof(gnutls_datum_t));
- virSecureErase(&iv_buf, sizeof(gnutls_datum_t));
if (rc < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to encrypt the data: '%s'"),
@@ -180,8 +178,6 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t
gnutls_enc_alg,
error:
virSecureErase(ciphertext, ciphertextlen);
g_free(ciphertext);
- virSecureErase(&enc_key, sizeof(gnutls_datum_t));
- virSecureErase(&iv_buf, sizeof(gnutls_datum_t));
return -1;
}
--
2.38.1
6:05 a.m.
New subject: [PATCH 01/11] virCryptoEncryptDataAESgnutls: Don't secure erase gnutls_datum_t structs
On a Friday in 2022, Peter Krempa wrote:
'gnutls_datum_t' simply holds pointers to the encryption key
and it's
*its
length. There's absolutely no point in securely erasing that.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/vircrypto.c | 4 ----
1 file changed, 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:28 a.m.
New subject: [PATCH 02/11] virCryptoEncryptDataAESgnutls: Properly initialize data structures
The initialization vector is not optional thus we also don't need to
check whether the caller passed it in. Additionally we can use c99
initializers for the gnutls_datum_t structs.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/vircrypto.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index 1bddb333dc..b28d3fc23d 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -125,8 +125,8 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t
gnutls_enc_alg,
int rc;
size_t i;
gnutls_cipher_hd_t handle = NULL;
- gnutls_datum_t enc_key;
- gnutls_datum_t iv_buf;
+ gnutls_datum_t enc_key = { .data = enckey, .size = enckeylen };
+ gnutls_datum_t iv_buf = { .data = iv, .size = ivlen };
uint8_t *ciphertext;
size_t ciphertextlen;
@@ -146,13 +146,6 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t
gnutls_enc_alg,
for (i = datalen; i < ciphertextlen; i++)
ciphertext[i] = ciphertextlen - datalen;
- /* Initialize the gnutls cipher */
- enc_key.size = enckeylen;
- enc_key.data = enckey;
- if (iv) {
- iv_buf.size = ivlen;
- iv_buf.data = iv;
- }
if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg,
&enc_key, &iv_buf)) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
--
2.38.1
6:06 a.m.
New subject: [PATCH 02/11] virCryptoEncryptDataAESgnutls: Properly initialize data structures
On a Friday in 2022, Peter Krempa wrote:
The initialization vector is not optional thus we also don't need
to
check whether the caller passed it in. Additionally we can use c99
initializers for the gnutls_datum_t structs.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/vircrypto.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:28 a.m.
New subject: [PATCH 03/11] virCryptoEncryptDataAESgnutls: Restructure control flow
Prepare the buffer for encryption only after initializing the cipher, so
that there's just one failure point. This allows to remove the 'error'
label.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/vircrypto.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
index b28d3fc23d..12d051a55a 100644
--- a/src/util/vircrypto.c
+++ b/src/util/vircrypto.c
@@ -127,9 +127,17 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t
gnutls_enc_alg,
gnutls_cipher_hd_t handle = NULL;
gnutls_datum_t enc_key = { .data = enckey, .size = enckeylen };
gnutls_datum_t iv_buf = { .data = iv, .size = ivlen };
- uint8_t *ciphertext;
+ g_autofree uint8_t *ciphertext = NULL;
size_t ciphertextlen;
+ if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg,
+ &enc_key, &iv_buf)) < 0) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("failed to initialize cipher: '%s'"),
+ gnutls_strerror(rc));
+ return -1;
+ }
+
/* Allocate a padded buffer, copy in the data.
*
* NB, we must *always* have at least 1 byte of
@@ -146,32 +154,20 @@ virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t
gnutls_enc_alg,
for (i = datalen; i < ciphertextlen; i++)
ciphertext[i] = ciphertextlen - datalen;
- if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg,
- &enc_key, &iv_buf)) < 0) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("failed to initialize cipher: '%s'"),
- gnutls_strerror(rc));
- goto error;
- }
-
/* Encrypt the data and free the memory for cipher operations */
rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen);
gnutls_cipher_deinit(handle);
if (rc < 0) {
+ virSecureErase(ciphertext, ciphertextlen);
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to encrypt the data: '%s'"),
gnutls_strerror(rc));
- goto error;
+ return -1;
}
- *ciphertextret = ciphertext;
+ *ciphertextret = g_steal_pointer(&ciphertext);
*ciphertextlenret = ciphertextlen;
return 0;
-
- error:
- virSecureErase(ciphertext, ciphertextlen);
- g_free(ciphertext);
- return -1;
}
--
2.38.1
10:28 a.m.
New subject: [PATCH 04/11] virStorageBackendISCSISetAuth: Don't bother securely erasing password
We fetch the password via RPC so it's already contained in an
un-sanitized buffer and pass it to 'iscsiadm' via virCommand where it's
in another un-sanitized buffer (and on the commandline!!).
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 968a70158b..78c86e6359 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -38,7 +38,6 @@
#include "virsecret.h"
#include "storage_util.h"
#include "virutil.h"
-#include "virsecureerase.h"
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -284,7 +283,6 @@ virStorageBackendISCSISetAuth(const char *portal,
secret_str = g_new0(char, secret_size + 1);
memcpy(secret_str, secret_value, secret_size);
- virSecureErase(secret_value, secret_size);
secret_str[secret_size] = '\0';
if (virISCSINodeUpdate(portal,
@@ -304,7 +302,6 @@ virStorageBackendISCSISetAuth(const char *portal,
ret = 0;
cleanup:
- virSecureErase(secret_str, secret_size);
virObjectUnref(conn);
return ret;
}
--
2.38.1
3:10 a.m.
New subject: [PATCH 04/11] virStorageBackendISCSISetAuth: Don't bother securely erasing password
On Fri, Dec 09, 2022 at 05:28:56PM +0100, Peter Krempa wrote:
We fetch the password via RPC so it's already contained in an
un-sanitized buffer and pass it to 'iscsiadm' via virCommand where it's
in another un-sanitized buffer (and on the commandline!!).
Just because there are other places in the code which are not
perfect, doesn't mean we should delete this.
Note, if iscsiadm really forces us to pass secrets on the CLI, that
is a significant flaw in its design, that really needs to be reported
as a security bug against iscsiadm IMHO. They need to provide a secure
channel to receiving passwords.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
10:28 a.m.
New subject: [PATCH 05/11] virStorageBackendISCSISetAuth: Use g_strndup to '\0' terminate data
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 78c86e6359..9f9aa01f05 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -281,9 +281,8 @@ virStorageBackendISCSISetAuth(const char *portal,
&secret_value, &secret_size) < 0)
goto cleanup;
- secret_str = g_new0(char, secret_size + 1);
- memcpy(secret_str, secret_value, secret_size);
- secret_str[secret_size] = '\0';
+ /* '\0' terminate the data into a string */
+ secret_str = g_strndup((char *) secret_value, secret_size);
if (virISCSINodeUpdate(portal,
source->devices[0].path,
--
2.38.1
10:28 a.m.
New subject: [PATCH 06/11] virStorageBackendISCSISetAuth: Refactor cleanup
Automatically free 'conn' and remove the 'cleanup' section and
'ret'
variable. 'datatypes.h' contains the declaration of the autoptr cleanup
function for virConnect.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_iscsi.c | 14 +++++---------
1 file changed, 5 insertions(+), 9 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c
index 9f9aa01f05..c5e3130a4f 100644
--- a/src/storage/storage_backend_iscsi.c
+++ b/src/storage/storage_backend_iscsi.c
@@ -26,6 +26,7 @@
#include <unistd.h>
#include <sys/stat.h>
+#include "datatypes.h"
#include "driver.h"
#include "storage_backend_iscsi.h"
#include "viralloc.h"
@@ -254,8 +255,7 @@ virStorageBackendISCSISetAuth(const char *portal,
size_t secret_size;
g_autofree char *secret_str = NULL;
virStorageAuthDef *authdef = source->auth;
- int ret = -1;
- virConnectPtr conn = NULL;
+ g_autoptr(virConnect) conn = NULL;
VIR_IDENTITY_AUTORESTORE virIdentity *oldident = NULL;
if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE)
@@ -279,7 +279,7 @@ virStorageBackendISCSISetAuth(const char *portal,
if (virSecretGetSecretString(conn, &authdef->seclookupdef,
VIR_SECRET_USAGE_TYPE_ISCSI,
&secret_value, &secret_size) < 0)
- goto cleanup;
+ return -1;
/* '\0' terminate the data into a string */
secret_str = g_strndup((char *) secret_value, secret_size);
@@ -296,13 +296,9 @@ virStorageBackendISCSISetAuth(const char *portal,
source->devices[0].path,
"node.session.auth.password",
secret_str) < 0)
- goto cleanup;
-
- ret = 0;
+ return -1;
- cleanup:
- virObjectUnref(conn);
- return ret;
+ return 0;
}
static int
--
2.38.1
10:28 a.m.
New subject: [PATCH 07/11] libxlMakeNetworkDiskSrc: Don't bother with secure erase of secrets
The contents of both 'secret' and 'base64secret' are part of different
buffers wich are not erased securely. Don't bother with virSecureErase*.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/libxl/libxl_conf.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index d13e48abb2..54e50a24cf 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -45,7 +45,6 @@
#include "xen_xl.h"
#include "virnetdevvportprofile.h"
#include "virenum.h"
-#include "virsecureerase.h"
#define VIR_FROM_THIS VIR_FROM_LIBXL
@@ -1047,11 +1046,9 @@ libxlMakeNetworkDiskSrc(virStorageSource *src, char **srcstr)
/* RBD expects an encoded secret */
base64secret = g_base64_encode(secret, secretlen);
- virSecureErase(secret, secretlen);
}
*srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret);
- virSecureEraseString(base64secret);
if (!*srcstr)
goto cleanup;
--
2.38.1
3:12 a.m.
New subject: [PATCH 07/11] libxlMakeNetworkDiskSrc: Don't bother with secure erase of secrets
On Fri, Dec 09, 2022 at 05:28:59PM +0100, Peter Krempa wrote:
The contents of both 'secret' and 'base64secret' are
part of different
buffers wich are not erased securely. Don't bother with virSecureErase*.
Again, just because other code isn't right, doesn't mean we should
delete this. Make the other buffer be erased securely instead.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
8:37 a.m.
New subject: [PATCH 07/11] libxlMakeNetworkDiskSrc: Don't bother with secure erase of secrets
On Mon, Dec 12, 2022 at 09:12:15 +0000, Daniel P. Berrangé wrote:
On Fri, Dec 09, 2022 at 05:28:59PM +0100, Peter Krempa wrote:
> The contents of both 'secret' and 'base64secret' are part of
different
> buffers wich are not erased securely. Don't bother with virSecureErase*.
Again, just because other code isn't right, doesn't mean we should
delete this. Make the other buffer be erased securely instead.
In many cases such as this one the secret is formatted into a buffer
that may be realloc'd while it's being constructed while the secret is
inside.
Assume the following program, which "decrypts" a secret and writes it to
a file. The buffer used for the operation is reallocd() and cleared:
$ cat secret.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <unistd.h>
#include <fcntl.h>
int main(int argc, const char *argv[])
{
char *buf;
const char *fragment1 = "STCSRNM";
const char *fragment2 = "EEATOOY";
char *buf2;
size_t i;
int b = atoi(argv[1]);
int fd = open("/dev/null", O_WRONLY);
char *r1;
char *r2;
buf = malloc(b);
buf2 = malloc(256);
memcpy(buf2, "somethingelse", strlen("somethingelse"));
for (i = 0; i < b; i++) {
if ((i % 2) == 0) {
buf[i] = fragment1[(i/2)%14];
} else {
buf[i] = fragment2[(i/2)%14];
}
}
write(fd, buf, b);
close(fd);
r1 = buf;
if (b != 1024)
buf = realloc(buf, 1024);
r2 = buf;
if (b != 666) {
for (i = 0; i < b; i++)
memcpy(buf + i, "X", 2);
}
memcpy(buf2, buf, 100);
getchar();
printf("r1: %p, r2: %p\n", r1, r2);
puts(buf);
puts(buf2);
}
Now see the following cases:
When I run it with a buffer > 1024 or the magic value 1024 when it's not
reallocated the result is:
$ ./a.out 1200 & sleep 1; gcore $! ; echo corestrings ; strings core.$! | grep SETE ;
rm core* ; fg
[1] 2832373
[1]+ Stopped ./a.out 1200
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Program received signal SIGTTIN, Stopped (tty input).
0x00007f6265553021 in read () from /lib64/libc.so.6
warning: target file /proc/2832373/cmdline contained unexpected null characters
warning: Memory read failed for corefile section, 4096 bytes at 0xffffffffff600000.
Saved corefile core.2832373
[Inferior 1 (process 2832373) detached]
[1]+ Stopped ./a.out 1200
corestrings
./a.out 1200
r1: 0x1a682a0, r2: 0x1a682a0
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
... but now if I run it with a smaller initial size:
$ ./a.out 150 & sleep 1; gcore $! ; echo corestrings ; strings core.$! | grep SETE ;
rm core* ; fg
[1] 2833245
[1]+ Stopped ./a.out 150
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib64/libthread_db.so.1".
Program received signal SIGTTIN, Stopped (tty input).
0x00007f8c366cd021 in read () from /lib64/libc.so.6
warning: target file /proc/2833245/cmdline contained unexpected null characters
warning: Memory read failed for corefile section, 4096 bytes at 0xffffffffff600000.
Saved corefile core.2833245
[Inferior 1 (process 2833245) detached]
[1]+ Stopped ./a.out 150
corestrings
jRE/EdAeTvO/OnSETECASTRONOMY
E/EdAeTvO/OnSETECASTRONOMY
E/EdAeTvO/OnSETECASTRONOMY
E/EdAeTvO/OnSETECASTRONOMY
E/EdAeTvO/OnSETECASTRO
E/EdSETECASTRONOMY
O/OnSETECASTRONO
E/EdSETECASTRONOMY
O/OnSETECASTRONO
E/EdSETECASTRONOMY
O/OnSETECASTRONO
./a.out 150
r1: 0xdbf2a0, r2: 0xdbf450
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
Thus ... for buffers where realloc is in use, the attempt to clear it
afterwards is basically equivalent in not clearing the buffer before
freeing it.
10 a.m.
New subject: [PATCH 07/11] libxlMakeNetworkDiskSrc: Don't bother with secure erase of secrets
On Mon, Dec 12, 2022 at 03:37:08PM +0100, Peter Krempa wrote:
On Mon, Dec 12, 2022 at 09:12:15 +0000, Daniel P. Berrangé wrote:
> On Fri, Dec 09, 2022 at 05:28:59PM +0100, Peter Krempa wrote:
> > The contents of both 'secret' and 'base64secret' are part of
different
> > buffers wich are not erased securely. Don't bother with virSecureErase*.
>
> Again, just because other code isn't right, doesn't mean we should
> delete this. Make the other buffer be erased securely instead.
In many cases such as this one the secret is formatted into a buffer
that may be realloc'd while it's being constructed while the secret is
inside.
snip
Thus ... for buffers where realloc is in use, the attempt to clear
it
afterwards is basically equivalent in not clearing the buffer before
freeing it.
That is very true, but it would be possible to address this limitation
by avoiding use of realloc in targetted places such as this. For example,
we could initialize the virBuffer with g_string_sized_new(1024) to
preallocate 1024 bytes, which would make realloc pretty unlikely to
happen in any normal config.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
10:29 a.m.
New subject: [PATCH 08/11] libxlMakeNetworkDiskSrc: Refactor cleanup
Automatically unref the 'conn' object and remove the 'cleanup' section
and 'ret' variable.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/libxl/libxl_conf.c | 21 +++++++--------------
1 file changed, 7 insertions(+), 14 deletions(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index 54e50a24cf..9f0d5717c7 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -1021,10 +1021,9 @@ libxlMakeNetworkDiskSrcStr(virStorageSource *src,
static int
libxlMakeNetworkDiskSrc(virStorageSource *src, char **srcstr)
{
- virConnectPtr conn = NULL;
+ g_autoptr(virConnect) conn = NULL;
g_autofree char *base64secret = NULL;
char *username = NULL;
- int ret = -1;
*srcstr = NULL;
if (src->auth && src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
@@ -1033,31 +1032,25 @@ libxlMakeNetworkDiskSrc(virStorageSource *src, char **srcstr)
VIR_IDENTITY_AUTORESTORE virIdentity *oldident = virIdentityElevateCurrent();
if (!oldident)
- goto cleanup;
+ return -1;
username = src->auth->username;
if (!(conn = virConnectOpen("xen:///system")))
- goto cleanup;
+ return -1;
if (virSecretGetSecretString(conn, &src->auth->seclookupdef,
VIR_SECRET_USAGE_TYPE_CEPH,
&secret, &secretlen) < 0)
- goto cleanup;
+ return -1;
/* RBD expects an encoded secret */
base64secret = g_base64_encode(secret, secretlen);
}
- *srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret);
-
- if (!*srcstr)
- goto cleanup;
-
- ret = 0;
+ if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret)))
+ return -1;
- cleanup:
- virObjectUnref(conn);
- return ret;
+ return 0;
}
int
--
2.38.1
10:29 a.m.
New subject: [PATCH 09/11] virStorageBackendRBDOpenRADOSConn: Don't log the RBD key
'virStorageBackendRBDRADOSConfSet' logs it's arguments but it's also
used to set the RBD secret/key.
All the security theatre with securely erasing the string we do to fetch
the secret would be quite pointless if we log it thus introduce
virStorageBackendRBDRADOSConfSetQuiet and use it to avoid logging the
password.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_rbd.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c
index 52407f8e6f..05b2c43f79 100644
--- a/src/storage/storage_backend_rbd.c
+++ b/src/storage/storage_backend_rbd.c
@@ -161,12 +161,10 @@ virStoragePoolDefRBDNamespaceFormatXML(virBuffer *buf,
static int
-virStorageBackendRBDRADOSConfSet(rados_t cluster,
- const char *option,
- const char *value)
+virStorageBackendRBDRADOSConfSetQuiet(rados_t cluster,
+ const char *option,
+ const char *value)
{
- VIR_DEBUG("Setting RADOS option '%s' to '%s'",
- option, value);
if (rados_conf_set(cluster, option, value) < 0) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("failed to set RADOS option: %s"),
@@ -177,6 +175,19 @@ virStorageBackendRBDRADOSConfSet(rados_t cluster,
return 0;
}
+
+static int
+virStorageBackendRBDRADOSConfSet(rados_t cluster,
+ const char *option,
+ const char *value)
+{
+ VIR_DEBUG("Setting RADOS option '%s' to '%s'",
+ option, value);
+
+ return virStorageBackendRBDRADOSConfSetQuiet(cluster, option, value);
+}
+
+
static int
virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDState *ptr,
virStoragePoolDef *def)
@@ -222,7 +233,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDState *ptr,
rados_key = g_base64_encode(secret_value, secret_value_size);
virSecureErase(secret_value, secret_value_size);
- rc = virStorageBackendRBDRADOSConfSet(ptr->cluster, "key",
rados_key);
+ VIR_DEBUG("Setting RADOS option 'key'");
+ rc = virStorageBackendRBDRADOSConfSetQuiet(ptr->cluster, "key",
rados_key);
virSecureEraseString(rados_key);
if (rc < 0)
--
2.38.1
6:10 a.m.
New subject: [PATCH 09/11] virStorageBackendRBDOpenRADOSConn: Don't log the RBD key
On a Friday in 2022, Peter Krempa wrote:
'virStorageBackendRBDRADOSConfSet' logs it's arguments but
it's also
*its
used to set the RBD secret/key.
All the security theatre with securely erasing the string we do to fetch
the secret would be quite pointless if we log it thus introduce
virStorageBackendRBDRADOSConfSetQuiet and use it to avoid logging the
password.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/storage/storage_backend_rbd.c | 24 ++++++++++++++++++------
1 file changed, 18 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:29 a.m.
New subject: [PATCH 10/11] datatypes: Register autoptr cleanup for virSecret
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/datatypes.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/datatypes.h b/src/datatypes.h
index 49cd9cd42c..0f9730d9e8 100644
--- a/src/datatypes.h
+++ b/src/datatypes.h
@@ -720,6 +720,7 @@ struct _virSecret {
char *usageID; /* the usage's unique identifier */
};
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(virSecret, virObjectUnref);
typedef int (*virStreamAbortFunc)(virStreamPtr, void *opaque);
typedef int (*virStreamFinishFunc)(virStreamPtr, void *opaque);
--
2.38.1
6:10 a.m.
New subject: [PATCH 10/11] datatypes: Register autoptr cleanup for virSecret
On a Friday in 2022, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/datatypes.h | 1 +
1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
10:29 a.m.
New subject: [PATCH 11/11] virSecretGetSecretString: Refactor cleanup
Automatically free 'sec' and remove the 'cleanup' section and
'ret'
variables.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virsecret.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/src/util/virsecret.c b/src/util/virsecret.c
index c01f3fb967..f2c13e27c9 100644
--- a/src/util/virsecret.c
+++ b/src/util/virsecret.c
@@ -139,8 +139,7 @@ virSecretGetSecretString(virConnectPtr conn,
uint8_t **secret,
size_t *secret_size)
{
- virSecretPtr sec = NULL;
- int ret = -1;
+ g_autoptr(virSecret) sec = NULL;
switch (seclookupdef->type) {
case VIR_SECRET_LOOKUP_TYPE_UUID:
@@ -154,7 +153,7 @@ virSecretGetSecretString(virConnectPtr conn,
}
if (!sec)
- goto cleanup;
+ return -1;
/* NB: NONE is a byproduct of the qemuxml2argvtest test mocking
* for UUID lookups. Normal secret XML processing would fail if
@@ -170,17 +169,11 @@ virSecretGetSecretString(virConnectPtr conn,
"expected '%s' type"),
uuidstr, virSecretUsageTypeToString(sec->usageType),
virSecretUsageTypeToString(secretUsageType));
- goto cleanup;
+ return -1;
}
- *secret = conn->secretDriver->secretGetValue(sec, secret_size, 0);
-
- if (!*secret)
- goto cleanup;
-
- ret = 0;
+ if (!(*secret = conn->secretDriver->secretGetValue(sec, secret_size, 0)))
+ return -1;
- cleanup:
- virObjectUnref(sec);
- return ret;
+ return 0;
}
--
2.38.1
6:10 a.m.
New subject: [PATCH 11/11] virSecretGetSecretString: Refactor cleanup
On a Friday in 2022, Peter Krempa wrote:
Automatically free 'sec' and remove the 'cleanup'
section and 'ret'
variables.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/util/virsecret.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
710
days inactive
713
days old
20 comments
3 participants
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Peter Krempa