[PATCH 00/11] Remove secure clearing of memory where it doesn't make sense
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
'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@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
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@redhat.com> --- src/util/vircrypto.c | 4 ---- 1 file changed, 4 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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
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@redhat.com> --- src/util/vircrypto.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
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@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
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@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
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 :|
Signed-off-by: Peter Krempa <pkrempa@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
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@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
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@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
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 :|
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.
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 :|
Automatically unref the 'conn' object and remove the 'cleanup' section and 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@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
'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@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
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@redhat.com> --- src/storage/storage_backend_rbd.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Signed-off-by: Peter Krempa <pkrempa@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
On a Friday in 2022, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/datatypes.h | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
Automatically free 'sec' and remove the 'cleanup' section and 'ret' variables. Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com> --- src/util/virsecret.c | 19 ++++++------------- 1 file changed, 6 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (3)
-
Daniel P. Berrangé -
Ján Tomko -
Peter Krempa