[PATCH 1/4] ecc: Clear point coordinates, use explicit_bzero

[PATCH 1/4] ecc: Clear point coordinates, use explicit_bzero

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1389 bytes --]

We would only zero out memory used by the scalar type because
Diffie-Hellman secrets are scalars, but in PWD the PWD and PK values may
be points so also clear those.  Update l_ecc_scalar_free to use
explicit_bzero too.
---
 ell/ecc.c  | 7 ++++++-
 ell/ecdh.c | 2 --
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/ell/ecc.c b/ell/ecc.c
index 88c8373..5fa91a8 100644
--- a/ell/ecc.c
+++ b/ell/ecc.c
@@ -541,6 +541,11 @@ LIB_EXPORT ssize_t l_ecc_point_get_data(const struct l_ecc_point *p, void *buf,
 
 LIB_EXPORT void l_ecc_point_free(struct l_ecc_point *p)
 {
+	if (unlikely(!p))
+		return;
+
+	explicit_bzero(p->x, p->curve->ndigits * 8);
+	explicit_bzero(p->y, p->curve->ndigits * 8);
 	l_free(p);
 }
 
@@ -612,7 +617,7 @@ LIB_EXPORT void l_ecc_scalar_free(struct l_ecc_scalar *c)
 	if (unlikely(!c))
 		return;
 
-	memset(c->c, 0, c->curve->ndigits * 8);
+	explicit_bzero(c->c, c->curve->ndigits * 8);
 	l_free(c);
 }
 
diff --git a/ell/ecdh.c b/ell/ecdh.c
index 5ecbd44..a10189f 100644
--- a/ell/ecdh.c
+++ b/ell/ecdh.c
@@ -100,8 +100,6 @@ LIB_EXPORT bool l_ecdh_generate_shared_secret(
 
 	*secret = _ecc_constant_new(curve, product->x, curve->ndigits * 8);
 
-	memset(product->x, 0, curve->ndigits * 8);
-	memset(product->y, 0, curve->ndigits * 8);
 	l_ecc_point_free(product);
 	l_ecc_scalar_free(z);
 
-- 
2.19.1

[PATCH 2/4] pkcs5: Memzero copies of secrets

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 1849 bytes --]

Those functions are used, among others, for decrypting private keys and
are passed the private key passphrases so make sure those are being
cleared.
---
 ell/pkcs5.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/ell/pkcs5.c b/ell/pkcs5.c
index 9fac479..bd7db90 100644
--- a/ell/pkcs5.c
+++ b/ell/pkcs5.c
@@ -89,11 +89,11 @@ LIB_EXPORT bool l_pkcs5_pbkdf1(enum l_checksum_type type, const char *password,
 
 	l_checksum_free(checksum);
 
-	if (iter_count)
-		return false;
+	if (!iter_count)
+		memcpy(out_dk, t, dk_len);
 
-	memcpy(out_dk, t, dk_len);
-	return true;
+	explicit_bzero(t, sizeof(t));
+	return !iter_count;
 }
 
 /* RFC8018 section 5.2 */
@@ -399,14 +399,13 @@ static struct l_cipher *pkcs5_cipher_from_pbes2_params(
 		return NULL;
 
 	cipher = l_cipher_new(enc_scheme->cipher_type, derived_key, key_len);
-	if (!cipher)
-		return NULL;
-
-	if (l_cipher_set_iv(cipher, params, enc_scheme->iv_size))
-		return cipher;
+	if (cipher && !l_cipher_set_iv(cipher, params, enc_scheme->iv_size)) {
+		l_cipher_free(cipher);
+		cipher = NULL;
+	}
 
-	l_cipher_free(cipher);
-	return NULL;
+	explicit_bzero(derived_key, 16);
+	return cipher;
 }
 
 struct l_cipher *pkcs5_cipher_from_alg_id(const uint8_t *id_asn1,
@@ -474,12 +473,11 @@ struct l_cipher *pkcs5_cipher_from_alg_id(const uint8_t *id_asn1,
 		return NULL;
 
 	cipher = l_cipher_new(pbes1_scheme->cipher_type, derived_key + 0, 8);
-	if (!cipher)
-		return NULL;
-
-	if (l_cipher_set_iv(cipher, derived_key + 8, 8))
-		return cipher;
+	if (cipher && !l_cipher_set_iv(cipher, derived_key + 8, 8)) {
+		l_cipher_free(cipher);
+		cipher = NULL;
+	}
 
-	l_cipher_free(cipher);
-	return NULL;
+	explicit_bzero(derived_key, 16);
+	return cipher;
 }
-- 
2.19.1

[PATCH 3/4] settings: Clear values in unescape_value on error

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 463 bytes --]

Clear the beginnings of strings loaded from file that later failed
parsing.
---
 ell/settings.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ell/settings.c b/ell/settings.c
index 98823a2..3b79431 100644
--- a/ell/settings.c
+++ b/ell/settings.c
@@ -139,6 +139,7 @@ static char *unescape_value(const char *value)
 			*n = '\\';
 			break;
 		default:
+			explicit_bzero(ret, n - ret);
 			l_free(ret);
 			return NULL;
 		}
-- 
2.19.1

[PATCH 4/4] all: Replace uses of memset with explicit_bzero

[Andrew Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 7595 bytes --]

Replace existing uses of memset to clear secrets with explicit_bzero to
make sure it doesn't get optimized out.  This has some side effects as
documented in gcc docs though but seems to be the recommended anyway.
---
 ell/key.c        |  4 ++--
 ell/pem.c        |  4 ++--
 ell/settings.c   |  2 +-
 ell/tls-suites.c | 14 +++++++-------
 ell/tls.c        | 27 +++++++++++++++------------
 5 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/ell/key.c b/ell/key.c
index ce4fbad..ed9941d 100644
--- a/ell/key.c
+++ b/ell/key.c
@@ -366,7 +366,7 @@ LIB_EXPORT bool l_key_extract(struct l_key *key, void *payload, size_t *len)
 	keylen = kernel_read_key(key->serial, payload, *len);
 
 	if (keylen < 0 || (size_t)keylen > *len) {
-		memset(payload, 0, *len);
+		explicit_bzero(payload, *len);
 		return false;
 	}
 
@@ -482,7 +482,7 @@ LIB_EXPORT struct l_key *l_key_generate_dh_private(const void *prime_buf,
 	buf[0] |= 1 << ((prime_bits - 2) % 8);
 
 	private = l_key_new(L_KEY_RAW, buf, private_bytes);
-	memset(buf, 0, private_bytes);
+	explicit_bzero(buf, private_bytes);
 	l_free(buf);
 	return private;
 }
diff --git a/ell/pem.c b/ell/pem.c
index 4a91bb3..63b28ec 100644
--- a/ell/pem.c
+++ b/ell/pem.c
@@ -405,7 +405,7 @@ LIB_EXPORT struct l_key *l_pem_load_private_key(const char *filename,
 		}
 
 		l_cipher_free(alg);
-		memset(content, 0, len);
+		explicit_bzero(content, len);
 		l_free(content);
 		content = decrypted;
 		len = data_len;
@@ -443,7 +443,7 @@ done:
 
 err:
 	if (content) {
-		memset(content, 0, len);
+		explicit_bzero(content, len);
 		l_free(content);
 	}
 
diff --git a/ell/settings.c b/ell/settings.c
index 3b79431..7de344f 100644
--- a/ell/settings.c
+++ b/ell/settings.c
@@ -68,7 +68,7 @@ static void setting_destroy(void *data)
 	struct setting_data *pair = data;
 
 	l_free(pair->key);
-	memset(pair->value, 0, strlen(pair->value));
+	explicit_bzero(pair->value, strlen(pair->value));
 	l_free(pair->value);
 	l_free(pair);
 }
diff --git a/ell/tls-suites.c b/ell/tls-suites.c
index 18ac366..0eb602a 100644
--- a/ell/tls-suites.c
+++ b/ell/tls-suites.c
@@ -265,7 +265,7 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 	tls_tx_handshake(tls, TLS_CLIENT_KEY_EXCHANGE, buf, ptr - buf);
 
 	tls_generate_master_secret(tls, pre_master_secret, 48);
-	memset(pre_master_secret, 0, 48);
+	explicit_bzero(pre_master_secret, 48);
 
 	return true;
 }
@@ -329,8 +329,8 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
 	}
 
 	tls_generate_master_secret(tls, pre_master_secret, 48);
-	memset(pre_master_secret, 0, 48);
-	memset(random_secret, 0, 46);
+	explicit_bzero(pre_master_secret, 48);
+	explicit_bzero(random_secret, 46);
 }
 
 static struct tls_key_exchange_algorithm tls_rsa_key_xchg = {
@@ -619,7 +619,7 @@ static bool tls_send_ecdhe_client_key_xchg(struct l_tls *tls)
 
 	tls_generate_master_secret(tls, pre_master_secret,
 					pre_master_secret_len);
-	memset(pre_master_secret, 0, pre_master_secret_len);
+	explicit_bzero(pre_master_secret, pre_master_secret_len);
 
 	return true;
 }
@@ -694,7 +694,7 @@ static void tls_handle_ecdhe_client_key_xchg(struct l_tls *tls,
 
 	tls_generate_master_secret(tls, pre_master_secret,
 					pre_master_secret_len);
-	memset(pre_master_secret, 0, pre_master_secret_len);
+	explicit_bzero(pre_master_secret, pre_master_secret_len);
 
 	return;
 
@@ -1014,7 +1014,7 @@ static bool tls_send_dhe_client_key_xchg(struct l_tls *tls)
 	tls_free_dhe_params(tls);
 	tls_generate_master_secret(tls, pre_master_secret + zeros,
 					pre_master_secret_len - zeros);
-	memset(pre_master_secret, 0, pre_master_secret_len);
+	explicit_bzero(pre_master_secret, pre_master_secret_len);
 	return true;
 }
 
@@ -1074,7 +1074,7 @@ static void tls_handle_dhe_client_key_xchg(struct l_tls *tls,
 	tls_free_dhe_params(tls);
 	tls_generate_master_secret(tls, pre_master_secret + zeros,
 					pre_master_secret_len - zeros);
-	memset(pre_master_secret, 0, pre_master_secret_len);
+	explicit_bzero(pre_master_secret, pre_master_secret_len);
 	return;
 
 decode_error:
diff --git a/ell/tls.c b/ell/tls.c
index 8aa58d5..e910ad1 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -154,7 +154,7 @@ LIB_EXPORT bool l_tls_prf_get_bytes(struct l_tls *tls, bool use_master_secret,
 	else
 		r = tls_prf_get_bytes(tls, "", 0, label, seed, 64, buf, len);
 
-	memset(seed, 0, 64);
+	explicit_bzero(seed, 64);
 
 	return r;
 }
@@ -180,7 +180,7 @@ static void tls_reset_handshake(struct l_tls *tls)
 {
 	enum handshake_hash_type hash;
 
-	memset(tls->pending.key_block, 0, sizeof(tls->pending.key_block));
+	explicit_bzero(tls->pending.key_block, sizeof(tls->pending.key_block));
 
 	if (tls->pending.cipher_suite &&
 			tls->pending.cipher_suite->key_xchg->free_params)
@@ -205,9 +205,9 @@ static void tls_reset_handshake(struct l_tls *tls)
 
 static void tls_cleanup_handshake(struct l_tls *tls)
 {
-	memset(tls->pending.client_random, 0, 32);
-	memset(tls->pending.server_random, 0, 32);
-	memset(tls->pending.master_secret, 0, 48);
+	explicit_bzero(tls->pending.client_random, 32);
+	explicit_bzero(tls->pending.server_random, 32);
+	explicit_bzero(tls->pending.master_secret, 48);
 }
 
 static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx,
@@ -242,7 +242,7 @@ static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx,
 	tls->record_iv_length[txrx] = 0;
 
 	if (tls->fixed_iv_length[txrx]) {
-		memset(tls->fixed_iv[txrx], 0, tls->fixed_iv_length[txrx]);
+		explicit_bzero(tls->fixed_iv[txrx], tls->fixed_iv_length[txrx]);
 		tls->fixed_iv_length[txrx] = 0;
 	}
 
@@ -267,7 +267,8 @@ static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx,
 						key_offset, mac->mac_length);
 
 		/* Wipe out the now unneeded part of the key block */
-		memset(tls->pending.key_block + key_offset, 0, mac->mac_length);
+		explicit_bzero(tls->pending.key_block + key_offset,
+				mac->mac_length);
 
 		if (!tls->mac[txrx]) {
 			if (error) {
@@ -309,7 +310,8 @@ static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx,
 		}
 
 		/* Wipe out the now unneeded part of the key block */
-		memset(tls->pending.key_block + key_offset, 0, enc->key_length);
+		explicit_bzero(tls->pending.key_block + key_offset,
+				enc->key_length);
 
 		if (!cipher) {
 			if (error) {
@@ -349,7 +351,8 @@ static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx,
 				key_offset, enc->iv_length);
 
 		/* Wipe out the now unneeded part of the key block */
-		memset(tls->pending.key_block + key_offset, 0, enc->iv_length);
+		explicit_bzero(tls->pending.key_block + key_offset,
+				enc->iv_length);
 	} else if (tls->cipher_suite[txrx]->encryption &&
 			tls->cipher_suite[txrx]->encryption->fixed_iv_length) {
 		enc = tls->cipher_suite[txrx]->encryption;
@@ -363,8 +366,8 @@ static bool tls_change_cipher_spec(struct l_tls *tls, bool txrx,
 			enc->fixed_iv_length);
 
 		/* Wipe out the now unneeded part of the key block */
-		memset(tls->pending.key_block + key_offset, 0,
-			enc->fixed_iv_length);
+		explicit_bzero(tls->pending.key_block + key_offset,
+				enc->fixed_iv_length);
 	}
 
 	return true;
@@ -1111,7 +1114,7 @@ void tls_generate_master_secret(struct l_tls *tls,
 				"key expansion", seed, 64,
 				tls->pending.key_block, key_block_size);
 
-	memset(seed, 0, 64);
+	explicit_bzero(seed, 64);
 }
 
 static void tls_get_handshake_hash(struct l_tls *tls,
-- 
2.19.1

Re: [PATCH 1/4] ecc: Clear point coordinates, use explicit_bzero

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 457 bytes --]

Hi Andrew,

On 03/18/2019 07:48 PM, Andrew Zaborowski wrote:
> We would only zero out memory used by the scalar type because
> Diffie-Hellman secrets are scalars, but in PWD the PWD and PK values may
> be points so also clear those.  Update l_ecc_scalar_free to use
> explicit_bzero too.
> ---
>   ell/ecc.c  | 7 ++++++-
>   ell/ecdh.c | 2 --
>   2 files changed, 6 insertions(+), 3 deletions(-)
> 

All applied, thanks.

Regards,
-Denis