[PATCH 0/7] various cleanups
Collection of cleanup patches from my previous series attempting to remove secrets clearing refactored so that it doesn't depend on the patches which were not accepted. Peter Krempa (7): virCryptoEncryptDataAESgnutls: Restructure control flow virStorageBackendISCSISetAuth: Refactor cleanup virStorageBackendISCSISetAuth: Use g_strndup to '\0' terminate data virStorageBackendISCSIDirectSetAuth: Refactor cleanup virStorageBackendISCSIDirectSetAuth: Use 'g_strndup' to '\0' terminate data libxlMakeNetworkDiskSrc: Refactor cleanup storageBackendCreateQemuImgSecretPath: Refactor cleanup src/libxl/libxl_conf.c | 17 ++++------ src/storage/storage_backend_iscsi.c | 17 ++++------ src/storage/storage_backend_iscsi_direct.c | 23 ++++++-------- src/storage/storage_util.c | 36 ++++++++++------------ src/util/vircrypto.c | 28 ++++++++--------- 5 files changed, 49 insertions(+), 72 deletions(-) -- 2.39.1
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.39.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 | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 968a70158b..e4fa49d05f 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" @@ -255,8 +256,8 @@ virStorageBackendISCSISetAuth(const char *portal, size_t secret_size; g_autofree char *secret_str = NULL; virStorageAuthDef *authdef = source->auth; - int ret = -1; - virConnectPtr conn = NULL; + int ret = 0; + g_autoptr(virConnect) conn = NULL; VIR_IDENTITY_AUTORESTORE virIdentity *oldident = NULL; if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) @@ -280,7 +281,7 @@ virStorageBackendISCSISetAuth(const char *portal, if (virSecretGetSecretString(conn, &authdef->seclookupdef, VIR_SECRET_USAGE_TYPE_ISCSI, &secret_value, &secret_size) < 0) - goto cleanup; + return -1; secret_str = g_new0(char, secret_size + 1); memcpy(secret_str, secret_value, secret_size); @@ -299,13 +300,9 @@ virStorageBackendISCSISetAuth(const char *portal, source->devices[0].path, "node.session.auth.password", secret_str) < 0) - goto cleanup; + ret = -1; - ret = 0; - - cleanup: virSecureErase(secret_str, secret_size); - virObjectUnref(conn); return ret; } -- 2.39.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_iscsi.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index e4fa49d05f..01900f6809 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -283,10 +283,8 @@ virStorageBackendISCSISetAuth(const char *portal, &secret_value, &secret_size) < 0) return -1; - secret_str = g_new0(char, secret_size + 1); - memcpy(secret_str, secret_value, secret_size); + secret_str = g_strndup((char *) secret_value, secret_size); virSecureErase(secret_value, secret_size); - secret_str[secret_size] = '\0'; if (virISCSINodeUpdate(portal, source->devices[0].path, -- 2.39.1
On Tue, Jan 31, 2023 at 05:02:15PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_iscsi.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index e4fa49d05f..01900f6809 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -283,10 +283,8 @@ virStorageBackendISCSISetAuth(const char *portal, &secret_value, &secret_size) < 0) return -1;
- secret_str = g_new0(char, secret_size + 1); - memcpy(secret_str, secret_value, secret_size); + secret_str = g_strndup((char *) secret_value, secret_size);
Unfortunately secrets can contain zero bytes in which case this function would pad everything after the first zero byte with more zero bytes. Fortunately (?) the functions that are called below do not take secret_size, so it won't affect this particular code block, but we might have other problems already existing in the code with this.
virSecureErase(secret_value, secret_size); - secret_str[secret_size] = '\0';
if (virISCSINodeUpdate(portal, source->devices[0].path, -- 2.39.1
On Tue, Jan 31, 2023 at 17:35:59 +0100, Martin Kletzander wrote:
On Tue, Jan 31, 2023 at 05:02:15PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_iscsi.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index e4fa49d05f..01900f6809 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -283,10 +283,8 @@ virStorageBackendISCSISetAuth(const char *portal, &secret_value, &secret_size) < 0) return -1;
- secret_str = g_new0(char, secret_size + 1); - memcpy(secret_str, secret_value, secret_size); + secret_str = g_strndup((char *) secret_value, secret_size);
Unfortunately secrets can contain zero bytes in which case this function would pad everything after the first zero byte with more zero bytes.
Fortunately (?) the functions that are called below do not take secret_size, so it won't affect this particular code block, but we might have other problems already existing in the code with this.
Indeed. If the secret itself contains NUL bytes it would indeed not work properly, but that's pre-existing. But with this patch and a NUL byte in a secret we'd actually write beyond the end of the buffer below when cleaning up as the cleanup is done via virSecureErase(secret_str, secret_size); thus attempting to clear more than the string allocated via g_strndup. at this point I think I can simply drop this + the other patch doing the same, as the difference is negligible.
virSecureErase(secret_value, secret_size); - secret_str[secret_size] = '\0';
if (virISCSINodeUpdate(portal, source->devices[0].path, -- 2.39.1
On Tue, Jan 31, 2023 at 05:41:57PM +0100, Peter Krempa wrote:
On Tue, Jan 31, 2023 at 17:35:59 +0100, Martin Kletzander wrote:
On Tue, Jan 31, 2023 at 05:02:15PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_iscsi.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index e4fa49d05f..01900f6809 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -283,10 +283,8 @@ virStorageBackendISCSISetAuth(const char *portal, &secret_value, &secret_size) < 0) return -1;
- secret_str = g_new0(char, secret_size + 1); - memcpy(secret_str, secret_value, secret_size); + secret_str = g_strndup((char *) secret_value, secret_size);
Unfortunately secrets can contain zero bytes in which case this function would pad everything after the first zero byte with more zero bytes.
Fortunately (?) the functions that are called below do not take secret_size, so it won't affect this particular code block, but we might have other problems already existing in the code with this.
Indeed. If the secret itself contains NUL bytes it would indeed not work properly, but that's pre-existing.
But with this patch and a NUL byte in a secret we'd actually write beyond the end of the buffer below when cleaning up as the cleanup is done via
virSecureErase(secret_str, secret_size);
thus attempting to clear more than the string allocated via g_strndup.
no, that's fine, g_strndup will allocate secret_size + 1.
at this point I think I can simply drop this + the other patch doing the same, as the difference is negligible.
virSecureErase(secret_value, secret_size); - secret_str[secret_size] = '\0';
if (virISCSINodeUpdate(portal, source->devices[0].path, -- 2.39.1
On Tue, Jan 31, 2023 at 17:56:09 +0100, Martin Kletzander wrote:
On Tue, Jan 31, 2023 at 05:41:57PM +0100, Peter Krempa wrote:
On Tue, Jan 31, 2023 at 17:35:59 +0100, Martin Kletzander wrote:
On Tue, Jan 31, 2023 at 05:02:15PM +0100, Peter Krempa wrote:
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_iscsi.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index e4fa49d05f..01900f6809 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -283,10 +283,8 @@ virStorageBackendISCSISetAuth(const char *portal, &secret_value, &secret_size) < 0) return -1;
- secret_str = g_new0(char, secret_size + 1); - memcpy(secret_str, secret_value, secret_size); + secret_str = g_strndup((char *) secret_value, secret_size);
Unfortunately secrets can contain zero bytes in which case this function would pad everything after the first zero byte with more zero bytes.
Fortunately (?) the functions that are called below do not take secret_size, so it won't affect this particular code block, but we might have other problems already existing in the code with this.
Indeed. If the secret itself contains NUL bytes it would indeed not work properly, but that's pre-existing.
But with this patch and a NUL byte in a secret we'd actually write beyond the end of the buffer below when cleaning up as the cleanup is done via
virSecureErase(secret_str, secret_size);
thus attempting to clear more than the string allocated via g_strndup.
no, that's fine, g_strndup will allocate secret_size + 1.
Ah right, the documentation says so. Anyways, we can't do anything about non-C-string passwords here because virISCSINodeUpdate actually invokes a command with the password as argument so it must be a C string regardless.
Use automatic pointer for 'conn' and remove the 'cleanup' label and 'ret' variable. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 5df5447066..63225c533e 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -91,8 +91,7 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi, 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) @@ -104,19 +103,19 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi, if (authdef->authType != VIR_STORAGE_AUTH_TYPE_CHAP) { virReportError(VIR_ERR_XML_ERROR, "%s", _("iscsi-direct pool only supports 'chap' auth type")); - return ret; + return -1; } if (!(oldident = virIdentityElevateCurrent())) return -1; if (!(conn = virGetConnectSecret())) - return ret; + return -1; if (virSecretGetSecretString(conn, &authdef->seclookupdef, VIR_SECRET_USAGE_TYPE_ISCSI, &secret_value, &secret_size) < 0) - goto cleanup; + return -1; secret_str = g_new0(char, secret_size + 1); memcpy(secret_str, secret_value, secret_size); @@ -125,17 +124,15 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi, if (iscsi_set_initiator_username_pwd(iscsi, authdef->username, secret_str) < 0) { + virSecureErase(secret_str, secret_size); virReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to set credential: %s"), iscsi_get_error(iscsi)); - goto cleanup; + return -1; } - - ret = 0; - cleanup: virSecureErase(secret_str, secret_size); - virObjectUnref(conn); - return ret; + + return 0; } static int -- 2.39.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_backend_iscsi_direct.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/storage/storage_backend_iscsi_direct.c b/src/storage/storage_backend_iscsi_direct.c index 63225c533e..ca906357c3 100644 --- a/src/storage/storage_backend_iscsi_direct.c +++ b/src/storage/storage_backend_iscsi_direct.c @@ -117,10 +117,8 @@ virStorageBackendISCSIDirectSetAuth(struct iscsi_context *iscsi, &secret_value, &secret_size) < 0) return -1; - secret_str = g_new0(char, secret_size + 1); - memcpy(secret_str, secret_value, secret_size); + secret_str = g_strndup((char *)secret_value, secret_size); virSecureErase(secret_value, secret_size); - secret_str[secret_size] = '\0'; if (iscsi_set_initiator_username_pwd(iscsi, authdef->username, secret_str) < 0) { -- 2.39.1
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 | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 17ac880634..cee9f827f7 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1026,10 +1026,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) { @@ -1038,16 +1037,16 @@ 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); @@ -1058,13 +1057,9 @@ libxlMakeNetworkDiskSrc(virStorageSource *src, char **srcstr) virSecureEraseString(base64secret); if (!*srcstr) - goto cleanup; - - ret = 0; + return -1; - cleanup: - virObjectUnref(conn); - return ret; + return 0; } int -- 2.39.1
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/storage/storage_util.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 520fdd03d0..b57128eb2e 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -1259,10 +1259,10 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObj *pool, virStorageVolDef *vol) { virStorageEncryption *enc = vol->target.encryption; - char *secretPath = NULL; - uint8_t *secret = NULL; + g_autofree char *secretPath = NULL; + g_autofree uint8_t *secret = NULL; size_t secretlen = 0; - virConnectPtr conn = NULL; + g_autoptr(virConnect) conn = NULL; VIR_AUTOCLOSE fd = -1; VIR_IDENTITY_AUTORESTORE virIdentity *oldident = NULL; @@ -1287,24 +1287,29 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObj *pool, return NULL; if (!(secretPath = virStoragePoolObjBuildTempFilePath(pool, vol))) - goto cleanup; + return NULL; if ((fd = g_mkstemp_full(secretPath, O_RDWR | O_CLOEXEC, S_IRUSR | S_IWUSR)) < 0) { virReportSystemError(errno, "%s", _("failed to open secret file for write")); - goto error; + return NULL; } if (virSecretGetSecretString(conn, &enc->secrets[0]->seclookupdef, VIR_SECRET_USAGE_TYPE_VOLUME, - &secret, &secretlen) < 0) - goto error; + &secret, &secretlen) < 0) { + unlink(secretPath); + return NULL; + } if (safewrite(fd, secret, secretlen) < 0) { + virSecureErase(secret, secretlen); virReportSystemError(errno, "%s", _("failed to write secret file")); - goto error; + unlink(secretPath); + return NULL; } + virSecureErase(secret, secretlen); if ((vol->target.perms->uid != (uid_t)-1) && (vol->target.perms->gid != (gid_t)-1)) { @@ -1312,21 +1317,12 @@ storageBackendCreateQemuImgSecretPath(virStoragePoolObj *pool, vol->target.perms->gid) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("failed to chown secret file")); - goto error; + unlink(secretPath); + return NULL; } } - cleanup: - virObjectUnref(conn); - virSecureErase(secret, secretlen); - g_free(secret); - - return secretPath; - - error: - unlink(secretPath); - VIR_FREE(secretPath); - goto cleanup; + return g_steal_pointer(&secretPath); } -- 2.39.1
On Tue, Jan 31, 2023 at 05:00:48PM +0100, Peter Krempa wrote:
Collection of cleanup patches from my previous series attempting to remove secrets clearing refactored so that it doesn't depend on the patches which were not accepted.
Peter Krempa (7): virCryptoEncryptDataAESgnutls: Restructure control flow virStorageBackendISCSISetAuth: Refactor cleanup virStorageBackendISCSISetAuth: Use g_strndup to '\0' terminate data virStorageBackendISCSIDirectSetAuth: Refactor cleanup virStorageBackendISCSIDirectSetAuth: Use 'g_strndup' to '\0' terminate data libxlMakeNetworkDiskSrc: Refactor cleanup storageBackendCreateQemuImgSecretPath: Refactor cleanup
Reviewed-by: Martin Kletzander <mkletzan@redhat.com>
src/libxl/libxl_conf.c | 17 ++++------ src/storage/storage_backend_iscsi.c | 17 ++++------ src/storage/storage_backend_iscsi_direct.c | 23 ++++++-------- src/storage/storage_util.c | 36 ++++++++++------------ src/util/vircrypto.c | 28 ++++++++--------- 5 files changed, 49 insertions(+), 72 deletions(-)
-- 2.39.1
participants (2)
-
Martin Kletzander -
Peter Krempa