Failure to set acl may alter group permissions

When changing a file's acl mask, __ext4_set_acl() will first set the group bits of i_mode to the value of the mask, and only then set the actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file had no acl attribute to begin with, the system will from now on assume that the mask permission bits are actual group permission bits, potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by Ernesto A. Fernández <[email protected]>

---
 fs/ext4/acl.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

fs/ext4/acl.c +11 -4

index 09441ae..2985cd0 100644
@@ -189,16 +189,17 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
 	void *value = NULL;
 	size_t size = 0;
 	int error;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		name_index = EXT4_XATTR_INDEX_POSIX_ACL_ACCESS;
 		if (acl) {
-			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			error = posix_acl_update_mode(inode, &mode, &acl);
 			if (error)
 				return error;
-			inode->i_ctime = current_time(inode);
-			ext4_mark_inode_dirty(handle, inode);
+			update_mode = 1;
 		}
 		break;
 

@@ -221,8 +222,14 @@ __ext4_set_acl(handle_t *handle, struct inode *inode, int type,
 				      value, size, xattr_flags);
 
 	kfree(value);
-	if (!error)
+	if (!error) {
 		set_cached_acl(inode, type, acl);
+		if (update_mode) {
+			inode->i_mode = mode;
+			inode->i_ctime = current_time(inode);
+			ext4_mark_inode_dirty(handle, inode);
+		}
+	}
 
 	return error;
 }

Signature · 1 line

2.1.4

When changing a file's acl mask, __ext4_set_acl() will first set the group bits of i_mode to the value of the mask, and only then set the actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file had no acl attribute to begin with, the system will from now on assume that the mask permission bits are actual group permission bits, potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by Ernesto A. Fernández <[email protected]>

When changing a file's acl mask, ext2_set_acl() will first set the group bits of i_mode to the value of the mask, and only then set the actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file had no acl attribute to begin with, the system will from now on assume that the mask permission bits are actual group permission bits, potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by Ernesto A. Fernández <[email protected]>

---
A line in acl.c is too long, and checkpatch.pl complains about it when run against this patch. In case it matters, the next patch will fix that.

 fs/ext2/acl.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

fs/ext2/acl.c +11 -4

index 79dafa7..4e04b7e 100644
@@ -185,16 +185,17 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	void *value = NULL;
 	size_t size = 0;
 	int error;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	switch(type) {
 		case ACL_TYPE_ACCESS:
 			name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS;
 			if (acl) {
-				error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+				error = posix_acl_update_mode(inode, &mode, &acl);
 				if (error)
 					return error;
-				inode->i_ctime = current_time(inode);
-				mark_inode_dirty(inode);
+				update_mode = 1;
 			}
 			break;
 

@@ -216,8 +217,14 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	error = ext2_xattr_set(inode, name_index, "", value, size, 0);
 
 	kfree(value);
-	if (!error)
+	if (!error) {
 		set_cached_acl(inode, type, acl);
+		if (update_mode) {
+			inode->i_mode = mode;
+			inode->i_ctime = current_time(inode);
+			mark_inode_dirty(inode);
+		}
+	}
 	return error;
 }
 

Signature · 1 line

2.1.4

Shorten the name of the error variable to 'err' so the call to posix_acl_update_mode() fits within 80 characters.

Signed-off-by Ernesto A. Fernández <[email protected]>

---
 fs/ext2/acl.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

fs/ext2/acl.c +7 -7

index 4e04b7e..d2cca5c 100644
@@ -184,7 +184,7 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	int name_index;
 	void *value = NULL;
 	size_t size = 0;
-	int error;
+	int err;
 	int update_mode = 0;
 	umode_t mode = inode->i_mode;
 

@@ -192,9 +192,9 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		case ACL_TYPE_ACCESS:
 			name_index = EXT2_XATTR_INDEX_POSIX_ACL_ACCESS;
 			if (acl) {
-				error = posix_acl_update_mode(inode, &mode, &acl);
-				if (error)
-					return error;
+				err = posix_acl_update_mode(inode, &mode, &acl);
+				if (err)
+					return err;
 				update_mode = 1;
 			}
 			break;

@@ -214,10 +214,10 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			return (int)PTR_ERR(value);
 	}
 
-	error = ext2_xattr_set(inode, name_index, "", value, size, 0);
+	err = ext2_xattr_set(inode, name_index, "", value, size, 0);
 
 	kfree(value);
-	if (!error) {
+	if (!err) {
 		set_cached_acl(inode, type, acl);
 		if (update_mode) {
 			inode->i_mode = mode;

@@ -225,7 +225,7 @@ ext2_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 			mark_inode_dirty(inode);
 		}
 	}
-	return error;
+	return err;
 }
 
 /*

Signature · 1 line

2.1.4

When changing a file's acl mask, __jfs_set_acl() will first set the group bits of i_mode to the value of the mask, and only then set the actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file had no acl attribute to begin with, the system will from now on assume that the mask permission bits are actual group permission bits, potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set.

Signed-off-by Ernesto A. Fernández <[email protected]>

---
 fs/jfs/acl.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

fs/jfs/acl.c +5 -4

index 7bc186f..db5a385 100644
@@ -73,16 +73,17 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
 	int rc;
 	int size = 0;
 	char *value = NULL;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		ea_name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			rc = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			rc = posix_acl_update_mode(inode, &mode, &acl);
 			if (rc)
 				return rc;
-			inode->i_ctime = current_time(inode);
-			mark_inode_dirty(inode);
+			update_mode = 1;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:

@@ -105,8 +106,14 @@ static int __jfs_set_acl(tid_t tid, struct inode *inode, int type,
 out:
 	kfree(value);
 
-	if (!rc)
+	if (!rc) {

When changing a file's acl mask, __reiserfs_set_acl() will first set the group bits of i_mode to the value of the mask, and only then set the actual extended attribute representing the new acl.

If the second part fails (due to lack of space, for example) and the file had no acl attribute to begin with, the system will from now on assume that the mask permission bits are actual group permission bits, potentially granting access to the wrong users.

Prevent this by only changing the inode mode after the acl has been set. Also make reiserfs_xattr_set_handle() return -ENODATA only in case of actual error, and return 0 when requested deletion of a nonexistent acl. This is more consistent with the behaviour of other *_xattr_set() functions and makes the patch simpler.

Signed-off-by Ernesto A. Fernández <[email protected]>

---
 fs/reiserfs/xattr.c     |  4 ++++
 fs/reiserfs/xattr_acl.c | 25 +++++++++----------------
 2 files changed, 13 insertions(+), 16 deletions(-)

fs/reiserfs/xattr.c +4 -0

index e87aa21..936a57a 100644
@@ -513,6 +513,10 @@ reiserfs_xattr_set_handle(struct reiserfs_transaction_handle *th,
 
 	if (!buffer) {
 		err = lookup_and_delete_xattr(inode, name);
+		if (flags & XATTR_REPLACE)
+			return err;
+		if (err == -ENODATA)
+			err = 0;
 		return err;
 	}
 

fs/reiserfs/xattr_acl.c +9 -16

index 3d2256a..c02e5ac 100644
@@ -237,14 +237,17 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
 	void *value = NULL;
 	size_t size = 0;
 	int error;
+	int update_mode = 0;
+	umode_t mode = inode->i_mode;
 
 	switch (type) {
 	case ACL_TYPE_ACCESS:
 		name = XATTR_NAME_POSIX_ACL_ACCESS;
 		if (acl) {
-			error = posix_acl_update_mode(inode, &inode->i_mode, &acl);
+			error = posix_acl_update_mode(inode, &mode, &acl);
 			if (error)
 				return error;
+			update_mode = 1;
 		}
 		break;
 	case ACL_TYPE_DEFAULT:

@@ -264,25 +267,15 @@ __reiserfs_set_acl(struct reiserfs_transaction_handle *th, struct inode *inode,
 
 	error = reiserfs_xattr_set_handle(th, inode, name, value, size, 0);
 
-	/*
-	 * Ensure that the inode gets dirtied if we're only using
-	 * the mode bits and an old ACL didn't exist. We don't need
-	 * to check if the inode is hashed here since we won't get
-	 * called by reiserfs_inherit_default_acl().
-	 */
-	if (error == -ENODATA) {
-		error = 0;
-		if (type == ACL_TYPE_ACCESS) {
+	kfree(value);
+	if (!error) {
+		set_cached_acl(inode, type, acl);
+		if (update_mode) {
+			inode->i_mode = mode;
 			inode->i_ctime = current_time(inode);
 			mark_inode_dirty(inode);
 		}
 	}
-
-	kfree(value);
-
-	if (!error)
-		set_cached_acl(inode, type, acl);
-
 	return error;
 }
 

Signature · 1 line

2.1.4

If your working directory is on a filesystem mounted with extended user attributes (and acl of course), this script will trigger the issue by filling the drive:

touch test.file
chmod go-rwx test.file
yes xxxxxxxxxx > test.file
i=1
while setfattr -n user.$i test.file; do
((++i))
done
setfacl -m m:r test.file

Check that the group permission bits of a file are not altered when setfacl fails. At the time of this patch the test fails for at least ext2, ext4 and jfs. It is not run against reiserfs, since xfstests claims that "attrs are not supported by this filesystem type".

The failure to set acls is induced by filling the device, so to save time this should probably be run with a small TEST_DEV.

Quoted 4 lines · Ernesto A. Fernández

Check that the group permission bits of a file are not altered when setfacl fails. At the time of this patch the test fails for at least ext2, ext4 and jfs. It is not run against reiserfs, since xfstests claims that "attrs are not supported by this filesystem type".

That's odd, as common/config explicitly turns on user xattrs:

Quoted 4 lines

The failure to set acls is induced by filling the device, so to save time this should probably be run with a small TEST_DEV.

If you run it on the scratch fs instead of the test fs you can use _scratch_mkfs_sized to hand craft a small fs, which would be nice to get a decent runtime.