Re: LSM and setxattr helpers

On 4/4/2021 3:27 AM, 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?

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.