| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 18 Mar 2011 10:21:20 +0100
From: Roberto Sassu <roberto.sassu@...ito.it>
To: Tyler Hicks <tyhicks@...ux.vnet.ibm.com>
Cc: kirkland@...onical.com, dhowells@...hat.com, jmorris@...ei.org,
linux-fsdevel@...r.kernel.org, keyrings@...ux-nfs.org,
linux-kernel@...r.kernel.org, ecryptfs-devel@...ts.launchpad.net
Subject: Re: [PATCH 3/5] eCryptfs: verify authentication tokens before their use
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@...ito.it> 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@...ito.it>
> > ---
> > 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@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists