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

On 05/13/2016 12:04 PM, Peter Krempa wrote: [...] Need to adjust the ATTRIBUTE_NONNULL... since (2) is being removed, then (4) becomes (3) too... John

On 05/13/2016 12:04 PM, Peter Krempa wrote: probably means #include "base64.h" is no longer necessary in places where the call has been removed. Don't think the return NULL would be necessary... just return ret Lost the VIR_FREE(value).... Could be VIR_DISPOSE(value) too. John

From: John Ferlan <jferlan(a)redhat.com&gt; Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- 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

On 05/13/2016 12:04 PM, Peter Krempa wrote: This should use the new VIR_DISPOSE_N John

From: John Ferlan <jferlan(a)redhat.com&gt; 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(a)redhat.com&gt; --- 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