Hi Amir,

On Sun, 2021-04-04 at 13:27 +0300, Amir Goldstein wrote:

[forking question about security modules]

quoted

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

quoted

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.