Re: LSM and setxattr helpers

Re: LSM and setxattr helpers

[Amir Goldstein]

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

Re: LSM and setxattr helpers

[Christian Brauner]

On Sun, Apr 04, 2021 at 01:27:21PM +0300, Amir Goldstein wrote:
> [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?

It feels like xattr changes that are essentially side-effects of another
operation should probably not generate fsnotify() events in general; at
least not without a good reason why userspace needs to know about the
event.

Christian

Re: LSM and setxattr helpers

[Mimi Zohar]

Hi Amir,

On Sun, 2021-04-04 at 13:27 +0300, Amir Goldstein wrote:
> [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?

Right, EVM is re-calculating the EVM HMAC, which is based on other LSM
xattrs and includes some misc file metadata (e.g. ino, generation, uid,
gid, mode).

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

Probably not.  Programs could open files R/W, but never modify the
file.  Perhaps to detect mutable file changes, but I'm not aware of
anyone doing so.

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

EVM is triggered when file metadata changes, causing the EVM HMAC to be
re-calculated. Before updating security.evm, EVM first verifies, on the
evm_inode_setattr/setxattr/removexattr() hooks, that the existing
security.evm value is correct.

On the _post hooks, security.evm is updated or removed, if no LSM xattr
exists.

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

EVM is triggered when an LSM updates/removes its xattr.   The LSM is
responsible for taking the inode lock.   Thus it is calling
__vfs_setxattr_noperm.

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

Ok

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

Originally, IMA took the inode lock really early, way before calling
setxattr.  Taking the inode lock can probably be deferred to setxattr. 
I have a vague recollection that SELinux also prevented IMA from
writing its own xattr label.  I don't know if that is still true.

thanks,

Mimi

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

Re: LSM and setxattr helpers

[Amir Goldstein]

security_inode_post_setxattr

On Mon, Apr 5, 2021 at 5:47 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Amir,
>
> On Sun, 2021-04-04 at 13:27 +0300, Amir Goldstein wrote:
> > [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?
>
> Right, EVM is re-calculating the EVM HMAC, which is based on other LSM
> xattrs and includes some misc file metadata (e.g. ino, generation, uid,
> gid, mode).
>

That explains why EVM registers to security_inode_post_setxattr() hook in
__vfs_setxattr_noperm() and which is the helper that selinux and smack call.

> > >
> > > 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.
>
> Probably not.  Programs could open files R/W, but never modify the
> file.  Perhaps to detect mutable file changes, but I'm not aware of
> anyone doing so.
>
> >
> > 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()
>
> EVM is triggered when file metadata changes, causing the EVM HMAC to be
> re-calculated. Before updating security.evm, EVM first verifies, on the
> evm_inode_setattr/setxattr/removexattr() hooks, that the existing
> security.evm value is correct.
>
> On the _post hooks, security.evm is updated or removed, if no LSM xattr
> exists.
>

I'm not sure I understand why evm_update_evmxattr() calls
__vfs_setxattr_noperm() and not __vfs_setxattr(), but it's not really important
for my needs to understand this. Neither helper will generate an fsnotify event.

> > 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?
>
> EVM is triggered when an LSM updates/removes its xattr.   The LSM is
> responsible for taking the inode lock.   Thus it is calling
> __vfs_setxattr_noperm.
>

Surely you need to call a variant that is __vfs_setxattr_locked() or
below it. I just did not understand why that variant is not  __vfs_setxattr().

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


OK. It looks like there is a consensus about losing those events.
That's what I thought, but wanted to check with you security guys.

Thanks,
Amir.

Re: LSM and setxattr helpers

[Casey Schaufler]

On 4/4/2021 3:27 AM, Amir Goldstein wrote:
> [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.

That seems reasonable to me, but the SELinux team would
have more experience with NFS deployemnts than Smack does.

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

Not that I know of.

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

My understanding is that the inode is always in the process of being
initialized, and that S_NOSEC should never have been set.

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