* [PATCH v5 2/5] cipher: Return number of read bytes from asymmetric ops
2016-06-17 20:47 [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign Mat Martineau
@ 2016-06-17 20:47 ` Mat Martineau
2016-06-17 20:47 ` [PATCH v5 3/5] unit: Check asymmetric cipher result lengths Mat Martineau
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Mat Martineau @ 2016-06-17 20:47 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 9734 bytes --]
When using asymmetric cipher operations, the data written to the
output buffer might be shorter than the full buffer size. The number
of bytes read is now returned from the asymmetric
encrypt/decrypt/sign/verify functions. A negative error code is
returned if the call fails.
---
ell/cipher.c | 41 ++++++++++++++++++++---------------------
ell/cipher.h | 16 ++++++++--------
ell/tls.c | 55 ++++++++++++++++++++++++++++++-------------------------
3 files changed, 58 insertions(+), 54 deletions(-)
diff --git a/ell/cipher.c b/ell/cipher.c
index 04ef838..d1b3845 100644
--- a/ell/cipher.c
+++ b/ell/cipher.c
@@ -192,7 +192,7 @@ LIB_EXPORT void l_cipher_free(struct l_cipher *cipher)
l_free(cipher);
}
-static bool operate_cipher(int sk, __u32 operation,
+static ssize_t operate_cipher(int sk, __u32 operation,
const void *in, void *out, size_t len_in,
size_t len_out)
{
@@ -200,6 +200,7 @@ static bool operate_cipher(int sk, __u32 operation,
struct msghdr msg;
struct cmsghdr *c_msg;
struct iovec iov;
+ ssize_t result;
memset(&c_msg_buf, 0, sizeof(c_msg_buf));
memset(&msg, 0, sizeof(msg));
@@ -219,13 +220,11 @@ static bool operate_cipher(int sk, __u32 operation,
msg.msg_iov = &iov;
msg.msg_iovlen = 1;
- if (sendmsg(sk, &msg, 0) < 0)
- return false;
-
- if (read(sk, out, len_out) < 0)
- return false;
+ result = sendmsg(sk, &msg, 0);
+ if (result < 0)
+ return result;
- return true;
+ return read(sk, out, len_out);
}
LIB_EXPORT bool l_cipher_encrypt(struct l_cipher *cipher,
@@ -238,7 +237,7 @@ LIB_EXPORT bool l_cipher_encrypt(struct l_cipher *cipher,
return false;
return operate_cipher(cipher->encrypt_sk, ALG_OP_ENCRYPT, in, out, len,
- len);
+ len) >= 0;
}
LIB_EXPORT bool l_cipher_decrypt(struct l_cipher *cipher,
@@ -251,7 +250,7 @@ LIB_EXPORT bool l_cipher_decrypt(struct l_cipher *cipher,
return false;
return operate_cipher(cipher->decrypt_sk, ALG_OP_DECRYPT, in, out, len,
- len);
+ len) >= 0;
}
LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t *iv,
@@ -454,9 +453,9 @@ LIB_EXPORT int l_asymmetric_cipher_get_key_size(
return cipher->key_size;
}
-LIB_EXPORT bool l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *cipher,
- const void *in, void *out,
- size_t len_in, size_t len_out)
+LIB_EXPORT ssize_t l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *cipher,
+ const void *in, void *out,
+ size_t len_in, size_t len_out)
{
if (unlikely(!cipher))
return false;
@@ -468,9 +467,9 @@ LIB_EXPORT bool l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *cipher,
in, out, len_in, len_out);
}
-LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher,
- const void *in, void *out,
- size_t len_in, size_t len_out)
+LIB_EXPORT ssize_t l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher,
+ const void *in, void *out,
+ size_t len_in, size_t len_out)
{
if (unlikely(!cipher))
return false;
@@ -482,9 +481,9 @@ LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher,
in, out, len_in, len_out);
}
-LIB_EXPORT bool l_asymmetric_cipher_sign(struct l_asymmetric_cipher *cipher,
- const void *in, void *out,
- size_t len_in, size_t len_out)
+LIB_EXPORT ssize_t l_asymmetric_cipher_sign(struct l_asymmetric_cipher *cipher,
+ const void *in, void *out,
+ size_t len_in, size_t len_out)
{
if (unlikely(!cipher))
return false;
@@ -496,9 +495,9 @@ LIB_EXPORT bool l_asymmetric_cipher_sign(struct l_asymmetric_cipher *cipher,
in, out, len_in, len_out);
}
-LIB_EXPORT bool l_asymmetric_cipher_verify(struct l_asymmetric_cipher *cipher,
- const void *in, void *out,
- size_t len_in, size_t len_out)
+LIB_EXPORT ssize_t l_asymmetric_cipher_verify(struct l_asymmetric_cipher *cipher,
+ const void *in, void *out,
+ size_t len_in, size_t len_out)
{
if (unlikely(!cipher))
return false;
diff --git a/ell/cipher.h b/ell/cipher.h
index b958804..45e51c7 100644
--- a/ell/cipher.h
+++ b/ell/cipher.h
@@ -65,21 +65,21 @@ void l_asymmetric_cipher_free(struct l_asymmetric_cipher *cipher);
int l_asymmetric_cipher_get_key_size(struct l_asymmetric_cipher *cipher);
-bool l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *cipher,
+ssize_t l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *cipher,
const void *in, void *out,
size_t len_in, size_t len_out);
-bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher,
+ssize_t l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher,
const void *in, void *out,
size_t len_in, size_t len_out);
-bool l_asymmetric_cipher_sign(struct l_asymmetric_cipher *cipher,
- const void *in, void *out,
- size_t len_in, size_t len_out);
+ssize_t l_asymmetric_cipher_sign(struct l_asymmetric_cipher *cipher,
+ const void *in, void *out,
+ size_t len_in, size_t len_out);
-bool l_asymmetric_cipher_verify(struct l_asymmetric_cipher *cipher,
- const void *in, void *out,
- size_t len_in, size_t len_out);
+ssize_t l_asymmetric_cipher_verify(struct l_asymmetric_cipher *cipher,
+ const void *in, void *out,
+ size_t len_in, size_t len_out);
#ifdef __cplusplus
}
diff --git a/ell/tls.c b/ell/tls.c
index 90404e4..55e2c50 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -861,7 +861,7 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
uint8_t pre_master_secret[48];
struct l_asymmetric_cipher *rsa_server_pubkey;
int key_size;
- bool result;
+ ssize_t bytes_encrypted;
if (!tls->peer_pubkey) {
tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
@@ -896,14 +896,14 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
}
l_put_be16(key_size, ptr);
- result = l_asymmetric_cipher_encrypt(rsa_server_pubkey,
- pre_master_secret, ptr + 2,
- 48, key_size);
+ bytes_encrypted = l_asymmetric_cipher_encrypt(rsa_server_pubkey,
+ pre_master_secret,
+ ptr + 2, 48, key_size);
ptr += key_size + 2;
l_asymmetric_cipher_free(rsa_server_pubkey);
- if (!result) {
+ if (bytes_encrypted != key_size) {
tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
return false;
@@ -924,6 +924,7 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out, size_t len,
uint8_t *privkey;
size_t key_size;
bool result;
+ ssize_t sign_output_len;
const struct tls_hash_algorithm *hash_type;
uint8_t hash[HANDSHAKE_HASH_MAX_SIZE];
uint8_t sign_input[HANDSHAKE_HASH_MAX_SIZE * 2 + 32];
@@ -983,18 +984,23 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out, size_t len,
if (len >= key_size + 2) {
l_put_be16(key_size, *out);
- result = l_asymmetric_cipher_sign(rsa_privkey, sign_input,
- *out + 2,
- sign_input_len,
- key_size);
- *out += key_size + 2;
+ sign_output_len = l_asymmetric_cipher_sign(rsa_privkey,
+ sign_input,
+ *out + 2,
+ sign_input_len,
+ key_size);
+ result = sign_output_len == (ssize_t)key_size;
+ if (result)
+ *out += sign_output_len + 2;
} else {
- tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
result = false;
}
l_asymmetric_cipher_free(rsa_privkey);
+ if (!result)
+ tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+
return result;
}
@@ -1003,7 +1009,7 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
{
struct l_asymmetric_cipher *rsa_client_pubkey;
size_t key_size;
- bool result;
+ ssize_t verify_bytes;
uint8_t hash[HANDSHAKE_HASH_MAX_SIZE];
size_t hash_len;
enum l_checksum_type hash_type;
@@ -1086,18 +1092,16 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
*/
}
- if (expected_len > key_size)
- goto err_free_rsa;
-
- digest_info = alloca(key_size);
+ digest_info = alloca(expected_len);
- result = l_asymmetric_cipher_verify(rsa_client_pubkey, in + 4,
- digest_info,
- key_size, key_size);
+ verify_bytes = l_asymmetric_cipher_verify(rsa_client_pubkey, in + 4,
+ digest_info, key_size,
+ expected_len);
l_asymmetric_cipher_free(rsa_client_pubkey);
- if (!result || memcmp(digest_info, expected, expected_len)) {
+ if (verify_bytes != (ssize_t)expected_len ||
+ memcmp(digest_info, expected, expected_len)) {
tls_disconnect(tls, TLS_ALERT_DECRYPT_ERROR, 0);
return false;
@@ -1755,7 +1759,7 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
struct l_asymmetric_cipher *rsa_server_privkey;
uint8_t *privkey;
size_t key_size;
- bool result;
+ ssize_t bytes_decrypted;
if (!tls->priv_key_path) {
tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
@@ -1805,9 +1809,10 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
return;
}
- result = l_asymmetric_cipher_decrypt(rsa_server_privkey, buf + 2,
- pre_master_secret,
- key_size, 48);
+ bytes_decrypted = l_asymmetric_cipher_decrypt(rsa_server_privkey,
+ buf + 2,
+ pre_master_secret,
+ key_size, 48);
l_asymmetric_cipher_free(rsa_server_privkey);
/*
@@ -1825,7 +1830,7 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
pre_master_secret[0] = tls->client_version >> 8;
pre_master_secret[1] = tls->client_version >> 0;
- if (!result)
+ if (bytes_decrypted != 48)
memcpy(pre_master_secret + 2, random_secret, 46);
tls_generate_master_secret(tls, pre_master_secret, 48);
--
2.9.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v5 3/5] unit: Check asymmetric cipher result lengths
2016-06-17 20:47 [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign Mat Martineau
2016-06-17 20:47 ` [PATCH v5 2/5] cipher: Return number of read bytes from asymmetric ops Mat Martineau
@ 2016-06-17 20:47 ` Mat Martineau
2016-06-17 20:47 ` [PATCH v5 4/5] unit: Check decryption against known ciphertext Mat Martineau
` (2 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Mat Martineau @ 2016-06-17 20:47 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 992 bytes --]
---
unit/test-cipher.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/unit/test-cipher.c b/unit/test-cipher.c
index 78d1fcb..8c27d03 100644
--- a/unit/test-cipher.c
+++ b/unit/test-cipher.c
@@ -184,16 +184,20 @@ static void test_rsa(const void *data)
{
struct l_asymmetric_cipher *cipher;
char buf[128];
+ ssize_t encrypted, decrypted;
cipher = l_asymmetric_cipher_new(L_CIPHER_RSA_PKCS1_V1_5,
rsa_priv_key,
sizeof(rsa_priv_key), false);
assert(cipher);
- assert(l_asymmetric_cipher_encrypt(cipher, FIXED_STR, buf, 100, 128));
+ encrypted = l_asymmetric_cipher_encrypt(cipher, FIXED_STR, buf,
+ 100, 128);
+ assert(encrypted == 128);
assert(memcmp(FIXED_STR, buf, 100));
- assert(l_asymmetric_cipher_decrypt(cipher, buf, buf, 128, 128));
+ decrypted = l_asymmetric_cipher_decrypt(cipher, buf, buf, 128, 128);
+ assert(decrypted == 100);
assert(!memcmp(FIXED_STR, buf, 100));
--
2.9.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v5 4/5] unit: Check decryption against known ciphertext
2016-06-17 20:47 [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign Mat Martineau
2016-06-17 20:47 ` [PATCH v5 2/5] cipher: Return number of read bytes from asymmetric ops Mat Martineau
2016-06-17 20:47 ` [PATCH v5 3/5] unit: Check asymmetric cipher result lengths Mat Martineau
@ 2016-06-17 20:47 ` Mat Martineau
2016-06-17 20:47 ` [PATCH v5 5/5] cipher: Use only one socket for asymmetric cipher Mat Martineau
2016-06-18 3:56 ` [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign Denis Kenzior
4 siblings, 0 replies; 8+ messages in thread
From: Mat Martineau @ 2016-06-17 20:47 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 2499 bytes --]
Include known-good ciphertext and make sure it can be correctly
decrypted. Also confirm that corrupted ciphertext triggers a
decryption error.
---
unit/test-cipher.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)
diff --git a/unit/test-cipher.c b/unit/test-cipher.c
index 8c27d03..12a922a 100644
--- a/unit/test-cipher.c
+++ b/unit/test-cipher.c
@@ -180,6 +180,29 @@ static uint8_t rsa_priv_key[] = {
0x05, 0x3e, 0x55, 0x14, 0xd9, 0xe4, 0xa4, 0x7f, 0xb7, 0x4f
};
+/* Reference ciphertext:
+ * $ openssl rsautl -in fixed_str -inkey privkey.der -keyform DER -encrypt \
+ * > -pkcs -out ciphertext
+ * $ xxd -i ciphertext
+ *
+ * where fixed_str is a file containing the first 100 characters of
+ * FIXED_STR (above) and privkey.der contains the binary data from the
+ * rsa_priv_key array.
+ */
+static uint8_t ciphertext[128] = {
+ 0x50, 0x86, 0x87, 0x72, 0x37, 0xc1, 0xc7, 0x99, 0xa9, 0xff, 0x56, 0x92,
+ 0x9b, 0x8a, 0xf6, 0x31, 0x9b, 0x11, 0x2c, 0x27, 0x1c, 0xa9, 0x07, 0x9b,
+ 0xac, 0xb9, 0x31, 0xcd, 0xc1, 0x10, 0x90, 0xd7, 0x3c, 0xa1, 0x43, 0xa1,
+ 0xdb, 0xb2, 0x67, 0x48, 0x28, 0xac, 0x0e, 0xbd, 0xd4, 0x62, 0x6b, 0xbd,
+ 0x81, 0xf9, 0x5b, 0xd0, 0x29, 0xe5, 0xc8, 0x9a, 0x71, 0x69, 0xd1, 0x61,
+ 0x72, 0x95, 0xa5, 0x10, 0x83, 0xee, 0xb4, 0x6d, 0x79, 0xf8, 0xae, 0xe1,
+ 0x49, 0xdd, 0x5b, 0x1f, 0x4d, 0x2e, 0xd7, 0xa9, 0xf0, 0xf0, 0x81, 0x01,
+ 0x38, 0x58, 0x78, 0x0f, 0x89, 0x3d, 0x60, 0xdb, 0x99, 0x19, 0xb0, 0x14,
+ 0x9d, 0xf7, 0xc8, 0x6e, 0xc3, 0x69, 0xdd, 0xb2, 0xcc, 0x07, 0x32, 0x3b,
+ 0x88, 0xd3, 0xfa, 0x72, 0xe9, 0xaa, 0x66, 0xc5, 0xd3, 0x4a, 0xff, 0x87,
+ 0x6a, 0x78, 0x05, 0x2d, 0x16, 0x7c, 0x98, 0x58
+};
+
static void test_rsa(const void *data)
{
struct l_asymmetric_cipher *cipher;
@@ -198,9 +221,22 @@ static void test_rsa(const void *data)
decrypted = l_asymmetric_cipher_decrypt(cipher, buf, buf, 128, 128);
assert(decrypted == 100);
+ assert(!memcmp(FIXED_STR, buf, 100));
+ /* Decrypt reference ciphertext */
+ memset(buf, 0, 128);
+ decrypted = l_asymmetric_cipher_decrypt(cipher, ciphertext, buf,
+ 128, 128);
+ assert(decrypted == 100);
assert(!memcmp(FIXED_STR, buf, 100));
+ /* Decrypt corrupted ciphertext */
+ ciphertext[0] = ciphertext[0] ^ (uint8_t)0xFF;
+ memset(buf, 0, 128);
+ decrypted = l_asymmetric_cipher_decrypt(cipher, ciphertext, buf,
+ 128, 128);
+ assert(decrypted < 0);
+
l_asymmetric_cipher_free(cipher);
}
--
2.9.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH v5 5/5] cipher: Use only one socket for asymmetric cipher
2016-06-17 20:47 [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign Mat Martineau
` (2 preceding siblings ...)
2016-06-17 20:47 ` [PATCH v5 4/5] unit: Check decryption against known ciphertext Mat Martineau
@ 2016-06-17 20:47 ` Mat Martineau
2016-06-18 3:56 ` [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign Denis Kenzior
4 siblings, 0 replies; 8+ messages in thread
From: Mat Martineau @ 2016-06-17 20:47 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 3355 bytes --]
---
ell/cipher.c | 40 +++++++++++++++-------------------------
1 file changed, 15 insertions(+), 25 deletions(-)
diff --git a/ell/cipher.c b/ell/cipher.c
index d1b3845..b51a96d 100644
--- a/ell/cipher.c
+++ b/ell/cipher.c
@@ -290,7 +290,8 @@ LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t *iv,
}
struct l_asymmetric_cipher {
- struct l_cipher cipher;
+ int type;
+ int sk;
int key_size;
};
@@ -403,7 +404,7 @@ LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new(
return NULL;
cipher = l_new(struct l_asymmetric_cipher, 1);
- cipher->cipher.type = type;
+ cipher->type = type;
switch (type) {
case L_CIPHER_RSA_PKCS1_V1_5:
@@ -414,23 +415,13 @@ LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new(
break;
}
- cipher->cipher.encrypt_sk = create_alg("akcipher", alg_name,
- key, key_length, public_key);
- if (cipher->cipher.encrypt_sk < 0)
+ cipher->sk = create_alg("akcipher", alg_name, key, key_length,
+ public_key);
+ if (cipher->sk < 0)
goto error_free;
- if (public_key) {
- cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
- key, key_length,
- public_key);
- if (cipher->cipher.decrypt_sk < 0)
- goto error_close;
- }
-
return cipher;
-error_close:
- close(cipher->cipher.encrypt_sk);
error_free:
l_free(cipher);
return NULL;
@@ -441,8 +432,7 @@ LIB_EXPORT void l_asymmetric_cipher_free(struct l_asymmetric_cipher *cipher)
if (unlikely(!cipher))
return;
- close(cipher->cipher.encrypt_sk);
- close(cipher->cipher.decrypt_sk);
+ close(cipher->sk);
l_free(cipher);
}
@@ -463,8 +453,8 @@ LIB_EXPORT ssize_t l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *ciphe
if (unlikely(!in) || unlikely(!out))
return false;
- return operate_cipher(cipher->cipher.encrypt_sk, ALG_OP_ENCRYPT,
- in, out, len_in, len_out);
+ return operate_cipher(cipher->sk, ALG_OP_ENCRYPT, in, out, len_in,
+ len_out);
}
LIB_EXPORT ssize_t l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher,
@@ -477,8 +467,8 @@ LIB_EXPORT ssize_t l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *ciphe
if (unlikely(!in) || unlikely(!out))
return false;
- return operate_cipher(cipher->cipher.decrypt_sk, ALG_OP_DECRYPT,
- in, out, len_in, len_out);
+ return operate_cipher(cipher->sk, ALG_OP_DECRYPT, in, out, len_in,
+ len_out);
}
LIB_EXPORT ssize_t l_asymmetric_cipher_sign(struct l_asymmetric_cipher *cipher,
@@ -491,8 +481,8 @@ LIB_EXPORT ssize_t l_asymmetric_cipher_sign(struct l_asymmetric_cipher *cipher,
if (unlikely(!in) || unlikely(!out))
return false;
- return operate_cipher(cipher->cipher.decrypt_sk, ALG_OP_SIGN,
- in, out, len_in, len_out);
+ return operate_cipher(cipher->sk, ALG_OP_SIGN, in, out, len_in,
+ len_out);
}
LIB_EXPORT ssize_t l_asymmetric_cipher_verify(struct l_asymmetric_cipher *cipher,
@@ -505,6 +495,6 @@ LIB_EXPORT ssize_t l_asymmetric_cipher_verify(struct l_asymmetric_cipher *cipher
if (unlikely(!in) || unlikely(!out))
return false;
- return operate_cipher(cipher->cipher.encrypt_sk, ALG_OP_VERIFY,
- in, out, len_in, len_out);
+ return operate_cipher(cipher->sk, ALG_OP_VERIFY, in, out, len_in,
+ len_out);
}
--
2.9.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign
2016-06-17 20:47 [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign Mat Martineau
` (3 preceding siblings ...)
2016-06-17 20:47 ` [PATCH v5 5/5] cipher: Use only one socket for asymmetric cipher Mat Martineau
@ 2016-06-18 3:56 ` Denis Kenzior
2016-06-20 16:51 ` Mat Martineau
4 siblings, 1 reply; 8+ messages in thread
From: Denis Kenzior @ 2016-06-18 3:56 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 4282 bytes --]
Hi Mat, Andrew,
On 06/17/2016 03:47 PM, Mat Martineau wrote:
> ---
> ell/tls-private.h | 2 +-
> ell/tls.c | 29 ++++++++++++++++++++++-------
> 2 files changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/ell/tls-private.h b/ell/tls-private.h
> index 184f2ae..9ea1554 100644
> --- a/ell/tls-private.h
> +++ b/ell/tls-private.h
> @@ -65,7 +65,7 @@ struct tls_key_exchange_algorithm {
> void (*handle_client_key_exchange)(struct l_tls *tls,
> const uint8_t *buf, size_t len);
>
> - bool (*sign)(struct l_tls *tls, uint8_t **out,
> + bool (*sign)(struct l_tls *tls, uint8_t **out, size_t len,
> tls_get_hash_t get_hash);
So what's the purpose of out being a uint8_t **? It seems it is being
rewritten inside tls_rsa_sign to return the number of bytes written to
the caller.
If so, can we instead modify the signature to be:
ssize_t (*sign)(struct l_tls *tls, uint8_t *dest, size_t len,
tls_get_hash_t get_hash);
?
> bool (*verify)(struct l_tls *tls, const uint8_t *in, size_t len,
> tls_get_hash_t get_hash);
> diff --git a/ell/tls.c b/ell/tls.c
> index 0baddf7..90404e4 100644
> --- a/ell/tls.c
> +++ b/ell/tls.c
> @@ -274,7 +274,7 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls);
> static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
> const uint8_t *buf, size_t len);
>
> -static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
> +static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out, size_t len,
> tls_get_hash_t get_hash);
> static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
> tls_get_hash_t get_hash);
> @@ -917,7 +917,7 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
> return true;
> }
>
> -static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
> +static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out, size_t len,
> tls_get_hash_t get_hash)
> {
> struct l_asymmetric_cipher *rsa_privkey;
> @@ -929,6 +929,12 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
> uint8_t sign_input[HANDSHAKE_HASH_MAX_SIZE * 2 + 32];
> size_t sign_input_len;
>
> + if (len < 2) {
> + tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
> +
> + return false;
> + }
> +
> if (!tls->priv_key_path) {
> tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
> TLS_ALERT_BAD_CERT);
> @@ -968,16 +974,24 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
>
> *(*out)++ = hash_type->tls_id;
> *(*out)++ = 1; /* RSA_sign */
We should avoid what I refer to as 'side-effects'. If an operation won't
succeed, don't scribble in the buffer if at all possible.
> + len -= 2;
This doesn't seem right. By my reading the total bytes being written
here are key_size (in cipher_sign) and 2 bytes just above.
Why are you checking for len >= key_size + 2 below?
> } else {
> get_hash(tls, 1, sign_input + 0, NULL, NULL); /* MD5 */
> get_hash(tls, 2, sign_input + 16, NULL, NULL); /* SHA1 */
> sign_input_len = 36;
> }
>
> - l_put_be16(key_size, *out);
> - result = l_asymmetric_cipher_sign(rsa_privkey, sign_input, *out + 2,
> - sign_input_len, key_size);
> - *out += key_size + 2;
So can we check the length ahead of time, e.g. something like:
if (len < needed) {
tls_disconnect();
l_asymmetric_cipher_free();
return -EMSGSIZE;
}
> + if (len >= key_size + 2) {
> + l_put_be16(key_size, *out);
> + result = l_asymmetric_cipher_sign(rsa_privkey, sign_input,
> + *out + 2,
> + sign_input_len,
> + key_size);
> + *out += key_size + 2;
> + } else {
> + tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
> + result = false;
> + }
>
> l_asymmetric_cipher_free(rsa_privkey);
>
> @@ -1143,7 +1157,8 @@ static bool tls_send_certificate_verify(struct l_tls *tls)
> /* Fill in the Certificate Verify body */
>
> if (!tls->pending.cipher_suite->key_xchg->sign(tls, &ptr,
> - tls_get_handshake_hash_by_id))
> + 2048 - TLS_HANDSHAKE_HEADER_SIZE,
> + tls_get_handshake_hash_by_id))
> return false;
>
> /* Stop maintaining handshake message hashes other than SHA256. */
>
Regards,
-Denis
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign
2016-06-18 3:56 ` [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign Denis Kenzior
@ 2016-06-20 16:51 ` Mat Martineau
2016-06-20 17:15 ` Denis Kenzior
0 siblings, 1 reply; 8+ messages in thread
From: Mat Martineau @ 2016-06-20 16:51 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 5212 bytes --]
On Fri, 17 Jun 2016, Denis Kenzior wrote:
> Hi Mat, Andrew,
>
> On 06/17/2016 03:47 PM, Mat Martineau wrote:
>> ---
>> ell/tls-private.h | 2 +-
>> ell/tls.c | 29 ++++++++++++++++++++++-------
>> 2 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/ell/tls-private.h b/ell/tls-private.h
>> index 184f2ae..9ea1554 100644
>> --- a/ell/tls-private.h
>> +++ b/ell/tls-private.h
>> @@ -65,7 +65,7 @@ struct tls_key_exchange_algorithm {
>> void (*handle_client_key_exchange)(struct l_tls *tls,
>> const uint8_t *buf, size_t
>> len);
>>
>> - bool (*sign)(struct l_tls *tls, uint8_t **out,
>> + bool (*sign)(struct l_tls *tls, uint8_t **out, size_t len,
>> tls_get_hash_t get_hash);
>
> So what's the purpose of out being a uint8_t **? It seems it is being
> rewritten inside tls_rsa_sign to return the number of bytes written to the
> caller.
That's my understanding.
>
> If so, can we instead modify the signature to be:
>
> ssize_t (*sign)(struct l_tls *tls, uint8_t *dest, size_t len,
> tls_get_hash_t get_hash);
>
> ?
Sure, that makes more sense to me.
>
>> bool (*verify)(struct l_tls *tls, const uint8_t *in, size_t len,
>> tls_get_hash_t get_hash);
>> diff --git a/ell/tls.c b/ell/tls.c
>> index 0baddf7..90404e4 100644
>> --- a/ell/tls.c
>> +++ b/ell/tls.c
>> @@ -274,7 +274,7 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls
>> *tls);
>> static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
>> const uint8_t *buf, size_t
>> len);
>>
>> -static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
>> +static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out, size_t len,
>> tls_get_hash_t get_hash);
>> static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t
>> len,
>> tls_get_hash_t get_hash);
>> @@ -917,7 +917,7 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls
>> *tls)
>> return true;
>> }
>>
>> -static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
>> +static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out, size_t len,
>> tls_get_hash_t get_hash)
>> {
>> struct l_asymmetric_cipher *rsa_privkey;
>> @@ -929,6 +929,12 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t
>> **out,
>> uint8_t sign_input[HANDSHAKE_HASH_MAX_SIZE * 2 + 32];
>> size_t sign_input_len;
>>
>> + if (len < 2) {
>> + tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
>> +
>> + return false;
>> + }
>> +
>> if (!tls->priv_key_path) {
>> tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
>> TLS_ALERT_BAD_CERT);
>> @@ -968,16 +974,24 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t
>> **out,
>>
>> *(*out)++ = hash_type->tls_id;
>> *(*out)++ = 1; /* RSA_sign */
>
> We should avoid what I refer to as 'side-effects'. If an operation won't
> succeed, don't scribble in the buffer if at all possible.
I agree. In this case, the buffer was being discarded on failure.
>
>> + len -= 2;
>
> This doesn't seem right. By my reading the total bytes being written here
> are key_size (in cipher_sign) and 2 bytes just above.
This is accounting for the two bytes written to 'out' above, which happens
in that block of the 'if' statement but not the 'else' block.
>
> Why are you checking for len >= key_size + 2 below?
The key_size bytes written by cipher_sign and the 2 bytes written by
le_put_be16.
>
>> } else {
>> get_hash(tls, 1, sign_input + 0, NULL, NULL); /* MD5 */
>> get_hash(tls, 2, sign_input + 16, NULL, NULL); /* SHA1 */
>> sign_input_len = 36;
>> }
>>
>> - l_put_be16(key_size, *out);
>> - result = l_asymmetric_cipher_sign(rsa_privkey, sign_input, *out + 2,
>> - sign_input_len, key_size);
>> - *out += key_size + 2;
>
> So can we check the length ahead of time, e.g. something like:
>
> if (len < needed) {
> tls_disconnect();
> l_asymmetric_cipher_free();
> return -EMSGSIZE;
> }
Yes.
As always, there's a balance to be struck between tweaking existing code
and refactoring it. My goal was to be more conservative by leaving as much
of the code alone as I could, but it's no problem to change things as you
suggested. I'll adjust my refactoring threshold for the future!
>
>> + if (len >= key_size + 2) {
>> + l_put_be16(key_size, *out);
>> + result = l_asymmetric_cipher_sign(rsa_privkey, sign_input,
>> + *out + 2,
>> + sign_input_len,
>> + key_size);
>> + *out += key_size + 2;
>> + } else {
>> + tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
>> + result = false;
>> + }
>>
>> l_asymmetric_cipher_free(rsa_privkey);
>>
>> @@ -1143,7 +1157,8 @@ static bool tls_send_certificate_verify(struct l_tls
>> *tls)
>> /* Fill in the Certificate Verify body */
>>
>> if (!tls->pending.cipher_suite->key_xchg->sign(tls, &ptr,
>> -
>> tls_get_handshake_hash_by_id))
>> + 2048 - TLS_HANDSHAKE_HEADER_SIZE,
>> + tls_get_handshake_hash_by_id))
>> return false;
>>
>> /* Stop maintaining handshake message hashes other than SHA256. */
>>
--
Mat Martineau
Intel OTC
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v5 1/5] tls: Check buffer bounds in tls_rsa_sign
2016-06-20 16:51 ` Mat Martineau
@ 2016-06-20 17:15 ` Denis Kenzior
0 siblings, 0 replies; 8+ messages in thread
From: Denis Kenzior @ 2016-06-20 17:15 UTC (permalink / raw)
To: ell
[-- Attachment #1: Type: text/plain, Size: 593 bytes --]
Hi Mat,
>>
>>> + len -= 2;
>>
>> This doesn't seem right. By my reading the total bytes being written
>> here are key_size (in cipher_sign) and 2 bytes just above.
>
> This is accounting for the two bytes written to 'out' above, which
> happens in that block of the 'if' statement but not the 'else' block.
>
Ah yes, you're right. We write key_size + 4 bytes in case of TLS 1.2.
>>
>> Why are you checking for len >= key_size + 2 below?
>
> The key_size bytes written by cipher_sign and the 2 bytes written by
> le_put_be16.
Yep, my bad.
Regards,
-Denis
^ permalink raw reply [flat|nested] 8+ messages in thread