Re: LSM and setxattr helpers

[Amir Goldstein <amir73il@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3903 bytes --]

[forking question about security modules]

>
> Nice thing about vfs_{set,remove}xattr() is that they already have
> several levels of __vfs_ helpers and nfsd already calls those, so
> we can hoist fsnotify_xattr() hooks hooks up from the __vfs_xxx
> helpers to the common vfs_xxx helpers and add fsnotify hooks to
> the very few callers of __vfs_ helpers.
>
> nfsd is consistently calling __vfs_{set,remove}xattr_locked() which
> do generate events, but ecryptfs mixes __vfs_setxattr_locked() with
> __vfs_removexattr(), which does not generate event and does not
> check permissions - it looks like an oversight.
>
> The thing is, right now __vfs_setxattr_noperm() generates events,
> but looking at all the security/* callers, it feels to me like those are
> very internal operations and that "noperm" should also imply "nonotify".
>
> To prove my point, all those callers call __vfs_removexattr() which
> does NOT generate an event.
>
> Also, I *think* the EVM setxattr is something that usually follows
> another file data/metadata change, so some event would have been
> generated by the original change anyway.
>
> Mimi,
>
> Do you have an opinion on that?
>
> The question is if you think it is important for an inotify/fanotify watcher
> that subscribed to IN_ATTRIB/FAN_ATTRIB events on a file to get an
> event when the IMA security blob changes.
>

Guys,

I was doing some re-factoring of the __vfs_setxattr helpers
and noticed some strange things.

The wider context is fsnotify_xattr() hooks inside internal
setxattr,removexattr calls. I would like to move those hooks
to the common vfs_{set,remove}xattr() helpers.

SMACK & SELINUX:
For the callers of __vfs_setxattr_noperm(),
smack_inode_setsecctx() and selinux_inode_setsecctx()
It seems that the only user is nfsd4_set_nfs4_label(), so it
makes sense for me to add the fsnotify_xattr() in nfsd context,
same as I did with other fsnotify_ hooks.

Are there any other expected callers of security_inode_setsecctx()
except nfsd in the future? If so they would need to also add the
fsnotify_xattr() hook, if at all the user visible FS_ATTRIB event is
considered desirable.

SMACK:
Just to be sure, is the call to __vfs_setxattr() from smack_d_instantiate()
guaranteed to be called for an inode whose S_NOSEC flag is already
cleared? Because the flag is being cleared by __vfs_setxattr_noperm().

EVM:
I couldn't find what's stopping this recursion:
evm_update_evmxattr() => __vfs_setxattr_noperm() =>
security_inode_post_setxattr() => evm_inode_post_removexattr() =>
evm_update_evmxattr()

It looks like the S_NOSEC should already be clear when
evm_update_evmxattr() is called(?), so it seems more logical to me to
call __vfs_setxattr() as there is no ->inode_setsecurity() hook for EVM.
Am I missing something?

It seems to me that updating the EVM hmac should not generate
a visible FS_ATTRIB event to listeners, because it is an internal
implementation detail and because update EVM hmac happens
following another change to the inode which anyway reports a
visible event to listeners.
Also, please note that evm_update_evmxattr() may also call
__vfs_removexattr() which does not call the fsnotify_xattr() hook.

IMA:
Similarly, ima_fix_xattr() should be called on an inode without
S_NOSEC flag and no other LSM should be interested in the
IMA hash update, right? So wouldn't it be better to use
__vfs_setxattr() in this case as well?

ima_fix_xattr() can be called after file data update, which again
will have other visible events, but it can also be called in "fix mode"
I suppose also when reading files? Still, it seems to me like an
internal implementation detail that should not generate a user
visible event.

If you agree with my proposed changes, please ACK the
respective bits of your subsystem from the attached patch.
Note that my patch does not contain the proposed change to
use __vfs_setxattr() in IMA/EVM.

Thanks,
Amir.

[-- Attachment #2: move-fsnotify_xattr-hooks-up-the-call-stack.patch.txt --]
[-- Type: text/plain, Size: 4986 bytes --]

From 76db3bf84ac1383c9c0b7e6f8268ab2527873d80 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Fri, 2 Apr 2021 11:28:13 +0300
Subject: [PATCH] fsnotify: move fsnotify_xattr() hooks up the call stack

Move the fsnotify hooks from internal __vfs_xxx helpers into the
more common vfs_xxx helpers.

Add the fsnotify hooks to the callers of the __vfs_xxx_locked helpers
nfsd and ecryptfs (ecryptfs gains an event on removexattr).

Add the fsnotify hooks to nfsd4_set_nfs4_label() to compensate for the
lost fsnotify hooks from the LSM inode_setsecctx() hooks of smack and
selinux.

Leave evm_update_evmxattr() and ima_fix_xattr() callers without fsnotify
hooks, because the update of IMA/EVM hash seems like information that
may not need to be exposed to fsnotify watchers and in most cases, the
hash update will follow update of inode data or metadata that will have
anyway generated an fsnotify event.

Cc: Tyler Hicks <code@tyhicks.com>
Cc: Mimi Zohar <zohar@linux.ibm.com>
Cc: James Morris <jmorris@namei.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: Paul Moore <paul@paul-moore.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/ecryptfs/inode.c |  5 +++++
 fs/nfsd/vfs.c       |  6 ++++++
 fs/xattr.c          | 14 ++++++--------
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 18e9285fbb4c..7f24830e8c5c 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -16,6 +16,7 @@
 #include <linux/namei.h>
 #include <linux/mount.h>
 #include <linux/fs_stack.h>
+#include <linux/fsnotify.h>
 #include <linux/slab.h>
 #include <linux/xattr.h>
 #include <asm/unaligned.h>
@@ -1047,6 +1048,8 @@ ecryptfs_setxattr(struct dentry *dentry, struct inode *inode,
 	}
 	inode_lock(lower_inode);
 	rc = __vfs_setxattr_locked(&init_user_ns, lower_dentry, name, value, size, flags, NULL);
+	if (!rc)
+		fsnotify_xattr(lower_dentry);
 	inode_unlock(lower_inode);
 	if (!rc && inode)
 		fsstack_copy_attr_all(inode, lower_inode);
@@ -1113,6 +1116,8 @@ static int ecryptfs_removexattr(struct dentry *dentry, struct inode *inode,
 	}
 	inode_lock(lower_inode);
 	rc = __vfs_removexattr(&init_user_ns, lower_dentry, name);
+	if (!rc)
+		fsnotify_xattr(lower_dentry);
 	inode_unlock(lower_inode);
 out:
 	return rc;
diff --git a/fs/nfsd/vfs.c b/fs/nfsd/vfs.c
index 611c4b8f3c74..75e22ab17cca 100644
--- a/fs/nfsd/vfs.c
+++ b/fs/nfsd/vfs.c
@@ -520,6 +520,8 @@ __be32 nfsd4_set_nfs4_label(struct svc_rqst *rqstp, struct svc_fh *fhp,
 
 	inode_lock(d_inode(dentry));
 	host_error = security_inode_setsecctx(dentry, label->data, label->len);
+	if (!host_error)
+		fsnotify_xattr(dentry);
 	inode_unlock(d_inode(dentry));
 	return nfserrno(host_error);
 }
@@ -2315,6 +2317,8 @@ nfsd_removexattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name)
 
 	ret = __vfs_removexattr_locked(&init_user_ns, fhp->fh_dentry,
 				       name, NULL);
+	if (!ret)
+		fsnotify_xattr(fhp->fh_dentry);
 
 	fh_unlock(fhp);
 	fh_drop_write(fhp);
@@ -2340,6 +2344,8 @@ nfsd_setxattr(struct svc_rqst *rqstp, struct svc_fh *fhp, char *name,
 
 	ret = __vfs_setxattr_locked(&init_user_ns, fhp->fh_dentry, name, buf,
 				    len, flags, NULL);
+	if (!ret)
+		fsnotify_xattr(fhp->fh_dentry);
 
 	fh_unlock(fhp);
 	fh_drop_write(fhp);
diff --git a/fs/xattr.c b/fs/xattr.c
index b3444e06cded..6c0ac300b519 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -213,11 +213,9 @@ int __vfs_setxattr_noperm(struct user_namespace *mnt_userns,
 	if (inode->i_opflags & IOP_XATTR) {
 		error = __vfs_setxattr(mnt_userns, dentry, inode, name, value,
 				       size, flags);
-		if (!error) {
-			fsnotify_xattr(dentry);
+		if (!error)
 			security_inode_post_setxattr(dentry, name, value,
 						     size, flags);
-		}
 	} else {
 		if (unlikely(is_bad_inode(inode)))
 			return -EIO;
@@ -230,8 +228,6 @@ int __vfs_setxattr_noperm(struct user_namespace *mnt_userns,
 
 			error = security_inode_setsecurity(inode, suffix, value,
 							   size, flags);
-			if (!error)
-				fsnotify_xattr(dentry);
 		}
 	}
 
@@ -299,6 +295,8 @@ vfs_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	inode_lock(inode);
 	error = __vfs_setxattr_locked(mnt_userns, dentry, name, value, size,
 				      flags, &delegated_inode);
+	if (!error)
+		fsnotify_xattr(dentry);
 	inode_unlock(inode);
 
 	if (delegated_inode) {
@@ -500,10 +498,8 @@ __vfs_removexattr_locked(struct user_namespace *mnt_userns,
 
 	error = __vfs_removexattr(mnt_userns, dentry, name);
 
-	if (!error) {
-		fsnotify_xattr(dentry);
+	if (!error)
 		evm_inode_post_removexattr(dentry, name);
-	}
 
 out:
 	return error;
@@ -522,6 +518,8 @@ vfs_removexattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	inode_lock(inode);
 	error = __vfs_removexattr_locked(mnt_userns, dentry,
 					 name, &delegated_inode);
+	if (!error)
+		fsnotify_xattr(dentry);
 	inode_unlock(inode);
 
 	if (delegated_inode) {
-- 
2.30.0