[PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[Mat Martineau]

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

There are some significant differences in the current iteration of
kernel AF_ALG akcipher support:
 * Kernel handles padding
 * Kernel accepts DER-encoded certs as-is (no need to extract values)
 * Must explicitly set private or public key
---
 ell/cipher-private.h |   3 -
 ell/cipher.c         | 311 ++++++++++++++-------------------------------------
 ell/tls.c            |  43 +++----
 3 files changed, 97 insertions(+), 260 deletions(-)

diff --git a/ell/cipher-private.h b/ell/cipher-private.h
index 7d115f6..77ddd06 100644
--- a/ell/cipher-private.h
+++ b/ell/cipher-private.h
@@ -20,9 +20,6 @@
  *
  */
 
-uint8_t *extract_rsakey(uint8_t *pkcs1_key, size_t pkcs1_key_len,
-			size_t *out_len);
-
 #define ASN1_ID(class, pc, tag)	(((class) << 6) | ((pc) << 5) | (tag))
 
 #define ASN1_CLASS_UNIVERSAL	0
diff --git a/ell/cipher.c b/ell/cipher.c
index 671071b..f23bdda 100644
--- a/ell/cipher.c
+++ b/ell/cipher.c
@@ -78,6 +78,17 @@ struct af_alg_iv {
 #define SOL_ALG 279
 #endif
 
+#ifndef ALG_OP_SIGN
+#define ALG_OP_SIGN	2
+#endif
+#ifndef ALG_OP_VERIFY
+#define ALG_OP_VERIFY	3
+#endif
+
+#ifndef ALG_SET_PUBKEY
+#define ALG_SET_PUBKEY	6
+#endif
+
 #define is_valid_type(type)  ((type) <= L_CIPHER_DES3_EDE_CBC)
 
 struct l_cipher {
@@ -87,10 +98,11 @@ struct l_cipher {
 };
 
 static int create_alg(const char *alg_type, const char *alg_name,
-				const void *key, size_t key_length)
+			const void *key, size_t key_length, bool public)
 {
 	struct sockaddr_alg salg;
 	int sk;
+	int keyopt;
 	int ret;
 
 	sk = socket(PF_ALG, SOCK_SEQPACKET | SOCK_CLOEXEC, 0);
@@ -107,7 +119,8 @@ static int create_alg(const char *alg_type, const char *alg_name,
 		return -1;
 	}
 
-	if (setsockopt(sk, SOL_ALG, ALG_SET_KEY, key, key_length) < 0) {
+	keyopt = public ? ALG_SET_PUBKEY : ALG_SET_KEY;
+	if (setsockopt(sk, SOL_ALG, keyopt, key, key_length) < 0) {
 		close(sk);
 		return -1;
 	}
@@ -149,11 +162,13 @@ LIB_EXPORT struct l_cipher *l_cipher_new(enum l_cipher_type type,
 		break;
 	}
 
-	cipher->encrypt_sk = create_alg("skcipher", alg_name, key, key_length);
+	cipher->encrypt_sk = create_alg("skcipher", alg_name, key, key_length,
+					false);
 	if (cipher->encrypt_sk < 0)
 		goto error_free;
 
-	cipher->decrypt_sk = create_alg("skcipher", alg_name, key, key_length);
+	cipher->decrypt_sk = create_alg("skcipher", alg_name, key, key_length,
+					false);
 	if (cipher->decrypt_sk < 0)
 		goto error_close;
 
@@ -178,7 +193,8 @@ LIB_EXPORT void l_cipher_free(struct l_cipher *cipher)
 }
 
 static bool operate_cipher(int sk, __u32 operation,
-				const void *in, void *out, size_t len)
+				const void *in, void *out, size_t len_in,
+				size_t len_out)
 {
 	char c_msg_buf[CMSG_SPACE(sizeof(operation))];
 	struct msghdr msg;
@@ -198,7 +214,7 @@ static bool operate_cipher(int sk, __u32 operation,
 	memcpy(CMSG_DATA(c_msg), &operation, sizeof(operation));
 
 	iov.iov_base = (void *) in;
-	iov.iov_len = len;
+	iov.iov_len = len_in;
 
 	msg.msg_iov = &iov;
 	msg.msg_iovlen = 1;
@@ -206,7 +222,7 @@ static bool operate_cipher(int sk, __u32 operation,
 	if (sendmsg(sk, &msg, 0) < 0)
 		return false;
 
-	if (read(sk, out, len) < 0)
+	if (read(sk, out, len_out) < 0)
 		return false;
 
 	return true;
@@ -221,7 +237,8 @@ LIB_EXPORT bool l_cipher_encrypt(struct l_cipher *cipher,
 	if (unlikely(!in) || unlikely(!out))
 		return false;
 
-	return operate_cipher(cipher->encrypt_sk, ALG_OP_ENCRYPT, in, out, len);
+	return operate_cipher(cipher->encrypt_sk, ALG_OP_ENCRYPT, in, out, len,
+				len);
 }
 
 LIB_EXPORT bool l_cipher_decrypt(struct l_cipher *cipher,
@@ -233,7 +250,8 @@ LIB_EXPORT bool l_cipher_decrypt(struct l_cipher *cipher,
 	if (unlikely(!in) || unlikely(!out))
 		return false;
 
-	return operate_cipher(cipher->decrypt_sk, ALG_OP_DECRYPT, in, out, len);
+	return operate_cipher(cipher->decrypt_sk, ALG_OP_DECRYPT, in, out, len,
+				len);
 }
 
 LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t *iv,
@@ -275,6 +293,7 @@ LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t *iv,
 struct l_asymmetric_cipher {
 	struct l_cipher cipher;
 	int key_size;
+	bool public_key;
 };
 
 static inline int parse_asn1_definite_length(const uint8_t **buf,
@@ -334,20 +353,31 @@ static bool parse_rsa_key(struct l_asymmetric_cipher *cipher, const void *key,
 	 * and cache the size of the modulus n for later use.
 	 * (RFC3279)
 	 */
+	size_t seq_length;
 	size_t n_length;
+	uint8_t *seq;
 	uint8_t *der;
 	uint8_t tag;
+	bool pubkey = true;
 
 	if (key_length < 8)
 		return false;
 
 	/* Unpack the outer SEQUENCE */
-	der = der_find_elem((uint8_t *) key, key_length, 0, &tag, &n_length);
-	if (!der || tag != ASN1_ID_SEQUENCE)
+	seq = der_find_elem((uint8_t *) key, key_length, 0, &tag, &seq_length);
+	if (!seq || tag != ASN1_ID_SEQUENCE)
 		return false;
 
-	/* Take first INTEGER as the modulus */
-	der = der_find_elem(der, n_length, 0, &tag, &n_length);
+	/* First INTEGER may be a 1-byte version (for private key) or
+	 * the modulus (public key)
+	 */
+	der = der_find_elem(seq, seq_length, 0, &tag, &n_length);
+	if (der && tag == ASN1_ID_INTEGER && n_length == 1) {
+		/* Found version number, implies this is a private key. */
+		der = der_find_elem(seq, seq_length, 1, &tag, &n_length);
+		pubkey = false;
+	}
+
 	if (!der || tag != ASN1_ID_INTEGER || n_length < 4)
 		return false;
 
@@ -358,95 +388,11 @@ static bool parse_rsa_key(struct l_asymmetric_cipher *cipher, const void *key,
 	}
 
 	cipher->key_size = n_length;
+	cipher->public_key = pubkey;
 
 	return true;
 }
 
-static void write_asn1_definite_length(uint8_t **buf, size_t len)
-{
-	int n;
-
-	if (len < 0x80) {
-		*(*buf)++ = len;
-
-		return;
-	}
-
-	for (n = 1; len >> (n * 8); n++);
-	*(*buf)++ = 0x80 | n;
-
-	while (n--)
-		*(*buf)++ = len >> (n * 8);
-}
-
-/*
- * Extract a ASN1 RsaKey-formatted public+private key structure in the
- * form used in the kernel.  It is simpler than the PKCS#1 form as it only
- * contains the N, E and D integers and also correctly parses as a PKCS#1
- * RSAPublicKey.
- */
-uint8_t *extract_rsakey(uint8_t *pkcs1_key, size_t pkcs1_key_len,
-			size_t *out_len)
-{
-	uint8_t *key, *ptr, *ver, *n, *e, *d;
-	uint8_t tag;
-	size_t ver_len, n_len, e_len, d_len;
-	int pos;
-
-	/* Unpack the outer SEQUENCE */
-	pkcs1_key = der_find_elem(pkcs1_key, pkcs1_key_len, 0, &tag,
-					&pkcs1_key_len);
-	if (!pkcs1_key || tag != ASN1_ID_SEQUENCE)
-		return NULL;
-
-	/* Check if the version element if present */
-	ver = der_find_elem(pkcs1_key, pkcs1_key_len, 0, &tag, &ver_len);
-	if (!ver || tag != ASN1_ID_INTEGER)
-		return NULL;
-
-	pos = (ver_len == 1 && ver[0] == 0x00) ? 1 : 0;
-
-	n = der_find_elem(pkcs1_key, pkcs1_key_len, pos + 0, &tag, &n_len);
-	if (!n || tag != ASN1_ID_INTEGER)
-		return NULL;
-
-	e = der_find_elem(pkcs1_key, pkcs1_key_len, pos + 1, &tag, &e_len);
-	if (!e || tag != ASN1_ID_INTEGER)
-		return NULL;
-
-	d = der_find_elem(pkcs1_key, pkcs1_key_len, pos + 2, &tag, &d_len);
-	if (!d || tag != ASN1_ID_INTEGER)
-		return NULL;
-
-	/* New SEQUENCE length including tags and lengths */
-	*out_len = 1 + (n_len >= 0x80 ? n_len >= 0x100 ? 3 : 2 : 1) + n_len +
-		1 + (e_len >= 0x80 ? e_len >= 0x100 ? 3 : 2 : 1) + e_len +
-		1 + (d_len >= 0x80 ? d_len >= 0x100 ? 3 : 2 : 1) + d_len;
-	ptr = key = l_malloc(*out_len +
-		1 + (*out_len >= 0x80 ? *out_len >= 0x100 ? 3 : 2 : 1));
-
-	*ptr++ = ASN1_ID_SEQUENCE;
-	write_asn1_definite_length(&ptr, *out_len);
-
-	*ptr++ = ASN1_ID_INTEGER;
-	write_asn1_definite_length(&ptr, n_len);
-	memcpy(ptr, n, n_len);
-	ptr += n_len;
-
-	*ptr++ = ASN1_ID_INTEGER;
-	write_asn1_definite_length(&ptr, e_len);
-	memcpy(ptr, e, e_len);
-	ptr += e_len;
-
-	*ptr++ = ASN1_ID_INTEGER;
-	write_asn1_definite_length(&ptr, d_len);
-	memcpy(ptr, d, d_len);
-	ptr += d_len;
-
-	*out_len = ptr - key;
-	return key;
-}
-
 LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new(
 					enum l_asymmetric_cipher_type type,
 					const void *key, size_t key_length)
@@ -468,19 +414,23 @@ LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new(
 		if (!parse_rsa_key(cipher, key, key_length))
 			goto error_free;
 
-		alg_name = "rsa";
+		alg_name = "pkcs1pad(rsa)";
 		break;
 	}
 
 	cipher->cipher.encrypt_sk = create_alg("akcipher", alg_name,
-						key, key_length);
+						key, key_length,
+						cipher->public_key);
 	if (cipher->cipher.encrypt_sk < 0)
 		goto error_free;
 
-	cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
-						key, key_length);
-	if (cipher->cipher.decrypt_sk < 0)
-		goto error_close;
+	if (!cipher->public_key) {
+		cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
+							key, key_length,
+							cipher->public_key);
+		if (cipher->cipher.decrypt_sk < 0)
+			goto error_close;
+	}
 
 	return cipher;
 
@@ -508,151 +458,58 @@ LIB_EXPORT int l_asymmetric_cipher_get_key_size(
 	return cipher->key_size;
 }
 
-static void getrandom_nonzero(uint8_t *buf, int len)
-{
-	while (len--) {
-		l_getrandom(buf, 1);
-		while (buf[0] == 0)
-			l_getrandom(buf, 1);
-
-		buf++;
-	}
-}
-
-LIB_EXPORT bool l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *cipher,
+LIB_EXPORT bool l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *acipher,
 					const void *in, void *out,
 					size_t len_in, size_t len_out)
 {
-	if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) {
-		/* PKCS#1 v1.5 RSA padding according to RFC3447 */
-		uint8_t buf[cipher->key_size];
-		int ps_len = cipher->key_size - len_in - 3;
-
-		if (len_in > (size_t) cipher->key_size - 11)
-			return false;
-		if (len_out != (size_t) cipher->key_size)
-			return false;
-
-		buf[0] = 0x00;
-		buf[1] = 0x02;
-		getrandom_nonzero(buf + 2, ps_len);
-		buf[ps_len + 2] = 0x00;
-		memcpy(buf + ps_len + 3, in, len_in);
-
-		if (!l_cipher_encrypt(&cipher->cipher, buf, out,
-					cipher->key_size))
-			return false;
-	}
+	if (unlikely(!acipher))
+		return false;
 
-	return true;
+	if (unlikely(!in) || unlikely(!out))
+		return false;
+
+	return operate_cipher(acipher->cipher.encrypt_sk, ALG_OP_ENCRYPT,
+				in, out, len_in, len_out);
 }
 
-LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher,
+LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *acipher,
 					const void *in, void *out,
 					size_t len_in, size_t len_out)
 {
-	if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) {
-		/* PKCS#1 v1.5 RSA padding according to RFC3447 */
-		uint8_t buf[cipher->key_size];
-		int pos;
-
-		if (len_in != (size_t) cipher->key_size)
-			return false;
-
-		if (!l_cipher_decrypt(&cipher->cipher, in, buf,
-					cipher->key_size))
-			return false;
-
-		if (buf[0] != 0x00)
-			return false;
-		if (buf[1] != 0x02)
-			return false;
-
-		for (pos = 2; pos < cipher->key_size; pos++)
-			if (buf[pos] == 0)
-				break;
-		if (pos < 10 || pos == cipher->key_size)
-			return false;
-
-		pos++;
-		if (len_out != (size_t) cipher->key_size - pos)
-			return false;
-
-		memcpy(out, buf + pos, cipher->key_size - pos);
-	}
+	if (unlikely(!acipher))
+		return false;
 
-	return true;
+	if (unlikely(!in) || unlikely(!out))
+		return false;
+
+	return operate_cipher(acipher->cipher.decrypt_sk, ALG_OP_DECRYPT,
+				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)
 {
-	if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) {
-		/* PKCS#1 v1.5 RSA padding according to RFC3447 */
-		uint8_t buf[cipher->key_size];
-		int ps_len = cipher->key_size - len_in - 3;
-
-		if (len_in > (size_t) cipher->key_size - 11)
-			return false;
-		if (len_out != (size_t) cipher->key_size)
-			return false;
-
-		buf[0] = 0x00;
-		buf[1] = 0x01;
-		memset(buf + 2, 0xff, ps_len);
-		buf[ps_len + 2] = 0x00;
-		memcpy(buf + ps_len + 3, in, len_in);
-
-		/*
-		 * The RSA signing operation uses the same primitive as
-		 * decryption so just call decrypt for now.
-		 */
-		if (!l_cipher_decrypt(&cipher->cipher, buf, out,
-					cipher->key_size))
-			return false;
-	}
+	if (unlikely(!acipher))
+		return false;
 
-	return true;
+	if (unlikely(!in) || unlikely(!out))
+		return false;
+
+	return operate_cipher(acipher->cipher.decrypt_sk, ALG_OP_SIGN,
+				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)
 {
-	if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) {
-		/* PKCS#1 v1.5 RSA padding according to RFC3447 */
-		uint8_t buf[cipher->key_size];
-		int pos;
-
-		if (len_in != (size_t) cipher->key_size)
-			return false;
-
-		/*
-		 * The RSA verify operation uses the same primitive as
-		 * encryption so just call encrypt.
-		 */
-		if (!l_cipher_encrypt(&cipher->cipher, in, buf,
-					cipher->key_size))
-			return false;
-
-		if (buf[0] != 0x00)
-			return false;
-		if (buf[1] != 0x01)
-			return false;
-
-		for (pos = 2; pos < cipher->key_size; pos++)
-			if (buf[pos] != 0xff)
-				break;
-		if (pos < 10 || pos == cipher->key_size || buf[pos] != 0)
-			return false;
-
-		pos++;
-		if (len_out != (size_t) cipher->key_size - pos)
-			return false;
-
-		memcpy(out, buf + pos, cipher->key_size - pos);
-	}
+	if (unlikely(!acipher))
+		return false;
 
-	return true;
+	if (unlikely(!in) || unlikely(!out))
+		return false;
+
+	return operate_cipher(acipher->cipher.encrypt_sk, ALG_OP_VERIFY,
+				in, out, len_in, len_out);
 }
diff --git a/ell/tls.c b/ell/tls.c
index a20236f..a55f24c 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -920,8 +920,8 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
 				tls_get_hash_t get_hash)
 {
 	struct l_asymmetric_cipher *rsa_privkey;
-	uint8_t *privkey, *privkey_short;
-	size_t key_size, short_size;
+	uint8_t *privkey;
+	size_t key_size;
 	bool result;
 	const struct tls_hash_algorithm *hash_type;
 	uint8_t hash[HANDSHAKE_HASH_MAX_SIZE];
@@ -945,19 +945,9 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
 		return false;
 	}
 
-	privkey_short = extract_rsakey(privkey, key_size, &short_size);
-	tls_free_key(privkey, key_size);
-
-	if (!privkey_short) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-				TLS_ALERT_BAD_CERT);
-
-		return false;
-	}
-
 	rsa_privkey = l_asymmetric_cipher_new(L_CIPHER_RSA_PKCS1_V1_5,
-						privkey_short, short_size);
-	tls_free_key(privkey_short, short_size);
+						privkey, key_size);
+	tls_free_key(privkey, key_size);
 
 	if (!rsa_privkey) {
 		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
@@ -1080,11 +1070,14 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 		 */
 	}
 
-	digest_info = alloca(expected_len);
+	if (expected_len > key_size)
+		goto err_free_rsa;
+
+	digest_info = alloca(key_size);
 
 	result = l_asymmetric_cipher_verify(rsa_client_pubkey, in + 4,
 						digest_info,
-						key_size, expected_len);
+						key_size, key_size);
 
 	l_asymmetric_cipher_free(rsa_client_pubkey);
 
@@ -1743,8 +1736,8 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
 {
 	uint8_t pre_master_secret[48], random_secret[46];
 	struct l_asymmetric_cipher *rsa_server_privkey;
-	uint8_t *privkey, *privkey_short;
-	size_t key_size, short_size;
+	uint8_t *privkey;
+	size_t key_size;
 	bool result;
 
 	if (!tls->priv_key_path) {
@@ -1764,19 +1757,9 @@ static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
 		return;
 	}
 
-	privkey_short = extract_rsakey(privkey, key_size, &short_size);
-	tls_free_key(privkey, key_size);
-
-	if (!privkey_short) {
-		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR,
-				TLS_ALERT_BAD_CERT);
-
-		return;
-	}
-
 	rsa_server_privkey = l_asymmetric_cipher_new(L_CIPHER_RSA_PKCS1_V1_5,
-						privkey_short, short_size);
-	tls_free_key(privkey_short, short_size);
+							privkey, key_size);
+	tls_free_key(privkey, key_size);
 
 	if (!rsa_server_privkey) {
 		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
-- 
2.8.4

[PATCH v3 2/5] unit: Update for akcipher changes

[Mat Martineau]

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

New private key, and no need to extract key elements.
---
 unit/test-cipher.c | 130 +++++++++++++++++++++++------------------------------
 1 file changed, 55 insertions(+), 75 deletions(-)

diff --git a/unit/test-cipher.c b/unit/test-cipher.c
index 3dc2fe0..2527dcc 100644
--- a/unit/test-cipher.c
+++ b/unit/test-cipher.c
@@ -124,96 +124,76 @@ static void test_arc4(const void *data)
 }
 
 /*
- * openssl genrsa 1024 | openssl rsa -outform DER | \
- *	hexdump -v -e '"0x" 1/1 "%02x" ", "'
+ * openssl genrsa 1024 | openssl rsa -outform DER | xxd -i
  */
 static uint8_t rsa_priv_key[] = {
-	0x30, 0x82, 0x02, 0x5c, /* SEQUENCE */
-	0x02, 0x01, /* INTEGER - version */
-	0x00,
-	0x02, 0x81, 0x81, /* INTEGER - n */
-	0x00, 0xbe, 0x8c, 0xe7, 0x6c, 0x7d, 0x97, 0xbd, 0x63, 0xf6, 0x00, 0x65,
-	0x34, 0x23, 0xd9, 0xc6, 0xce, 0x6d, 0x1b, 0x30, 0x8f, 0xfa, 0x9b, 0x8e,
-	0xbe, 0x97, 0x01, 0x10, 0x5b, 0x49, 0x7c, 0xba, 0xd5, 0x47, 0x5e, 0xb0,
-	0x62, 0x24, 0xd9, 0xb0, 0xe2, 0x35, 0x5b, 0x05, 0xeb, 0x0a, 0x69, 0xc9,
-	0x5c, 0x02, 0x6d, 0xf2, 0x89, 0x6d, 0xab, 0xaa, 0x43, 0x53, 0x52, 0xaa,
-	0xed, 0xc9, 0xb6, 0x98, 0x17, 0x8a, 0x38, 0x9a, 0x80, 0x5b, 0xd1, 0x70,
-	0xc7, 0xc9, 0x16, 0x1f, 0x34, 0xb3, 0x27, 0x40, 0xa2, 0x39, 0xbe, 0xf8,
-	0x01, 0xfa, 0x9e, 0x04, 0x9f, 0x58, 0xc1, 0xa1, 0x19, 0x78, 0xcf, 0xfe,
-	0x9c, 0x76, 0x43, 0xba, 0x69, 0x05, 0x67, 0x9a, 0xfc, 0xc4, 0x56, 0xbb,
-	0xc1, 0x7f, 0xdb, 0xf0, 0xb9, 0x31, 0x2c, 0x4f, 0xc2, 0xd9, 0x42, 0xf6,
-	0x3d, 0x1b, 0x48, 0x31, 0x88, 0xc2, 0xb5, 0xb9, 0xa5,
-	0x02, 0x03, /* INTEGER - e */
-	0x01, 0x00, 0x01,
-	0x02, 0x81, 0x80, /* INTEGER - d */
-	0x0b, 0x34, 0xa2, 0x0f, 0x61, 0x3b, 0x61, 0x29, 0xd5, 0xb7, 0xa4, 0x3b,
-	0xf2, 0xb7, 0xc5, 0xd7, 0x31, 0xd7, 0x5d, 0x7d, 0xba, 0x11, 0x17, 0xcd,
-	0xe1, 0x77, 0x70, 0x8c, 0xcd, 0xbf, 0x86, 0x05, 0x30, 0xd3, 0x42, 0xb0,
-	0x22, 0xd6, 0xa2, 0x6e, 0x4b, 0x10, 0xf5, 0x42, 0x23, 0x34, 0xa1, 0x60,
-	0xc5, 0xcb, 0xcd, 0x6d, 0x83, 0x83, 0x8a, 0xd9, 0xb6, 0xb6, 0xaf, 0xd2,
-	0x98, 0x00, 0x22, 0xe5, 0x75, 0x67, 0xbe, 0x0b, 0x95, 0xe7, 0x76, 0xee,
-	0x22, 0x03, 0xdd, 0x49, 0x02, 0x86, 0xd7, 0x2b, 0x96, 0xee, 0xb1, 0x1e,
-	0x49, 0x02, 0xdf, 0xa9, 0x2e, 0x0f, 0x71, 0x33, 0xb5, 0xc6, 0x37, 0xb5,
-	0x6b, 0x58, 0xa1, 0xbb, 0x1c, 0xb9, 0xa2, 0x33, 0x70, 0x01, 0xca, 0xd4,
-	0xae, 0x96, 0xc3, 0x7d, 0x36, 0xc2, 0xe7, 0x50, 0x33, 0x17, 0xcc, 0xdc,
-	0x28, 0x7c, 0x7b, 0xdf, 0x25, 0x06, 0x57, 0xa9,
-	0x02, 0x41, /* INTEGER - p */
-	0x00, 0xf1, 0xec, 0x4f, 0xdf, 0xb7, 0x05, 0x2e, 0xdd, 0x1c, 0x85, 0x9d,
-	0x99, 0xb7, 0x60, 0x8c, 0x9b, 0x47, 0x1c, 0x04, 0x2f, 0x6f, 0x8a, 0xd5,
-	0x89, 0x1c, 0x2e, 0xe2, 0x0d, 0x68, 0x9c, 0xf6, 0xb0, 0x09, 0x32, 0x70,
-	0x06, 0x9f, 0x52, 0x7e, 0xc5, 0x46, 0x93, 0x60, 0x57, 0x77, 0xa8, 0xd3,
-	0x92, 0xbb, 0x24, 0x9c, 0x24, 0x35, 0x14, 0x99, 0x62, 0xe2, 0xce, 0xa2,
-	0x9b, 0x62, 0xd3, 0xe0, 0x0f,
-	0x02, 0x41, /* INTEGER - q */
-	0x00, 0xc9, 0xa3, 0x58, 0x17, 0x03, 0xfb, 0x19, 0x06, 0xa4, 0x75, 0xfb,
-	0xa2, 0xb3, 0x84, 0x2a, 0x7f, 0xf7, 0x66, 0x80, 0xb5, 0xd6, 0xda, 0xcf,
-	0xb2, 0x9e, 0x4f, 0x0a, 0xd3, 0xcd, 0xa5, 0x94, 0xc4, 0x80, 0x53, 0xc6,
-	0xbf, 0x7e, 0xe0, 0x2c, 0xc0, 0x71, 0xdb, 0x9c, 0xe0, 0x92, 0x66, 0x41,
-	0x6b, 0x64, 0x9e, 0x7a, 0xf3, 0xe7, 0x45, 0x15, 0x3f, 0xe4, 0xbc, 0xc1,
-	0x91, 0x40, 0x2a, 0x57, 0x0b,
-	0x02, 0x41, /* INTEGER - d mod (p - 1) */
-	0x00, 0x88, 0x9f, 0x4f, 0x00, 0x65, 0x68, 0x9c, 0xed, 0xac, 0x14, 0xdd,
-	0x4b, 0x19, 0x1f, 0x82, 0x68, 0x92, 0xc1, 0x04, 0xb0, 0x11, 0x4b, 0x13,
-	0x8a, 0xaa, 0x0a, 0xe4, 0x08, 0x74, 0x82, 0xe8, 0x61, 0xc3, 0xdf, 0xe3,
-	0x1a, 0x2a, 0x51, 0xb9, 0x5c, 0x09, 0x9e, 0x63, 0x33, 0x22, 0x55, 0x8a,
-	0x9e, 0x7b, 0xe7, 0x91, 0xf2, 0x74, 0xb3, 0x9c, 0x68, 0x16, 0xf4, 0x61,
-	0x2a, 0x65, 0xa6, 0x88, 0x0b,
-	0x02, 0x40, /* INTEGER - d mod (q - 1) */
-	0x56, 0x3c, 0xab, 0x07, 0x24, 0xe7, 0xb6, 0x5b, 0x55, 0xe9, 0x33, 0xd6,
-	0xf1, 0x09, 0xfc, 0x97, 0x40, 0x3b, 0x31, 0x9f, 0x13, 0xa5, 0xff, 0xa0,
-	0x77, 0xfe, 0x7c, 0x35, 0xfb, 0xc4, 0xee, 0x6c, 0x60, 0x29, 0xf4, 0x5d,
-	0xa0, 0x28, 0xc6, 0x5b, 0x04, 0x17, 0x15, 0xf0, 0x22, 0x0c, 0xe3, 0xbb,
-	0xc7, 0x8b, 0xd4, 0x30, 0x0e, 0x60, 0x48, 0x67, 0x4c, 0x2f, 0xc2, 0x65,
-	0x99, 0xd8, 0xc1, 0xe3,
-	0x02, 0x40, /* INTEGER - inv q mod p */
-	0x12, 0x89, 0x47, 0x76, 0xb6, 0xbb, 0x18, 0x64, 0xc3, 0x19, 0xe7, 0xa9,
-	0x93, 0xce, 0xec, 0xd6, 0x4b, 0xaa, 0xad, 0x46, 0x4f, 0x24, 0x0d, 0xf3,
-	0xbf, 0xc2, 0x7c, 0x95, 0x3e, 0xc9, 0x27, 0x8a, 0x4b, 0xbc, 0x9e, 0x56,
-	0x2a, 0x8c, 0x07, 0x5f, 0xb6, 0x07, 0x21, 0xb7, 0xc9, 0xae, 0x8c, 0x58,
-	0x04, 0x65, 0x02, 0xa2, 0x08, 0x03, 0x40, 0xf3, 0x71, 0x8c, 0x89, 0xf7,
-	0xaa, 0xb0, 0x6a, 0x75,
+	0x30, 0x82, 0x02, 0x5e, 0x02, 0x01, 0x00, 0x02, 0x81, 0x81, 0x00, 0xe1,
+	0xec, 0x78, 0x3c, 0x5f, 0x62, 0x74, 0x1e, 0x6d, 0x1d, 0x44, 0xac, 0x40,
+	0xb3, 0xec, 0x01, 0x96, 0x01, 0x8a, 0xfe, 0xcf, 0x5d, 0xc5, 0xe6, 0x0c,
+	0x36, 0x03, 0x2c, 0x4e, 0x84, 0x8f, 0x51, 0xf3, 0xc5, 0x32, 0x4f, 0xc4,
+	0x73, 0x22, 0x92, 0x30, 0x7c, 0x75, 0xd7, 0x4b, 0xae, 0xc6, 0xd0, 0x59,
+	0x6b, 0xd8, 0x46, 0x79, 0xbc, 0x6a, 0x6e, 0xde, 0x27, 0x11, 0x2f, 0xde,
+	0x84, 0xe3, 0x64, 0x84, 0x07, 0x82, 0x83, 0xbf, 0x90, 0xf5, 0x80, 0x6f,
+	0x63, 0x3a, 0xd1, 0x74, 0xd5, 0x6d, 0x2f, 0xde, 0xdc, 0xea, 0xab, 0xe5,
+	0x20, 0x7d, 0x26, 0x3e, 0x20, 0x99, 0x97, 0x41, 0x47, 0x81, 0x04, 0x7e,
+	0x53, 0x5c, 0xb2, 0xa9, 0xe0, 0x3d, 0x72, 0x37, 0x85, 0xcc, 0x5c, 0xda,
+	0x04, 0x96, 0xfa, 0x02, 0xc2, 0x23, 0x8b, 0x20, 0x5d, 0xe1, 0x2a, 0x69,
+	0xec, 0xcd, 0xce, 0x85, 0xc2, 0xf5, 0x49, 0x02, 0x03, 0x01, 0x00, 0x01,
+	0x02, 0x81, 0x81, 0x00, 0xa5, 0x31, 0x72, 0xf9, 0x32, 0x05, 0x9b, 0x42,
+	0x64, 0x26, 0x72, 0x80, 0x41, 0x0f, 0x4e, 0x12, 0x1a, 0xcd, 0x26, 0x05,
+	0x0b, 0x3b, 0x55, 0xe8, 0xd0, 0x24, 0xee, 0x4d, 0x07, 0x5c, 0x86, 0x2f,
+	0x36, 0x3f, 0x8a, 0x7a, 0x28, 0xfa, 0xc6, 0xdc, 0x7d, 0xf7, 0x83, 0x72,
+	0xd9, 0x34, 0x02, 0xcb, 0x75, 0x97, 0x15, 0x9c, 0xf2, 0x86, 0x82, 0x8c,
+	0x6e, 0x83, 0xc2, 0x5d, 0x6e, 0x27, 0x5c, 0xdc, 0x52, 0xb8, 0x8d, 0xa8,
+	0x0d, 0x09, 0xcf, 0x69, 0xae, 0x61, 0x0e, 0xcb, 0x6a, 0x76, 0xac, 0xdd,
+	0x85, 0xda, 0x9c, 0xac, 0x2b, 0xf0, 0xf6, 0x2e, 0x2e, 0x4d, 0x9b, 0xc7,
+	0x67, 0xc2, 0xfa, 0x7b, 0x0e, 0x68, 0xf7, 0x1e, 0x03, 0x28, 0xea, 0x0e,
+	0x9a, 0xd6, 0xc3, 0x28, 0x3d, 0xde, 0x11, 0x26, 0xb1, 0x95, 0xf6, 0x10,
+	0x2f, 0x81, 0xa5, 0x60, 0x2c, 0x4f, 0x37, 0x5c, 0x2a, 0xd2, 0x30, 0x01,
+	0x02, 0x41, 0x00, 0xf2, 0x80, 0xa2, 0x57, 0x5c, 0xe0, 0x41, 0x82, 0x00,
+	0xac, 0x0b, 0xbd, 0xad, 0x98, 0x04, 0x33, 0x49, 0x64, 0x0b, 0x94, 0x94,
+	0xc3, 0xd7, 0xd9, 0xfe, 0x1f, 0xa3, 0xd1, 0x83, 0x42, 0x3a, 0x2d, 0xaf,
+	0xc5, 0x4c, 0xa4, 0x1b, 0xe4, 0x1c, 0x9c, 0x17, 0x8e, 0x28, 0xe9, 0xa5,
+	0xd4, 0xbd, 0x9a, 0xce, 0x6e, 0x33, 0xb4, 0xaf, 0xce, 0x13, 0xd2, 0xab,
+	0x0c, 0x4b, 0x34, 0x0d, 0x03, 0x87, 0xa1, 0x02, 0x41, 0x00, 0xee, 0x7f,
+	0x9b, 0xb4, 0x3c, 0x21, 0x76, 0xf2, 0x0c, 0xdf, 0xb6, 0xea, 0xc9, 0x31,
+	0xd4, 0xeb, 0x8f, 0x46, 0x41, 0x9b, 0xc1, 0x60, 0x4f, 0x50, 0x54, 0x32,
+	0xd2, 0xf4, 0xfd, 0xd0, 0xc8, 0x58, 0x6d, 0x17, 0x4e, 0xac, 0x5f, 0x9e,
+	0xb7, 0xd4, 0xfc, 0xce, 0xe0, 0x92, 0x0e, 0x1d, 0xd1, 0xa7, 0x54, 0xd3,
+	0x98, 0xca, 0x5b, 0x9c, 0x41, 0x68, 0xbf, 0x0d, 0x1b, 0xe2, 0xdb, 0xa6,
+	0xec, 0xa9, 0x02, 0x40, 0x0b, 0xc1, 0x72, 0x9d, 0x3b, 0x92, 0x5f, 0x7a,
+	0x96, 0xdf, 0xc0, 0x3d, 0xf4, 0xb1, 0x5e, 0xda, 0xc1, 0x9f, 0x08, 0xf4,
+	0xad, 0xf5, 0x84, 0x7c, 0x3b, 0xd6, 0x7a, 0xd1, 0x88, 0x44, 0x68, 0x9f,
+	0x98, 0x5a, 0xbf, 0x29, 0x61, 0x74, 0xc0, 0x72, 0x4c, 0xae, 0x06, 0x8b,
+	0xb5, 0x0f, 0x48, 0x15, 0xbe, 0x16, 0x17, 0x89, 0x95, 0xd0, 0x2e, 0xa3,
+	0xd2, 0xc8, 0xe8, 0xc8, 0x60, 0x2d, 0x20, 0xa1, 0x02, 0x41, 0x00, 0xdb,
+	0x39, 0xbf, 0x14, 0xf8, 0x24, 0xc6, 0xa2, 0x0d, 0xc5, 0x61, 0xed, 0x05,
+	0x0d, 0x62, 0x2b, 0x38, 0xe2, 0x9a, 0x92, 0x22, 0x39, 0x76, 0x0e, 0x5f,
+	0xa6, 0xec, 0x14, 0xb8, 0x6e, 0x3e, 0x8a, 0x51, 0x94, 0x98, 0x03, 0x88,
+	0x4d, 0x6b, 0xab, 0x42, 0xca, 0xa2, 0xd0, 0x7e, 0x5b, 0x58, 0x88, 0x98,
+	0x47, 0x7b, 0xed, 0x9e, 0x31, 0xce, 0x4a, 0x0b, 0x3b, 0x70, 0x83, 0xa1,
+	0xe6, 0x19, 0x29, 0x02, 0x41, 0x00, 0x9c, 0x88, 0xbb, 0x56, 0x6b, 0x4a,
+	0x81, 0x2c, 0xb3, 0x70, 0xdc, 0xf5, 0x65, 0x45, 0xd4, 0xed, 0xdd, 0xc3,
+	0xdc, 0xc5, 0x27, 0xa3, 0xa0, 0x66, 0x5c, 0x51, 0xeb, 0x52, 0x8c, 0x8d,
+	0x4e, 0xa6, 0x8f, 0x42, 0x5d, 0xb8, 0xa4, 0xa4, 0x26, 0xf3, 0xd6, 0xe5,
+	0x01, 0x6b, 0x51, 0x8a, 0xa4, 0xee, 0xec, 0xff, 0x71, 0x8c, 0xbb, 0xba,
+	0x05, 0x3e, 0x55, 0x14, 0xd9, 0xe4, 0xa4, 0x7f, 0xb7, 0x4f
 };
 
 static void test_rsa(const void *data)
 {
 	struct l_asymmetric_cipher *cipher;
 	char buf[128];
-	uint8_t *short_key;
-	size_t short_key_len;
-
-	short_key = extract_rsakey(rsa_priv_key, sizeof(rsa_priv_key),
-					&short_key_len);
 
 	cipher = l_asymmetric_cipher_new(L_CIPHER_RSA_PKCS1_V1_5,
-						short_key, short_key_len);
-	l_free(short_key);
-
+						rsa_priv_key,
+						sizeof(rsa_priv_key));
 	assert(cipher);
 	assert(l_asymmetric_cipher_encrypt(cipher, FIXED_STR, buf, 100, 128));
 
 	assert(memcmp(FIXED_STR, buf, 100));
 
-	assert(l_asymmetric_cipher_decrypt(cipher, buf, buf, 128, 100));
+	assert(l_asymmetric_cipher_decrypt(cipher, buf, buf, 128, 128));
 
 	assert(!memcmp(FIXED_STR, buf, 100));
 
-- 
2.8.4

[PATCH v3 3/5] cipher: Return result length from asymmetric cipher

[Mat Martineau]

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

Short reads from AF_ALG akcipher sockets can return an error rather
than a partial buffer so it's important to provide both an output
buffer length (which is typically longer than the expected data), and
to find out how many bytes were read.
---
 ell/cipher.c      | 31 +++++++++-----------
 ell/cipher.h      | 16 +++++------
 ell/tls-private.h |  2 +-
 ell/tls.c         | 86 ++++++++++++++++++++++++++++++++++++-------------------
 4 files changed, 80 insertions(+), 55 deletions(-)

diff --git a/ell/cipher.c b/ell/cipher.c
index f23bdda..e370a0e 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)
 {
@@ -222,10 +222,7 @@ static bool operate_cipher(int sk, __u32 operation,
 	if (sendmsg(sk, &msg, 0) < 0)
 		return false;
 
-	if (read(sk, out, len_out) < 0)
-		return false;
-
-	return true;
+	return read(sk, out, len_out);
 }
 
 LIB_EXPORT bool l_cipher_encrypt(struct l_cipher *cipher,
@@ -458,9 +455,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 *acipher,
-					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 *acipher,
+						const void *in, void *out,
+						size_t len_in, size_t len_out)
 {
 	if (unlikely(!acipher))
 		return false;
@@ -472,9 +469,9 @@ LIB_EXPORT bool l_asymmetric_cipher_encrypt(struct l_asymmetric_cipher *acipher,
 				in, out, len_in, len_out);
 }
 
-LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *acipher,
-					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 *acipher,
+						const void *in, void *out,
+						size_t len_in, size_t len_out)
 {
 	if (unlikely(!acipher))
 		return false;
@@ -486,9 +483,9 @@ LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *acipher,
 				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 *acipher,
+						const void *in, void *out,
+						size_t len_in, size_t len_out)
 {
 	if (unlikely(!acipher))
 		return false;
@@ -500,9 +497,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 *acipher,
+						const void *in, void *out,
+						size_t len_in, size_t len_out)
 {
 	if (unlikely(!acipher))
 		return false;
diff --git a/ell/cipher.h b/ell/cipher.h
index 329f667..a65903e 100644
--- a/ell/cipher.h
+++ b/ell/cipher.h
@@ -64,21 +64,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-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);
 	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 a55f24c..59fe0d0 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);
@@ -858,10 +858,10 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 {
 	uint8_t buf[1024 + 32];
 	uint8_t *ptr = buf + TLS_HANDSHAKE_HEADER_SIZE;
-	uint8_t pre_master_secret[48];
+	uint8_t *pre_master_secret;
 	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);
@@ -869,10 +869,6 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 		return false;
 	}
 
-	pre_master_secret[0] = (uint8_t) (TLS_VERSION >> 8);
-	pre_master_secret[1] = (uint8_t) (TLS_VERSION >> 0);
-	l_getrandom(pre_master_secret + 2, 46);
-
 	/* Fill in the RSA Client Key Exchange body */
 
 	rsa_server_pubkey = l_asymmetric_cipher_new(L_CIPHER_RSA_PKCS1_V1_5,
@@ -894,16 +890,23 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 		return false;
 	}
 
+	pre_master_secret = l_malloc(key_size);
+
+	pre_master_secret[0] = (uint8_t) (TLS_VERSION >> 8);
+	pre_master_secret[1] = (uint8_t) (TLS_VERSION >> 0);
+	l_getrandom(pre_master_secret + 2, 46);
+
 	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);
+		l_free(pre_master_secret);
 
 		return false;
 	}
@@ -912,22 +915,30 @@ static bool tls_send_rsa_client_key_xchg(struct l_tls *tls)
 
 	tls_generate_master_secret(tls, pre_master_secret, 48);
 	memset(pre_master_secret, 0, 48);
+	l_free(pre_master_secret);
 
 	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;
 	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];
 	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);
@@ -967,16 +978,25 @@ static bool tls_rsa_sign(struct l_tls *tls, uint8_t **out,
 
 		*(*out)++ = hash_type->tls_id;
 		*(*out)++ = 1;	/* RSA_sign */
+		len -= 2;
 	} 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;
+	if (len >= key_size + 2) {
+		l_put_be16(key_size, *out);
+		sign_output_len = l_asymmetric_cipher_sign(rsa_privkey,
+							   sign_input, *out + 2,
+							   sign_input_len,
+							   key_size);
+		*out += sign_output_len + 2;
+		result = sign_output_len == (ssize_t)key_size;
+	} else {
+		tls_disconnect(tls, TLS_ALERT_INTERNAL_ERROR, 0);
+		result = false;
+	}
 
 	l_asymmetric_cipher_free(rsa_privkey);
 
@@ -988,7 +1008,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;
@@ -1075,13 +1095,14 @@ static bool tls_rsa_verify(struct l_tls *tls, const uint8_t *in, size_t len,
 
 	digest_info = alloca(key_size);
 
-	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,
+							key_size);
 
 	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;
@@ -1141,7 +1162,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. */
@@ -1734,11 +1756,12 @@ static void tls_handle_server_hello_done(struct l_tls *tls,
 static void tls_handle_rsa_client_key_xchg(struct l_tls *tls,
 						const uint8_t *buf, size_t len)
 {
-	uint8_t pre_master_secret[48], random_secret[46];
+	uint8_t *pre_master_secret;
+	uint8_t random_secret[46];
 	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,
@@ -1787,9 +1810,12 @@ 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);
+	pre_master_secret = l_malloc(key_size);
+
+	bytes_decrypted = l_asymmetric_cipher_decrypt(rsa_server_privkey,
+							buf + 2,
+							pre_master_secret,
+							key_size, key_size);
 	l_asymmetric_cipher_free(rsa_server_privkey);
 
 	/*
@@ -1807,12 +1833,14 @@ 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);
-	memset(pre_master_secret, 0, 48);
+	memset(pre_master_secret, 0, key_size);
 	memset(random_secret, 0, 46);
+
+	l_free(pre_master_secret);
 }
 
 static bool tls_get_prev_digest_by_id(struct l_tls *tls, uint8_t hash_id,
-- 
2.8.4

[PATCH v3 4/5] unit: Check asymmetric cipher result lengths

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 985 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 2527dcc..23021bf 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));
 	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.8.4

[PATCH v3 5/5] unit: Check decryption against known ciphertext

[Mat Martineau]

[-- 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 23021bf..30fe64a 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.8.4

Re: [PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[Denis Kenzior]

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

Hi Mat,

On 06/10/2016 12:09 PM, Mat Martineau wrote:
> There are some significant differences in the current iteration of
> kernel AF_ALG akcipher support:
>   * Kernel handles padding
>   * Kernel accepts DER-encoded certs as-is (no need to extract values)
>   * Must explicitly set private or public key
> ---
>   ell/cipher-private.h |   3 -
>   ell/cipher.c         | 311 ++++++++++++++-------------------------------------
>   ell/tls.c            |  43 +++----
>   3 files changed, 97 insertions(+), 260 deletions(-)
>

<snip>

>   LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t *iv,
> @@ -275,6 +293,7 @@ LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t *iv,
>   struct l_asymmetric_cipher {
>   	struct l_cipher cipher;
>   	int key_size;
> +	bool public_key;

Do we really need this variable?  It seems that we only use it inside 
l_asymmetric_cipher_new.  In which case we can have a local variable to 
that function and have parse_rsa_key have an out parameter for this.

>   };
>
>   static inline int parse_asn1_definite_length(const uint8_t **buf,
> @@ -334,20 +353,31 @@ static bool parse_rsa_key(struct l_asymmetric_cipher *cipher, const void *key,
>   	 * and cache the size of the modulus n for later use.
>   	 * (RFC3279)
>   	 */
> +	size_t seq_length;
>   	size_t n_length;
> +	uint8_t *seq;
>   	uint8_t *der;
>   	uint8_t tag;
> +	bool pubkey = true;
>
>   	if (key_length < 8)
>   		return false;
>
>   	/* Unpack the outer SEQUENCE */
> -	der = der_find_elem((uint8_t *) key, key_length, 0, &tag, &n_length);
> -	if (!der || tag != ASN1_ID_SEQUENCE)
> +	seq = der_find_elem((uint8_t *) key, key_length, 0, &tag, &seq_length);
> +	if (!seq || tag != ASN1_ID_SEQUENCE)
>   		return false;
>
> -	/* Take first INTEGER as the modulus */
> -	der = der_find_elem(der, n_length, 0, &tag, &n_length);
> +	/* First INTEGER may be a 1-byte version (for private key) or
> +	 * the modulus (public key)
> +	 */
> +	der = der_find_elem(seq, seq_length, 0, &tag, &n_length);
> +	if (der && tag == ASN1_ID_INTEGER && n_length == 1) {
> +		/* Found version number, implies this is a private key. */
> +		der = der_find_elem(seq, seq_length, 1, &tag, &n_length);
> +		pubkey = false;
> +	}
> +
>   	if (!der || tag != ASN1_ID_INTEGER || n_length < 4)
>   		return false;
>
> @@ -358,95 +388,11 @@ static bool parse_rsa_key(struct l_asymmetric_cipher *cipher, const void *key,
>   	}
>
>   	cipher->key_size = n_length;
> +	cipher->public_key = pubkey;
>

I'm okay with this for now, but really I'd rather that we didn't have 
any ASN.1 parsers inside ell.

Should we go in the direction of doing something like:

acipher = l_asymmetric_cipher_new(type);

l_asymmetric_cipher_set_pubkey(acipher, key, key_size);
l_asymmetric_cipher_set_privkey(acipher, key, key_size);

l_asymmetric_cipher_get_key_size(acipher);

>   	return true;
>   }
>

<snip>

>   LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new(
>   					enum l_asymmetric_cipher_type type,
>   					const void *key, size_t key_length)
> @@ -468,19 +414,23 @@ LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new(
>   		if (!parse_rsa_key(cipher, key, key_length))
>   			goto error_free;
>
> -		alg_name = "rsa";
> +		alg_name = "pkcs1pad(rsa)";
>   		break;
>   	}
>
>   	cipher->cipher.encrypt_sk = create_alg("akcipher", alg_name,
> -						key, key_length);
> +						key, key_length,
> +						cipher->public_key);
>   	if (cipher->cipher.encrypt_sk < 0)
>   		goto error_free;
>
> -	cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
> -						key, key_length);
> -	if (cipher->cipher.decrypt_sk < 0)
> -		goto error_close;
> +	if (!cipher->public_key) {
> +		cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
> +							key, key_length,
> +							cipher->public_key);
> +		if (cipher->cipher.decrypt_sk < 0)
> +			goto error_close;
> +	}
>
>   	return cipher;
>

<snip>

> -LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *cipher,
> +LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher *acipher,
>   					const void *in, void *out,
>   					size_t len_in, size_t len_out)
>   {
> -	if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) {
> -		/* PKCS#1 v1.5 RSA padding according to RFC3447 */
> -		uint8_t buf[cipher->key_size];
> -		int pos;
> -
> -		if (len_in != (size_t) cipher->key_size)
> -			return false;
> -
> -		if (!l_cipher_decrypt(&cipher->cipher, in, buf,
> -					cipher->key_size))
> -			return false;
> -
> -		if (buf[0] != 0x00)
> -			return false;
> -		if (buf[1] != 0x02)
> -			return false;
> -
> -		for (pos = 2; pos < cipher->key_size; pos++)
> -			if (buf[pos] == 0)
> -				break;
> -		if (pos < 10 || pos == cipher->key_size)
> -			return false;
> -
> -		pos++;
> -		if (len_out != (size_t) cipher->key_size - pos)
> -			return false;
> -
> -		memcpy(out, buf + pos, cipher->key_size - pos);
> -	}
> +	if (unlikely(!acipher))
> +		return false;
>
> -	return true;
> +	if (unlikely(!in) || unlikely(!out))
> +		return false;
> +

This should probably check for validity of decrypt_sk

> +	return operate_cipher(acipher->cipher.decrypt_sk, ALG_OP_DECRYPT,
> +				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)
>   {
> -	if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) {
> -		/* PKCS#1 v1.5 RSA padding according to RFC3447 */
> -		uint8_t buf[cipher->key_size];
> -		int ps_len = cipher->key_size - len_in - 3;
> -
> -		if (len_in > (size_t) cipher->key_size - 11)
> -			return false;
> -		if (len_out != (size_t) cipher->key_size)
> -			return false;
> -
> -		buf[0] = 0x00;
> -		buf[1] = 0x01;
> -		memset(buf + 2, 0xff, ps_len);
> -		buf[ps_len + 2] = 0x00;
> -		memcpy(buf + ps_len + 3, in, len_in);
> -
> -		/*
> -		 * The RSA signing operation uses the same primitive as
> -		 * decryption so just call decrypt for now.
> -		 */
> -		if (!l_cipher_decrypt(&cipher->cipher, buf, out,
> -					cipher->key_size))
> -			return false;
> -	}
> +	if (unlikely(!acipher))
> +		return false;
>
> -	return true;
> +	if (unlikely(!in) || unlikely(!out))
> +		return false;
> +
> +	return operate_cipher(acipher->cipher.decrypt_sk, ALG_OP_SIGN,
> +				in, out, len_in, len_out);

Same here

>   }
>

<snip>

Regards,
-Denis

Re: [PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[Mat Martineau]

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


On Mon, 13 Jun 2016, Denis Kenzior wrote:

> Hi Mat,
>
> On 06/10/2016 12:09 PM, Mat Martineau wrote:
>> There are some significant differences in the current iteration of
>> kernel AF_ALG akcipher support:
>>   * Kernel handles padding
>>   * Kernel accepts DER-encoded certs as-is (no need to extract values)
>>   * Must explicitly set private or public key
>> ---
>>   ell/cipher-private.h |   3 -
>>   ell/cipher.c         | 311 
>> ++++++++++++++-------------------------------------
>>   ell/tls.c            |  43 +++----
>>   3 files changed, 97 insertions(+), 260 deletions(-)
>> 
>
> <snip>
>
>>   LIB_EXPORT bool l_cipher_set_iv(struct l_cipher *cipher, const uint8_t 
>> *iv,
>> @@ -275,6 +293,7 @@ LIB_EXPORT bool l_cipher_set_iv(struct l_cipher 
>> *cipher, const uint8_t *iv,
>>   struct l_asymmetric_cipher {
>>   	struct l_cipher cipher;
>>   	int key_size;
>> +	bool public_key;
>
> Do we really need this variable?  It seems that we only use it inside 
> l_asymmetric_cipher_new.  In which case we can have a local variable to that 
> function and have parse_rsa_key have an out parameter for this.

I'll take it out.

>
>>   };
>>
>>   static inline int parse_asn1_definite_length(const uint8_t **buf,
>> @@ -334,20 +353,31 @@ static bool parse_rsa_key(struct l_asymmetric_cipher 
>> *cipher, const void *key,
>>   	 * and cache the size of the modulus n for later use.
>>   	 * (RFC3279)
>>   	 */
>> +	size_t seq_length;
>>   	size_t n_length;
>> +	uint8_t *seq;
>>   	uint8_t *der;
>>   	uint8_t tag;
>> +	bool pubkey = true;
>>
>>   	if (key_length < 8)
>>   		return false;
>>
>>   	/* Unpack the outer SEQUENCE */
>> -	der = der_find_elem((uint8_t *) key, key_length, 0, &tag, &n_length);
>> -	if (!der || tag != ASN1_ID_SEQUENCE)
>> +	seq = der_find_elem((uint8_t *) key, key_length, 0, &tag, 
>> &seq_length);
>> +	if (!seq || tag != ASN1_ID_SEQUENCE)
>>   		return false;
>> 
>> -	/* Take first INTEGER as the modulus */
>> -	der = der_find_elem(der, n_length, 0, &tag, &n_length);
>> +	/* First INTEGER may be a 1-byte version (for private key) or
>> +	 * the modulus (public key)
>> +	 */
>> +	der = der_find_elem(seq, seq_length, 0, &tag, &n_length);
>> +	if (der && tag == ASN1_ID_INTEGER && n_length == 1) {
>> +		/* Found version number, implies this is a private key. */
>> +		der = der_find_elem(seq, seq_length, 1, &tag, &n_length);
>> +		pubkey = false;
>> +	}
>> +
>>   	if (!der || tag != ASN1_ID_INTEGER || n_length < 4)
>>   		return false;
>> 
>> @@ -358,95 +388,11 @@ static bool parse_rsa_key(struct l_asymmetric_cipher 
>> *cipher, const void *key,
>>   	}
>>
>>   	cipher->key_size = n_length;
>> +	cipher->public_key = pubkey;
>> 
>
> I'm okay with this for now, but really I'd rather that we didn't have any 
> ASN.1 parsers inside ell.

I agree that it would be good to leave all the ASN.1 parsing to the 
kernel.

>
> Should we go in the direction of doing something like:
>
> acipher = l_asymmetric_cipher_new(type);
>
> l_asymmetric_cipher_set_pubkey(acipher, key, key_size);
> l_asymmetric_cipher_set_privkey(acipher, key, key_size);
>
> l_asymmetric_cipher_get_key_size(acipher);

Last week I pushed back on adding a public/private flag to 
l_asymmetric_cipher_new, but as I keep working on this the flag is 
becoming the most appealing. If there are separate key setting functions, 
those key set function end up being thin wrappers around a common internal 
function anyway, and makes the asym cipher api more complicated to use and 
implement.

Here's what I'd like to do:

  * Add public/private flag to l_asymmetric_cipher_new
  * Move TLS to keyctl-based crypto
  * Remove l_asymmetric_cipher_get_key_size (TLS is the only user) and the 
ASN.1 parsing code.

>
>>   	return true;
>>   }
>> 
>
> <snip>
>
>>   LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new(
>>   					enum l_asymmetric_cipher_type type,
>>   					const void *key, size_t key_length)
>> @@ -468,19 +414,23 @@ LIB_EXPORT struct l_asymmetric_cipher 
>> *l_asymmetric_cipher_new(
>>   		if (!parse_rsa_key(cipher, key, key_length))
>>   			goto error_free;
>> 
>> -		alg_name = "rsa";
>> +		alg_name = "pkcs1pad(rsa)";
>>   		break;
>>   	}
>>
>>   	cipher->cipher.encrypt_sk = create_alg("akcipher", alg_name,
>> -						key, key_length);
>> +						key, key_length,
>> +						cipher->public_key);
>>   	if (cipher->cipher.encrypt_sk < 0)
>>   		goto error_free;
>> 
>> -	cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
>> -						key, key_length);
>> -	if (cipher->cipher.decrypt_sk < 0)
>> -		goto error_close;
>> +	if (!cipher->public_key) {
>> +		cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
>> +							key, key_length,
>> +							cipher->public_key);
>> +		if (cipher->cipher.decrypt_sk < 0)
>> +			goto error_close;
>> +	}
>>
>>   	return cipher;
>> 
>
> <snip>
>
>> -LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher 
>> *cipher,
>> +LIB_EXPORT bool l_asymmetric_cipher_decrypt(struct l_asymmetric_cipher 
>> *acipher,
>>   					const void *in, void *out,
>>   					size_t len_in, size_t len_out)
>>   {
>> -	if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) {
>> -		/* PKCS#1 v1.5 RSA padding according to RFC3447 */
>> -		uint8_t buf[cipher->key_size];
>> -		int pos;
>> -
>> -		if (len_in != (size_t) cipher->key_size)
>> -			return false;
>> -
>> -		if (!l_cipher_decrypt(&cipher->cipher, in, buf,
>> -					cipher->key_size))
>> -			return false;
>> -
>> -		if (buf[0] != 0x00)
>> -			return false;
>> -		if (buf[1] != 0x02)
>> -			return false;
>> -
>> -		for (pos = 2; pos < cipher->key_size; pos++)
>> -			if (buf[pos] == 0)
>> -				break;
>> -		if (pos < 10 || pos == cipher->key_size)
>> -			return false;
>> -
>> -		pos++;
>> -		if (len_out != (size_t) cipher->key_size - pos)
>> -			return false;
>> -
>> -		memcpy(out, buf + pos, cipher->key_size - pos);
>> -	}
>> +	if (unlikely(!acipher))
>> +		return false;
>> 
>> -	return true;
>> +	if (unlikely(!in) || unlikely(!out))
>> +		return false;
>> +
>
> This should probably check for validity of decrypt_sk

There should never be a struct l_asymmetric_cipher with a bad decrypt_sk - 
validity is checked in l_asymmetric_cipher_new and it never returns a 
struct with a bad socket handle. If the socket gets corrupted somehow, 
operate_cipher will let us know. Is this only important if the set key 
functions are split out?

Do you know why we have separate encrypt_sk and decrypt_sk sockets? I 
tried using the same socket for all operations and it works fine.

>
>> +	return operate_cipher(acipher->cipher.decrypt_sk, ALG_OP_DECRYPT,
>> +				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)
>>   {
>> -	if (cipher->cipher.type == L_CIPHER_RSA_PKCS1_V1_5) {
>> -		/* PKCS#1 v1.5 RSA padding according to RFC3447 */
>> -		uint8_t buf[cipher->key_size];
>> -		int ps_len = cipher->key_size - len_in - 3;
>> -
>> -		if (len_in > (size_t) cipher->key_size - 11)
>> -			return false;
>> -		if (len_out != (size_t) cipher->key_size)
>> -			return false;
>> -
>> -		buf[0] = 0x00;
>> -		buf[1] = 0x01;
>> -		memset(buf + 2, 0xff, ps_len);
>> -		buf[ps_len + 2] = 0x00;
>> -		memcpy(buf + ps_len + 3, in, len_in);
>> -
>> -		/*
>> -		 * The RSA signing operation uses the same primitive as
>> -		 * decryption so just call decrypt for now.
>> -		 */
>> -		if (!l_cipher_decrypt(&cipher->cipher, buf, out,
>> -					cipher->key_size))
>> -			return false;
>> -	}
>> +	if (unlikely(!acipher))
>> +		return false;
>> 
>> -	return true;
>> +	if (unlikely(!in) || unlikely(!out))
>> +		return false;
>> +
>> +	return operate_cipher(acipher->cipher.decrypt_sk, ALG_OP_SIGN,
>> +				in, out, len_in, len_out);
>
> Same here
>>   }
>> 
>
> <snip>
>

--
Mat Martineau
Intel OTC

Re: [PATCH v3 3/5] cipher: Return result length from asymmetric cipher

[Mat Martineau]

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


On Fri, 10 Jun 2016, Mat Martineau wrote:

> Short reads from AF_ALG akcipher sockets can return an error rather
> than a partial buffer so it's important to provide both an output
> buffer length (which is typically longer than the expected data), and
> to find out how many bytes were read.
> ---
> ell/cipher.c      | 31 +++++++++-----------
> ell/cipher.h      | 16 +++++------
> ell/tls-private.h |  2 +-
> ell/tls.c         | 86 ++++++++++++++++++++++++++++++++++++-------------------
> 4 files changed, 80 insertions(+), 55 deletions(-)
>
> diff --git a/ell/cipher.c b/ell/cipher.c
> index f23bdda..e370a0e 100644
> --- a/ell/cipher.c
> +++ b/ell/cipher.c
> @@ -222,10 +222,7 @@ static bool operate_cipher(int sk, __u32 operation,
> 	if (sendmsg(sk, &msg, 0) < 0)
> 		return false;

'return false' won't look like an error to the caller, I'll change it to 
return an informative error number.

>
> -	if (read(sk, out, len_out) < 0)
> -		return false;
> -
> -	return true;
> +	return read(sk, out, len_out);
> }
>
> LIB_EXPORT bool l_cipher_encrypt(struct l_cipher *cipher,

--
Mat Martineau
Intel OTC

Re: [PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[Denis Kenzior]

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

Hi Mat,

 >>
>> Should we go in the direction of doing something like:
>>
>> acipher = l_asymmetric_cipher_new(type);
>>
>> l_asymmetric_cipher_set_pubkey(acipher, key, key_size);
>> l_asymmetric_cipher_set_privkey(acipher, key, key_size);
>>
>> l_asymmetric_cipher_get_key_size(acipher);
>
> Last week I pushed back on adding a public/private flag to
> l_asymmetric_cipher_new, but as I keep working on this the flag is
> becoming the most appealing. If there are separate key setting
> functions, those key set function end up being thin wrappers around a
> common internal function anyway, and makes the asym cipher api more
> complicated to use and implement.
>
> Here's what I'd like to do:
>
>   * Add public/private flag to l_asymmetric_cipher_new
>   * Move TLS to keyctl-based crypto
>   * Remove l_asymmetric_cipher_get_key_size (TLS is the only user) and
> the ASN.1 parsing code.
>

Do we still need to know the key size in order to provide a proper 
buffer for certain implementations?

>>> -    cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
>>> -                        key, key_length);
>>> -    if (cipher->cipher.decrypt_sk < 0)
>>> -        goto error_close;
>>> +    if (!cipher->public_key) {
>>> +        cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
>>> +                            key, key_length,
>>> +                            cipher->public_key);
>>> +        if (cipher->cipher.decrypt_sk < 0)
>>> +            goto error_close;
>>> +    }

By my reading decrypt_sk is only initialized if cipher->public_key is 
false...

>> This should probably check for validity of decrypt_sk
>
> There should never be a struct l_asymmetric_cipher with a bad decrypt_sk
> - validity is checked in l_asymmetric_cipher_new and it never returns a
> struct with a bad socket handle. If the socket gets corrupted somehow,
> operate_cipher will let us know. Is this only important if the set key
> functions are split out?
>

Why bother operating on invalid sockets.  If we were provided a public 
key, then we can't decrypt, right?

> Do you know why we have separate encrypt_sk and decrypt_sk sockets? I
> tried using the same socket for all operations and it works fine.
>

This is another weirdness in the userspace API.  I think it is an issue 
in certain skcipher implementations.  They don't keep a separate encrypt 
/ decrypt state.  So if you don't keep two separate sockets, you get 
screwy results.  I think ARC4 is our biggest culprit.  Look in the 
history of cipher.c for details.  git show 
632dfc8fc9d53718e5596cad25e0a71c373cf964

Regards,
-Denis

Re: [PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[Mat Martineau]

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

On Mon, 13 Jun 2016, Denis Kenzior wrote:

> Hi Mat,
>
>>>
>>> Should we go in the direction of doing something like:
>>> 
>>> acipher = l_asymmetric_cipher_new(type);
>>> 
>>> l_asymmetric_cipher_set_pubkey(acipher, key, key_size);
>>> l_asymmetric_cipher_set_privkey(acipher, key, key_size);
>>> 
>>> l_asymmetric_cipher_get_key_size(acipher);
>> 
>> Last week I pushed back on adding a public/private flag to
>> l_asymmetric_cipher_new, but as I keep working on this the flag is
>> becoming the most appealing. If there are separate key setting
>> functions, those key set function end up being thin wrappers around a
>> common internal function anyway, and makes the asym cipher api more
>> complicated to use and implement.
>> 
>> Here's what I'd like to do:
>>
>>   * Add public/private flag to l_asymmetric_cipher_new
>>   * Move TLS to keyctl-based crypto
>>   * Remove l_asymmetric_cipher_get_key_size (TLS is the only user) and
>> the ASN.1 parsing code.
>> 
>
> Do we still need to know the key size in order to provide a proper buffer for 
> certain implementations?

We won't have a direct need for it within ELL (keyctl will provide the key 
length for TLS purposes). If this isn't part of the l_asymmetric_cipher 
API, the key length information is available from l_key if the key is 
loaded there. l_asymmetric_cipher users would be no worse off then normal 
AF_ALG akcipher users in terms of choosing optimal read buffer sizes.

Long term, we could implement a getsockopt call to get the 
key length - but it seems worthwhile to trim the ASN.1 parser out of ELL 
in the near term.

>>>> -    cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
>>>> -                        key, key_length);
>>>> -    if (cipher->cipher.decrypt_sk < 0)
>>>> -        goto error_close;
>>>> +    if (!cipher->public_key) {
>>>> +        cipher->cipher.decrypt_sk = create_alg("akcipher", alg_name,
>>>> +                            key, key_length,
>>>> +                            cipher->public_key);
>>>> +        if (cipher->cipher.decrypt_sk < 0)
>>>> +            goto error_close;
>>>> +    }
>
> By my reading decrypt_sk is only initialized if cipher->public_key is 
> false...

Yes, you're right. I had conflated my "use just one socket for akcipher" 
code and the validity checking question.

>
>>> This should probably check for validity of decrypt_sk
>> 
>> There should never be a struct l_asymmetric_cipher with a bad decrypt_sk
>> - validity is checked in l_asymmetric_cipher_new and it never returns a
>> struct with a bad socket handle. If the socket gets corrupted somehow,
>> operate_cipher will let us know. Is this only important if the set key
>> functions are split out?
>> 
>
> Why bother operating on invalid sockets.  If we were provided a public key, 
> then we can't decrypt, right?

If we have two sockets and one might not be initialized, you're right that 
it does makes sense to check before calling.

I'd like to change it to use one socket for asymmetric ciphers, in which 
case the check would be up to the kernel (unless we keep track of the key 
type just for this purpose). This significantly streamlines the common 
case.

>
>> Do you know why we have separate encrypt_sk and decrypt_sk sockets? I
>> tried using the same socket for all operations and it works fine.
>> 
>
> This is another weirdness in the userspace API.  I think it is an issue in 
> certain skcipher implementations.  They don't keep a separate encrypt / 
> decrypt state.  So if you don't keep two separate sockets, you get screwy 
> results.  I think ARC4 is our biggest culprit.  Look in the history of 
> cipher.c for details.  git show 632dfc8fc9d53718e5596cad25e0a71c373cf964

One socket is working ok for akcipher, even having four different 
operations on that one socket.

--
Mat Martineau
Intel OTC

Re: [PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[Denis Kenzior]

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

Hi Mat,

 >>
>> Do we still need to know the key size in order to provide a proper
>> buffer for certain implementations?
>
> We won't have a direct need for it within ELL (keyctl will provide the
> key length for TLS purposes). If this isn't part of the
> l_asymmetric_cipher API, the key length information is available from
> l_key if the key is loaded there. l_asymmetric_cipher users would be no
> worse off then normal AF_ALG akcipher users in terms of choosing optimal
> read buffer sizes.
>
> Long term, we could implement a getsockopt call to get the key length -
> but it seems worthwhile to trim the ASN.1 parser out of ELL in the near
> term.

My feeling is, lets make asymmetric ciphers work for now, at least until 
the key stuff is ready.  So we can keep the ASN.1 parser to get the key 
length for now, with an eye to migrate it to getsockopt when that 
feature is added to the kernel.

> I'd like to change it to use one socket for asymmetric ciphers, in which
> case the check would be up to the kernel (unless we keep track of the
> key type just for this purpose). This significantly streamlines the
> common case.
>

Since the only asymmetric cipher is RSA, we might be getting lucky here. 
  I have no problem moving to a single socket semantic, as long as the 
kernel guarantees that this will work for all akcipher algorithms.

I assumed a single socket was fine for skcipher as well, but was proven 
wrong.  The current semantics in the kernel are weird.  You almost have 
to know the underlying algorithm implementation in order to do the right 
thing.

>
> One socket is working ok for akcipher, even having four different
> operations on that one socket.
>

Because there's no 'state'.  In RC4 results of an operation are carried 
into the subsequent operation(s).  Highly unlikely that any akciphers 
will work this way, but who knows.  The semantics need to be nailed down 
in the docs.

Regards,
-Denis

Re: [PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[Mat Martineau]

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


Denis,

On Mon, 13 Jun 2016, Denis Kenzior wrote:

> Hi Mat,
>
>>>
>>> Do we still need to know the key size in order to provide a proper
>>> buffer for certain implementations?
>> 
>> We won't have a direct need for it within ELL (keyctl will provide the
>> key length for TLS purposes). If this isn't part of the
>> l_asymmetric_cipher API, the key length information is available from
>> l_key if the key is loaded there. l_asymmetric_cipher users would be no
>> worse off then normal AF_ALG akcipher users in terms of choosing optimal
>> read buffer sizes.
>> 
>> Long term, we could implement a getsockopt call to get the key length -
>> but it seems worthwhile to trim the ASN.1 parser out of ELL in the near
>> term.
>
> My feeling is, lets make asymmetric ciphers work for now, at least until the 
> key stuff is ready.  So we can keep the ASN.1 parser to get the key length 
> for now, with an eye to migrate it to getsockopt when that feature is added 
> to the kernel.

Ok

>
>> I'd like to change it to use one socket for asymmetric ciphers, in which
>> case the check would be up to the kernel (unless we keep track of the
>> key type just for this purpose). This significantly streamlines the
>> common case.
>> 
>
> Since the only asymmetric cipher is RSA, we might be getting lucky here.  I 
> have no problem moving to a single socket semantic, as long as the kernel 
> guarantees that this will work for all akcipher algorithms.

There is a check in algif_akcipher to make sure there is not input data 
pending for a previous operation.

> I assumed a single socket was fine for skcipher as well, but was proven 
> wrong.  The current semantics in the kernel are weird.  You almost have to 
> know the underlying algorithm implementation in order to do the right thing.
>
>> 
>> One socket is working ok for akcipher, even having four different
>> operations on that one socket.
>> 
>
> Because there's no 'state'.  In RC4 results of an operation are carried into 
> the subsequent operation(s).  Highly unlikely that any akciphers will work 
> this way, but who knows.  The semantics need to be nailed down in the docs.

Having the pending data check is one part of clarifying the semantics, and 
I agree that docs are important for this. I'll check up on the 
documentation side of things (Stephan usually includes docs, but they're 
missing from akcipher).

--
Mat Martineau
Intel OTC

Re: [PATCH v3 1/5] cipher: Update for current kernel akcipher interface

[Denis Kenzior]

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

Hi Mat,

 >
> Having the pending data check is one part of clarifying the semantics,
> and I agree that docs are important for this. I'll check up on the
> documentation side of things (Stephan usually includes docs, but they're
> missing from akcipher).
>

Go ahead and make a judgement call.  If you think that 1 socket is 
enough / what the kernel intends, then I'm fine taking such a patch.

Regards,
-Denis