Re: [PATCH 9/9] KEYS: Fix encrypted key type update method

[Mimi Zohar]

On Mon, 2013-11-04 at 16:23 +0000, David Howells wrote:
> The encrypted key type was using the update method to change the master key
> used to encrypt a key without passing in all the requisite parameters to
> completely replace the key contents (it was taking some parameters from the
> old key contents).  Unfortunately, this has a number of problems:
> 
>  (1) Update is conceptually meant to be the same as add_key() except that it
>      replaces the contents of the nominated key completely rather than creating
>      a new key.
> 
>  (2) add_key() can call ->update() if it detects that the key exists.  This can
>      cause the operation to fail if the caller embedded the wrong command in
>      the payload when they called it.  The caller cannot know what the right
>      command is without someway to lock the keyring.
> 
>  (3) keyctl_update() and add_key() can thus race with adding, linking,
>      requesting and unlinking a key.
> 
> The best way to fix this is to offload this operation to keyctl_control() and
> make encrypted_update() just replace the contents of a key entirely (or maybe
> just not permit updates if this would be a problem).
> 
> Signed-off-by: David Howells <dhowells@xxxxxxxxxx>

The code looks good, other than breaking the existing userspace/kernel
API, but I haven't tested it yet.  Is there a keyutils git repo with a
version of keyctl that supports the control option?

A couple of minor comments:
- type size_t is unsigned, no need to verify that it is negative.
- missing Documentation/security/keys-trusted-encrypted.txt updates
- the encrypted_preparse() comment still says 'encrypted_instantiate'

thanks,

Mimi

> ---
> 
>  security/keys/encrypted-keys/encrypted.c |  101 ++++++++++++++----------------
>  1 file changed, 47 insertions(+), 54 deletions(-)
> 
> diff --git a/security/keys/encrypted-keys/encrypted.c b/security/keys/encrypted-keys/encrypted.c
> index f9e7b808fa47..c9e4fa5b1e2c 100644
> --- a/security/keys/encrypted-keys/encrypted.c
> +++ b/security/keys/encrypted-keys/encrypted.c
> @@ -595,7 +595,7 @@ out:
>  }
> 
>  /* Allocate memory for decrypted key and datablob. */
> -static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
> +static struct encrypted_key_payload *encrypted_key_alloc(size_t *_quotalen,
>  							 const char *format,
>  							 const char *master_desc,
>  							 const char *datalen)
> @@ -632,10 +632,7 @@ static struct encrypted_key_payload *encrypted_key_alloc(struct key *key,
>  	datablob_len = format_len + 1 + strlen(master_desc) + 1
>  	    + strlen(datalen) + 1 + ivsize + 1 + encrypted_datalen;
> 
> -	ret = key_payload_reserve(key, payload_datalen + datablob_len
> -				  + HASH_SIZE + 1);
> -	if (ret < 0)
> -		return ERR_PTR(ret);
> +	*_quotalen = payload_datalen + datablob_len + HASH_SIZE + 1;
> 
>  	epayload = kzalloc(sizeof(*epayload) + payload_datalen +
>  			   datablob_len + HASH_SIZE + 1, GFP_KERNEL);
> @@ -773,8 +770,7 @@ static int encrypted_init(struct encrypted_key_payload *epayload,
>   *
>   * On success, return 0. Otherwise return errno.
>   */
> -static int encrypted_instantiate(struct key *key,
> -				 struct key_preparsed_payload *prep)
> +static int encrypted_preparse(struct key_preparsed_payload *prep)

>  {
>  	struct encrypted_key_payload *epayload = NULL;
>  	char *datablob = NULL;
> @@ -798,25 +794,55 @@ static int encrypted_instantiate(struct key *key,
>  	if (ret < 0)
>  		goto out;
> 
> -	epayload = encrypted_key_alloc(key, format, master_desc,
> +	epayload = encrypted_key_alloc(&prep->quotalen, format, master_desc,
>  				       decrypted_datalen);
>  	if (IS_ERR(epayload)) {
>  		ret = PTR_ERR(epayload);
>  		goto out;
>  	}
> -	ret = encrypted_init(epayload, key->description, format, master_desc,
> +	ret = encrypted_init(epayload, prep->description, format, master_desc,
>  			     decrypted_datalen, hex_encoded_iv);
>  	if (ret < 0) {
>  		kfree(epayload);
>  		goto out;
>  	}
> 
> -	rcu_assign_keypointer(key, epayload);
> +	prep->payload[0] = epayload;
> +
>  out:
>  	kfree(datablob);
>  	return ret;
>  }
> 
> +/*
> + * encrypted_free_preparse - Clean up preparse data for an encrypted key
> + */
> +static void encrypted_free_preparse(struct key_preparsed_payload *prep)
> +{
> +	struct encrypted_key_payload *epayload = prep->payload[0];
> +
> +	if (epayload) {
> +		memset(epayload->decrypted_data, 0,
> +		       epayload->decrypted_datalen);
> +		kfree(epayload);
> +	}
> +}
> +
> +static int encrypted_instantiate(struct key *key,
> +				 struct key_preparsed_payload *prep)
> +{
> +	struct encrypted_key_payload *epayload = prep->payload[0];
> +	int ret;
> +
> +	ret = key_payload_reserve(key, prep->quotalen);
> +	if (ret < 0)
> +		return ret;
> +
> +	rcu_assign_keypointer(key, epayload);
> +	prep->payload[0] = NULL;
> +	return 0;
> +}
> +
>  static void encrypted_rcu_free(struct rcu_head *rcu)
>  {
>  	struct encrypted_key_payload *epayload;
> @@ -837,50 +863,15 @@ static void encrypted_rcu_free(struct rcu_head *rcu)
>   */
>  static int encrypted_update(struct key *key, struct key_preparsed_payload *prep)
>  {
> -	struct encrypted_key_payload *epayload = key->payload.data;
> -	struct encrypted_key_payload *new_epayload;
> -	char *buf;
> -	char *new_master_desc = NULL;
> -	const char *format = NULL;
> -	size_t datalen = prep->datalen;
> -	int ret = 0;
> -
> -	if (datalen <= 0 || datalen > 32767 || !prep->data)
> -		return -EINVAL;
> +	struct encrypted_key_payload *old;
> +	struct encrypted_key_payload *new = prep->payload[0];
> 
> -	buf = kmalloc(datalen + 1, GFP_KERNEL);
> -	if (!buf)
> -		return -ENOMEM;
> -
> -	buf[datalen] = 0;
> -	memcpy(buf, prep->data, datalen);
> -	ret = datablob_parse(buf, &format, &new_master_desc, NULL, NULL);
> -	if (ret < 0)
> -		goto out;
> +	old = rcu_dereference_protected(key->payload.rcudata, &key->sem);
> 
> -	ret = valid_master_desc(new_master_desc, epayload->master_desc);
> -	if (ret < 0)
> -		goto out;
> -
> -	new_epayload = encrypted_key_alloc(key, epayload->format,
> -					   new_master_desc, epayload->datalen);
> -	if (IS_ERR(new_epayload)) {
> -		ret = PTR_ERR(new_epayload);
> -		goto out;
> -	}
> -
> -	__ekey_init(new_epayload, epayload->format, new_master_desc,
> -		    epayload->datalen);
> -
> -	memcpy(new_epayload->iv, epayload->iv, ivsize);
> -	memcpy(new_epayload->payload_data, epayload->payload_data,
> -	       epayload->payload_datalen);
> -
> -	rcu_assign_keypointer(key, new_epayload);
> -	call_rcu(&epayload->rcu, encrypted_rcu_free);
> -out:
> -	kfree(buf);
> -	return ret;
> +	rcu_assign_keypointer(key, new);
> +	prep->payload[0] = NULL;
> +	call_rcu(&old->rcu, encrypted_rcu_free);
> +	return 0;
>  }
> 
>  /*
> @@ -893,7 +884,7 @@ static long encrypted_control(struct key *key, char *command,
>  	struct encrypted_key_payload *epayload, *new_epayload;
>  	char *new_master_desc = NULL;
>  	const char *format = NULL;
> -	size_t datalen;
> +	size_t datalen, quotalen;
>  	int ret;
> 
>  	if (memcmp(command, expected_command, sizeof(expected_command) - 1) != 0)
> @@ -917,7 +908,7 @@ static long encrypted_control(struct key *key, char *command,
>  		return ret;
>  	}
> 
> -	new_epayload = encrypted_key_alloc(key, epayload->format,
> +	new_epayload = encrypted_key_alloc(&quotalen, epayload->format,
>  					   new_master_desc, epayload->datalen);
>  	if (IS_ERR(new_epayload)) {
>  		up_write(&key->sem);
> @@ -1023,6 +1014,8 @@ static void encrypted_destroy(struct key *key)
> 
>  struct key_type key_type_encrypted = {
>  	.name = "encrypted",
> +	.preparse = encrypted_preparse,
> +	.free_preparse = encrypted_free_preparse,
>  	.instantiate = encrypted_instantiate,
>  	.update = encrypted_update,
>  	.match = user_match,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/