| Message ID | 1313048881-23568-1-git-send-email-john.johansen@canonical.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 11, 2011 at 12:48:00AM -0700, John Johansen wrote: > Close a TOCTOU race for mounts done via ecryptfs-mount-private. The mount > source (device) can be raced when the ownership test is done in userspace. > Provide Ecryptfs a means to force the uid check at mount time. > > (cherry picked from commit 764355487ea220fdc2faf128d577d7f679b91f97 git://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git for-linus) > CVE-2011-1833 > BugLink: http://bugs.launchpad.net/bugs/732628 > > Signed-off-by: John Johansen <john.johansen@canonical.com> > --- > fs/ecryptfs/main.c | 25 ++++++++++++++++++++++--- > 1 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c > index cbd4e18..2be138a 100644 > --- a/fs/ecryptfs/main.c > +++ b/fs/ecryptfs/main.c > @@ -208,7 +208,8 @@ enum { ecryptfs_opt_sig, ecryptfs_opt_ecryptfs_sig, > ecryptfs_opt_passthrough, ecryptfs_opt_xattr_metadata, > ecryptfs_opt_encrypted_view, ecryptfs_opt_fnek_sig, > ecryptfs_opt_fn_cipher, ecryptfs_opt_fn_cipher_key_bytes, > - ecryptfs_opt_unlink_sigs, ecryptfs_opt_err }; > + ecryptfs_opt_unlink_sigs, ecryptfs_opt_check_dev_ruid, > + ecryptfs_opt_err }; > > static const match_table_t tokens = { > {ecryptfs_opt_sig, "sig=%s"}, > @@ -223,6 +224,7 @@ static const match_table_t tokens = { > {ecryptfs_opt_fn_cipher, "ecryptfs_fn_cipher=%s"}, > {ecryptfs_opt_fn_cipher_key_bytes, "ecryptfs_fn_key_bytes=%u"}, > {ecryptfs_opt_unlink_sigs, "ecryptfs_unlink_sigs"}, > + {ecryptfs_opt_check_dev_ruid, "ecryptfs_check_dev_ruid"}, > {ecryptfs_opt_err, NULL} > }; > > @@ -266,6 +268,7 @@ static void ecryptfs_init_mount_crypt_stat( > * ecryptfs_parse_options > * @sb: The ecryptfs super block > * @options: The options pased to the kernel > + * @check_ruid: set to 1 if device uid should be checked against the ruid > * > * Parse mount options: > * debug=N - ecryptfs_verbosity level for debug output > @@ -281,7 +284,8 @@ static void ecryptfs_init_mount_crypt_stat( > * > * Returns zero on success; non-zero on error > */ > -static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) > +static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options, > + uid_t *check_ruid) > { > char *p; > int rc = 0; > @@ -306,6 +310,8 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) > char *cipher_key_bytes_src; > char *fn_cipher_key_bytes_src; > > + *check_ruid = 0; > + > if (!options) { > rc = -EINVAL; > goto out; > @@ -406,6 +412,9 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) > case ecryptfs_opt_unlink_sigs: > mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS; > break; > + case ecryptfs_opt_check_dev_ruid: > + *check_ruid = 1; > + break; > case ecryptfs_opt_err: > default: > printk(KERN_WARNING > @@ -497,6 +506,7 @@ static struct file_system_type ecryptfs_fs_type; > static int ecryptfs_read_super(struct super_block *sb, const char *dev_name) > { > struct path path; > + uid_t check_ruid; Where do we initialise this to anything? As far as I can tell we look this up in ecryptfs_get_sb() and then call ecryptfs_read_super() and yet do not pass it in, we then just use it in the next hunk. I suspect this actually should be a parameter to the function. > int rc; > > rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path); > @@ -511,6 +521,15 @@ static int ecryptfs_read_super(struct super_block *sb, const char *dev_name) > "known incompatibilities\n"); > goto out_free; > } > + > + if (check_ruid && path.dentry->d_inode->i_uid != current_uid()) { > + rc = -EPERM; > + printk(KERN_ERR "Mount of device (uid: %d) not owned by " > + "requested user (uid: %d)\n", > + path.dentry->d_inode->i_uid, current_uid()); > + goto out_free; > + } > + > ecryptfs_set_superblock_lower(sb, path.dentry->d_sb); > sb->s_maxbytes = path.dentry->d_sb->s_maxbytes; > sb->s_blocksize = path.dentry->d_sb->s_blocksize; > @@ -556,7 +575,7 @@ static int ecryptfs_get_sb(struct file_system_type *fs_type, int flags, > goto out; > } > > - rc = ecryptfs_parse_options(sbi, raw_data); > + rc = ecryptfs_parse_options(sbi, raw_data, &check_ruid); > if (rc) { > err = "Error parsing options"; > goto out; -apw
On 08/11/2011 06:33 AM, Andy Whitcroft wrote: > On Thu, Aug 11, 2011 at 12:48:00AM -0700, John Johansen wrote: >> Close a TOCTOU race for mounts done via ecryptfs-mount-private. The mount >> source (device) can be raced when the ownership test is done in userspace. >> Provide Ecryptfs a means to force the uid check at mount time. >> >> (cherry picked from commit 764355487ea220fdc2faf128d577d7f679b91f97 git://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git for-linus) >> CVE-2011-1833 >> BugLink: http://bugs.launchpad.net/bugs/732628 >> >> Signed-off-by: John Johansen<john.johansen@canonical.com> >> --- >> fs/ecryptfs/main.c | 25 ++++++++++++++++++++++--- >> 1 files changed, 22 insertions(+), 3 deletions(-) >> >> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c >> index cbd4e18..2be138a 100644 >> --- a/fs/ecryptfs/main.c >> +++ b/fs/ecryptfs/main.c >> @@ -208,7 +208,8 @@ enum { ecryptfs_opt_sig, ecryptfs_opt_ecryptfs_sig, >> ecryptfs_opt_passthrough, ecryptfs_opt_xattr_metadata, >> ecryptfs_opt_encrypted_view, ecryptfs_opt_fnek_sig, >> ecryptfs_opt_fn_cipher, ecryptfs_opt_fn_cipher_key_bytes, >> - ecryptfs_opt_unlink_sigs, ecryptfs_opt_err }; >> + ecryptfs_opt_unlink_sigs, ecryptfs_opt_check_dev_ruid, >> + ecryptfs_opt_err }; >> >> static const match_table_t tokens = { >> {ecryptfs_opt_sig, "sig=%s"}, >> @@ -223,6 +224,7 @@ static const match_table_t tokens = { >> {ecryptfs_opt_fn_cipher, "ecryptfs_fn_cipher=%s"}, >> {ecryptfs_opt_fn_cipher_key_bytes, "ecryptfs_fn_key_bytes=%u"}, >> {ecryptfs_opt_unlink_sigs, "ecryptfs_unlink_sigs"}, >> + {ecryptfs_opt_check_dev_ruid, "ecryptfs_check_dev_ruid"}, >> {ecryptfs_opt_err, NULL} >> }; >> >> @@ -266,6 +268,7 @@ static void ecryptfs_init_mount_crypt_stat( >> * ecryptfs_parse_options >> * @sb: The ecryptfs super block >> * @options: The options pased to the kernel >> + * @check_ruid: set to 1 if device uid should be checked against the ruid >> * >> * Parse mount options: >> * debug=N - ecryptfs_verbosity level for debug output >> @@ -281,7 +284,8 @@ static void ecryptfs_init_mount_crypt_stat( >> * >> * Returns zero on success; non-zero on error >> */ >> -static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) >> +static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options, >> + uid_t *check_ruid) >> { >> char *p; >> int rc = 0; >> @@ -306,6 +310,8 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) >> char *cipher_key_bytes_src; >> char *fn_cipher_key_bytes_src; >> >> + *check_ruid = 0; >> + >> if (!options) { >> rc = -EINVAL; >> goto out; >> @@ -406,6 +412,9 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) >> case ecryptfs_opt_unlink_sigs: >> mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS; >> break; >> + case ecryptfs_opt_check_dev_ruid: >> + *check_ruid = 1; >> + break; >> case ecryptfs_opt_err: >> default: >> printk(KERN_WARNING >> @@ -497,6 +506,7 @@ static struct file_system_type ecryptfs_fs_type; >> static int ecryptfs_read_super(struct super_block *sb, const char *dev_name) >> { >> struct path path; >> + uid_t check_ruid; > > Where do we initialise this to anything? As far as I can tell we look > this up in ecryptfs_get_sb() and then call ecryptfs_read_super() and yet > do not pass it in, we then just use it in the next hunk. I suspect this > actually should be a parameter to the function. > oops, it looks like I grabbed the wrong patch (its the natty one) and it applied :/. resubmitting with the correct patch.
diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c index cbd4e18..2be138a 100644 --- a/fs/ecryptfs/main.c +++ b/fs/ecryptfs/main.c @@ -208,7 +208,8 @@ enum { ecryptfs_opt_sig, ecryptfs_opt_ecryptfs_sig, ecryptfs_opt_passthrough, ecryptfs_opt_xattr_metadata, ecryptfs_opt_encrypted_view, ecryptfs_opt_fnek_sig, ecryptfs_opt_fn_cipher, ecryptfs_opt_fn_cipher_key_bytes, - ecryptfs_opt_unlink_sigs, ecryptfs_opt_err }; + ecryptfs_opt_unlink_sigs, ecryptfs_opt_check_dev_ruid, + ecryptfs_opt_err }; static const match_table_t tokens = { {ecryptfs_opt_sig, "sig=%s"}, @@ -223,6 +224,7 @@ static const match_table_t tokens = { {ecryptfs_opt_fn_cipher, "ecryptfs_fn_cipher=%s"}, {ecryptfs_opt_fn_cipher_key_bytes, "ecryptfs_fn_key_bytes=%u"}, {ecryptfs_opt_unlink_sigs, "ecryptfs_unlink_sigs"}, + {ecryptfs_opt_check_dev_ruid, "ecryptfs_check_dev_ruid"}, {ecryptfs_opt_err, NULL} }; @@ -266,6 +268,7 @@ static void ecryptfs_init_mount_crypt_stat( * ecryptfs_parse_options * @sb: The ecryptfs super block * @options: The options pased to the kernel + * @check_ruid: set to 1 if device uid should be checked against the ruid * * Parse mount options: * debug=N - ecryptfs_verbosity level for debug output @@ -281,7 +284,8 @@ static void ecryptfs_init_mount_crypt_stat( * * Returns zero on success; non-zero on error */ -static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) +static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options, + uid_t *check_ruid) { char *p; int rc = 0; @@ -306,6 +310,8 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) char *cipher_key_bytes_src; char *fn_cipher_key_bytes_src; + *check_ruid = 0; + if (!options) { rc = -EINVAL; goto out; @@ -406,6 +412,9 @@ static int ecryptfs_parse_options(struct ecryptfs_sb_info *sbi, char *options) case ecryptfs_opt_unlink_sigs: mount_crypt_stat->flags |= ECRYPTFS_UNLINK_SIGS; break; + case ecryptfs_opt_check_dev_ruid: + *check_ruid = 1; + break; case ecryptfs_opt_err: default: printk(KERN_WARNING @@ -497,6 +506,7 @@ static struct file_system_type ecryptfs_fs_type; static int ecryptfs_read_super(struct super_block *sb, const char *dev_name) { struct path path; + uid_t check_ruid; int rc; rc = kern_path(dev_name, LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path); @@ -511,6 +521,15 @@ static int ecryptfs_read_super(struct super_block *sb, const char *dev_name) "known incompatibilities\n"); goto out_free; } + + if (check_ruid && path.dentry->d_inode->i_uid != current_uid()) { + rc = -EPERM; + printk(KERN_ERR "Mount of device (uid: %d) not owned by " + "requested user (uid: %d)\n", + path.dentry->d_inode->i_uid, current_uid()); + goto out_free; + } + ecryptfs_set_superblock_lower(sb, path.dentry->d_sb); sb->s_maxbytes = path.dentry->d_sb->s_maxbytes; sb->s_blocksize = path.dentry->d_sb->s_blocksize; @@ -556,7 +575,7 @@ static int ecryptfs_get_sb(struct file_system_type *fs_type, int flags, goto out; } - rc = ecryptfs_parse_options(sbi, raw_data); + rc = ecryptfs_parse_options(sbi, raw_data, &check_ruid); if (rc) { err = "Error parsing options"; goto out;
Close a TOCTOU race for mounts done via ecryptfs-mount-private. The mount source (device) can be raced when the ownership test is done in userspace. Provide Ecryptfs a means to force the uid check at mount time. (cherry picked from commit 764355487ea220fdc2faf128d577d7f679b91f97 git://git.kernel.org/pub/scm/linux/kernel/git/ecryptfs/ecryptfs-2.6.git for-linus) CVE-2011-1833 BugLink: http://bugs.launchpad.net/bugs/732628 Signed-off-by: John Johansen <john.johansen@canonical.com> --- fs/ecryptfs/main.c | 25 ++++++++++++++++++++++--- 1 files changed, 22 insertions(+), 3 deletions(-)