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

["Günther Noack" <gnoack@google.com>]

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;
> +	}
> +

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

 - 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.

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.)

Thanks!
—Günther

-- 
Sent using Mutt 🐕 Woof Woof