Messages in this thread | | | Date | Wed, 12 Aug 2009 17:03:12 +0800 | From | Amerigo Wang <> | Subject | Re: [Patch v3] vfs: allow file truncations when both suid and write permissions set |
| |
OGAWA Hirofumi wrote: > Stephen Smalley <sds@tycho.nsa.gov> 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!
| |