Re: [PATCH v3 14/25] security: Introduce file_post_open hook

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On Thu, 2023-10-12 at 08:36 -0400, Mimi Zohar wrote:
> On Mon, 2023-09-04 at 15:34 +0200, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > In preparation to move IMA and EVM to the LSM infrastructure, introduce the
> > file_post_open hook. Also, export security_file_post_open() for NFS.
> > 
> > It is useful for IMA to calculate the dhigest of the file content, and to
> > decide based on that digest whether the file should be made accessible to
> > the requesting process.
> 
> Please remove "It is usefile for".   Perhaps something along the lines:
> 
> 
> Based on policy, IMA calculates the digest of the file content and
> decides ...

Ok.

> > 
> > LSMs should use this hook instead of file_open, if they need to make their
> > decision based on an opened file (for example by inspecting the file
> > content). The file is not open yet in the file_open hook.
> 
> The security hooks were originally defined for enforcing access
> control.  As a result the hooks were placed before the action.  The
> usage of the LSM hooks is not limited to just enforcing access control
> these days.  For IMA/EVM to become full LSMs additional hooks are
> needed post action.  Other LSMs, probably non-access control ones,
> could similarly take some action post action, in this case successful
> file open.

I don't know, I would not exclude LSMs to enforce access control. The
post action can be used to update the state, which can be used to check
next accesses (exactly what happens for EVM).

> Having to justify the new LSM post hooks in terms of the existing LSMs,
> which enforce access control, is really annoying and makes no sense. 
> Please don't.

Well, there is a relationship between the pre and post. But if you
prefer, I remove this comparison.

Thanks

Roberto

> > The new hook can
> > return an error and can cause the open to be aborted.
> 
> Please make this a separate pagraph.
> 
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  fs/namei.c                    |  2 ++
> >  fs/nfsd/vfs.c                 |  6 ++++++
> >  include/linux/lsm_hook_defs.h |  1 +
> >  include/linux/security.h      |  6 ++++++
> >  security/security.c           | 17 +++++++++++++++++
> >  5 files changed, 32 insertions(+)
> > 
> > diff --git a/fs/namei.c b/fs/namei.c
> > index 1f5ec71360de..7dc4626859f0 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -3634,6 +3634,8 @@ static int do_open(struct nameidata *nd,
> >  	error = may_open(idmap, &nd->path, acc_mode, open_flag);
> >  	if (!error && !(file->f_mode & FMODE_OPENED))
> >  		error = vfs_open(&nd->path, file);
> > +	if (!error)
> > +		error = security_file_post_open(file, op->acc_mode);
> >  	if (!error)
> >  		error = ima_file_check(file, op->acc_mode);
> >  	if (!error && do_truncate)
> > diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
> > index 8a2321d19194..3450bb1c8a18 100644
> > --- a/fs/nfsd/vfs.c
> > +++ b/fs/nfsd/vfs.c
> > @@ -862,6 +862,12 @@ __nfsd_open(struct svc_rqst *rqstp, struct svc_fh *fhp, umode_t type,
> >  		goto out_nfserr;
> >  	}
> >  
> > +	host_err = security_file_post_open(file, may_flags);
> > +	if (host_err) {
> > +		fput(file);
> > +		goto out_nfserr;
> > +	}
> > +
> >  	host_err = ima_file_check(file, may_flags);
> >  	if (host_err) {
> >  		fput(file);
> > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> > index 1153e7163b8b..60ed33f0c80d 100644
> > --- a/include/linux/lsm_hook_defs.h
> > +++ b/include/linux/lsm_hook_defs.h
> > @@ -188,6 +188,7 @@ LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
> >  	 struct fown_struct *fown, int sig)
> >  LSM_HOOK(int, 0, file_receive, struct file *file)
> >  LSM_HOOK(int, 0, file_open, struct file *file)
> > +LSM_HOOK(int, 0, file_post_open, struct file *file, int mask)
> >  LSM_HOOK(int, 0, file_truncate, struct file *file)
> >  LSM_HOOK(int, 0, task_alloc, struct task_struct *task,
> >  	 unsigned long clone_flags)
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 665bba3e0081..a0f16511c059 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -403,6 +403,7 @@ int security_file_send_sigiotask(struct task_struct *tsk,
> >  				 struct fown_struct *fown, int sig);
> >  int security_file_receive(struct file *file);
> >  int security_file_open(struct file *file);
> > +int security_file_post_open(struct file *file, int mask);
> >  int security_file_truncate(struct file *file);
> >  int security_task_alloc(struct task_struct *task, unsigned long clone_flags);
> >  void security_task_free(struct task_struct *task);
> > @@ -1044,6 +1045,11 @@ static inline int security_file_open(struct file *file)
> >  	return 0;
> >  }
> >  
> > +static inline int security_file_post_open(struct file *file, int mask)
> > +{
> > +	return 0;
> > +}
> > +
> >  static inline int security_file_truncate(struct file *file)
> >  {
> >  	return 0;
> > diff --git a/security/security.c b/security/security.c
> > index 3947159ba5e9..3e0078b51e46 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -2856,6 +2856,23 @@ int security_file_open(struct file *file)
> >  	return fsnotify_perm(file, MAY_OPEN);
> >  }
> >  
> > +/**
> > + * security_file_post_open() - Recheck access to a file after it has been opened
> 
> The LSM post hooks aren't needed to enforce access control.   Probably
> better to say something along the lines of "take some action after
> successful file open".
> 
> > + * @file: the file
> > + * @mask: access mask
> > + *
> > + * Recheck access with mask after the file has been opened. The hook is useful
> > + * for LSMs that require the file content to be available in order to make
> > + * decisions.
> 
> And reword the above accordingly.
> 
> > + *
> > + * Return: Returns 0 if permission is granted.
> > + */
> > +int security_file_post_open(struct file *file, int mask)
> > +{
> > +	return call_int_hook(file_post_open, 0, file, mask);
> > +}
> > +EXPORT_SYMBOL_GPL(security_file_post_open);
> > +
> >  /**
> >   * security_file_truncate() - Check if truncating a file is allowed
> >   * @file: file
>