Patchwork [1/2,maverick,CVE,1/2] Add mount option to check uid of device being mounted = expect uid

login
register
mail settings
Submitter John Johansen
Date Aug. 11, 2011, 7:48 a.m.
Message ID <1313048881-23568-1-git-send-email-john.johansen@canonical.com>
Download mbox | patch
Permalink /patch/109564/
State New
Headers show

Comments

John Johansen - Aug. 11, 2011, 7:48 a.m.
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(-)
Andy Whitcroft - Aug. 11, 2011, 1:33 p.m.
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
John Johansen - Aug. 11, 2011, 4:52 p.m.
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.

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;