Re: LSM and setxattr helpers

security_inode_post_setxattr

On Mon, Apr 5, 2021 at 5:47 PM Mimi Zohar [off-list ref] wrote:

Hi Amir,

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

quoted

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

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.

quoted

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.

quoted

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.

quoted

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

quoted

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.