Re: [PATCH 2/2] Version 9 (2.6.24-rc1) Smack: Simplified Mandatory Access Control Kernel

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: Stephen Smalley
Date: Thursday, October 25, 2007 - 8:07 am

On Wed, 2007-10-24 at 20:46 -0700, Casey Schaufler wrote:

Nit: SELinux does not implement DTE.  DTE was a scheme introduced by Lee
Badger et al to apply implicit typing based on pathname as a variant of
the original type enforcement model.  SELinux is an implementation of
the Flask security architecture for flexible MAC, along with an example
security server that implements RBAC, TE, and MLS models, but not
limited to them.


They are certainly hokey.  Move them to magic.h and include it.


Doesn't account for forced signals or signals from the kernel - see
selinux_task_kill() and check_kill_permission().    Or that could
possibly be handled once for all in check_kill_permission() and never
call the hook in that case.


Bug?  Under what conditions does this happen?
Don't try to silently hide real bugs.


Racy if it ever does happen.  Allocation should always happen from the
alloc hook.


Racy?


Bug?


Hmm...set before state is fully initialized, and no locking?


Bug?


Bug?


Hmmm...no check on the file itself, even though creating a hard link
changes the state of the inode?  If you only care about the directory,
then that should already have been handled by vfs_ functions calling
fs/namei.c:may_create() and checking both write and search to the
directory.  In which case you don't need to implement this hook at all.


Ditto.


And again.


vfs_rename() calls may_delete() on the old pair and either 1)
may_create() on the new pair if the target does not exist or 2)
may_delete() on the new pair if the target does exist.  For your
purposes, that means checking write and search on both directories
already before it reaches this hook.


You need an exception for iattr->ia_valid & ATTR_FORCE or you'll prevent
proper clearing of suid upon successful writes.  See
selinux_inode_setattr.


You don't want to invoke cap_inode_setxattr if the attribute was the
smack attribute; otherwise, it will end up checking CAP_SYS_ADMIN (for
anything in the security namespace).


Same as for setxattr.


At least some of those tests might hide a real bug elsewhere.


Race - allocation has to happen from the alloc hook.


Bug.


Hmmm...ordering of hooks is random?
Anyway, same comment here as for the earlier inode hooks - if you only
care about directory checks, that happens before they are reached.


And again.


Bug?


Bug?


Bug?


Bug?


Bug?


Bug?


You get the idea.


wait() is a write flow?  Parent is receiving status from child.


I'm a little surprised you do anything on wait at all, given that you
don't permit label changes on exec and your privilege model is
orthogonal to your MAC model.

wait hook is a bit questionable anyway as something has to ultimately
reap the child.

-- 
Stephen Smalley
National Security Agency

-
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
Re: [PATCH 2/2] Version 9 (2.6.24-rc1) Smack: Simplified M ..., Stephen Smalley, (Thu Oct 25, 8:07 am)