Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Stephen Smalley <sds@tycho.nsa.gov> writes:

>> > > 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 <eparis@redhat.com>
>> > 
>> > 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.
>
> 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].

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>