Re: [RFC] [Stacking v4 3/3] Cleaned up stacker patch

From: Stephen Smalley (sds@private)
Date: Mon Dec 06 2004 - 08:44:44 PST

On Fri, 2004-12-03 at 13:04, Serge Hallyn wrote:
> Changes:
>   1. uses RCU to protect the list elements.
>   2. capable() now just uses RETURN_ERROR_IF_ANY_ERROR explicitly.
>   3. stacker exports lsm_adopt_next_secondary(char *name), which will
> delegate stacking of the next loaded LSM to the module identified by
> (name).
>   4. stacker doesn't touch the get/setprocattr functions.  There's no
> point messing with that unless/until userspace code can handle it, as
> there's no clean way to handle all cases otherwise.
>   5. mod_unreg_security is removed.  All modules use unregister_security
> () regardless of whether they were loaded using register_security() or
> mod_reg_security().  In this way, if capability() was escalated by
> selinux from secondary to a primary module, it doesn't end up
> unregistering the wrong way.

- When would we not want stacker to short circuit processing upon
encountering an error from any security module for hooks that return
error codes?  The rationale in the comments is that some modules may
change state when called, but the question is whether any such state
update should occur when the operation is going to fail anyway.  In the
current built-in "stacking" support in SELinux, we intentionally short
circuit when the secondary module returns an error to avoid filling the
logs with audit messages that would have failed anyway due to the normal
capability check.  Further, in the capget and capset_set cases, we
intentionally short circuit when the SELinux permission check fails to
avoid having the capability module get or set the capability bits, as we
don't want that to happen if access is denied by SELinux.

- The last point highlights another issue if we were to try to stack
capabilities and SELinux via stacker rather than via SELinux
secondary_ops: we presently apply different orderings for capget and
capset_set than for other hooks.  Possibly reflects a more basic problem
in the capset code and hooks; capset_check is supposed to handle
security checking while capset_set is supposed to just update state, but
we often don't have the specific target task in capset_check, so
capset_set has to apply a check too for completeness.  Also, looking at
the core kernel, it looks like capset_check isn't serving a useful
purpose anymore, as the core kernel is applying the same checks
directly.  capget may not be a real issue, as it is only getting the
bits into a kernel data structure and will not copy to userspace if we
returned an error.  Possibly need two separate hooks at the same points
where capset_set is presently being invoked to cleanly separate
permission checking hooks from state update.

- It seems like the alloc_security hooks are a problem for stacker,
because you could have one or more modules succeed in allocating the
security state before one fails, and the core kernel doesn't call the
free_security hook upon an error from the alloc_security hook, as it
reasonably assumes cleanup of any partially allocated state prior to the
hook returning.  Either the stacker needs to internally call the
free_security() hooks for all modules that returned success from
alloc_security() to cleanup, or the core kernel needs to be changed to
always call free_security() after any call to alloc_security(), even if
it failed.

Stephen Smalley <sds@private>
National Security Agency

This archive was generated by hypermail 2.1.3 : Mon Dec 06 2004 - 08:50:16 PST