On 6/12/2019 10:41 AM, David Howells wrote: > Casey Schaufler <casey@xxxxxxxxxxxxxxxx> 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. View is good enough. > (*) Mount/superblock watches - I've made these require execute permission on > the specified directory. Could be read permission instead. Execute is good enough. > (*) Device events (block/usb) don't require any permissions, but currently > only deliver hardware notifications. How do you specify what device you want to watch? Don't you have to access a /dev/something? "currently" makes me nervous. >> 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. Not even your own? >>> 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. How are buffers shared? Who besides the buffer creator can use it? >>> (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. Can you glean information from the watch being deleted? I wouldn't think so, and it seems like a one-time event from the system, so I don't think an access check would be required. > > David
