Re: What do LSMs *actually* need for checks on notifications?

[David Howells <dhowells@redhat.com>]

Casey Schaufler <casey@schaufler-ca.com> wrote:

> >  (4) The security attributes of the object on which the watch was set (uid,
> >      gid, mode, labels).
> 
> Smack needs this to set a watch on the named object (file, key, ...).
> I am going to say that if you can't access an object you can't watch it.

So for the things I've so far defined:

 (*) Keys/keyrings require View permission, but it could be Read permission
     instead - though that may get disabled if the key type does not support
     KEYCTL_READ.

 (*) Mount/superblock watches - I've made these require execute permission on
     the specified directory.  Could be read permission instead.

 (*) Device events (block/usb) don't require any permissions, but currently
     only deliver hardware notifications.

> I think that read access is sufficient provided that no one else can
> see what watches I've created.

You can't find out what watches exist.

> > At the moment, when post_one_notification() wants to write a notification
> > into a queue, it calls security_post_notification() to ask if it should be
> > allowed to do so.  This is passed (1) and (3) above plus the notification
> > record.
> 
> Is "current" (2)? Smack needs (2) for the event delivery access check.

(2) was current_cred() when watch_sb(), KEYCTL_NOTIFY, etc. was called, but
isn't necessarily current_cred() at the point post_one_notification() is
called.  The latter is called at the point the event is generated and
current_cred() is the creds of whatever thread that is called in (which may be
a work_queue thread if it got deferred).

At the moment, I'm storing the creds of whoever opened the queue (ie. (1)) and
using that, but I could cache the creds of whoever created each watch
(ie. (2)) and pass that to post_one_notification() instead.

However, it should be noted that (1) is the creds of the buffer owner.

> >  (e) All the points at which we walk over an object in a chain from (c) to
> >      find the watch on which we can effect (d) (eg. we walk rootwards from a
> >      mountpoint to find watches on a branch in the mount topology).
> 
> Smack does not require anything beyond existing checks.

I'm glad to hear that, as this might be sufficiently impractical as to render
it unusable with Smack.  Calling i_op->permissions() a lot would suck.

> >  (y) What checks should be done on object destruction after final put and
> >      what contexts need to be supplied?
> 
> Classically there is no such thing as filesystem object deletion.
> By making it possible to set a watch on that you've inadvertently
> added a security relevant action to the security model. :o

That wasn't my original intention - I intended fsmount(2) to mount directly as
mount(2) does, but Al had other ideas.

Now fsmount(2) produces a file descriptor referring to a new mount object that
can be mounted into by move_mount(2) before being spliced into the mount
topology by move_mount(2).  However, if the fd is closed before the last step,
close() will destroy the mount subtree attached to it (kind of quasi-unmount).

That wouldn't be a problem, except that the fd from fsmount(2) can be used
anywhere an O_PATH fd can be used - including watch_mount(2), fchdir(2), ...
Further, FMODE_NEED_UNMOUNT gets cleared if the mount is spliced in at least
once.

Okay, having tried it you don't get an unmount event (since there's no actual
unmount), but you do get an event to say that your watch got deleted (if the
directory on which the watch was placed got removed from the system).

So...  does the "your watch got deleted" message need checking?  In my
opinion, it shouldn't get discarded because then the watcher doesn't know
their watch went away.

David