From f2245b53b402025712e32db03dbf9e46d753bd8b Mon Sep 17 00:00:00 2001 From: Alexey Tikhonov Date: Nov 29 2019 10:22:28 +0000 Subject: util/memory: helper(s) to securely erase mem was reworked Specially designated for this purpose `explicit_bzero()` function is used in case it is available. Otherwise well known trick with a volatile pointer to memset() is used to prevent compiler optimization. Relates: https://pagure.io/SSSD/sssd/issue/3956 Reviewed-by: Sumit Bose --- diff --git a/Makefile.am b/Makefile.am index ed50c25..d8b84c2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -4582,6 +4582,7 @@ krb5_child_SOURCES = \ src/util/sss_iobuf.c \ src/util/find_uid.c \ src/util/atomic_io.c \ + src/util/memory.c \ src/util/authtok.c \ src/util/authtok-utils.c \ src/util/util.c \ @@ -4614,6 +4615,7 @@ ldap_child_SOURCES = \ src/util/sss_krb5.c \ src/util/sss_iobuf.c \ src/util/atomic_io.c \ + src/util/memory.c \ src/util/authtok.c \ src/util/authtok-utils.c \ src/util/util.c \ diff --git a/configure.ac b/configure.ac index 53a8c6b..c369413 100644 --- a/configure.ac +++ b/configure.ac @@ -92,6 +92,8 @@ LIBS=$SAVE_LIBS AC_CHECK_FUNCS([ utimensat \ futimens ]) +AC_CHECK_FUNCS([ explicit_bzero ]) + #Check for endian headers AC_CHECK_HEADERS([endian.h sys/endian.h byteswap.h]) diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index c26f863..6738907 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -305,8 +305,7 @@ static inline checker pick_checker(int format) static int token_pin_destructor(char *mem) { - safezero(mem, strlen(mem)); - return 0; + return sss_erase_talloc_mem_securely(mem); } static krb5_error_code tokeninfo_matches_2fa(TALLOC_CTX *mem_ctx, diff --git a/src/providers/krb5/krb5_delayed_online_authentication.c b/src/providers/krb5/krb5_delayed_online_authentication.c index 1cb7ead..8572d12 100644 --- a/src/providers/krb5/krb5_delayed_online_authentication.c +++ b/src/providers/krb5/krb5_delayed_online_authentication.c @@ -86,7 +86,7 @@ static void authenticate_user(struct tevent_context *ev, } ret = sss_authtok_set_password(pd->authtok, password, keysize); - safezero(password, keysize); + sss_erase_mem_securely(password, keysize); free(password); if (ret) { DEBUG(SSSDBG_CRIT_FAILURE, diff --git a/src/responder/kcm/kcmsrv_ccache_mem.c b/src/responder/kcm/kcmsrv_ccache_mem.c index 18c3878..62165d3 100644 --- a/src/responder/kcm/kcmsrv_ccache_mem.c +++ b/src/responder/kcm/kcmsrv_ccache_mem.c @@ -140,7 +140,8 @@ static int ccwrap_destructor(void *ptr) if (ccwrap->cc != NULL) { if (ccwrap->cc->creds) { - safezero(sss_iobuf_get_data(ccwrap->cc->creds->cred_blob), + sss_erase_mem_securely( + sss_iobuf_get_data(ccwrap->cc->creds->cred_blob), sss_iobuf_get_size(ccwrap->cc->creds->cred_blob)); } } diff --git a/src/tools/sss_seed.c b/src/tools/sss_seed.c index 91bfb4c..5ee54ac 100644 --- a/src/tools/sss_seed.c +++ b/src/tools/sss_seed.c @@ -229,7 +229,8 @@ static int seed_password_input_prompt(TALLOC_CTX *mem_ctx, char **_password) goto done; } - talloc_set_destructor((TALLOC_CTX *)password, password_destructor); + talloc_set_destructor((TALLOC_CTX *)password, + sss_erase_talloc_mem_securely); temp = getpass("Enter temporary password again:"); if (temp == NULL) { diff --git a/src/util/authtok.c b/src/util/authtok.c index 0cac245..64b773a 100644 --- a/src/util/authtok.c +++ b/src/util/authtok.c @@ -181,7 +181,7 @@ void sss_authtok_set_empty(struct sss_auth_token *tok) case SSS_AUTHTOK_TYPE_2FA: case SSS_AUTHTOK_TYPE_SC_PIN: case SSS_AUTHTOK_TYPE_2FA_SINGLE: - safezero(tok->data, tok->length); + sss_erase_mem_securely(tok->data, tok->length); break; case SSS_AUTHTOK_TYPE_CCFILE: case SSS_AUTHTOK_TYPE_SC_KEYPAD: @@ -289,7 +289,7 @@ void sss_authtok_wipe_password(struct sss_auth_token *tok) return; } - safezero(tok->data, tok->length); + sss_erase_mem_securely(tok->data, tok->length); } errno_t sss_auth_unpack_2fa_blob(TALLOC_CTX *mem_ctx, diff --git a/src/util/authtok.h b/src/util/authtok.h index dae3ff6..fd213e8 100644 --- a/src/util/authtok.h +++ b/src/util/authtok.h @@ -121,7 +121,7 @@ errno_t sss_authtok_set_ccfile(struct sss_auth_token *tok, * * @param tok A pointer to an sss_auth_token structure to reset * - * NOTE: This function uses safezero() on the payload if the type + * NOTE: This function uses sss_erase_mem_securely() on the payload if the type * is SSS_AUTHTOK_TYPE_PASSWORD */ void sss_authtok_set_empty(struct sss_auth_token *tok); @@ -156,8 +156,8 @@ errno_t sss_authtok_copy(struct sss_auth_token *src, struct sss_auth_token *dst); /** - * @brief Uses safezero to wipe the password from memory if the - * authtoken contains a password, otherwise does nothing. + * @brief Uses sss_erase_mem_securely to wipe the password from memory + * if the authtoken contains a password, otherwise does nothing. * * @param tok A pointer to an sss_auth_token structure to change * diff --git a/src/util/memory.c b/src/util/memory.c index 7805f4f..4fb8cfa 100644 --- a/src/util/memory.c +++ b/src/util/memory.c @@ -22,17 +22,51 @@ #include "util/util.h" -int password_destructor(void *memctx) + +#ifdef HAVE_EXPLICIT_BZERO + +#include + +#else + +typedef void *(*_sss_memset_t)(void *, int, size_t); + +static volatile _sss_memset_t memset_func = memset; + +static void explicit_bzero(void *s, size_t n) { - char *password = (char *)memctx; - int i; + memset_func(s, 0, n); +} - /* zero out password */ - for (i = 0; password[i]; i++) password[i] = '\0'; +#endif + + +int sss_erase_talloc_mem_securely(void *p) +{ + if (p == NULL) { + return 0; + } + + size_t size = talloc_get_size(p); + if (size == 0) { + return 0; + } + + explicit_bzero(p, size); return 0; } +void sss_erase_mem_securely(void *p, size_t size) +{ + if ((p == NULL) || (size == 0)) { + return; + } + + explicit_bzero(p, size); +} + + struct mem_holder { void *mem; void_destructor_fn_t *fn; diff --git a/src/util/util.c b/src/util/util.c index ec8ca6e..4a985b0 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -618,15 +618,6 @@ errno_t add_string_to_list(TALLOC_CTX *mem_ctx, const char *string, return EOK; } -void safezero(void *data, size_t size) -{ - volatile uint8_t *p = data; - - while (size--) { - *p++ = 0; - } -} - int domain_to_basedn(TALLOC_CTX *memctx, const char *domain, char **basedn) { const char *s; diff --git a/src/util/util.h b/src/util/util.h index d7c1141..c13faf5 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -206,7 +206,11 @@ typedef int (void_destructor_fn_t)(void *); */ int sss_mem_attach(TALLOC_CTX *mem_ctx, void *ptr, void_destructor_fn_t *fn); -int password_destructor(void *memctx); +/* sss_erase_talloc_mem_securely() function always returns 0 as an int value + * to make it possible to use it as talloc destructor. + */ +int sss_erase_talloc_mem_securely(void *p); +void sss_erase_mem_securely(void *p, size_t size); /* from usertools.c */ char *get_uppercase_realm(TALLOC_CTX *memctx, const char *name); @@ -484,15 +488,6 @@ errno_t add_string_to_list(TALLOC_CTX *mem_ctx, const char *string, bool string_in_list(const char *string, char **list, bool case_sensitive); -/** - * @brief Safely zero a segment of memory, - * prevents the compiler from optimizing out - * - * @param data The address of buffer to wipe - * @param size Size of the buffer - */ -void safezero(void *data, size_t size); - int domain_to_basedn(TALLOC_CTX *memctx, const char *domain, char **basedn); bool is_host_in_domain(const char *host, const char *domain);