Re: [PATCH 1/3] ecryptfs: release keys loaded inecryptfs_keyring_auth_tok_for_sig()

[Tyler Hicks]

On Wed Oct 06, 2010 at 06:31:06PM +0200, Roberto Sassu <roberto.sassu@xxxxxxxxx> wrote:
> This patch allows keys requested in the function ecryptfs_keyring_auth_tok_for_sig() 
> to be released when they are no longer required. In particular keys are directly released 
> in the same function if the obtained authentication token is not valid.
> Further, a new function parameter 'auth_tok_key' has been added to 
> ecryptfs_find_auth_tok_for_sig() in order to provide callers the key pointer to be passed 
> to key_put().
> 
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxx>
> ---

Hi Roberto - Thanks for the fix. This is messy (calling key_put *some*
of the time), but there's not a better way to do it. In the future, we
may not want to save off a pointer to the key in the global_auth_tok and
always ask the kernel keyring for the key. Then we will pick up the
access checks provided by the keyring. But until then, this will do.

BTW, your commit message body should be wrapped at 72 columns for nice
formatting in the git log. I fixed that, made a small change (see
below), and committed the patch to
git://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git#next

Thanks again for noticing and fixing this bug.

>  fs/ecryptfs/keystore.c |   34 ++++++++++++++++++++++++++++------
>  1 files changed, 28 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> index 73811cf..77580db 100644
> --- a/fs/ecryptfs/keystore.c
> +++ b/fs/ecryptfs/keystore.c
> @@ -446,6 +446,7 @@ out:
>   */
>  static int
>  ecryptfs_find_auth_tok_for_sig(
> +	struct key **auth_tok_key,
>  	struct ecryptfs_auth_tok **auth_tok,
>  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat,
>  	char *sig)
> @@ -453,12 +454,12 @@ ecryptfs_find_auth_tok_for_sig(
>  	struct ecryptfs_global_auth_tok *global_auth_tok;
>  	int rc = 0;
>  
> +	(*auth_tok_key) = NULL;
>  	(*auth_tok) = NULL;
>  	if (ecryptfs_find_global_auth_tok_for_sig(&global_auth_tok,
>  						  mount_crypt_stat, sig)) {
> -		struct key *auth_tok_key;
>  
> -		rc = ecryptfs_keyring_auth_tok_for_sig(&auth_tok_key, auth_tok,
> +		rc = ecryptfs_keyring_auth_tok_for_sig(auth_tok_key, auth_tok,
>  						       sig);
>  	} else
>  		(*auth_tok) = global_auth_tok->global_auth_tok;
> @@ -509,6 +510,7 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
>  			     char *filename, size_t filename_size)
>  {
>  	struct ecryptfs_write_tag_70_packet_silly_stack *s;
> +	struct key *auth_tok_key = NULL;
>  	int rc = 0;
>  
>  	s = kmalloc(sizeof(*s), GFP_KERNEL);
> @@ -606,6 +608,7 @@ ecryptfs_write_tag_70_packet(char *dest, size_t *remaining_bytes,
>  	}
>  	dest[s->i++] = s->cipher_code;
>  	rc = ecryptfs_find_auth_tok_for_sig(
> +		&auth_tok_key,
>  		&s->auth_tok, mount_crypt_stat,
>  		mount_crypt_stat->global_default_fnek_sig);
>  	if (rc) {
> @@ -753,6 +756,8 @@ out_free_unlock:
>  out_unlock:
>  	mutex_unlock(s->tfm_mutex);
>  out:
> +	if (auth_tok_key)
> +		key_put(auth_tok_key);
>  	kfree(s);
>  	return rc;
>  }
> @@ -798,6 +803,7 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
>  			     char *data, size_t max_packet_size)
>  {
>  	struct ecryptfs_parse_tag_70_packet_silly_stack *s;
> +	struct key *auth_tok_key = NULL;
>  	int rc = 0;
>  
>  	(*packet_size) = 0;
> @@ -910,7 +916,8 @@ ecryptfs_parse_tag_70_packet(char **filename, size_t *filename_size,
>  	 * >= ECRYPTFS_MAX_IV_BYTES. */
>  	memset(s->iv, 0, ECRYPTFS_MAX_IV_BYTES);
>  	s->desc.info = s->iv;
> -	rc = ecryptfs_find_auth_tok_for_sig(&s->auth_tok, mount_crypt_stat,
> +	rc = ecryptfs_find_auth_tok_for_sig(&auth_tok_key,
> +					    &s->auth_tok, mount_crypt_stat,
>  					    s->fnek_sig_hex);
>  	if (rc) {
>  		printk(KERN_ERR "%s: Error attempting to find auth tok for "
> @@ -986,6 +993,8 @@ out:
>  		(*filename_size) = 0;
>  		(*filename) = NULL;
>  	}
> +	if (auth_tok_key)
> +		key_put(auth_tok_key);
>  	kfree(s);
>  	return rc;
>  }
> @@ -1557,14 +1566,19 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
>  		       ECRYPTFS_VERSION_MAJOR,
>  		       ECRYPTFS_VERSION_MINOR);
>  		rc = -EINVAL;
> -		goto out;
> +		goto out_release_key;
>  	}
>  	if ((*auth_tok)->token_type != ECRYPTFS_PASSWORD
>  	    && (*auth_tok)->token_type != ECRYPTFS_PRIVATE_KEY) {
>  		printk(KERN_ERR "Invalid auth_tok structure "
>  		       "returned from key query\n");
>  		rc = -EINVAL;
> -		goto out;
> +		goto out_release_key;
> +	}
> +out_release_key:
> +	if (rc) {
> +		key_put(*auth_tok_key);
> +		(*auth_tok_key) = NULL;
>  	}
>  out:
>  	return rc;
> @@ -1688,6 +1702,7 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
>  	struct ecryptfs_auth_tok_list_item *auth_tok_list_item;
>  	size_t tag_11_contents_size;
>  	size_t tag_11_packet_size;
> +	struct key *auth_tok_key;

This needs to be initialized to NULL. Otherwise, the key_put()'s below
can do bad things in bad places. :)

>  	int rc = 0;
>  
>  	INIT_LIST_HEAD(&auth_tok_list);
> @@ -1784,6 +1799,10 @@ int ecryptfs_parse_packet_set(struct ecryptfs_crypt_stat *crypt_stat,
>  	 * just one will be sufficient to decrypt to get the FEK. */
>  find_next_matching_auth_tok:
>  	found_auth_tok = 0;
> +	if (auth_tok_key) {
> +		key_put(auth_tok_key);
> +		auth_tok_key = NULL;
> +	}
>  	list_for_each_entry(auth_tok_list_item, &auth_tok_list, list) {
>  		candidate_auth_tok = &auth_tok_list_item->auth_tok;
>  		if (unlikely(ecryptfs_verbosity > 0)) {
> @@ -1800,7 +1819,8 @@ find_next_matching_auth_tok:
>  			rc = -EINVAL;
>  			goto out_wipe_list;
>  		}
> -		ecryptfs_find_auth_tok_for_sig(&matching_auth_tok,
> +		ecryptfs_find_auth_tok_for_sig(&auth_tok_key,
> +					       &matching_auth_tok,
>  					       crypt_stat->mount_crypt_stat,
>  					       candidate_auth_tok_sig);
>  		if (matching_auth_tok) {
> @@ -1866,6 +1886,8 @@ found_matching_auth_tok:
>  out_wipe_list:
>  	wipe_auth_tok_list(&auth_tok_list);
>  out:
> +	if (auth_tok_key)
> +		key_put(auth_tok_key);
>  	return rc;
>  }
>  
> -- 
> 1.7.2.3


--
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/