Re: [PATCH 3/5] eCryptfs: verify authentication tokens before their use

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Friday, March 18, 2011 04:05:39 am Tyler Hicks wrote:
> On Thu Mar 17, 2011 at 12:48:52PM +0100, Roberto Sassu <roberto.sassu@xxxxxxxxx> wrote:
> > Authentication tokens content may change if another requestor calls the
> > update() method of the corresponding key. The new function
> > ecryptfs_verify_auth_tok_from_key() retrieves the authentication token from
> > the provided key and verifies if it is still valid before being used to
> > encrypt or decrypt an eCryptfs file.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxx>
> > ---
> >  fs/ecryptfs/ecryptfs_kernel.h |    3 +-
> >  fs/ecryptfs/keystore.c        |  107 ++++++++++++++++++++++++++++-------------
> >  fs/ecryptfs/main.c            |    4 +-
> >  3 files changed, 77 insertions(+), 37 deletions(-)
> > 
> > diff --git a/fs/ecryptfs/ecryptfs_kernel.h b/fs/ecryptfs/ecryptfs_kernel.h
> > index 8282031..85728e9 100644
> > --- a/fs/ecryptfs/ecryptfs_kernel.h
> > +++ b/fs/ecryptfs/ecryptfs_kernel.h
> > @@ -333,7 +333,6 @@ struct ecryptfs_global_auth_tok {
> >  	u32 flags;
> >  	struct list_head mount_crypt_stat_list;
> >  	struct key *global_auth_tok_key;
> > -	struct ecryptfs_auth_tok *global_auth_tok;
> >  	unsigned char sig[ECRYPTFS_SIG_SIZE_HEX + 1];
> >  };
> > 
> > @@ -726,6 +725,8 @@ int ecryptfs_get_tfm_and_mutex_for_cipher_name(struct crypto_blkcipher **tfm,
> >  int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> >  				      struct ecryptfs_auth_tok **auth_tok,
> >  				      char *sig);
> > +int ecryptfs_verify_auth_tok_from_key(struct key *auth_tok_key,
> > +				      struct ecryptfs_auth_tok **auth_tok);
> 
> This shouldn't go in the header file. It only seems to be used in
> keystore.c.
> 
> >  int ecryptfs_write_lower(struct inode *ecryptfs_inode, char *data,
> >  			 loff_t offset, size_t size);
> >  int ecryptfs_write_lower_page_segment(struct inode *ecryptfs_inode,
> > diff --git a/fs/ecryptfs/keystore.c b/fs/ecryptfs/keystore.c
> > index 36b68a6..e35d745 100644
> > --- a/fs/ecryptfs/keystore.c
> > +++ b/fs/ecryptfs/keystore.c
> > @@ -405,25 +405,47 @@ out:
> > 
> >  static int
> >  ecryptfs_find_global_auth_tok_for_sig(
> > -	struct ecryptfs_global_auth_tok **global_auth_tok,
> > +	struct key **auth_tok_key,
> > +	struct ecryptfs_auth_tok **auth_tok,
> >  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat, char *sig)
> >  {
> >  	struct ecryptfs_global_auth_tok *walker;
> >  	int rc = 0;
> > 
> > -	(*global_auth_tok) = NULL;
> > +	(*auth_tok_key) = NULL;
> > +	(*auth_tok) = NULL;
> >  	mutex_lock(&mount_crypt_stat->global_auth_tok_list_mutex);
> >  	list_for_each_entry(walker,
> >  			    &mount_crypt_stat->global_auth_tok_list,
> >  			    mount_crypt_stat_list) {
> >  		if (memcmp(walker->sig, sig, ECRYPTFS_SIG_SIZE_HEX) == 0) {
> 
> Since we're adding more logic inside this conditional, I'd like to see
> something like this:
> 
> if (!memcmp(...))
> 	continue;
> 
> Then proceed with all the newly added logic.
> 
> The eCryptfs source is full of long, too descriptive function and
> variable names. The less nesting we do now, the easier it will be to
> read later. :)
> 
> > +			if (walker->flags & ECRYPTFS_AUTH_TOK_INVALID) {
> > +				rc = -EINVAL;
> > +				goto out;
> > +			}
> > +
> >  			rc = key_validate(walker->global_auth_tok_key);
> > -			if (!rc)
> > -				(*global_auth_tok) = walker;
> > +			if (rc)
> > +				goto out;
> > +
> > +			rc = ecryptfs_verify_auth_tok_from_key(
> > +					walker->global_auth_tok_key, auth_tok);
> > +			if (rc) {
> > +				printk(KERN_WARNING
> > +				      "Invalidating auth tok with sig = [%s]\n",
> > +				      sig);
> 
> Off by one space on the indenting here.
> 
> > +				walker->flags |= ECRYPTFS_AUTH_TOK_INVALID;
> > +				key_put(walker->global_auth_tok_key);
> > +				walker->global_auth_tok_key = NULL;
> > +				mount_crypt_stat->num_global_auth_toks--;
> 
> It looks like num_global_auth_toks can go away. We increment and
> decrement it, but never actually check it.
> 
> > +				goto out;
> > +			}
> > +			(*auth_tok_key) = walker->global_auth_tok_key;
> > +			key_get(*auth_tok_key);
> >  			goto out;
> >  		}
> >  	}
> > -	rc = -EINVAL;
> > +	rc = -ENOENT;
> >  out:
> >  	mutex_unlock(&mount_crypt_stat->global_auth_tok_list_mutex);
> >  	return rc;
> > @@ -451,14 +473,11 @@ ecryptfs_find_auth_tok_for_sig(
> >  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat,
> >  	char *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)) {
> > -
> > +	rc = ecryptfs_find_global_auth_tok_for_sig(auth_tok_key, auth_tok,
> > +						   mount_crypt_stat, sig);
> > +	if (rc == -ENOENT) {
> >  		/* if the flag ECRYPTFS_GLOBAL_MOUNT_AUTH_TOK_ONLY is set in the
> >  		 * mount_crypt_stat structure, we prevent to use auth toks that
> >  		 * are not inserted through the ecryptfs_add_global_auth_tok
> > @@ -470,8 +489,8 @@ ecryptfs_find_auth_tok_for_sig(
> > 
> >  		rc = ecryptfs_keyring_auth_tok_for_sig(auth_tok_key, auth_tok,
> >  						       sig);
> > -	} else
> > -		(*auth_tok) = global_auth_tok->global_auth_tok;
> > +	}
> > +
> >  	return rc;
> >  }
> > 
> > @@ -1566,7 +1585,23 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> >  		(*auth_tok_key) = NULL;
> >  		goto out;
> >  	}
> > -	(*auth_tok) = ecryptfs_get_key_payload_data(*auth_tok_key);
> > +
> > +	rc = ecryptfs_verify_auth_tok_from_key(*auth_tok_key, auth_tok);
> > +	if (rc) {
> > +		key_put(*auth_tok_key);
> > +		(*auth_tok_key) = NULL;
> > +		goto out;
> > +	}
> > +out:
> > +	return rc;
> > +}
> > +
> > +int ecryptfs_verify_auth_tok_from_key(struct key *auth_tok_key,
> > +				      struct ecryptfs_auth_tok **auth_tok)
> 
> Can be marked static.
> 
> > +{
> > +	int rc = 0;
> > +
> > +	(*auth_tok) = ecryptfs_get_key_payload_data(auth_tok_key);
> >  	if (ecryptfs_verify_version((*auth_tok)->version)) {
> >  		printk(KERN_ERR
> >  		       "Data structure version mismatch. "
> > @@ -1576,19 +1611,14 @@ int ecryptfs_keyring_auth_tok_for_sig(struct key **auth_tok_key,
> >  		       ECRYPTFS_VERSION_MAJOR,
> >  		       ECRYPTFS_VERSION_MINOR);
> >  		rc = -EINVAL;
> > -		goto out_release_key;
> > +		goto out;
> >  	}
> >  	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_release_key;
> > -	}
> > -out_release_key:
> > -	if (rc) {
> > -		key_put(*auth_tok_key);
> > -		(*auth_tok_key) = NULL;
> > +		goto out;
> >  	}
> >  out:
> >  	return rc;
> > @@ -2325,13 +2355,14 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> >  				 size_t max)
> >  {
> >  	struct ecryptfs_auth_tok *auth_tok;
> > -	struct ecryptfs_global_auth_tok *global_auth_tok;
> > +	struct key *auth_tok_key = NULL;
> >  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat =
> >  		&ecryptfs_superblock_to_private(
> >  			ecryptfs_dentry->d_sb)->mount_crypt_stat;
> >  	size_t written;
> >  	struct ecryptfs_key_record *key_rec;
> >  	struct ecryptfs_key_sig *key_sig;
> > +	int auth_tok_count = 0;
> >  	int rc = 0;
> > 
> >  	(*len) = 0;
> > @@ -2344,21 +2375,18 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> >  	list_for_each_entry(key_sig, &crypt_stat->keysig_list,
> >  			    crypt_stat_list) {
> >  		memset(key_rec, 0, sizeof(*key_rec));
> > -		rc = ecryptfs_find_global_auth_tok_for_sig(&global_auth_tok,
> > +		rc = ecryptfs_find_global_auth_tok_for_sig(&auth_tok_key,
> > +							   &auth_tok,
> >  							   mount_crypt_stat,
> >  							   key_sig->keysig);
> >  		if (rc) {
> > -			printk(KERN_ERR "Error attempting to get the global "
> > -			       "auth_tok; rc = [%d]\n", rc);
> > -			goto out_free;
> > -		}
> > -		if (global_auth_tok->flags & ECRYPTFS_AUTH_TOK_INVALID) {
> >  			printk(KERN_WARNING
> >  			       "Skipping invalid auth tok with sig = [%s]\n",
> 
> "invalid auth tok" isn't necessarily accurate here. A global auth tok
> with that sig simply may not have been found.
> 
> > -			       global_auth_tok->sig);
> > +			       key_sig->keysig);
> > +			rc = 0;
> >  			continue;
> 
> In my opinion, this is an error condition that can't be ignored. The
> user trusts us to wrap the file encryption key of all newly created
> files with the key associated with this auth tok. If the auth tok is not
> found or is unusable, I don't feel like we should act like everything is
> going as planned. What do you think?
> 

Hi Tyler

yes,  returning an error probably is the best choice, as this is a
way to say a new file cannot be created as expected.

Roberto


> >  		}
> > -		auth_tok = global_auth_tok->global_auth_tok;
> > +		auth_tok_count++;
> >  		if (auth_tok->token_type == ECRYPTFS_PASSWORD) {
> >  			rc = write_tag_3_packet((dest_base + (*len)),
> >  						&max, auth_tok,
> > @@ -2367,7 +2395,7 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> >  			if (rc) {
> >  				ecryptfs_printk(KERN_WARNING, "Error "
> >  						"writing tag 3 packet\n");
> > -				goto out_free;
> > +				goto out_release_key;
> >  			}
> >  			(*len) += written;
> >  			/* Write auth tok signature packet */
> > @@ -2377,7 +2405,7 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> >  			if (rc) {
> >  				ecryptfs_printk(KERN_ERR, "Error writing "
> >  						"auth tok signature packet\n");
> > -				goto out_free;
> > +				goto out_release_key;
> >  			}
> >  			(*len) += written;
> >  		} else if (auth_tok->token_type == ECRYPTFS_PRIVATE_KEY) {
> > @@ -2387,15 +2415,23 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> >  			if (rc) {
> >  				ecryptfs_printk(KERN_WARNING, "Error "
> >  						"writing tag 1 packet\n");
> > -				goto out_free;
> > +				goto out_release_key;
> >  			}
> >  			(*len) += written;
> >  		} else {
> >  			ecryptfs_printk(KERN_WARNING, "Unsupported "
> >  					"authentication token type\n");
> >  			rc = -EINVAL;
> > -			goto out_free;
> > +			goto out_release_key;
> >  		}
> > +		key_put(auth_tok_key);
> > +	}
> > +	if (!auth_tok_count) {
> > +		printk(KERN_WARNING
> > +			"Unable to create a new file without a valid "
> > +			"authentication token\n");
> > +		rc = -EINVAL;
> > +		goto out_free;
> >  	}
> >  	if (likely(max > 0)) {
> >  		dest_base[(*len)] = 0x00;
> > @@ -2403,6 +2439,9 @@ ecryptfs_generate_key_packet_set(char *dest_base,
> >  		ecryptfs_printk(KERN_ERR, "Error writing boundary byte\n");
> >  		rc = -EIO;
> >  	}
> > +out_release_key:
> > +	if (rc)
> > +		key_put(auth_tok_key);
> >  out_free:
> >  	kmem_cache_free(ecryptfs_key_record_cache, key_rec);
> >  out:
> > diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> > index 758323a..f079473 100644
> > --- a/fs/ecryptfs/main.c
> > +++ b/fs/ecryptfs/main.c
> > @@ -241,14 +241,14 @@ static int ecryptfs_init_global_auth_toks(
> >  	struct ecryptfs_mount_crypt_stat *mount_crypt_stat)
> >  {
> >  	struct ecryptfs_global_auth_tok *global_auth_tok;
> > +	struct ecryptfs_auth_tok *auth_tok;
> >  	int rc = 0;
> > 
> >  	list_for_each_entry(global_auth_tok,
> >  			    &mount_crypt_stat->global_auth_tok_list,
> >  			    mount_crypt_stat_list) {
> >  		rc = ecryptfs_keyring_auth_tok_for_sig(
> > -			&global_auth_tok->global_auth_tok_key,
> > -			&global_auth_tok->global_auth_tok,
> > +			&global_auth_tok->global_auth_tok_key, &auth_tok,
> >  			global_auth_tok->sig);
> >  		if (rc) {
> >  			printk(KERN_ERR "Could not find valid key in user "
> 
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [Samba]     [Device Mapper]     [CEPH Development]
  Powered by Linux