| Message ID | 20220929153041.500115-14-brauner@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | acl: add vfs posix acl api | expand |
Hi Christian, On Thu, 2022-09-29 at 17:30 +0200, Christian Brauner wrote: > The security_inode_post_setxattr() hook is used by security modules to > update their own security.* xattrs. Consequently none of the security > modules operate on posix acls. So we don't need an additional security > hook when post setting posix acls. > > However, the integrity subsystem wants to be informed about posix acl > changes and specifically evm to update their hashes when the xattrs > change. ^... to be informed about posix acl changes in order to reset the EVM status flag. > The callchain for evm_inode_post_setxattr() is: > > -> evm_inode_post_setxattr() Resets the EVM status flag for both EVM signatures and HMAC. > -> evm_update_evmxattr() evm_update_evmxattr() is only called for "security.evm", not acls. > -> evm_calc_hmac() > -> evm_calc_hmac_or_hash() > > and evm_cacl_hmac_or_hash() walks the global list of protected xattr > names evm_config_xattrnames. This global list can be modified via > /sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is > restricted to security.* xattrs and the default xattrs in > evm_config_xattrnames only contains security.* xattrs as well. > > So the actual value for posix acls is currently completely irrelevant > for evm during evm_inode_post_setxattr() and frankly it should stay that > way in the future to not cause the vfs any more headaches. But if the > actual posix acl values matter then evm shouldn't operate on the binary > void blob and try to hack around in the uapi struct anyway. Instead it > should then in the future add a dedicated hook which takes a struct > posix_acl argument passing the posix acls in the proper vfs format. > > For now it is sufficient to make evm_inode_post_set_acl() a wrapper > around evm_inode_post_setxattr() not passing any actual values down. > This will still cause the hashes to be updated as before. ^This will cause the EVM status flag to be reset. > > Reviewed-by: Paul Moore <paul@paul-moore.com> > Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
On Thu, 2022-09-29 at 21:44 -0400, Mimi Zohar wrote: > Hi Christian, > > On Thu, 2022-09-29 at 17:30 +0200, Christian Brauner wrote: > > The security_inode_post_setxattr() hook is used by security modules to > > update their own security.* xattrs. Consequently none of the security > > modules operate on posix acls. So we don't need an additional security > > hook when post setting posix acls. > > > > However, the integrity subsystem wants to be informed about posix acl > > changes and specifically evm to update their hashes when the xattrs > > change. > > ^... to be informed about posix acl changes in order to reset the EVM > status flag. > > > The callchain for evm_inode_post_setxattr() is: > > > > -> evm_inode_post_setxattr() > > Resets the EVM status flag for both EVM signatures and HMAC. > > > -> evm_update_evmxattr() > > evm_update_evmxattr() is only called for "security.evm", not acls. > > > -> evm_calc_hmac() > > -> evm_calc_hmac_or_hash() > > > > and evm_cacl_hmac_or_hash() walks the global list of protected xattr > > names evm_config_xattrnames. This global list can be modified via > > /sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is > > restricted to security.* xattrs and the default xattrs in > > evm_config_xattrnames only contains security.* xattrs as well. > > > > So the actual value for posix acls is currently completely irrelevant > > for evm during evm_inode_post_setxattr() and frankly it should stay that > > way in the future to not cause the vfs any more headaches. But if the > > actual posix acl values matter then evm shouldn't operate on the binary > > void blob and try to hack around in the uapi struct anyway. Instead it > > should then in the future add a dedicated hook which takes a struct > > posix_acl argument passing the posix acls in the proper vfs format. > > > > For now it is sufficient to make evm_inode_post_set_acl() a wrapper > > around evm_inode_post_setxattr() not passing any actual values down. > > This will still cause the hashes to be updated as before. > > ^This will cause the EVM status flag to be reset. Sorry, please ignore these comments for the moment.
On Thu, Sep 29, 2022 at 09:44:45PM -0400, Mimi Zohar wrote: > Hi Christian, > > On Thu, 2022-09-29 at 17:30 +0200, Christian Brauner wrote: > > The security_inode_post_setxattr() hook is used by security modules to > > update their own security.* xattrs. Consequently none of the security > > modules operate on posix acls. So we don't need an additional security > > hook when post setting posix acls. > > > > However, the integrity subsystem wants to be informed about posix acl > > changes and specifically evm to update their hashes when the xattrs > > change. > > ^... to be informed about posix acl changes in order to reset the EVM > status flag. Substituted. > > > The callchain for evm_inode_post_setxattr() is: > > > > -> evm_inode_post_setxattr() > > Resets the EVM status flag for both EVM signatures and HMAC. > > > -> evm_update_evmxattr() > > evm_update_evmxattr() is only called for "security.evm", not acls. I've added both comments but note that I'm explaining this in the paragraph below as well. > > > -> evm_calc_hmac() > > -> evm_calc_hmac_or_hash() > > > > and evm_cacl_hmac_or_hash() walks the global list of protected xattr > > names evm_config_xattrnames. This global list can be modified via > > /sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is > > restricted to security.* xattrs and the default xattrs in > > evm_config_xattrnames only contains security.* xattrs as well. > > > > So the actual value for posix acls is currently completely irrelevant > > for evm during evm_inode_post_setxattr() and frankly it should stay that > > way in the future to not cause the vfs any more headaches. But if the > > actual posix acl values matter then evm shouldn't operate on the binary > > void blob and try to hack around in the uapi struct anyway. Instead it > > should then in the future add a dedicated hook which takes a struct > > posix_acl argument passing the posix acls in the proper vfs format. > > > > For now it is sufficient to make evm_inode_post_set_acl() a wrapper > > around evm_inode_post_setxattr() not passing any actual values down. > > This will still cause the hashes to be updated as before. > > ^This will cause the EVM status flag to be reset. Substituted.
Hi Christian, On Fri, 2022-09-30 at 10:44 +0200, Christian Brauner wrote: > On Thu, Sep 29, 2022 at 09:44:45PM -0400, Mimi Zohar wrote: > > On Thu, 2022-09-29 at 17:30 +0200, Christian Brauner wrote: > > > The security_inode_post_setxattr() hook is used by security modules to > > > update their own security.* xattrs. Consequently none of the security > > > modules operate on posix acls. So we don't need an additional security > > > hook when post setting posix acls. > > > > > > However, the integrity subsystem wants to be informed about posix acl > > > changes and specifically evm to update their hashes when the xattrs > > > change. > > > > ^... to be informed about posix acl changes in order to reset the EVM > > status flag. > > Substituted. > > > > > > > The callchain for evm_inode_post_setxattr() is: > > > > > > -> evm_inode_post_setxattr() > > > > Resets the EVM status flag for both EVM signatures and HMAC. > > > > > -> evm_update_evmxattr() > > > > evm_update_evmxattr() is only called for "security.evm", not acls. After re-reading the code with fresh eyes, I made a mistake here. Please revert these suggestions. > > I've added both comments but note that I'm explaining this in the > paragraph below as well. Agreed. > > > > > > -> evm_calc_hmac() > > > -> evm_calc_hmac_or_hash() > > > > > > and evm_cacl_hmac_or_hash() walks the global list of protected xattr > > > names evm_config_xattrnames. This global list can be modified via > > > /sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is > > > restricted to security.* xattrs and the default xattrs in > > > evm_config_xattrnames only contains security.* xattrs as well. > > > > > > So the actual value for posix acls is currently completely irrelevant > > > for evm during evm_inode_post_setxattr() and frankly it should stay that > > > way in the future to not cause the vfs any more headaches. But if the > > > actual posix acl values matter then evm shouldn't operate on the binary > > > void blob and try to hack around in the uapi struct anyway. Instead it > > > should then in the future add a dedicated hook which takes a struct > > > posix_acl argument passing the posix acls in the proper vfs format. > > > > > > For now it is sufficient to make evm_inode_post_set_acl() a wrapper > > > around evm_inode_post_setxattr() not passing any actual values down. > > > This will still cause the hashes to be updated as before. > > > > ^This will cause the EVM status flag to be reset. > > Substituted. My mistake. Can you replace it with: This will still cause the EVM status flag to be reset and EVM HMAC's to be updated as before.
On Fri, Sep 30, 2022 at 07:48:31AM -0400, Mimi Zohar wrote: > Hi Christian, > > On Fri, 2022-09-30 at 10:44 +0200, Christian Brauner wrote: > > On Thu, Sep 29, 2022 at 09:44:45PM -0400, Mimi Zohar wrote: > > > On Thu, 2022-09-29 at 17:30 +0200, Christian Brauner wrote: > > > > The security_inode_post_setxattr() hook is used by security modules to > > > > update their own security.* xattrs. Consequently none of the security > > > > modules operate on posix acls. So we don't need an additional security > > > > hook when post setting posix acls. > > > > > > > > However, the integrity subsystem wants to be informed about posix acl > > > > changes and specifically evm to update their hashes when the xattrs > > > > change. > > > > > > ^... to be informed about posix acl changes in order to reset the EVM > > > status flag. > > > > Substituted. > > > > > > > > > > > The callchain for evm_inode_post_setxattr() is: > > > > > > > > -> evm_inode_post_setxattr() > > > > > > Resets the EVM status flag for both EVM signatures and HMAC. > > > > > > > -> evm_update_evmxattr() > > > > > > evm_update_evmxattr() is only called for "security.evm", not acls. > > After re-reading the code with fresh eyes, I made a mistake here. > Please revert these suggestions. Ok. > > > > > I've added both comments but note that I'm explaining this in the > > paragraph below as well. > > Agreed. > > > > > > > > > > -> evm_calc_hmac() > > > > -> evm_calc_hmac_or_hash() > > > > > > > > and evm_cacl_hmac_or_hash() walks the global list of protected xattr > > > > names evm_config_xattrnames. This global list can be modified via > > > > /sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is > > > > restricted to security.* xattrs and the default xattrs in > > > > evm_config_xattrnames only contains security.* xattrs as well. > > > > > > > > So the actual value for posix acls is currently completely irrelevant > > > > for evm during evm_inode_post_setxattr() and frankly it should stay that > > > > way in the future to not cause the vfs any more headaches. But if the > > > > actual posix acl values matter then evm shouldn't operate on the binary > > > > void blob and try to hack around in the uapi struct anyway. Instead it > > > > should then in the future add a dedicated hook which takes a struct > > > > posix_acl argument passing the posix acls in the proper vfs format. > > > > > > > > For now it is sufficient to make evm_inode_post_set_acl() a wrapper > > > > around evm_inode_post_setxattr() not passing any actual values down. > > > > This will still cause the hashes to be updated as before. > > > > > > ^This will cause the EVM status flag to be reset. > > > > Substituted. > > My mistake. Can you replace it with: > > This will still cause the EVM status flag to be reset and EVM HMAC's to > be updated as before. Sure.
diff --git a/include/linux/evm.h b/include/linux/evm.h index 86139be48992..117ac01b2432 100644 --- a/include/linux/evm.h +++ b/include/linux/evm.h @@ -44,6 +44,12 @@ static inline int evm_inode_remove_acl(struct user_namespace *mnt_userns, { return evm_inode_set_acl(mnt_userns, dentry, acl_name, NULL); } +static inline void evm_inode_post_set_acl(struct dentry *dentry, + const char *acl_name, + struct posix_acl *kacl) +{ + return evm_inode_post_setxattr(dentry, acl_name, NULL, 0); +} extern int evm_inode_init_security(struct inode *inode, const struct xattr *xattr_array, struct xattr *evm); @@ -131,6 +137,13 @@ static inline int evm_inode_remove_acl(struct user_namespace *mnt_userns, return 0; } +static inline void evm_inode_post_set_acl(struct dentry *dentry, + const char *acl_name, + struct posix_acl *kacl) +{ + return; +} + static inline int evm_inode_init_security(struct inode *inode, const struct xattr *xattr_array, struct xattr *evm)
The security_inode_post_setxattr() hook is used by security modules to update their own security.* xattrs. Consequently none of the security modules operate on posix acls. So we don't need an additional security hook when post setting posix acls. However, the integrity subsystem wants to be informed about posix acl changes and specifically evm to update their hashes when the xattrs change. The callchain for evm_inode_post_setxattr() is: -> evm_inode_post_setxattr() -> evm_update_evmxattr() -> evm_calc_hmac() -> evm_calc_hmac_or_hash() and evm_cacl_hmac_or_hash() walks the global list of protected xattr names evm_config_xattrnames. This global list can be modified via /sys/security/integrity/evm/evm_xattrs. The write to "evm_xattrs" is restricted to security.* xattrs and the default xattrs in evm_config_xattrnames only contains security.* xattrs as well. So the actual value for posix acls is currently completely irrelevant for evm during evm_inode_post_setxattr() and frankly it should stay that way in the future to not cause the vfs any more headaches. But if the actual posix acl values matter then evm shouldn't operate on the binary void blob and try to hack around in the uapi struct anyway. Instead it should then in the future add a dedicated hook which takes a struct posix_acl argument passing the posix acls in the proper vfs format. For now it is sufficient to make evm_inode_post_set_acl() a wrapper around evm_inode_post_setxattr() not passing any actual values down. This will still cause the hashes to be updated as before. Reviewed-by: Paul Moore <paul@paul-moore.com> Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- Notes: /* v2 */ unchanged /* v3 */ Reviewed-by: Paul Moore <paul@paul-moore.com> /* v4 */ unchanged include/linux/evm.h | 13 +++++++++++++ 1 file changed, 13 insertions(+)