[libvirt] [PATCH 0/6] Cleanup of handling of secrets for disks
I also wanted to get rid of yet another copy of virSecretGetSecretString in the RBD driver code but I didn't figure out the linking yet, so I'll send the patch separately. John Ferlan (2): util: string: Introduce helper to determine whether a byte buffer is printable secret: Alter virSecretGetSecretString Peter Krempa (4): util: alloc: Introduce freeing helpers that clear the memory before freeing secret: util: Refactor virSecretGetSecretString util: string: Introduce virStringEncodeBase64 qemu: domain: Fix names for functions that clear security info po/POTFILES.in | 1 - src/conf/virsecretobj.c | 7 +--- src/libvirt_private.syms | 3 ++ src/libxl/libxl_conf.c | 23 +++++++----- src/qemu/qemu_agent.c | 6 +-- src/qemu/qemu_command.c | 18 +++++++-- src/qemu/qemu_domain.c | 27 +++++-------- src/qemu/qemu_domain.h | 3 +- src/secret/secret_util.c | 79 +++++++++++---------------------------- src/secret/secret_util.h | 12 +++--- src/storage/storage_backend_rbd.c | 17 ++------- src/util/viralloc.c | 36 ++++++++++++++++++ src/util/viralloc.h | 56 +++++++++++++++++++++++++++ src/util/virstring.c | 43 +++++++++++++++++++++ src/util/virstring.h | 3 ++ tests/viralloctest.c | 37 ++++++++++++++++++ tools/virsh-secret.c | 6 +-- 17 files changed, 256 insertions(+), 121 deletions(-) -- 2.8.2
For a few cases where we handle secret information it's good to clear the buffers containing sensitive data before freeing them. Introduce VIR_DISPOSE, VIR_DISPOSE_N and VIR_DISPOSE_STRING that allow simple clearing fo the buffers holding sensitive information on cleanup paths. --- src/libvirt_private.syms | 1 + src/util/viralloc.c | 36 +++++++++++++++++++++++++++++++ src/util/viralloc.h | 56 ++++++++++++++++++++++++++++++++++++++++++++++++ tests/viralloctest.c | 37 ++++++++++++++++++++++++++++++++ 4 files changed, 130 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a980a32..9c1abbb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1127,6 +1127,7 @@ virAllocTestInit; virAllocTestOOM; virAllocVar; virDeleteElementsN; +virDispose; virExpandN; virFree; virInsertElementsN; diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 63f43d0..812aa5b 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -583,3 +583,39 @@ void virFree(void *ptrptr) *(void**)ptrptr = NULL; errno = save_errno; } + + +/** + * virDispose: + * @ptrptr: pointer to pointer for address of memory to be sanitized and freed + * @count: count of elements in the array to dispose + * @elemet_size: size of one element + * @countptr: pointer to the count variable to clear (may be NULL) + * + * Clear and release the chunk of memory in the pointer pointed to by 'prtptr'. + * + * If @countptr is provided, it's value is used instead of @count and it's set + * to 0 after clearing and freeing the memory. + * + * After release, 'ptrptr' will be updated to point to NULL. + */ +void virDispose(void *ptrptr, + size_t count, + size_t element_size, + size_t *countptr) +{ + int save_errno = errno; + + if (countptr) + count = *countptr; + + if (*(void**)ptrptr && count > 0) + memset(*(void **)ptrptr, 0, count * element_size); + + free(*(void**)ptrptr); + *(void**)ptrptr = NULL; + + if (countptr) + *countptr = 0; + errno = save_errno; +} diff --git a/src/util/viralloc.h b/src/util/viralloc.h index bf85c16..5f4e27b 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -78,6 +78,9 @@ int virAllocVar(void *ptrptr, size_t struct_size, size_t element_size, size_t co ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); +void virDispose(void *ptrptr, size_t count, size_t element_size, size_t *countptr) + ATTRIBUTE_NONNULL(1); + /** * VIR_ALLOC: * @ptr: pointer to hold address of allocated memory @@ -561,6 +564,59 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); # define VIR_FREE(ptr) virFree(&(ptr)) # endif + +/** + * VIR_DISPOSE_N: + * @ptr: pointer holding address to be cleared and freed + * @count: count of elements in @ptr + * + * Clear the memory of the array of elemets pointed to by 'ptr' of 'count' + * elements and free it. Update the pointer/count to NULL/0. + * + * This macro is safe to use on arguments with side effects. + */ +# if !STATIC_ANALYSIS +/* See explanation in VIR_FREE */ +# define VIR_DISPOSE_N(ptr, count) virDispose(1 ? (void *) &(ptr) : (ptr), 0, \ + sizeof(*(ptr)), &(count)) +# else +# define VIR_DISPOSE_N(ptr, count) virDispose(&(ptr), 0, sizeof(*(ptr)), &(count)) +# endif + + +/** + * VIR_DISPOSE_STRING: + * @ptr: pointer to a string to be cleared and freed + * + * Clears the string and frees the corresponding memory. + * + * This macro is not safe to be used on arguments with side effects. + */ +# if !STATIC_ANALYSIS +/* See explanation in VIR_FREE */ +# define VIR_DISPOSE_STRING(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), \ + (ptr) ? strlen((ptr)) : 0, 1, NULL) +# else +# define VIR_DISPOSE_STRING(ptr) virDispose(&(ptr), (ptr) ? strlen((ptr)) : 1, NULL) +# endif + + +/** + * VIR_DISPOSE: + * @ptr: pointer to memory to be cleared and freed + * + * Clears and frees the corresponding memory. + * + * This macro is safe to be used on arguments with side effects. + */ +# if !STATIC_ANALYSIS +/* See explanation in VIR_FREE */ +# define VIR_DISPOSE(ptr) virDispose(1 ? (void *) &(ptr) : (ptr), 1, sizeof(*(ptr)), NULL) +# else +# define VIR_DISPOSE(ptr) virDispose(&(ptr), 1, sizeof(*(ptr)), NULL) +# endif + + void virAllocTestInit(void); int virAllocTestCount(void); void virAllocTestOOM(int n, int m); diff --git a/tests/viralloctest.c b/tests/viralloctest.c index 8c0826f..dd54f81 100644 --- a/tests/viralloctest.c +++ b/tests/viralloctest.c @@ -384,6 +384,41 @@ testInsertArray(const void *opaque ATTRIBUTE_UNUSED) static int +testDispose(const void *opaque ATTRIBUTE_UNUSED) +{ + int *num = NULL; + int *nums = NULL; + size_t nnums = 0; + char *str = NULL; + + VIR_DISPOSE(num); + VIR_DISPOSE_N(nums, nnums); + VIR_DISPOSE_STRING(str); + + nnums = 10; + VIR_DISPOSE_N(nums, nnums); + + if (VIR_ALLOC(num) < 0) + return -1; + + VIR_DISPOSE(num); + + nnums = 10; + if (VIR_ALLOC_N(nums, nnums) < 0) + return -1; + + VIR_DISPOSE_N(nums, nnums); + + if (VIR_STRDUP(str, "test") < 0) + return -1; + + VIR_DISPOSE_STRING(str); + + return 0; +} + + +static int mymain(void) { int ret = 0; @@ -400,6 +435,8 @@ mymain(void) ret = -1; if (virtTestRun("insert array", testInsertArray, NULL) < 0) ret = -1; + if (virtTestRun("dispose tests", testDispose, NULL) < 0) + ret = -1; return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.8.2
Call the internal driver callbacks rather than the public APIs to avoid calling unnecessarily the error dispatching code and don't overwrite the error messages provided by the APIs. They are good enough to describe which secret is missing either by UUID or the usage (basically name). --- po/POTFILES.in | 1 - src/libxl/libxl_conf.c | 3 --- src/qemu/qemu_domain.c | 4 +--- src/secret/secret_util.c | 39 +++++++-------------------------------- src/secret/secret_util.h | 1 - 5 files changed, 8 insertions(+), 40 deletions(-) diff --git a/po/POTFILES.in b/po/POTFILES.in index 506d535..0d92448 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -153,7 +153,6 @@ src/rpc/virnetsocket.c src/rpc/virnetsshsession.c src/rpc/virnettlscontext.c src/secret/secret_driver.c -src/secret/secret_util.c src/security/security_apparmor.c src/security/security_dac.c src/security/security_driver.c diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index d927b37..b08ee14 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1024,14 +1024,11 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) *srcstr = NULL; if (src->auth && src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { - const char *protocol = virStorageNetProtocolTypeToString(src->protocol); - username = src->auth->username; if (!(conn = virConnectOpen("xen:///system"))) goto cleanup; if (!(secret = virSecretGetSecretString(conn, - protocol, true, src->auth, VIR_SECRET_USAGE_TYPE_CEPH))) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 39a50e6..87f0dbd 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -872,7 +872,6 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, { bool encode = false; int secretType = VIR_SECRET_USAGE_TYPE_ISCSI; - const char *protocolstr = virStorageNetProtocolTypeToString(protocol); secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN; if (VIR_STRDUP(secinfo->s.plain.username, authdef->username) < 0) @@ -885,8 +884,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, } if (!(secinfo->s.plain.secret = - virSecretGetSecretString(conn, protocolstr, encode, - authdef, secretType))) + virSecretGetSecretString(conn, encode, authdef, secretType))) return -1; return 0; diff --git a/src/secret/secret_util.c b/src/secret/secret_util.c index 217584f..d69f7ba 100644 --- a/src/secret/secret_util.c +++ b/src/secret/secret_util.c @@ -37,7 +37,6 @@ VIR_LOG_INIT("secret.secret_util"); /* virSecretGetSecretString: * @conn: Pointer to the connection driver to make secret driver call - * @scheme: Unique enough string for error message to help determine cause * @encoded: Whether the returned secret needs to be base64 encoded * @authdef: Pointer to the disk storage authentication * @secretUsageType: Type of secret usage for authdef lookup @@ -50,7 +49,6 @@ VIR_LOG_INIT("secret.secret_util"); */ char * virSecretGetSecretString(virConnectPtr conn, - const char *scheme, bool encoded, virStorageAuthDefPtr authdef, virSecretUsageType secretUsageType) @@ -58,49 +56,26 @@ virSecretGetSecretString(virConnectPtr conn, size_t secret_size; virSecretPtr sec = NULL; char *secret = NULL; - char uuidStr[VIR_UUID_STRING_BUFLEN]; - /* look up secret */ switch (authdef->secretType) { case VIR_STORAGE_SECRET_TYPE_UUID: - sec = virSecretLookupByUUID(conn, authdef->secret.uuid); - virUUIDFormat(authdef->secret.uuid, uuidStr); + sec = conn->secretDriver->secretLookupByUUID(conn, authdef->secret.uuid); break; + case VIR_STORAGE_SECRET_TYPE_USAGE: - sec = virSecretLookupByUsage(conn, secretUsageType, - authdef->secret.usage); + sec = conn->secretDriver->secretLookupByUsage(conn, secretUsageType, + authdef->secret.usage); break; } - if (!sec) { - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virReportError(VIR_ERR_NO_SECRET, - _("%s no secret matches uuid '%s'"), - scheme, uuidStr); - } else { - virReportError(VIR_ERR_NO_SECRET, - _("%s no secret matches usage value '%s'"), - scheme, authdef->secret.usage); - } + if (!sec) goto cleanup; - } secret = (char *)conn->secretDriver->secretGetValue(sec, &secret_size, 0, VIR_SECRET_GET_VALUE_INTERNAL_CALL); - if (!secret) { - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get value of the secret for " - "username '%s' using uuid '%s'"), - authdef->username, uuidStr); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get value of the secret for " - "username '%s' using usage value '%s'"), - authdef->username, authdef->secret.usage); - } + + if (!secret) goto cleanup; - } if (encoded) { char *base64 = NULL; diff --git a/src/secret/secret_util.h b/src/secret/secret_util.h index c707599..adc6c31 100644 --- a/src/secret/secret_util.h +++ b/src/secret/secret_util.h @@ -26,7 +26,6 @@ # include "virstoragefile.h" char *virSecretGetSecretString(virConnectPtr conn, - const char *scheme, bool encoded, virStorageAuthDefPtr authdef, virSecretUsageType secretUsageType) -- 2.8.2
On 05/13/2016 12:04 PM, Peter Krempa wrote:
Call the internal driver callbacks rather than the public APIs to avoid calling unnecessarily the error dispatching code and don't overwrite the error messages provided by the APIs. They are good enough to describe which secret is missing either by UUID or the usage (basically name). --- po/POTFILES.in | 1 - src/libxl/libxl_conf.c | 3 --- src/qemu/qemu_domain.c | 4 +--- src/secret/secret_util.c | 39 +++++++-------------------------------- src/secret/secret_util.h | 1 - 5 files changed, 8 insertions(+), 40 deletions(-)
[...]
--- a/src/secret/secret_util.h +++ b/src/secret/secret_util.h @@ -26,7 +26,6 @@ # include "virstoragefile.h"
char *virSecretGetSecretString(virConnectPtr conn, - const char *scheme, bool encoded, virStorageAuthDefPtr authdef, virSecretUsageType secretUsageType)
Need to adjust the ATTRIBUTE_NONNULL... since (2) is being removed, then (4) becomes (3) too... John
Add a new helper that sanitizes error semantics of base64_encode_alloc. --- src/conf/virsecretobj.c | 7 ++----- src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c | 6 ++---- src/storage/storage_backend_rbd.c | 17 ++++------------- src/util/virstring.c | 24 ++++++++++++++++++++++++ src/util/virstring.h | 2 ++ tools/virsh-secret.c | 6 +++--- 7 files changed, 38 insertions(+), 25 deletions(-) diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 721e0b4..4babd31 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -30,6 +30,7 @@ #include "virfile.h" #include "virhash.h" #include "virlog.h" +#include "virstring.h" #include "base64.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -730,12 +731,8 @@ virSecretObjSaveData(virSecretObjPtr secret) if (!secret->value) return 0; - base64_encode_alloc((const char *)secret->value, secret->value_size, - &base64); - if (base64 == NULL) { - virReportOOMError(); + if (!(base64 = virStringEncodeBase64(secret->value, secret->value_size))) goto cleanup; - } if (virFileRewrite(secret->base64File, S_IRUSR | S_IWUSR, virSecretRewriteFile, base64) < 0) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9c1abbb..ca2b6b8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2302,6 +2302,7 @@ virSkipSpacesBackwards; virStrcpy; virStrdup; virStringArrayHasString; +virStringEncodeBase64; virStringFreeList; virStringFreeListCount; virStringGetFirstWithPrefix; diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index c55f304..cbc0995 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2142,11 +2142,9 @@ qemuAgentSetUserPassword(qemuAgentPtr mon, virJSONValuePtr reply = NULL; char *password64 = NULL; - base64_encode_alloc(password, strlen(password), &password64); - if (!password64) { - virReportOOMError(); + if (!(password64 = virStringEncodeBase64((unsigned char *) password, + strlen(password)))) goto cleanup; - } if (!(cmd = qemuAgentMakeCommand("guest-set-user-password", "b:crypted", crypted, diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 6e92ff7..ac46b9d 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -59,7 +59,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, int r = 0; virStorageAuthDefPtr authdef = source->auth; unsigned char *secret_value = NULL; - size_t secret_value_size; + size_t secret_value_size = 0; char *rados_key = NULL; virBuffer mon_host = VIR_BUFFER_INITIALIZER; virSecretPtr secret = NULL; @@ -129,15 +129,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, goto cleanup; } - base64_encode_alloc((char *)secret_value, - secret_value_size, &rados_key); - memset(secret_value, 0, secret_value_size); - - if (rados_key == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to decode the RADOS key")); + if (!(rados_key = virStringEncodeBase64(secret_value, secret_value_size))) goto cleanup; - } VIR_DEBUG("Found cephx key: %s", rados_key); if (rados_conf_set(ptr->cluster, "key", rados_key) < 0) { @@ -147,8 +140,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, goto cleanup; } - memset(rados_key, 0, strlen(rados_key)); - if (rados_conf_set(ptr->cluster, "auth_supported", "cephx") < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to set RADOS option: %s"), @@ -233,8 +224,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, ret = 0; cleanup: - VIR_FREE(secret_value); - VIR_FREE(rados_key); + VIR_DISPOSE_N(secret_value, secret_value_size); + VIR_DISPOSE_STRING(rados_key); virObjectUnref(secret); diff --git a/src/util/virstring.c b/src/util/virstring.c index 735e65b..2702cec 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -25,6 +25,7 @@ #include <stdio.h> #include <regex.h> +#include "base64.h" #include "c-ctype.h" #include "virstring.h" #include "viralloc.h" @@ -1066,3 +1067,26 @@ virStringIsPrintable(const char *str) return true; } + + +/** + * virStringEncodeBase64: + * @buf: buffer of bytes to encode + * @buflen: number of bytes to encode + * + * Encodes @buf to base 64 and returns the resulting string. The caller is + * responsible for freeing the result. + */ +char * +virStringEncodeBase64(const uint8_t *buf, size_t buflen) +{ + char *ret; + + base64_encode_alloc((const char *) buf, buflen, &ret); + if (!ret) { + virReportOOMError(); + return NULL; + } + + return ret; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index fd2868a..6bc2726 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -277,4 +277,6 @@ void virStringStripControlChars(char *str); bool virStringIsPrintable(const char *str); +char *virStringEncodeBase64(const uint8_t *buf, size_t buflen); + #endif /* __VIR_STRING_H__ */ diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 087c2ed..532c4d5 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -32,6 +32,7 @@ #include "viralloc.h" #include "virfile.h" #include "virutil.h" +#include "virstring.h" #include "conf/secret_conf.h" static virSecretPtr @@ -265,9 +266,8 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) if (value == NULL) goto cleanup; - base64_encode_alloc((char *)value, value_size, &base64); - memset(value, 0, value_size); - VIR_FREE(value); + if (!(base64 = virStringEncodeBase64(value, value_size))) + goto cleanup; if (base64 == NULL) { vshError(ctl, "%s", _("Failed to allocate memory")); -- 2.8.2
On 05/13/2016 12:04 PM, Peter Krempa wrote:
Add a new helper that sanitizes error semantics of base64_encode_alloc. --- src/conf/virsecretobj.c | 7 ++----- src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c | 6 ++---- src/storage/storage_backend_rbd.c | 17 ++++------------- src/util/virstring.c | 24 ++++++++++++++++++++++++ src/util/virstring.h | 2 ++ tools/virsh-secret.c | 6 +++--- 7 files changed, 38 insertions(+), 25 deletions(-)
probably means #include "base64.h" is no longer necessary in places where the call has been removed.
diff --git a/src/conf/virsecretobj.c b/src/conf/virsecretobj.c index 721e0b4..4babd31 100644 --- a/src/conf/virsecretobj.c +++ b/src/conf/virsecretobj.c @@ -30,6 +30,7 @@ #include "virfile.h" #include "virhash.h" #include "virlog.h" +#include "virstring.h" #include "base64.h"
#define VIR_FROM_THIS VIR_FROM_SECRET @@ -730,12 +731,8 @@ virSecretObjSaveData(virSecretObjPtr secret) if (!secret->value) return 0;
- base64_encode_alloc((const char *)secret->value, secret->value_size, - &base64); - if (base64 == NULL) { - virReportOOMError(); + if (!(base64 = virStringEncodeBase64(secret->value, secret->value_size)))
goto cleanup; - }
if (virFileRewrite(secret->base64File, S_IRUSR | S_IWUSR, virSecretRewriteFile, base64) < 0) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9c1abbb..ca2b6b8 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2302,6 +2302,7 @@ virSkipSpacesBackwards; virStrcpy; virStrdup; virStringArrayHasString; +virStringEncodeBase64; virStringFreeList; virStringFreeListCount; virStringGetFirstWithPrefix; diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index c55f304..cbc0995 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -2142,11 +2142,9 @@ qemuAgentSetUserPassword(qemuAgentPtr mon, virJSONValuePtr reply = NULL; char *password64 = NULL;
- base64_encode_alloc(password, strlen(password), &password64); - if (!password64) { - virReportOOMError(); + if (!(password64 = virStringEncodeBase64((unsigned char *) password, + strlen(password)))) goto cleanup; - }
if (!(cmd = qemuAgentMakeCommand("guest-set-user-password", "b:crypted", crypted, diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 6e92ff7..ac46b9d 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -59,7 +59,7 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, int r = 0; virStorageAuthDefPtr authdef = source->auth; unsigned char *secret_value = NULL; - size_t secret_value_size; + size_t secret_value_size = 0; char *rados_key = NULL; virBuffer mon_host = VIR_BUFFER_INITIALIZER; virSecretPtr secret = NULL; @@ -129,15 +129,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, goto cleanup; }
- base64_encode_alloc((char *)secret_value, - secret_value_size, &rados_key); - memset(secret_value, 0, secret_value_size); - - if (rados_key == NULL) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("failed to decode the RADOS key")); + if (!(rados_key = virStringEncodeBase64(secret_value, secret_value_size))) goto cleanup; - }
VIR_DEBUG("Found cephx key: %s", rados_key); if (rados_conf_set(ptr->cluster, "key", rados_key) < 0) { @@ -147,8 +140,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, goto cleanup; }
- memset(rados_key, 0, strlen(rados_key)); - if (rados_conf_set(ptr->cluster, "auth_supported", "cephx") < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("failed to set RADOS option: %s"), @@ -233,8 +224,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, ret = 0;
cleanup: - VIR_FREE(secret_value); - VIR_FREE(rados_key); + VIR_DISPOSE_N(secret_value, secret_value_size); + VIR_DISPOSE_STRING(rados_key);
virObjectUnref(secret);
diff --git a/src/util/virstring.c b/src/util/virstring.c index 735e65b..2702cec 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -25,6 +25,7 @@ #include <stdio.h> #include <regex.h>
+#include "base64.h" #include "c-ctype.h" #include "virstring.h" #include "viralloc.h" @@ -1066,3 +1067,26 @@ virStringIsPrintable(const char *str)
return true; } + + +/** + * virStringEncodeBase64: + * @buf: buffer of bytes to encode + * @buflen: number of bytes to encode + * + * Encodes @buf to base 64 and returns the resulting string. The caller is + * responsible for freeing the result. + */ +char * +virStringEncodeBase64(const uint8_t *buf, size_t buflen) +{ + char *ret; + + base64_encode_alloc((const char *) buf, buflen, &ret); + if (!ret) { + virReportOOMError(); + return NULL; + }
Don't think the return NULL would be necessary... just return ret
+ + return ret; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index fd2868a..6bc2726 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -277,4 +277,6 @@ void virStringStripControlChars(char *str);
bool virStringIsPrintable(const char *str);
+char *virStringEncodeBase64(const uint8_t *buf, size_t buflen); + #endif /* __VIR_STRING_H__ */ diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index 087c2ed..532c4d5 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -32,6 +32,7 @@ #include "viralloc.h" #include "virfile.h" #include "virutil.h" +#include "virstring.h" #include "conf/secret_conf.h"
static virSecretPtr @@ -265,9 +266,8 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) if (value == NULL) goto cleanup;
- base64_encode_alloc((char *)value, value_size, &base64); - memset(value, 0, value_size); - VIR_FREE(value);
Lost the VIR_FREE(value).... Could be VIR_DISPOSE(value) too. John
+ if (!(base64 = virStringEncodeBase64(value, value_size))) + goto cleanup;
if (base64 == NULL) { vshError(ctl, "%s", _("Failed to allocate memory"));
On Fri, May 13, 2016 at 14:01:32 -0400, John Ferlan wrote:
On 05/13/2016 12:04 PM, Peter Krempa wrote:
Add a new helper that sanitizes error semantics of base64_encode_alloc. --- src/conf/virsecretobj.c | 7 ++----- src/libvirt_private.syms | 1 + src/qemu/qemu_agent.c | 6 ++---- src/storage/storage_backend_rbd.c | 17 ++++------------- src/util/virstring.c | 24 ++++++++++++++++++++++++ src/util/virstring.h | 2 ++ tools/virsh-secret.c | 6 +++--- 7 files changed, 38 insertions(+), 25 deletions(-)
probably means #include "base64.h" is no longer necessary in places where the call has been removed.
Except for places that also call base64_decode_alloc. [...]
+char * +virStringEncodeBase64(const uint8_t *buf, size_t buflen) +{ + char *ret; + + base64_encode_alloc((const char *) buf, buflen, &ret); + if (!ret) { + virReportOOMError(); + return NULL; + }
Don't think the return NULL would be necessary... just return ret
It's the same. Return ret saves one byte in the source, but is more explicit in seeing what is going to be returned.
+ + return ret; +} diff --git a/src/util/virstring.h b/src/util/virstring.h index fd2868a..6bc2726 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h
[...]
@@ -265,9 +266,8 @@ cmdSecretGetValue(vshControl *ctl, const vshCmd *cmd) if (value == NULL) goto cleanup;
- base64_encode_alloc((char *)value, value_size, &base64); - memset(value, 0, value_size); - VIR_FREE(value);
Lost the VIR_FREE(value).... Could be VIR_DISPOSE(value) too.
Indeed. Also in the cleanup label.
John
+ if (!(base64 = virStringEncodeBase64(value, value_size))) + goto cleanup;
if (base64 == NULL) { vshError(ctl, "%s", _("Failed to allocate memory"));
Also this is no longer needed. Peter
From: John Ferlan <jferlan@redhat.com> Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virstring.c | 19 +++++++++++++++++++ src/util/virstring.h | 1 + 3 files changed, 21 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ca2b6b8..28c54d6 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2302,6 +2302,7 @@ virSkipSpacesBackwards; virStrcpy; virStrdup; virStringArrayHasString; +virStringBufferIsPrintable; virStringEncodeBase64; virStringFreeList; virStringFreeListCount; diff --git a/src/util/virstring.c b/src/util/virstring.c index 2702cec..0177a95 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -1070,6 +1070,25 @@ virStringIsPrintable(const char *str) /** + * virBufferIsPrintable: + * + * Returns true if @buf of @buflen contains only printable characters + */ +bool +virStringBufferIsPrintable(const uint8_t *buf, + size_t buflen) +{ + size_t i; + + for (i = 0; i < buflen; i++) + if (!c_isprint(buf[i])) + return false; + + return true; +} + + +/** * virStringEncodeBase64: * @buf: buffer of bytes to encode * @buflen: number of bytes to encode diff --git a/src/util/virstring.h b/src/util/virstring.h index 6bc2726..040771e 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -276,6 +276,7 @@ bool virStringHasControlChars(const char *str); void virStringStripControlChars(char *str); bool virStringIsPrintable(const char *str); +bool virStringBufferIsPrintable(const uint8_t *buf, size_t buflen); char *virStringEncodeBase64(const uint8_t *buf, size_t buflen); -- 2.8.2
They don't free the structure itself so they should be called *Clear rather than *Free. --- src/qemu/qemu_domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 87f0dbd..7e741a7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -728,7 +728,7 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm) static void -qemuDomainSecretPlainFree(qemuDomainSecretPlain secret) +qemuDomainSecretPlainClear(qemuDomainSecretPlain secret) { VIR_FREE(secret.username); memset(secret.secret, 0, strlen(secret.secret)); @@ -737,7 +737,7 @@ qemuDomainSecretPlainFree(qemuDomainSecretPlain secret) static void -qemuDomainSecretAESFree(qemuDomainSecretAES secret) +qemuDomainSecretAESClear(qemuDomainSecretAES secret) { VIR_FREE(secret.username); VIR_FREE(secret.alias); @@ -754,11 +754,11 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) switch ((qemuDomainSecretInfoType) (*secinfo)->type) { case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: - qemuDomainSecretPlainFree((*secinfo)->s.plain); + qemuDomainSecretPlainClear((*secinfo)->s.plain); break; case VIR_DOMAIN_SECRET_INFO_TYPE_AES: - qemuDomainSecretAESFree((*secinfo)->s.aes); + qemuDomainSecretAESClear((*secinfo)->s.aes); break; case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: -- 2.8.2
On 05/13/2016 12:04 PM, Peter Krempa wrote:
They don't free the structure itself so they should be called *Clear rather than *Free. --- src/qemu/qemu_domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 87f0dbd..7e741a7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -728,7 +728,7 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm)
static void -qemuDomainSecretPlainFree(qemuDomainSecretPlain secret) +qemuDomainSecretPlainClear(qemuDomainSecretPlain secret) { VIR_FREE(secret.username); memset(secret.secret, 0, strlen(secret.secret));
This should use the new VIR_DISPOSE_N John
@@ -737,7 +737,7 @@ qemuDomainSecretPlainFree(qemuDomainSecretPlain secret)
static void -qemuDomainSecretAESFree(qemuDomainSecretAES secret) +qemuDomainSecretAESClear(qemuDomainSecretAES secret) { VIR_FREE(secret.username); VIR_FREE(secret.alias); @@ -754,11 +754,11 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo)
switch ((qemuDomainSecretInfoType) (*secinfo)->type) { case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: - qemuDomainSecretPlainFree((*secinfo)->s.plain); + qemuDomainSecretPlainClear((*secinfo)->s.plain); break;
case VIR_DOMAIN_SECRET_INFO_TYPE_AES: - qemuDomainSecretAESFree((*secinfo)->s.aes); + qemuDomainSecretAESClear((*secinfo)->s.aes); break;
case VIR_DOMAIN_SECRET_INFO_TYPE_LAST:
On Fri, May 13, 2016 at 14:04:51 -0400, John Ferlan wrote:
On 05/13/2016 12:04 PM, Peter Krempa wrote:
They don't free the structure itself so they should be called *Clear rather than *Free. --- src/qemu/qemu_domain.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 87f0dbd..7e741a7 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -728,7 +728,7 @@ qemuDomainMasterKeyCreate(virDomainObjPtr vm)
static void -qemuDomainSecretPlainFree(qemuDomainSecretPlain secret) +qemuDomainSecretPlainClear(qemuDomainSecretPlain secret) { VIR_FREE(secret.username); memset(secret.secret, 0, strlen(secret.secret));
This should use the new VIR_DISPOSE_N
Not in the patch that is fixing function names.
From: John Ferlan <jferlan@redhat.com> Rather than returning a "char *" indicating perhaps some sized set of characters that is NUL terminated, alter the function to return 0 or -1 for success/failure and add two parameters to handle returning the buffer and it's size. The function no longer encodes the returned secret, rather it returns the unencoded secret forcing callers to make the necessary adjustments. Alter the callers to handle the adjusted model. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libxl/libxl_conf.c | 20 +++++++++++++------- src/qemu/qemu_command.c | 18 +++++++++++++++--- src/qemu/qemu_domain.c | 17 +++++------------ src/qemu/qemu_domain.h | 3 ++- src/secret/secret_util.c | 42 ++++++++++++++++-------------------------- src/secret/secret_util.h | 11 ++++++----- 6 files changed, 57 insertions(+), 54 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index b08ee14..d9dafd5 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1018,7 +1018,9 @@ static int libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) { virConnectPtr conn = NULL; - char *secret = NULL; + uint8_t *secret = NULL; + char *base64secret = NULL; + size_t secretlen = 0; char *username = NULL; int ret = -1; @@ -1028,20 +1030,24 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) if (!(conn = virConnectOpen("xen:///system"))) goto cleanup; - if (!(secret = virSecretGetSecretString(conn, - true, - src->auth, - VIR_SECRET_USAGE_TYPE_CEPH))) + if (virSecretGetSecretString(conn, src->auth, + VIR_SECRET_USAGE_TYPE_CEPH, + &secret, &secretlen) < 0) + goto cleanup; + + /* RBD expects an encoded secret */ + if (!(base64secret = virStringEncodeBase64(secret, secretlen))) goto cleanup; } - if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, secret))) + if (!(*srcstr = libxlMakeNetworkDiskSrcStr(src, username, base64secret))) goto cleanup; ret = 0; cleanup: - VIR_FREE(secret); + VIR_DISPOSE_N(secret, secretlen); + VIR_DISPOSE_STRING(base64secret); virObjectUnref(conn); return ret; } diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0d6d5f8..ea3d17e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -628,6 +628,12 @@ qemuBuildGeneralSecinfoURI(virURIPtr uri, switch ((qemuDomainSecretInfoType) secinfo->type) { case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: if (secinfo->s.plain.secret) { + if (!virStringBufferIsPrintable(secinfo->s.plain.secret, + secinfo->s.plain.secretlen)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("found non printable characters in secret")); + return -1; + } if (virAsprintf(&uri->user, "%s:%s", secinfo->s.plain.username, secinfo->s.plain.secret) < 0) @@ -662,6 +668,8 @@ static int qemuBuildRBDSecinfoURI(virBufferPtr buf, qemuDomainSecretInfoPtr secinfo) { + char *base64secret = NULL; + if (!secinfo) { virBufferAddLit(buf, ":auth_supported=none"); return 0; @@ -669,11 +677,15 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, switch ((qemuDomainSecretInfoType) secinfo->type) { case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: - virBufferEscape(buf, '\\', ":", ":id=%s", - secinfo->s.plain.username); + if (!(base64secret = virStringEncodeBase64(secinfo->s.plain.secret, + secinfo->s.plain.secretlen))) + return -1; + virBufferEscape(buf, '\\', ":", ":id=%s", secinfo->s.plain.username); virBufferEscape(buf, '\\', ":", ":key=%s:auth_supported=cephx\\;none", - secinfo->s.plain.secret); + base64secret); + VIR_DISPOSE_STRING(base64secret); + VIR_FREE(base64secret); break; case VIR_DOMAIN_SECRET_INFO_TYPE_AES: diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7e741a7..d7f6cc9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -731,8 +731,7 @@ static void qemuDomainSecretPlainClear(qemuDomainSecretPlain secret) { VIR_FREE(secret.username); - memset(secret.secret, 0, strlen(secret.secret)); - VIR_FREE(secret.secret); + VIR_DISPOSE_N(secret.secret, secret.secretlen); } @@ -870,24 +869,18 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, virStorageNetProtocol protocol, virStorageAuthDefPtr authdef) { - bool encode = false; int secretType = VIR_SECRET_USAGE_TYPE_ISCSI; secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN; if (VIR_STRDUP(secinfo->s.plain.username, authdef->username) < 0) return -1; - if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { - /* qemu requires the secret to be encoded for RBD */ - encode = true; + if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) secretType = VIR_SECRET_USAGE_TYPE_CEPH; - } - - if (!(secinfo->s.plain.secret = - virSecretGetSecretString(conn, encode, authdef, secretType))) - return -1; - return 0; + return virSecretGetSecretString(conn, authdef, secretType, + &secinfo->s.plain.secret, + &secinfo->s.plain.secretlen); } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index dd90e67..baf8bd8 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -251,7 +251,8 @@ typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain; typedef struct _qemuDomainSecretPlain *qemuDomainSecretPlainPtr; struct _qemuDomainSecretPlain { char *username; - char *secret; + uint8_t *secret; + size_t secretlen; }; # define QEMU_DOMAIN_AES_IV_LEN 16 /* 16 bytes for 128 bit random */ diff --git a/src/secret/secret_util.c b/src/secret/secret_util.c index d69f7ba..5602401 100644 --- a/src/secret/secret_util.c +++ b/src/secret/secret_util.c @@ -27,7 +27,6 @@ #include "virlog.h" #include "virobject.h" #include "viruuid.h" -#include "base64.h" #include "datatypes.h" #define VIR_FROM_THIS VIR_FROM_SECRET @@ -37,25 +36,26 @@ VIR_LOG_INIT("secret.secret_util"); /* virSecretGetSecretString: * @conn: Pointer to the connection driver to make secret driver call - * @encoded: Whether the returned secret needs to be base64 encoded * @authdef: Pointer to the disk storage authentication * @secretUsageType: Type of secret usage for authdef lookup + * @secret: returned secret as a sized stream of unsigned chars + * @secret_size: Return size of the secret - either raw text or base64 * - * Lookup the secret for the authdef usage type and return it either as - * raw text or encoded based on the caller's need. + * Lookup the secret for the authdef usage type and return it as raw text. + * It is up to the caller to encode the secret further. * - * Returns a pointer to memory that needs to be cleared and free'd after - * usage or NULL on error. + * Returns 0 on success, -1 on failure. On success the memory in secret + * needs to be cleared and free'd after usage. */ -char * +int virSecretGetSecretString(virConnectPtr conn, - bool encoded, virStorageAuthDefPtr authdef, - virSecretUsageType secretUsageType) + virSecretUsageType secretUsageType, + uint8_t **secret, + size_t *secret_size) { - size_t secret_size; virSecretPtr sec = NULL; - char *secret = NULL; + int ret = -1; switch (authdef->secretType) { case VIR_STORAGE_SECRET_TYPE_UUID: @@ -71,25 +71,15 @@ virSecretGetSecretString(virConnectPtr conn, if (!sec) goto cleanup; - secret = (char *)conn->secretDriver->secretGetValue(sec, &secret_size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); + *secret = conn->secretDriver->secretGetValue(sec, secret_size, 0, + VIR_SECRET_GET_VALUE_INTERNAL_CALL); - if (!secret) + if (!*secret) goto cleanup; - if (encoded) { - char *base64 = NULL; - - base64_encode_alloc(secret, secret_size, &base64); - VIR_FREE(secret); - if (!base64) { - virReportOOMError(); - goto cleanup; - } - secret = base64; - } + ret = 0; cleanup: virObjectUnref(sec); - return secret; + return ret; } diff --git a/src/secret/secret_util.h b/src/secret/secret_util.h index adc6c31..a039662 100644 --- a/src/secret/secret_util.h +++ b/src/secret/secret_util.h @@ -25,10 +25,11 @@ # include "internal.h" # include "virstoragefile.h" -char *virSecretGetSecretString(virConnectPtr conn, - bool encoded, - virStorageAuthDefPtr authdef, - virSecretUsageType secretUsageType) +int virSecretGetSecretString(virConnectPtr conn, + virStorageAuthDefPtr authdef, + virSecretUsageType secretUsageType, + uint8_t **ret_secret, + size_t *ret_secret_size) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) - ATTRIBUTE_RETURN_CHECK; + ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; #endif /* __VIR_SECRET_H__ */ -- 2.8.2
On 05/13/2016 12:04 PM, Peter Krempa wrote:
I also wanted to get rid of yet another copy of virSecretGetSecretString in the RBD driver code but I didn't figure out the linking yet, so I'll send the patch separately.
John Ferlan (2): util: string: Introduce helper to determine whether a byte buffer is printable secret: Alter virSecretGetSecretString
Peter Krempa (4): util: alloc: Introduce freeing helpers that clear the memory before freeing secret: util: Refactor virSecretGetSecretString util: string: Introduce virStringEncodeBase64 qemu: domain: Fix names for functions that clear security info
po/POTFILES.in | 1 - src/conf/virsecretobj.c | 7 +--- src/libvirt_private.syms | 3 ++ src/libxl/libxl_conf.c | 23 +++++++----- src/qemu/qemu_agent.c | 6 +-- src/qemu/qemu_command.c | 18 +++++++-- src/qemu/qemu_domain.c | 27 +++++-------- src/qemu/qemu_domain.h | 3 +- src/secret/secret_util.c | 79 +++++++++++---------------------------- src/secret/secret_util.h | 12 +++--- src/storage/storage_backend_rbd.c | 17 ++------- src/util/viralloc.c | 36 ++++++++++++++++++ src/util/viralloc.h | 56 +++++++++++++++++++++++++++ src/util/virstring.c | 43 +++++++++++++++++++++ src/util/virstring.h | 3 ++ tests/viralloctest.c | 37 ++++++++++++++++++ tools/virsh-secret.c | 6 +-- 17 files changed, 256 insertions(+), 121 deletions(-)
ACK series w/ the minor adjustments noted to your patches. John
participants (2)
-
John Ferlan -
Peter Krempa