[PATCH v9 1/8] landlock: Add IOCTL access right

[Mickaël Salaün]

On Fri, Feb 16, 2024 at 03:11:18PM +0100, Mickaël Salaün wrote:
> On Sat, Feb 10, 2024 at 12:18:06PM +0100, Günther Noack wrote:
> > On Fri, Feb 09, 2024 at 06:06:05PM +0100, Günther Noack wrote:
> > > diff --git a/security/landlock/fs.c b/security/landlock/fs.c
> > > index 73997e63734f..84efea3f7c0f 100644
> > > --- a/security/landlock/fs.c
> > > +++ b/security/landlock/fs.c
> > > @@ -1333,7 +1520,9 @@ static int hook_file_open(struct file *const file)
> > >  {
> > >  	layer_mask_t layer_masks[LANDLOCK_NUM_ACCESS_FS] = {};
> > >  	access_mask_t open_access_request, full_access_request, allowed_access;
> > > -	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE;
> > > +	const access_mask_t optional_access = LANDLOCK_ACCESS_FS_TRUNCATE |
> > > +					      LANDLOCK_ACCESS_FS_IOCTL |
> > > +					      IOCTL_GROUPS;
> > >  	const struct landlock_ruleset *const dom = get_current_fs_domain();
> > >  
> > >  	if (!dom)
> > > @@ -1375,6 +1564,16 @@ static int hook_file_open(struct file *const file)
> > >  		}
> > >  	}
> > >  
> > > +	/*
> > > +	 * Named pipes should be treated just like anonymous pipes.
> > > +	 * Therefore, we permit all IOCTLs on them.
> > > +	 */
> > > +	if (S_ISFIFO(file_inode(file)->i_mode)) {
> > > +		allowed_access |= LANDLOCK_ACCESS_FS_IOCTL |
> > > +				  LANDLOCK_ACCESS_FS_IOCTL_RW |
> > > +				  LANDLOCK_ACCESS_FS_IOCTL_RW_FILE;
> 
> Why not LANDLOCK_ACCESS_FS_IOCTL | IOCTL_GROUPS instead?
> 
> > > +	}
> > > +
> > 
> > Hello Mickaël, this "if" is a change I'd like to draw your attention
> > to -- this special case was necessary so that all IOCTLs are permitted
> > on named pipes. (There is also a test for it in another commit.)
> > 
> > Open questions here are:
> > 
> >  - I'm a bit on the edge whether it's worth it to have these special
> >    cases here.  After all, users can very easily just permit all
> >    IOCTLs through the ruleset if needed, and it might simplify the
> >    mental model that we have to explain in the documentation
> 
> It might simplify the kernel implementation a bit but it would make the
> Landlock security policies more complex, and could encourage people to
> allow all IOCTLs on a directory because this directory might contain
> (dynamically created) named pipes.
> 
> I suggest to extend this check with S_ISFIFO(mode) || S_ISSOCK(mode).
> A comment should explain that LANDLOCK_ACCESS_FS_* rights are not meant
> to restrict IPCs.
> 
> > 
> >  - I've put the special case into the file open hook, under the
> >    assumption that it would simplify the Landlock audit support to
> >    have the correct rights on the struct file.  The implementation
> >    could alternatively also be done in the ioctl hook. Let me know
> >    which one makes more sense to you.
> 
> I like your approach, thanks!  Also, in theory this approach should be
> better for performance reasons, even if it should not be visible in
> practice. Anyway, keeping a consistent set of access rights is
> definitely useful for observability.
> 
> I'm wondering if we should do the same mode check for
> LANDLOCK_ACCESS_FS_TRUNCATE too... It would not be visible to user space
> anyway because the LSM hooks are called after the file mode checks for
> truncate(2) and ftruncate(2). But because we need this kind of check for
> IOCTL, it might be a good idea to make it common to all optional_access
> values, at least to document what is really handled. Adding dedicated
> truncate and ftruncate tests (before this commit) would guarantee that
> the returned error codes are unchanged.
> 
> Moving this check before the is_access_to_paths_allowed() call would
> enable to avoid looking for always-allowed access rights by removing
> them from the full_access_request. This could help improve performance
> when opening named pipe because no optional_access would be requested.
> 
> A new helper similar to get_required_file_open_access() could help.
> 
> > 
> > BTW, named UNIX domain sockets can apparently not be opened with open() and
> > therefore they don't hit the LSM file_open hook.  (It is done with the BSD
> > socket API instead.)
> 
> What about /proc/*/fd/* ? We can test with open_proc_fd() to make sure
> our assumptions are correct.

Actually, these fifo and socket checks (and related optimizations)
should already be handled with is_nouser_or_private() called by
is_access_to_paths_allowed(). Some new dedicated tests should help
though.