| Message ID | 20231107134012.682009-11-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | security: Move IMA and EVM to the LSM infrastructure | expand |
On 11/7/2023 5:39 AM, Roberto Sassu wrote: > From: Roberto Sassu <roberto.sassu@huawei.com> > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce > the inode_post_setattr hook. > > At inode_setattr hook, EVM verifies the file's existing HMAC value. At > inode_post_setattr, EVM re-calculates the file's HMAC based on the modified > file attributes and other file metadata. > > Other LSMs could similarly take some action after successful file attribute > change. > > The new hook cannot return an error and cannot cause the operation to be > reverted. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> Acked-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/attr.c | 1 + > include/linux/lsm_hook_defs.h | 2 ++ > include/linux/security.h | 7 +++++++ > security/security.c | 16 ++++++++++++++++ > 4 files changed, 26 insertions(+) > > diff --git a/fs/attr.c b/fs/attr.c > index 498e673bdf06..221d2bb0a906 100644 > --- a/fs/attr.c > +++ b/fs/attr.c > @@ -502,6 +502,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry, > > if (!error) { > fsnotify_change(dentry, ia_valid); > + security_inode_post_setattr(idmap, dentry, ia_valid); > ima_inode_post_setattr(idmap, dentry, ia_valid); > evm_inode_post_setattr(idmap, dentry, ia_valid); > } > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index f5db5e993cd8..67410e085205 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -137,6 +137,8 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode, > LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask) > LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry, > struct iattr *attr) > +LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap, > + struct dentry *dentry, int ia_valid) > LSM_HOOK(int, 0, inode_getattr, const struct path *path) > LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap, > struct dentry *dentry, const char *name, const void *value, > diff --git a/include/linux/security.h b/include/linux/security.h > index 750130a7b9dd..664df46b22a9 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -361,6 +361,8 @@ int security_inode_follow_link(struct dentry *dentry, struct inode *inode, > int security_inode_permission(struct inode *inode, int mask); > int security_inode_setattr(struct mnt_idmap *idmap, > struct dentry *dentry, struct iattr *attr); > +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > + int ia_valid); > int security_inode_getattr(const struct path *path); > int security_inode_setxattr(struct mnt_idmap *idmap, > struct dentry *dentry, const char *name, > @@ -877,6 +879,11 @@ static inline int security_inode_setattr(struct mnt_idmap *idmap, > return 0; > } > > +static inline void > +security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > + int ia_valid) > +{ } > + > static inline int security_inode_getattr(const struct path *path) > { > return 0; > diff --git a/security/security.c b/security/security.c > index 7935d11d58b5..ce3bc7642e18 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2222,6 +2222,22 @@ int security_inode_setattr(struct mnt_idmap *idmap, > } > EXPORT_SYMBOL_GPL(security_inode_setattr); > > +/** > + * security_inode_post_setattr() - Update the inode after a setattr operation > + * @idmap: idmap of the mount > + * @dentry: file > + * @ia_valid: file attributes set > + * > + * Update inode security field after successful setting file attributes. > + */ > +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > + int ia_valid) > +{ > + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > + return; > + call_void_hook(inode_post_setattr, idmap, dentry, ia_valid); > +} > + > /** > * security_inode_getattr() - Check if getting file attributes is allowed > * @path: file
On Nov 7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce > the inode_post_setattr hook. > > At inode_setattr hook, EVM verifies the file's existing HMAC value. At > inode_post_setattr, EVM re-calculates the file's HMAC based on the modified > file attributes and other file metadata. > > Other LSMs could similarly take some action after successful file attribute > change. > > The new hook cannot return an error and cannot cause the operation to be > reverted. > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > Acked-by: Casey Schaufler <casey@schaufler-ca.com> > --- > fs/attr.c | 1 + > include/linux/lsm_hook_defs.h | 2 ++ > include/linux/security.h | 7 +++++++ > security/security.c | 16 ++++++++++++++++ > 4 files changed, 26 insertions(+) ... > diff --git a/security/security.c b/security/security.c > index 7935d11d58b5..ce3bc7642e18 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -2222,6 +2222,22 @@ int security_inode_setattr(struct mnt_idmap *idmap, > } > EXPORT_SYMBOL_GPL(security_inode_setattr); > > +/** > + * security_inode_post_setattr() - Update the inode after a setattr operation > + * @idmap: idmap of the mount > + * @dentry: file > + * @ia_valid: file attributes set > + * > + * Update inode security field after successful setting file attributes. > + */ > +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > + int ia_valid) > +{ > + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > + return; I may be missing it, but I don't see the S_PRIVATE flag check in the existing IMA or EVM hooks so I'm curious as to why it is added here? Please don't misunderstand me, I think it makes sense to return early on private dentrys/inodes, but why aren't we doing that now? > + call_void_hook(inode_post_setattr, idmap, dentry, ia_valid); > +} > + > /** > * security_inode_getattr() - Check if getting file attributes is allowed > * @path: file > -- > 2.34.1 -- paul-moore.com
On Wed, 2023-11-15 at 23:33 -0500, Paul Moore wrote: > On Nov 7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce > > the inode_post_setattr hook. > > > > At inode_setattr hook, EVM verifies the file's existing HMAC value. At > > inode_post_setattr, EVM re-calculates the file's HMAC based on the modified > > file attributes and other file metadata. > > > > Other LSMs could similarly take some action after successful file attribute > > change. > > > > The new hook cannot return an error and cannot cause the operation to be > > reverted. > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > > Acked-by: Casey Schaufler <casey@schaufler-ca.com> > > --- > > fs/attr.c | 1 + > > include/linux/lsm_hook_defs.h | 2 ++ > > include/linux/security.h | 7 +++++++ > > security/security.c | 16 ++++++++++++++++ > > 4 files changed, 26 insertions(+) > > ... > > > diff --git a/security/security.c b/security/security.c > > index 7935d11d58b5..ce3bc7642e18 100644 > > --- a/security/security.c > > +++ b/security/security.c > > @@ -2222,6 +2222,22 @@ int security_inode_setattr(struct mnt_idmap *idmap, > > } > > EXPORT_SYMBOL_GPL(security_inode_setattr); > > > > +/** > > + * security_inode_post_setattr() - Update the inode after a setattr operation > > + * @idmap: idmap of the mount > > + * @dentry: file > > + * @ia_valid: file attributes set > > + * > > + * Update inode security field after successful setting file attributes. > > + */ > > +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > > + int ia_valid) > > +{ > > + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > + return; > > I may be missing it, but I don't see the S_PRIVATE flag check in the > existing IMA or EVM hooks so I'm curious as to why it is added here? > Please don't misunderstand me, I think it makes sense to return early > on private dentrys/inodes, but why aren't we doing that now? My first motivation was that it is in the pre hooks, so it should be in the post hook as well. Thinking more about it, suppose that the post don't have the check, private inodes would gain an HMAC without checking the validity of the current HMAC first (done in the pre hooks), which would be even worse. So, my idea about this is that at least we are consistent. If IMA and EVM should look at private inodes is a different question, which would require a discussion. Thanks Roberto > > + call_void_hook(inode_post_setattr, idmap, dentry, ia_valid); > > +} > > + > > /** > > * security_inode_getattr() - Check if getting file attributes is allowed > > * @path: file > > -- > > 2.34.1 > > -- > paul-moore.com
On Thu, Nov 16, 2023 at 4:44 AM Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > On Wed, 2023-11-15 at 23:33 -0500, Paul Moore wrote: > > On Nov 7, 2023 Roberto Sassu <roberto.sassu@huaweicloud.com> wrote: > > > > > > In preparation for moving IMA and EVM to the LSM infrastructure, introduce > > > the inode_post_setattr hook. > > > > > > At inode_setattr hook, EVM verifies the file's existing HMAC value. At > > > inode_post_setattr, EVM re-calculates the file's HMAC based on the modified > > > file attributes and other file metadata. > > > > > > Other LSMs could similarly take some action after successful file attribute > > > change. > > > > > > The new hook cannot return an error and cannot cause the operation to be > > > reverted. > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> > > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com> > > > Reviewed-by: Mimi Zohar <zohar@linux.ibm.com> > > > Acked-by: Casey Schaufler <casey@schaufler-ca.com> > > > --- > > > fs/attr.c | 1 + > > > include/linux/lsm_hook_defs.h | 2 ++ > > > include/linux/security.h | 7 +++++++ > > > security/security.c | 16 ++++++++++++++++ > > > 4 files changed, 26 insertions(+) > > > > ... > > > > > diff --git a/security/security.c b/security/security.c > > > index 7935d11d58b5..ce3bc7642e18 100644 > > > --- a/security/security.c > > > +++ b/security/security.c > > > @@ -2222,6 +2222,22 @@ int security_inode_setattr(struct mnt_idmap *idmap, > > > } > > > EXPORT_SYMBOL_GPL(security_inode_setattr); > > > > > > +/** > > > + * security_inode_post_setattr() - Update the inode after a setattr operation > > > + * @idmap: idmap of the mount > > > + * @dentry: file > > > + * @ia_valid: file attributes set > > > + * > > > + * Update inode security field after successful setting file attributes. > > > + */ > > > +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, > > > + int ia_valid) > > > +{ > > > + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) > > > + return; > > > > I may be missing it, but I don't see the S_PRIVATE flag check in the > > existing IMA or EVM hooks so I'm curious as to why it is added here? > > Please don't misunderstand me, I think it makes sense to return early > > on private dentrys/inodes, but why aren't we doing that now? > > My first motivation was that it is in the pre hooks, so it should be in > the post hook as well. > > Thinking more about it, suppose that the post don't have the check, > private inodes would gain an HMAC without checking the validity of the > current HMAC first (done in the pre hooks), which would be even worse. > > So, my idea about this is that at least we are consistent. > > If IMA and EVM should look at private inodes is a different question, > which would require a discussion. As I said above, I can understand why having the IS_PRIVATE() macro check might be a good idea, I am just concerned that the current IMA/EVM hooks don't check for S_PRIVATE and thus moving to this new LSM hook would potentially be a change in behavior (like I said, I could be missing a subtle detail). I'd just like a quick confirmation from Mimi that either there is no difference because of X, or she is aware of the difference and is okay with it. It's very possible she is fine with it, she did provide her 'Reviewed-by', but I worry this is the sort of thing that might have gone unnoticed during review.
diff --git a/fs/attr.c b/fs/attr.c index 498e673bdf06..221d2bb0a906 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -502,6 +502,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry, if (!error) { fsnotify_change(dentry, ia_valid); + security_inode_post_setattr(idmap, dentry, ia_valid); ima_inode_post_setattr(idmap, dentry, ia_valid); evm_inode_post_setattr(idmap, dentry, ia_valid); } diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index f5db5e993cd8..67410e085205 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -137,6 +137,8 @@ LSM_HOOK(int, 0, inode_follow_link, struct dentry *dentry, struct inode *inode, LSM_HOOK(int, 0, inode_permission, struct inode *inode, int mask) LSM_HOOK(int, 0, inode_setattr, struct mnt_idmap *idmap, struct dentry *dentry, struct iattr *attr) +LSM_HOOK(void, LSM_RET_VOID, inode_post_setattr, struct mnt_idmap *idmap, + struct dentry *dentry, int ia_valid) LSM_HOOK(int, 0, inode_getattr, const struct path *path) LSM_HOOK(int, 0, inode_setxattr, struct mnt_idmap *idmap, struct dentry *dentry, const char *name, const void *value, diff --git a/include/linux/security.h b/include/linux/security.h index 750130a7b9dd..664df46b22a9 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -361,6 +361,8 @@ int security_inode_follow_link(struct dentry *dentry, struct inode *inode, int security_inode_permission(struct inode *inode, int mask); int security_inode_setattr(struct mnt_idmap *idmap, struct dentry *dentry, struct iattr *attr); +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, + int ia_valid); int security_inode_getattr(const struct path *path); int security_inode_setxattr(struct mnt_idmap *idmap, struct dentry *dentry, const char *name, @@ -877,6 +879,11 @@ static inline int security_inode_setattr(struct mnt_idmap *idmap, return 0; } +static inline void +security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, + int ia_valid) +{ } + static inline int security_inode_getattr(const struct path *path) { return 0; diff --git a/security/security.c b/security/security.c index 7935d11d58b5..ce3bc7642e18 100644 --- a/security/security.c +++ b/security/security.c @@ -2222,6 +2222,22 @@ int security_inode_setattr(struct mnt_idmap *idmap, } EXPORT_SYMBOL_GPL(security_inode_setattr); +/** + * security_inode_post_setattr() - Update the inode after a setattr operation + * @idmap: idmap of the mount + * @dentry: file + * @ia_valid: file attributes set + * + * Update inode security field after successful setting file attributes. + */ +void security_inode_post_setattr(struct mnt_idmap *idmap, struct dentry *dentry, + int ia_valid) +{ + if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) + return; + call_void_hook(inode_post_setattr, idmap, dentry, ia_valid); +} + /** * security_inode_getattr() - Check if getting file attributes is allowed * @path: file