Re: [PATCH] crypto: AF_ALG - add support for keys/asymmetric-type

[Stephan Mueller <smueller-T9tCv8IpfcWELgA04lAiVw@public.gmane.org>]

Am Montag, 21. Dezember 2015, 12:51:07 schrieb Tadeusz Struk:

Hi Tadeusz,

> From: Tadeusz Struk <tadeusz.struk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> Created on top of patchset from Stephan Mueller <smueller-T9tCv8IpfcWELgA04lAiVw@public.gmane.org>
> https://patchwork.kernel.org/patch/7877921/
> https://patchwork.kernel.org/patch/7877971/
> https://patchwork.kernel.org/patch/7877961/
> 
> This patch adds support for asymmetric key type to AF_ALG.
> It will work as follows: A new PF_ALG socket options will be
> added on top of existing ALG_SET_KEY and ALG_SET_PUBKEY, namely
> ALG_SET_PUBKEY_ID and ALG_SET_KEY_ID for setting public and
> private keys respectively. When these new options will be used
> the user instead of providing the key material, will provide a
> key id and the key itself will be obtained from kernel keyring
> subsystem. The user will use the standard tools (keyctl tool
> or the keyctl syscall) for key instantiation and to obtain the
> key id. The key id can also be obtained by reading the
> /proc/keys file.
> 
> When a key will be found, the request_key() function will
> return a requested key. Next the asymmetric key subtype will be
> used to obtain the public_key, which can be either a public key
> or a private key from the cryptographic point of view, and the
> key payload will be passed to the akcipher pf_alg subtype.
> Pf_alg code will then call crypto API functions, either the
> crypto_akcipher_set_priv_key or the crypto_akcipher_set_pub_key,
> depending on the used option. Subsequently the asymmetric key
> will be freed and return code returned back to the user.
> 
> Currently the interface will be restricted only to asymmetric
> ciphers, but it can be extended later to work with symmetric
> ciphers if required.
> 
> The assumption is that access rights for a given user will be
> verified by the key subsystem so the pf_alg interface can call
> the request_key() without checking if the user has appropriate
> rights (Please verify this assumption).
> 
> Signed-off-by: Tadeusz Struk <tadeusz.struk-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  crypto/af_alg.c             |   41
> +++++++++++++++++++++++++++++++++++++---- include/uapi/linux/if_alg.h |   
> 2 ++
>  2 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index 767a134..d2ec357 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -22,6 +22,8 @@
>  #include <linux/net.h>
>  #include <linux/rwsem.h>
>  #include <linux/security.h>
> +#include <crypto/public_key.h>
> +#include <keys/asymmetric-type.h>
> 
>  struct alg_type_list {
>  	const struct af_alg_type *type;
> @@ -173,7 +175,7 @@ static int alg_bind(struct socket *sock, struct sockaddr
> *uaddr, int addr_len) }
> 
>  static int alg_setkey(struct sock *sk, char __user *ukey,
> -		      unsigned int keylen,
> +		      unsigned int keylen, bool key_id,
>  		      int (*setkey)(void *private, const u8 *key,
>  				    unsigned int keylen))
>  {
> @@ -192,7 +194,30 @@ static int alg_setkey(struct sock *sk, char __user
> *ukey, if (copy_from_user(key, ukey, keylen))
>  		goto out;
> 
> -	err = setkey(ask->private, key, keylen);
> +	if (key_id) {

Wouldn't it make sense to rather have a complete separate function for setting 
the key based on the ID? I.e. we have one function for setting the key based 
on a user-given buffer. A second function handles your additional code. As 
both are unrelated, I would not suggest to clutter one function with the logic 
of the other.

> +		struct key *keyring;
> +		struct public_key *pkey;
> +		char key_name[12];
> +		u32 keyid = *((u32 *)key);
> +
> +		sprintf(key_name, "id:%08x", keyid);
> +		keyring = request_key(&key_type_asymmetric, key_name, NULL);
> +
> +		err = -ENOKEY;
> +		if (IS_ERR(keyring))
> +			goto out;
> +
> +		pkey = keyring->payload.data[asym_crypto];
> +		if (!pkey) {
> +			key_put(keyring);
> +			goto out;
> +		}
> +
> +		err = setkey(ask->private, pkey->key, pkey->keylen);
> +		key_put(keyring);
> +	} else {
> +		err = setkey(ask->private, key, keylen);
> +	}
> 
>  out:
>  	sock_kzfree_s(sk, key, keylen);
> @@ -207,6 +232,8 @@ static int alg_setsockopt(struct socket *sock, int
> level, int optname, struct alg_sock *ask = alg_sk(sk);
>  	const struct af_alg_type *type;
>  	int err = -ENOPROTOOPT;
> +	bool key_id = ((optname == ALG_SET_PUBKEY_ID) ||
> +		       (optname == ALG_SET_KEY_ID));
> 
>  	lock_sock(sk);
>  	type = ask->type;
> @@ -216,16 +243,22 @@ static int alg_setsockopt(struct socket *sock, int
> level, int optname,
> 
>  	switch (optname) {
>  	case ALG_SET_KEY:
> +	case ALG_SET_KEY_ID:
>  		if (sock->state == SS_CONNECTED)
>  			goto unlock;
> 
> -		err = alg_setkey(sk, optval, optlen, type->setkey);
> +		/* ALG_SET_KEY_ID is only for akcipher */
> +		if (!strcmp(type->name, "akcipher") && key_id)

Why do you want to limit it to akcipher? I would think it can apply to all 
types of keys. You mention that you want to restrict it to akcipher, but where 
do you see the limitation for HMAC / skcipher?

> +			goto unlock;
> +
> +		err = alg_setkey(sk, optval, optlen, key_id, type->setkey);
>  		break;
>  	case ALG_SET_PUBKEY:
> +	case ALG_SET_PUBKEY_ID:
>  		if (sock->state == SS_CONNECTED)
>  			goto unlock;
> 
> -		err = alg_setkey(sk, optval, optlen, type->setpubkey);
> +		err = alg_setkey(sk, optval, optlen, key_id, type->setpubkey);
>  		break;
>  	case ALG_SET_AEAD_AUTHSIZE:
>  		if (sock->state == SS_CONNECTED)
> diff --git a/include/uapi/linux/if_alg.h b/include/uapi/linux/if_alg.h
> index 02e6162..0379766 100644
> --- a/include/uapi/linux/if_alg.h
> +++ b/include/uapi/linux/if_alg.h
> @@ -35,6 +35,8 @@ struct af_alg_iv {
>  #define ALG_SET_AEAD_ASSOCLEN		4
>  #define ALG_SET_AEAD_AUTHSIZE		5
>  #define ALG_SET_PUBKEY			6
> +#define ALG_SET_PUBKEY_ID		7
> +#define ALG_SET_KEY_ID			8
> 
>  /* Operations */
>  #define ALG_OP_DECRYPT			0


-- 
Ciao
Stephan