[PATCH 1/3] util: Add a constant time version of l_memeq

[PATCH 1/3] util: Add a constant time version of l_memeq

[Andrew Zaborowski]

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

---
 ell/ell.sym |  1 +
 ell/util.c  | 12 ++++++++++++
 ell/util.h  |  1 +
 3 files changed, 14 insertions(+)

diff --git a/ell/ell.sym b/ell/ell.sym
index aa0c046..9938216 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -21,6 +21,7 @@ global:
 	l_util_debug;
 	l_util_get_debugfs_path;
 	l_memeq;
+	l_secure_memeq;
 	/* test */
 	l_test_init;
 	l_test_run;
diff --git a/ell/util.c b/ell/util.c
index 6896bc8..5b443a9 100644
--- a/ell/util.c
+++ b/ell/util.c
@@ -652,3 +652,15 @@ LIB_EXPORT bool l_memeq(const void *field, size_t size, uint8_t byte)
 
 	return true;
 }
+
+LIB_EXPORT bool l_secure_memeq(const void *field, size_t size, uint8_t byte)
+{
+	const volatile uint8_t *mem = field;
+	size_t i;
+	bool diff = false;
+
+	for (i = 0; i < size; i++)
+		diff |= mem[i] != byte;
+
+	return !diff;
+}
diff --git a/ell/util.h b/ell/util.h
index 11708c2..b112878 100644
--- a/ell/util.h
+++ b/ell/util.h
@@ -412,6 +412,7 @@ static inline int l_secure_memcmp(const void *a, const void *b,
 }
 
 bool l_memeq(const void *field, size_t size, uint8_t byte);
+bool l_secure_memeq(const void *field, size_t size, uint8_t byte);
 
 static inline bool l_memeqzero(const void *field, size_t size)
 {
-- 
2.27.0

[PATCH 2/3] pem/cert/tls: Use l_secure_memeq for verifying padding

[Andrew Zaborowski]

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

Instead of open-coding for loops for verifying padding, use
l_secure_memeq.  This simplifies the code and makes the intent of
the logic easier to understand.
---
 ell/cert.c       | 24 +++++++++++-------------
 ell/pem.c        | 12 ++++++------
 ell/tls-record.c |  7 +++----
 3 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/ell/cert.c b/ell/cert.c
index c26051f..3f2db1e 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -599,7 +599,7 @@ struct l_key *cert_key_from_pkcs8_encrypted_private_key_info(const uint8_t *der,
 	size_t key_info_len, alg_id_len, data_len, tmp_len;
 	struct l_cipher *alg;
 	uint8_t *decrypted;
-	int i;
+	uint8_t pad;
 	struct l_key *pkey;
 	bool r;
 	bool is_block;
@@ -639,24 +639,22 @@ struct l_key *cert_key_from_pkcs8_encrypted_private_key_info(const uint8_t *der,
 	}
 
 	decrypted_len = data_len;
+	pad = decrypted[data_len - 1];
 
-	if (is_block) {
-		/*
-		 * For block ciphers strip padding as defined in RFC8018
-		 * (for PKCS#5 v1) or RFC1423 / RFC5652 (for v2).
-		 */
+	/*
+	 * For block ciphers strip padding as defined in RFC8018
+	 * (for PKCS#5 v1) or RFC1423 / RFC5652 (for v2).
+	 */
+	if (is_block && pad) {
 		pkey = NULL;
 
-		if (decrypted[data_len - 1] >= data_len ||
-				decrypted[data_len - 1] > 16)
+		if (pad > data_len || pad > 16)
 			goto cleanup;
 
-		for (i = 1; i < decrypted[data_len - 1]; i++)
-			if (decrypted[data_len - 1 - i] !=
-					decrypted[data_len - 1])
-				goto cleanup;
+		if (!l_secure_memeq(decrypted + data_len - pad, pad - 1U, pad))
+			goto cleanup;
 
-		decrypted_len -= decrypted[data_len - 1];
+		decrypted_len -= pad;
 	}
 
 	pkey = cert_key_from_pkcs8_private_key_info(decrypted, decrypted_len);
diff --git a/ell/pem.c b/ell/pem.c
index 67d2af1..d33cf68 100644
--- a/ell/pem.c
+++ b/ell/pem.c
@@ -707,8 +707,8 @@ struct l_key *pem_load_private_key(uint8_t *content, size_t len, char *label,
 		if (dekalgid) {
 			struct l_cipher *alg;
 			bool r;
-			int i;
 			size_t block_len;
+			uint8_t pad;
 
 			if (encrypted)
 				*encrypted = true;
@@ -731,14 +731,14 @@ struct l_key *pem_load_private_key(uint8_t *content, size_t len, char *label,
 				goto err;
 
 			/* Remove padding like in RFC1423 Section 1.1 */
-			if (content[len - 1] > block_len)
+			pad = content[len - 1];
+			if (pad > block_len)
 				goto err;
 
-			for (i = 1; i < content[len - 1]; i++)
-				if (content[len - 1 - i] != content[len - 1])
-					goto err;
+			if (!l_secure_memeq(content + len - pad, pad - 1U, pad))
+				goto err;
 
-			len -= content[len - 1];
+			len -= pad;
 		}
 
 		pkey = cert_key_from_pkcs1_rsa_private_key(content, len);
diff --git a/ell/tls-record.c b/ell/tls-record.c
index 505263d..cdbd0b8 100644
--- a/ell/tls-record.c
+++ b/ell/tls-record.c
@@ -522,10 +522,9 @@ static bool tls_handle_ciphertext(struct l_tls *tls)
 			tls->mac_length[0];
 		l_put_be16(compressed_len, compressed + 11);
 
-		for (i = 0; i < padding_len; i++)
-			if (compressed[13 + cipher_output_len - 1 -
-					padding_len + i] != padding_len)
-				error = 1;
+		error |= !l_secure_memeq(compressed + 13 + cipher_output_len -
+						1 - padding_len, padding_len,
+						padding_len);
 
 		/* Calculate the MAC if needed */
 		tls_write_mac(tls, compressed + 8, 5 + compressed_len,
-- 
2.27.0

[PATCH 3/3] pem/cert: Ensure RFC8018/RFC1423 padding isn't 0

[Andrew Zaborowski]

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

---
 ell/cert.c | 8 ++++----
 ell/pem.c  | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/ell/cert.c b/ell/cert.c
index 3f2db1e..1413563 100644
--- a/ell/cert.c
+++ b/ell/cert.c
@@ -599,7 +599,6 @@ struct l_key *cert_key_from_pkcs8_encrypted_private_key_info(const uint8_t *der,
 	size_t key_info_len, alg_id_len, data_len, tmp_len;
 	struct l_cipher *alg;
 	uint8_t *decrypted;
-	uint8_t pad;
 	struct l_key *pkey;
 	bool r;
 	bool is_block;
@@ -639,16 +638,17 @@ struct l_key *cert_key_from_pkcs8_encrypted_private_key_info(const uint8_t *der,
 	}
 
 	decrypted_len = data_len;
-	pad = decrypted[data_len - 1];
 
 	/*
 	 * For block ciphers strip padding as defined in RFC8018
 	 * (for PKCS#5 v1) or RFC1423 / RFC5652 (for v2).
 	 */
-	if (is_block && pad) {
+	if (is_block) {
+		uint8_t pad = decrypted[data_len - 1];
+
 		pkey = NULL;
 
-		if (pad > data_len || pad > 16)
+		if (pad > data_len || pad > 16 || pad == 0)
 			goto cleanup;
 
 		if (!l_secure_memeq(decrypted + data_len - pad, pad - 1U, pad))
diff --git a/ell/pem.c b/ell/pem.c
index d33cf68..aec59ef 100644
--- a/ell/pem.c
+++ b/ell/pem.c
@@ -732,7 +732,7 @@ struct l_key *pem_load_private_key(uint8_t *content, size_t len, char *label,
 
 			/* Remove padding like in RFC1423 Section 1.1 */
 			pad = content[len - 1];
-			if (pad > block_len)
+			if (pad > block_len || pad == 0)
 				goto err;
 
 			if (!l_secure_memeq(content + len - pad, pad - 1U, pad))
-- 
2.27.0

Re: [PATCH 1/3] util: Add a constant time version of l_memeq

[Denis Kenzior]

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

Hi Andrew,

On 2/10/21 5:36 PM, Andrew Zaborowski wrote:
> ---
>   ell/ell.sym |  1 +
>   ell/util.c  | 12 ++++++++++++
>   ell/util.h  |  1 +
>   3 files changed, 14 insertions(+)
> 

All applied, thanks.

Regards,
-Denis