OGAWA Hirofumi wrote:
Stephen Smalley <sds@xxxxxxxxxxxxx> writes:
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)
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.
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.
I see. Yes, exactly. And for the user of non file owner case, I'm
thinking we would like to pass ATTR_FORCE too.
Ok, I know what you are talking about now...
Maybe I should make another patch to fix this... :-/ I need some time to
understand SELinux, before I finish it, keeping this patch is fine.
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
