Problems with some of the current hooks

From: Stephen Smalley (sdsat_private)
Date: Fri Aug 03 2001 - 11:03:34 PDT

  • Next message: jmjonesat_private: "Re: Making forward progress"

    I've made a quick review of the non-networking LSM hooks, and listed
    below some notes about problems I saw.  When a fix was obvious and
    didn't appear to require further discussion, I also listed a FIX.
    Otherwise, I mention that the problem is left UNRESOLVED.  I plan on
    creating a patch to implement the FIXes unless I hear objections to
    some of them.  Here are my notes:
    As noted by Chris Wright, bprm_ops->alloc_security can be called
    multiple times in one code path.  Furthermore, bprm_ops->free_security
    might be called without ever calling bprm_ops->alloc_security.  FIX:
    Move the bprm_ops->alloc_security call to do_execve just prior to
    calling prepare_binprm (so that we know that it is always called
    once).  Replace the current bprm_ops->alloc_security call with a new
    bprm_ops->set_security call.
    There is a race condition in get_empty_super if sb_ops->alloc_security
    uses blocking allocation (e.g.  a super_block with a null s_dev field
    is found, sb_ops->alloc_security is called, it goes to sleep waiting
    for memory, and another process comes along and takes the same
    super_block.  FIX: Move the sb_ops->alloc_security call after the
    setting of s_dev in read_super.
    inode_ops->alloc_security has no way to propagate errors back to the
    caller.  FIX: Move this call from clean_inode to the callers of
    clean_inode, i.e. get_empty_inode and get_new_inode.
    inode_ops->readlink takes a user space pointer (buf) and a
    seemingly useless bufsize parameter.  FIX:  Remove these parameters. 
    inode_ops->truncate seems redundant with inode_ops->setattr.  
    Does it need to be kept as a separate hook?  UNRESOLVED.
    inode_ops->permission is called prior to the DAC logic. FIX: Move
    inode_ops->permission call after the DAC logic.
    inode_ops->post_lookup is not called after a successful i_op->lookup
    call in fs/namei.c:lookup_hash.  Was that intentional or just an
    oversight?  UNRESOLVED.
    It would be nice to have a inode_ops->delete hook (called by
    fs/inode.c:iput when an inode is released and its hard link count is
    zero).  This is more useful than post_unlink/post_rmdir hooks would
    be, because this is the point where we would actually clear the
    inode's security label in the persistent label mapping.  FIX: Add such
    a hook.
    It might be useful to have explicit hooks in critical places like
    fs/proc/kcore.c:open_kcore and linux/drivers/char/mem.c:open_port.
    However, these locations are already covered by capable() calls, and
    they can also be controlled using file controls, so it isn't clear if
    this is necessary.  UNRESOLVED.
    The file_ops->read, file_ops->readv, file_ops->write, and
    file_ops->writev hooks are not called by the kernel.  Instead,
    file_ops->permission is used for the read (and readv and pread), write
    (and writev and pwrite), readdir, and sendfile operations.  This is
    sufficient for access control purposes.  Should the read, readv, write
    and writev hooks be removed from file_ops?  Or should these hooks be
    used in these operations (with expanded parameters) and new hooks
    added for pread, pwrite, sendfile and readdir.  UNRESOLVED.
    file_ops->lock doesn't currently take the cmd parameter.  This might
    be useful for general purpose usage.  FIX: Add the cmd parameter.
    file_ops->fcntl and file_ops->fcntl64 take the same parameters.  The
    only difference is that fcntl64 has some additional possible cmd
    values.  But this doesn't seem to require separate hooks.  FIX:
    Coalesce these two hooks into a single hook, but continue to call it
    from both sys_fcntl and sys_fcntl64.
    file_ops->ioctl and file_ops->fcntl take a generic arg parameter that
    can be a user space pointer, but it can also be a simple integer
    value.  For example, a fcntl(fd, F_SETFL, arg) request passes the new
    descriptor flags as a simple integer in the arg parameter.  SELinux
    uses this value to determine if the process is trying to clear the
    O_APPEND flag on a file, and verifies that the process is allowed to
    write to the file in that case (since SELinux may have only granted
    the process append permission when the file was originally opened).
    So it seems desirable to pass this parameter, but we should add a note
    in security.h warning module writers that if this parameter is a
    pointer (as opposed to a simple value), then it is a user space
    pointer.  FIX:  Add a comment to security.h.
    task_ops->create is not called by the kernel.  There is the separate
    task_ops->alloc_security hook, but they have different intended
    purposes.  task_ops->create (for us at least) was intended to control
    the ability to fork new processes - a very coarse-grained resource
    limitation.  task_ops->alloc_security is (naturally) called after the
    task structure has already been allocated to allocate the security
    blob.  So, we could use task_ops->alloc_security to prevent a given
    process from forking at all, but the process would see -ENOMEM rather
    than -EACCES.  Perhaps that isn't a big deal.  Should we remove
    task_ops->create or place it at the beginning of do_fork?  UNRESOLVED.
    (Nit) The module_ops hooks still say "name_user" and "mod_user" in the
    function prototypes and hook functions, even though these parameters
    have been changed to be the kernel copies rather than user space
    pointers.  FIX: Remove the '_user' suffix.
    The reboot hook takes a void *arg parameter that is a user space
    pointer.  FIX:  Remove this parameter.
    The remount hook no longer appears to be useful as a separate
    hook from the mount hook.  FIX:  Remove this hook.
    Some people might want the umount hook call moved up to sys_umount so
    that the complete nd is available.  Anyone?  UNRESOLVED.
    The ptrace and capable hooks belong in the task_ops structure.  FIX:
    Move them.
    The acct hook belongs in the file_ops structure.  FIX:  Move it.
    The setcapability hook is not called by the kernel and it has no
    parameters defined.  My current thinking is that we should move the
    entire capset and capget system calls out of the base kernel into the
    capability plug (so the base kernel would have sys_ni_syscall for
    these entries, and the capability module would insert them into the
    syscall table just as the SELinux module does with its new calls).
    Both the interface and the logic of these calls seems like it should
    be encapsulated by the capability plug, so that the Linux-Privs people
    are free to evolve them as desired.  So, if we move these system calls
    in their entirety into the module, then we probably don't need a
    setcapability hook at all.  UNRESOLVED.
    Stephen D. Smalley, NAI Labs
    linux-security-module mailing list

    This archive was generated by hypermail 2b30 : Fri Aug 03 2001 - 11:06:07 PDT