Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932375Ab1CRJbh (ORCPT ); Fri, 18 Mar 2011 05:31:37 -0400 Received: from out2.smtp.messagingengine.com ([66.111.4.26]:52128 "EHLO out2.smtp.messagingengine.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932216Ab1CRJba (ORCPT ); Fri, 18 Mar 2011 05:31:30 -0400 X-Greylist: delayed 441 seconds by postgrey-1.27 at vger.kernel.org; Fri, 18 Mar 2011 05:31:30 EDT X-Sasl-enc: GX2vM/OGrTtcd8kZJR+eJt2SoiufEW8L49kX4pEpkCRw 1300440249 From: Roberto Sassu Organization: Politecnico di Torino To: Tyler Hicks Subject: Re: [PATCH 3/5] eCryptfs: verify authentication tokens before their use Date: Fri, 18 Mar 2011 10:21:20 +0100 User-Agent: KMail/1.13.6 (Linux/2.6.35.11-83.fc14.x86_64; KDE/4.5.5; x86_64; ; ) Cc: kirkland@canonical.com, dhowells@redhat.com, jmorris@namei.org, linux-fsdevel@vger.kernel.org, keyrings@linux-nfs.org, linux-kernel@vger.kernel.org, ecryptfs-devel@lists.launchpad.net References: <1300362538-11502-1-git-send-email-roberto.sassu@polito.it> <1300362538-11502-4-git-send-email-roberto.sassu@polito.it> <20110318030539.GA16889@boyd.l.tihix.com> In-Reply-To: <20110318030539.GA16889@boyd.l.tihix.com> MIME-Version: 1.0 Message-Id: <201103181021.20448.roberto.sassu@polito.it> Content-Type: Text/Plain; charset="iso-8859-1" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11852 Lines: 342 On Friday, March 18, 2011 04:05:39 am Tyler Hicks wrote: > On Thu Mar 17, 2011 at 12:48:52PM +0100, Roberto Sassu 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 > > --- > > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/