Patchwork Lucid CVE-2011-1833

login
register
mail settings
Submitter Tim Gardner
Date Aug. 11, 2011, 7:41 p.m.
Message ID <4E443070.8080801@canonical.com>
Download mbox | patch
Permalink /patch/109676/
State New
Headers show

Comments

Tim Gardner - Aug. 11, 2011, 7:41 p.m.
John Johansen - Aug. 11, 2011, 7:51 p.m.
On 08/11/2011 12:41 PM, Tim Gardner wrote:
>  From bb684cd8022bd9a6a5eb5fda9d61009df80e11d0 Mon Sep 17 00:00:00 2001
> From: Tim Gardner<tim.gardner@canonical.com>
> Date: Thu, 11 Aug 2011 13:33:18 -0600
> Subject: [PATCH] Add mount option to check uid of device being mounted = expect uid, CVE-2011-1833
>
> 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.
>
> (backported from commit 764355487ea220fdc2faf128d577d7f679b91f97)
> CVE-2011-1833
> BugLink:http://bugs.launchpad.net/bugs/732628
>
> Signed-off-by: John Johansen<john.johansen@canonical.com>
> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>

yep, I am good with the change of passing check_ruid down into ecryptfs_read_super

Acked-by: John Johansen <john.johansen@canonical.com>


> ---
>   fs/ecryptfs/main.c |   30 +++++++++++++++++++++++++-----
>   1 files changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
> index c6ac85d..f6cd392 100644
> --- a/fs/ecryptfs/main.c
> +++ b/fs/ecryptfs/main.c
> @@ -212,7 +212,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"},
> @@ -227,6 +228,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}
>   };
>
> @@ -270,6 +272,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
> @@ -285,7 +288,8 @@ static void ecryptfs_init_mount_crypt_stat(
>    *
>    * Returns zero on success; non-zero on error
>    */
> -static int ecryptfs_parse_options(struct super_block *sb, char *options)
> +static int ecryptfs_parse_options(struct super_block *sb, char *options,
> +					uid_t *check_ruid)
>   {
>   	char *p;
>   	int rc = 0;
> @@ -310,6 +314,8 @@ static int ecryptfs_parse_options(struct super_block *sb, char *options)
>   	char *cipher_key_bytes_src;
>   	char *fn_cipher_key_bytes_src;
>
> +	*check_ruid = 0;
> +
>   	if (!options) {
>   		rc = -EINVAL;
>   		goto out;
> @@ -410,6 +416,9 @@ static int ecryptfs_parse_options(struct super_block *sb, 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
> @@ -551,7 +560,8 @@ out:
>    * ecryptfs_interpose to create our initial inode and super block
>    * struct.
>    */
> -static int ecryptfs_read_super(struct super_block *sb, const char *dev_name)
> +static int ecryptfs_read_super(struct super_block *sb, const char *dev_name,
> +				uid_t check_ruid)
>   {
>   	struct path path;
>   	int rc;
> @@ -561,6 +571,15 @@ static int ecryptfs_read_super(struct super_block *sb, const char *dev_name)
>   		ecryptfs_printk(KERN_WARNING, "path_lookup() failed\n");
>   		goto out;
>   	}
> +
> +	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;
> @@ -599,6 +618,7 @@ static int ecryptfs_get_sb(struct file_system_type *fs_type, int flags,
>   {
>   	int rc;
>   	struct super_block *sb;
> +	uid_t check_ruid;
>
>   	rc = get_sb_nodev(fs_type, flags, raw_data, ecryptfs_fill_super, mnt);
>   	if (rc<  0) {
> @@ -606,12 +626,12 @@ static int ecryptfs_get_sb(struct file_system_type *fs_type, int flags,
>   		goto out;
>   	}
>   	sb = mnt->mnt_sb;
> -	rc = ecryptfs_parse_options(sb, raw_data);
> +	rc = ecryptfs_parse_options(sb, raw_data,&check_ruid);
>   	if (rc) {
>   		printk(KERN_ERR "Error parsing options; rc = [%d]\n", rc);
>   		goto out_abort;
>   	}
> -	rc = ecryptfs_read_super(sb, dev_name);
> +	rc = ecryptfs_read_super(sb, dev_name, check_ruid);
>   	if (rc) {
>   		printk(KERN_ERR "Reading sb failed; rc = [%d]\n", rc);
>   		goto out_abort;
> -- 1.7.0.4
Tim Gardner - Aug. 11, 2011, 7:54 p.m.
On 08/11/2011 01:51 PM, John Johansen wrote:
> On 08/11/2011 12:41 PM, Tim Gardner wrote:
>> From bb684cd8022bd9a6a5eb5fda9d61009df80e11d0 Mon Sep 17 00:00:00 2001
>> From: Tim Gardner<tim.gardner@canonical.com>
>> Date: Thu, 11 Aug 2011 13:33:18 -0600
>> Subject: [PATCH] Add mount option to check uid of device being mounted
>> = expect uid, CVE-2011-1833
>>
>> 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.
>>
>> (backported from commit 764355487ea220fdc2faf128d577d7f679b91f97)
>> CVE-2011-1833
>> BugLink:http://bugs.launchpad.net/bugs/732628
>>
>> Signed-off-by: John Johansen<john.johansen@canonical.com>
>> Signed-off-by: Tim Gardner<tim.gardner@canonical.com>
>
> yep, I am good with the change of passing check_ruid down into
> ecryptfs_read_super
>
> Acked-by: John Johansen <john.johansen@canonical.com>
>
>
>> ---
>> fs/ecryptfs/main.c | 30 +++++++++++++++++++++++++-----
>> 1 files changed, 25 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
>> index c6ac85d..f6cd392 100644
>> --- a/fs/ecryptfs/main.c
>> +++ b/fs/ecryptfs/main.c
>> @@ -212,7 +212,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"},
>> @@ -227,6 +228,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}
>> };
>>
>> @@ -270,6 +272,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
>> @@ -285,7 +288,8 @@ static void ecryptfs_init_mount_crypt_stat(
>> *
>> * Returns zero on success; non-zero on error
>> */
>> -static int ecryptfs_parse_options(struct super_block *sb, char *options)
>> +static int ecryptfs_parse_options(struct super_block *sb, char *options,
>> + uid_t *check_ruid)
>> {
>> char *p;
>> int rc = 0;
>> @@ -310,6 +314,8 @@ static int ecryptfs_parse_options(struct
>> super_block *sb, char *options)
>> char *cipher_key_bytes_src;
>> char *fn_cipher_key_bytes_src;
>>
>> + *check_ruid = 0;
>> +
>> if (!options) {
>> rc = -EINVAL;
>> goto out;
>> @@ -410,6 +416,9 @@ static int ecryptfs_parse_options(struct
>> super_block *sb, 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
>> @@ -551,7 +560,8 @@ out:
>> * ecryptfs_interpose to create our initial inode and super block
>> * struct.
>> */
>> -static int ecryptfs_read_super(struct super_block *sb, const char
>> *dev_name)
>> +static int ecryptfs_read_super(struct super_block *sb, const char
>> *dev_name,
>> + uid_t check_ruid)
>> {
>> struct path path;
>> int rc;
>> @@ -561,6 +571,15 @@ static int ecryptfs_read_super(struct super_block
>> *sb, const char *dev_name)
>> ecryptfs_printk(KERN_WARNING, "path_lookup() failed\n");
>> goto out;
>> }
>> +
>> + 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;
>> @@ -599,6 +618,7 @@ static int ecryptfs_get_sb(struct file_system_type
>> *fs_type, int flags,
>> {
>> int rc;
>> struct super_block *sb;
>> + uid_t check_ruid;
>>
>> rc = get_sb_nodev(fs_type, flags, raw_data, ecryptfs_fill_super, mnt);
>> if (rc< 0) {
>> @@ -606,12 +626,12 @@ static int ecryptfs_get_sb(struct
>> file_system_type *fs_type, int flags,
>> goto out;
>> }
>> sb = mnt->mnt_sb;
>> - rc = ecryptfs_parse_options(sb, raw_data);
>> + rc = ecryptfs_parse_options(sb, raw_data,&check_ruid);
>> if (rc) {
>> printk(KERN_ERR "Error parsing options; rc = [%d]\n", rc);
>> goto out_abort;
>> }
>> - rc = ecryptfs_read_super(sb, dev_name);
>> + rc = ecryptfs_read_super(sb, dev_name, check_ruid);
>> if (rc) {
>> printk(KERN_ERR "Reading sb failed; rc = [%d]\n", rc);
>> goto out_abort;
>> -- 1.7.0.4
>

Patch

From bb684cd8022bd9a6a5eb5fda9d61009df80e11d0 Mon Sep 17 00:00:00 2001
From: Tim Gardner <tim.gardner@canonical.com>
Date: Thu, 11 Aug 2011 13:33:18 -0600
Subject: [PATCH] Add mount option to check uid of device being mounted = expect uid, CVE-2011-1833

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.

(backported from commit 764355487ea220fdc2faf128d577d7f679b91f97)
CVE-2011-1833
BugLink: http://bugs.launchpad.net/bugs/732628

Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Tim Gardner <tim.gardner@canonical.com>
---
 fs/ecryptfs/main.c |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/fs/ecryptfs/main.c b/fs/ecryptfs/main.c
index c6ac85d..f6cd392 100644
--- a/fs/ecryptfs/main.c
+++ b/fs/ecryptfs/main.c
@@ -212,7 +212,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"},
@@ -227,6 +228,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}
 };
 
@@ -270,6 +272,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
@@ -285,7 +288,8 @@  static void ecryptfs_init_mount_crypt_stat(
  *
  * Returns zero on success; non-zero on error
  */
-static int ecryptfs_parse_options(struct super_block *sb, char *options)
+static int ecryptfs_parse_options(struct super_block *sb, char *options,
+					uid_t *check_ruid)
 {
 	char *p;
 	int rc = 0;
@@ -310,6 +314,8 @@  static int ecryptfs_parse_options(struct super_block *sb, char *options)
 	char *cipher_key_bytes_src;
 	char *fn_cipher_key_bytes_src;
 
+	*check_ruid = 0;
+
 	if (!options) {
 		rc = -EINVAL;
 		goto out;
@@ -410,6 +416,9 @@  static int ecryptfs_parse_options(struct super_block *sb, 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
@@ -551,7 +560,8 @@  out:
  * ecryptfs_interpose to create our initial inode and super block
  * struct.
  */
-static int ecryptfs_read_super(struct super_block *sb, const char *dev_name)
+static int ecryptfs_read_super(struct super_block *sb, const char *dev_name,
+				uid_t check_ruid)
 {
 	struct path path;
 	int rc;
@@ -561,6 +571,15 @@  static int ecryptfs_read_super(struct super_block *sb, const char *dev_name)
 		ecryptfs_printk(KERN_WARNING, "path_lookup() failed\n");
 		goto out;
 	}
+
+	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;
@@ -599,6 +618,7 @@  static int ecryptfs_get_sb(struct file_system_type *fs_type, int flags,
 {
 	int rc;
 	struct super_block *sb;
+	uid_t check_ruid;
 
 	rc = get_sb_nodev(fs_type, flags, raw_data, ecryptfs_fill_super, mnt);
 	if (rc < 0) {
@@ -606,12 +626,12 @@  static int ecryptfs_get_sb(struct file_system_type *fs_type, int flags,
 		goto out;
 	}
 	sb = mnt->mnt_sb;
-	rc = ecryptfs_parse_options(sb, raw_data);
+	rc = ecryptfs_parse_options(sb, raw_data, &check_ruid);
 	if (rc) {
 		printk(KERN_ERR "Error parsing options; rc = [%d]\n", rc);
 		goto out_abort;
 	}
-	rc = ecryptfs_read_super(sb, dev_name);
+	rc = ecryptfs_read_super(sb, dev_name, check_ruid);
 	if (rc) {
 		printk(KERN_ERR "Reading sb failed; rc = [%d]\n", rc);
 		goto out_abort;
-- 
1.7.0.4