Re: [PATCH 08/11] cipher: Update for current kernel akcipher interface

[Denis Kenzior <denkenz@gmail.com>]

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

Hi Mat,

On 06/06/2016 03:54 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         | 300 +++++++++++++--------------------------------------
>   ell/tls.c            |  43 +++-----
>   3 files changed, 85 insertions(+), 261 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..04bbd82 100644
> --- a/ell/cipher.c
> +++ b/ell/cipher.c
> @@ -78,6 +78,10 @@ struct af_alg_iv {
>   #define SOL_ALG 279
>   #endif
>
> +#ifndef ALG_SET_PUBKEY
> +#define ALG_SET_PUBKEY	6
> +#endif
> +

So we don't put #ifdef guards around
ALG_SET_KEY, SET_IV, SET_OP.  Why is this different? Should we have 
guards around all of these?

>   #define is_valid_type(type)  ((type) <= L_CIPHER_DES3_EDE_CBC)
>
>   struct l_cipher {
> @@ -87,10 +91,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 +112,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 +155,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 +186,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 +207,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 +215,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 +230,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 +243,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 +286,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 +346,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;

I'm a bit unsure that we really want to know this information.  What is 
it's use actually?  We should always implicitly know what type of 
certificate we are giving to l_asymmetric_cipher_new, so it might just 
need to be a parameter.

>
>   	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 +381,11 @@ static bool parse_rsa_key(struct l_asymmetric_cipher *cipher, const void *key,
>   	}
>
>   	cipher->key_size = n_length;

This probably belongs as a setsockopt into the kernel...

> +	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)
> @@ -473,14 +412,18 @@ LIB_EXPORT struct l_asymmetric_cipher *l_asymmetric_cipher_new(
>   	}
>
>   	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 +451,52 @@ 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);
> -	}

I'm all for getting rid of the padding stuff, but as mentioned 
previously, I'm not sure the pkcs1pad template is part of the rsa 
akcipher by default.  Is it?

> +	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;
> -	}
> -
> -	return true;
> +	/*
> +	 * The RSA signing operation uses the same primitive as
> +	 * decryption so just call decrypt for now.
> +	 */
> +	return l_asymmetric_cipher_decrypt(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)
>   {
> -	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);
> -	}
> -
> -	return true;
> +	/*
> +	 * The RSA verify operation uses the same primitive as
> +	 * encryption so just call encrypt.
> +	 */
> +	return l_asymmetric_cipher_encrypt(cipher, in, out, len_in, len_out);
>   }
> diff --git a/ell/tls.c b/ell/tls.c
> index e47e8bf..6896a05 100644
> --- a/ell/tls.c
> +++ b/ell/tls.c
> @@ -918,8 +918,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];
> @@ -943,19 +943,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);

Should we be reporting BAD_CERT here?

> @@ -1078,11 +1068,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);

This looks like a separate fix and belongs in a separate patch.

>
>   	result = l_asymmetric_cipher_verify(rsa_client_pubkey, in + 4,
>   						digest_info,
> -						key_size, expected_len);
> +						key_size, key_size);

Same here?

>
>   	l_asymmetric_cipher_free(rsa_client_pubkey);
>
> @@ -1741,8 +1734,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) {
> @@ -1762,19 +1755,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);
>

Regards,
-Denis