Re: Some feedback on the hooks

From: Chris Wright (chrisat_private)
Date: Wed Apr 25 2001 - 19:03:30 PDT

  • Next message: Chris Wright: "Re: 2001_04_25 patch against 2.4.3"

    * Stephen Smalley (sdsat_private) wrote:
    > 
    > Here is some feedback on the hooks in the current patch.  This
    > is by no means a complete assessment, just a start.  We'd be willing to
    > assist with implementing some of these proposed changes and additions, but
    > you'll have to let us know what areas we can safely work on without
    > conflicting with your ongoing changes.
    
    Stephen, thanks for the execllent feedback!  I'm not sure of the best way to
    divide the work.  For starters, getting your specific needs is paramount.
    
    > arch/i386/kernel/ptrace.c (sys_ptrace):  SELinux performs
    > a check between the SIDs of the "tracer" and the "tracee".
    > Which process is the tracer and which is the tracee depends
    > on the request (PTRACE_TRACEME vs. the others).  So for SELinux,
    > it would be best if two task_struct pointers were passed to the
    > hook, and if the hook was called from the PTRACE_TRACEME block
    > on (current->p_pptr, current) and after the child lookup
    > on (current, child).  Additionally, SELinux makes this
    > same check in fs/proc/base.c:MAY_PTRACE to control access to
    > the /proc/PID/mem files and in fs/exec.c:compute_creds (along the same
    > lines as must_not_trace_exec) to control tracing across an execve 
    > (but this is also based on the bprm, as discussed below).
    
    yes, i had the same needs and made ptrace take two task_struct pointers.
    
    > fs/exec.c:  We need a security field in the linux_binprm struct
    > as a temporary place for the new security attributes of a process
    > during an execve, used in the same manner as the existing uid,
    > gid, and capability set fields.  
    
    i added this also.  the existing uid and gid fields are left there, but the
    capabliities are pushed into the opaque security blob.
    
    > fs/exec.c (prepare_binprm):  We need a hook on the bprm
    > to compute the new process security field, temporarily storing it
    > in the bprm until/if compute_creds is called by the binfmt code.
    
    i added this (as well as a way to free it ;-)
    
    > fs/exec.c (compute_creds):  We need a hook on the bprm to check sharing
    > of state across an execve (similar to the existing checking
    > performed for sharing state across a set[ug]id/capability
    > enhancement exec), change the attributes of the process,
    > and wakeup any sleeping waiters if the attributes changed
    > so that the wait4 permissions can be rechecked.  
    
    i added this.  compute_creds is now part of the lsm interface.
    
    > fs/namei.c:  I would have suggested a hook after each successful 
    > call to i_op->lookup to allow the security module to 
    > set the security field of the inode when it is first looked up
    > (since useful information like its inode number and mode should
    > then be available, which isn't available in get_new_inode
    > after the read_inode call for many file system types).  But I'm not sure
    > how to sync that with Serge's attach_pathlabel hooks - it seems like we
    > may be trying to provide the same functionality but
    > on different levels (inode-based vs. pathname-based).
    > It also seems like Serge's attach_pathlabel hooks
    > overlap somewhat with the idea of post-create hooks,
    > but again dealing at different levels - the vfsmount isn't
    > available in the vfs_create/mkdir/etc routines.
    
    yeah Serge's pathlable hooks are often near the post-create hooks.  it would
    be nice to collapse these if possible.
    
    > kernel/exit.c (sys_wait4):  We need a hook on p called here to
    > verify that the parent is authorized to receive the exit status of 
    > the child.  The hook would be called just before the flag gets set to 1.
    > 
    > kernel/fork.c (do_fork):  We would like a hook here to ensure that the
    > process is allowed to fork (for cases where a process has no legitimate
    > reason for forking at all).
    
    that was the idea behind the task_ops->create() (i.e. can you create a task
    structure?).  perhaps a better name and actually placing a hook would help ;-)
    consider this underway ;-)
    
    > kernel/sys.c,sched.c:  We need hooks for a variety of process operations
    > that can be performed on other processes:  setpriority, getpriority,
    > setpgid, setpgid, getsid, setscheduler, getscheduler, etc.  
    
    i added all the setXid family, the setpriority (as well as sys_nice), and
    the setscheduler.  but i didn't add any of the corresponding getX functions.
    we can add these.  someone else (david wagner iirc) mentioned the need
    for protecting "reading to" not just "writing from" kernel objects.
    
    > include/linux/sched.h (capable):  SELinux currently has a permission
    > check inserted into the capable function that mirrors the capability
    > check with a corresponding permission check to the SELinux 
    > security server (both must authorize the operation).  That allowed us to
    > leverage the work already done by the Linux-Privs people to
    > allow our policy to control many privileged operations without
    > manually inserting our own hooks into the same locations.  How
    > is that supposed to work in the new scheme of things?  I was
    > surprised that the current patch tries to effectively replace
    > some of the existing capable calls with separate hooks rather
    > than just inserting a hook into capable() that passes the
    > requested capability.  Of course, that offers greater flexibility
    > since you can pass parameters specific to each operation, but
    > it seems like a high burden for your initial implementation.
    
    i've been working on moving all capable() calls to the lsm interface rather
    than moving lsm hooks into capable().  the first cut was simply turning all
    current calls to capable() into the interface.  so the lsm interface looked
    like one function, the capable multiplexor.  This was too limiting, and not
    that useful.  The next step was adding new stuff.  Here is where conflicts
    started.  Something like CAP_SYS_ADMIN is such a kitchen sink that we needed
    better granularity.  I looked briefly changing to something like
    cap_sys_admin(token) where token told you what you were doing, but this felt
    too much like a hack.  ultimately, the capabilities model is too course
    grained to be the generalized lsm interface (i'm sure we can all agree there).  so why not create the "right" interface and then fit capabilities to it?
    i agree it is a lot of work ( >550 calls to capable()), but i don't think we
    will lose the privs work, just expand on it.
    
    the capability module's interface is exported, so any module that cares about
    privs can call privs first then carry on with local policy (or vice-versa...
    whatever).  this is where chaining cooperating modules is useful ;-)  so far,
    other than exporting privs interface) we are providing no way to chain modules.
    we may not end there, but this is where we are starting.
    > 
    > fs/fcntl.c (do_fcntl):  We need a hook on the parameters to
    > this function to authorize the operation.  Also, we need a security
    > field in struct fown_struct to save the attributes of the owning
    > process, just as the uid/gid is currently saved, for later
    > use in send_sigio_to_task.  
    > 
    > fs/fcntl.c (sys_fcntl64):  We need a hook here to deal with a few
    > additional operations not handled by do_fcntl.
    > 
    > fs/fcntl.c (send_sigio_to_task):  We need a hook here on 
    > fown and p to check signal permissions between the "owner"
    > of the file descriptor and the target process.
    > 
    > fs/locks.c (sys_flock):  We need a hook here on filp and cmd.
    > 
    > fs/ioctl.c (sys_ioctl):  We need a hook here on file and cmd.
    > 
    > fs/readdir.c (vfs_readdir):  We need a call to the
    > permission hook here to check read access.
    > 
    > fs/select.c (do_select, do_pollfd):  We need a poll hook here on
    > the struct file.  
    > 
    > kernel/kmod.c (exec_usermodehelper):  We need a hook here to
    > set the security field for the kernel module loader, just
    > as the uid,gid, and capability sets are set.  That lets us
    > distinguish between what the kernel module loader can do and
    > what the process that triggered the kernel module loader can do.
    > 
    > kernel/signal.c (bad_signal or send_sig_info):  We need a
    > hook here based on sig, info, and t.  
    
    i already augmented the task_ops->kill() interface to take t and sig...i can
    add info ;-)
    
    -chris
    
    _______________________________________________
    linux-security-module mailing list
    linux-security-moduleat_private
    http://mail.wirex.com/mailman/listinfo/linux-security-module
    



    This archive was generated by hypermail 2b30 : Wed Apr 25 2001 - 19:10:55 PDT