vfs: allow file truncations when both suid and write permissions set

V2 -> V3:
Call notify_change() before clearing suid/sgid.
Thanks to OGAWA Hirofumi.

V1 -> V2:
Introduce dentry_remove_suid(), and use it in do_truncate(). Thanks to Eric Paris.

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

Steps to reproduce this bug:

% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1 % ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1

This patch fixes it.

Signed-off-by WANG Cong <[email protected]>

Cc Eric Sandeen <[email protected]>

Cc Eric Paris <[email protected]>

Cc Eugene Teo <[email protected]>

Cc Al Viro <[email protected]>

Cc [email protected]

Quoted 28 lines · Amerigo Wang

V2 -> V3:
Call notify_change() before clearing suid/sgid.
Thanks to OGAWA Hirofumi.

V1 -> V2:
Introduce dentry_remove_suid(), and use it in do_truncate(). Thanks to Eric Paris.

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

Steps to reproduce this bug:

% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1 % ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1

Quoted 8 lines

This patch fixes it.

Signed-off-by WANG Cong <[email protected]>

Cc Eric Sandeen <[email protected]>

Cc Eric Paris <[email protected]>

Cc Eugene Teo <[email protected]>

Cc Al Viro <[email protected]>

Cc [email protected]

I was thinking about this and kept telling myself I was going to test v2 before I ack/nak. Clearly we shouldn't for the dropping of SUID if the process didn't have permission to change the ATTR_SIZE.

Acked-by Eric Paris <[email protected]>

Quoted 42 lines · Eric Paris

Quoted 28 lines · Amerigo Wang

V2 -> V3:
Call notify_change() before clearing suid/sgid.
Thanks to OGAWA Hirofumi.

V1 -> V2:
Introduce dentry_remove_suid(), and use it in do_truncate(). Thanks to Eric Paris.

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

Steps to reproduce this bug:

% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1 % ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1

Quoted 8 lines

This patch fixes it.

Signed-off-by WANG Cong <[email protected]>

Cc Eric Sandeen <[email protected]>

Cc Eric Paris <[email protected]>

Cc Eugene Teo <[email protected]>

Cc Al Viro <[email protected]>

Cc [email protected]

I was thinking about this and kept telling myself I was going to test v2 before I ack/nak. Clearly we shouldn't for the dropping of SUID if the process didn't have permission to change the ATTR_SIZE.

Quoted 1 line

Acked-by Eric Paris <[email protected]>

BTW, Do you know why doesn't security modules fix the handling of do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to pass ATTR_FORCE for it?

Quoted 9 lines

Quoted 5 lines

I was thinking about this and kept telling myself I was going to test v2 before I ack/nak. Clearly we shouldn't for the dropping of SUID if the process didn't have permission to change the ATTR_SIZE.

Acked-by Eric Paris <[email protected]>

BTW, Do you know why doesn't security modules fix the handling of do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to pass ATTR_FORCE for it?

I'm not sure what you mean. I understood ATTR_FORCE to mean 'I am magic and get to override all security checks." Which is why nothing should ever be using ATTR_FORCE with things other than SUID.

I guess we could somehow force logic into the LSM to make it only apply to SUID and friends but I'm not sure it buys us anything.

Quoted 10 lines · Eric Paris

Quoted 10 lines

Quoted 6 lines

I was thinking about this and kept telling myself I was going to test v2 before I ack/nak. Clearly we shouldn't for the dropping of SUID if the process didn't have permission to change the ATTR_SIZE.

Acked-by Eric Paris <[email protected]>

BTW, Do you know why doesn't security modules fix the handling of do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to pass ATTR_FORCE for it?

Quoted 7 lines

I'm not sure what you mean. I understood ATTR_FORCE to mean 'I am magic and get to override all security checks." Which is why nothing should ever be using ATTR_FORCE with things other than SUID.

I guess we could somehow force logic into the LSM to make it only apply to SUID and friends but I'm not sure it buys us anything.

Yes, I think it's good way. Don't we want to do the following?

	if (permission check of job)
		return error;
	if (do job at once)
        	return error;

But currently way is,

if (permission check of first part)
return error
if (do first part of job)
return error
if (permission check of second part)
return error
if (do second part of job)
return error

So, if second part was error, we may want to undo the job of first part in theory. But, to undo is just hard and strange.

Quoted 39 lines · OGAWA Hirofumi

Quoted 10 lines · Eric Paris

Quoted 10 lines

Quoted 6 lines

I was thinking about this and kept telling myself I was going to test v2 before I ack/nak. Clearly we shouldn't for the dropping of SUID if the process didn't have permission to change the ATTR_SIZE.

Acked-by Eric Paris <[email protected]>

BTW, Do you know why doesn't security modules fix the handling of do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to pass ATTR_FORCE for it?

Quoted 7 lines

I'm not sure what you mean. I understood ATTR_FORCE to mean 'I am magic and get to override all security checks." Which is why nothing should ever be using ATTR_FORCE with things other than SUID.

I guess we could somehow force logic into the LSM to make it only apply to SUID and friends but I'm not sure it buys us anything.

Yes, I think it's good way. Don't we want to do the following?

	if (permission check of job)
		return error;
	if (do job at once)
        	return error;

But currently way is,

if (permission check of first part)
return error
if (do first part of job)
return error
if (permission check of second part)
return error
if (do second part of job)
return error

Quoted 3 lines

So, if second part was error, we may want to undo the job of first part in theory. But, to undo is just hard and strange.

Yeah, the problem is currently we don't have such wrappers, only notify_change(). :-/

Quoted 44 lines · OGAWA Hirofumi

Quoted 42 lines · Eric Paris

Quoted 28 lines · Amerigo Wang

V2 -> V3:
Call notify_change() before clearing suid/sgid.
Thanks to OGAWA Hirofumi.

V1 -> V2:
Introduce dentry_remove_suid(), and use it in do_truncate(). Thanks to Eric Paris.

When suid is set and the non-owner user has write permission, any writing into this file should be allowed and suid should be removed after that.

However, current kernel only allows writing without truncations, when we do truncations on that file, we get EPERM. This is a bug.

Steps to reproduce this bug:

% ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h > rootdir/file1
zsh: operation not permitted: rootdir/file1 % ls -l rootdir/file1
-rwsrwsrwx 1 root root 3 Jun 25 15:42 rootdir/file1 % echo h >> rootdir/file1
% ls -l rootdir/file1
-rwxrwxrwx 1 root root 5 Jun 25 16:34 rootdir/file1

Quoted 8 lines

This patch fixes it.

Signed-off-by WANG Cong <[email protected]>

Cc Eric Sandeen <[email protected]>

Cc Eric Paris <[email protected]>

Cc Eugene Teo <[email protected]>

Cc Al Viro <[email protected]>

Cc [email protected]

I was thinking about this and kept telling myself I was going to test v2 before I ack/nak. Clearly we shouldn't for the dropping of SUID if the process didn't have permission to change the ATTR_SIZE.

Quoted 5 lines

Quoted 1 line

Acked-by Eric Paris <[email protected]>

BTW, Do you know why doesn't security modules fix the handling of do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to pass ATTR_FORCE for it?

I'm not sure what you mean. I understood ATTR_FORCE to mean 'I am magic and get to override all security checks." Which is why nothing should ever be using ATTR_FORCE with things other than SUID.

I guess we could somehow force logic into the LSM to make it only apply to SUID and friends but I'm not sure it buys us anything.

Quoted 14 lines

Quoted 9 lines

Quoted 5 lines

I was thinking about this and kept telling myself I was going to test v2 before I ack/nak. Clearly we shouldn't for the dropping of SUID if the process didn't have permission to change the ATTR_SIZE.

Acked-by Eric Paris <[email protected]>

BTW, Do you know why doesn't security modules fix the handling of do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to pass ATTR_FORCE for it?

I'm not sure what you mean. I understood ATTR_FORCE to mean 'I am magic and get to override all security checks." Which is why nothing should ever be using ATTR_FORCE with things other than SUID.

Quoted 2 lines

I guess we could somehow force logic into the LSM to make it only apply to SUID and friends but I'm not sure it buys us anything.

SELinux shouldn't apply a permission check for the clearing of the suid bit on write or truncate. It should only apply a permission check for the actual truncate or write operation, and then the clearing of the suid bit should always be forced if that check passed.

Quoted 14 lines · Stephen Smalley

Quoted 14 lines

Quoted 9 lines

Quoted 5 lines

I was thinking about this and kept telling myself I was going to test v2 before I ack/nak. Clearly we shouldn't for the dropping of SUID if the process didn't have permission to change the ATTR_SIZE.

Acked-by Eric Paris <[email protected]>

BTW, Do you know why doesn't security modules fix the handling of do_truncate() (i.e. ATTR_MODE | ATTR_SIZE). And why doesn't it allow to pass ATTR_FORCE for it?

I'm not sure what you mean. I understood ATTR_FORCE to mean 'I am magic and get to override all security checks." Which is why nothing should ever be using ATTR_FORCE with things other than SUID.

Quoted 7 lines

Quoted 2 lines

I guess we could somehow force logic into the LSM to make it only apply to SUID and friends but I'm not sure it buys us anything.

SELinux shouldn't apply a permission check for the clearing of the suid bit on write or truncate. It should only apply a permission check for the actual truncate or write operation, and then the clearing of the suid bit should always be forced if that check passed.

Ok. Yes. So, to do it efficiently without problem, I'm suggesting the following or something (I don't know whether LSM should do this or not).

selinux_inode_setattr(),

	ia_valid = iattr->ia_valid;
	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
		if (err)
                	return err;
		ia_valid &= ~ATTR_FORCE_MASK;
	}
	if (ia_valid & ATTR_NOT_FORCE_MASK)
	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
	return err;

I guess ATTR_FORCE_MASK would be (ATTR_MODE | ATTR_UID | ATTR_GID |
ATTR_ATIME_SET | ATTR_MTIME_SET) or something,
and ATTR_NOT_FORCE_MASK would be ATTR_SIZE or something.

I'm not sure this is the right code what selinux want to do though, but, I hope it is clear what I want to say. (I'm assuming FILE__WRITE is for check of ATTR_SIZE)

With this change, the caller can pass "(ATTR_SIZE | ATTR_MODE)" or "(ATTR_SIZE | ATTR_MODE | ATTR_FORCE)" etc. for truncate().

[btw, "(ATTR_SIZE | ATTR_MODE)" is what do_truncate() does currently].

Quoted 21 lines

Quoted 4 lines

SELinux shouldn't apply a permission check for the clearing of the suid bit on write or truncate. It should only apply a permission check for the actual truncate or write operation, and then the clearing of the suid bit should always be forced if that check passed.

Ok. Yes. So, to do it efficiently without problem, I'm suggesting the following or something (I don't know whether LSM should do this or not).

selinux_inode_setattr(),

	ia_valid = iattr->ia_valid;
	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
		if (err)
                	return err;
		ia_valid &= ~ATTR_FORCE_MASK;
	}
	if (ia_valid & ATTR_NOT_FORCE_MASK)
	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
	return err;

Quoted 7 lines

I guess ATTR_FORCE_MASK would be (ATTR_MODE | ATTR_UID | ATTR_GID |
ATTR_ATIME_SET | ATTR_MTIME_SET) or something,
and ATTR_NOT_FORCE_MASK would be ATTR_SIZE or something.

I'm not sure this is the right code what selinux want to do though, but, I hope it is clear what I want to say. (I'm assuming FILE__WRITE is for check of ATTR_SIZE)

The logic is supposed to map certain attribute changes (mode, owner, group, explicit setting of atime or mtime to a specific value rather than the current time) to the SELinux setattr permission, while mapping other attribute changes that occur naturally on a write (size, setting of mtime to current time) to the SELinux write permission. That doesn't seem clear from using ATTR_FORCE_MASK vs ATTR_NOT_FORCE_MASK above - I'd use different naming conventions for clarity.

Quoted 4 lines

With this change, the caller can pass "(ATTR_SIZE | ATTR_MODE)" or "(ATTR_SIZE | ATTR_MODE | ATTR_FORCE)" etc. for truncate().

[btw, "(ATTR_SIZE | ATTR_MODE)" is what do_truncate() does currently].

That was a change in do_truncate(), commit
7b82dc0e64e93f430182f36b46b79fcee87d3532.

It makes sense, but no one ever updated selinux_inode_setattr() to match that change.

Quoted 29 lines · Stephen Smalley

Quoted 21 lines

Quoted 5 lines

SELinux shouldn't apply a permission check for the clearing of the suid bit on write or truncate. It should only apply a permission check for the actual truncate or write operation, and then the clearing of the suid bit should always be forced if that check passed.

Ok. Yes. So, to do it efficiently without problem, I'm suggesting the following or something (I don't know whether LSM should do this or not).

selinux_inode_setattr(),

	ia_valid = iattr->ia_valid;
	if (!(ia_valid & ATTR_FORCE) && (ia_valid & ATTR_FORCE_MASK)) {
		err = dentry_has_perm(cred, NULL, dentry, FILE__SETATTR);
		if (err)
                	return err;
		ia_valid &= ~ATTR_FORCE_MASK;
	}
	if (ia_valid & ATTR_NOT_FORCE_MASK)
	 	err = dentry_has_perm(cred, NULL, dentry, FILE__WRITE);
	return err;

Quoted 8 lines

I guess ATTR_FORCE_MASK would be (ATTR_MODE | ATTR_UID | ATTR_GID |
ATTR_ATIME_SET | ATTR_MTIME_SET) or something,
and ATTR_NOT_FORCE_MASK would be ATTR_SIZE or something.

I'm not sure this is the right code what selinux want to do though, but, I hope it is clear what I want to say. (I'm assuming FILE__WRITE is for check of ATTR_SIZE)

Quoted 8 lines

The logic is supposed to map certain attribute changes (mode, owner, group, explicit setting of atime or mtime to a specific value rather than the current time) to the SELinux setattr permission, while mapping other attribute changes that occur naturally on a write (size, setting of mtime to current time) to the SELinux write permission. That doesn't seem clear from using ATTR_FORCE_MASK vs ATTR_NOT_FORCE_MASK above - I'd use different naming conventions for clarity.

I see. Yes, the naming of this code doesn't matter at all. The code was just intended to explain what I'm suggesting.

Quoted 8 lines

Quoted 5 lines

With this change, the caller can pass "(ATTR_SIZE | ATTR_MODE)" or "(ATTR_SIZE | ATTR_MODE | ATTR_FORCE)" etc. for truncate().

[btw, "(ATTR_SIZE | ATTR_MODE)" is what do_truncate() does currently].

That was a change in do_truncate(), commit
7b82dc0e64e93f430182f36b46b79fcee87d3532.

Quoted 3 lines

It makes sense, but no one ever updated selinux_inode_setattr() to match that change.

I see. Yes, exactly. And for the user of non file owner case, I'm thinking we would like to pass ATTR_FORCE too.