> -----Original Message-----
> From: Roberto Sassu
> Sent: Wednesday, April 29, 2020 9:40 AM
> To: zohar@xxxxxxxxxxxxx; david.safford@xxxxxxxxx;
> viro@xxxxxxxxxxxxxxxxxx; jmorris@xxxxxxxxx
> Cc: linux-fsdevel@xxxxxxxxxxxxxxx; linux-integrity@xxxxxxxxxxxxxxx; linux-
> security-module@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Silviu
> Vlasceanu <Silviu.Vlasceanu@xxxxxxxxxx>; Roberto Sassu
> <roberto.sassu@xxxxxxxxxx>
> Subject: [RFC][PATCH 1/3] evm: Move hooks outside LSM infrastructure
Any thought on this? The implementation can be discussed later.
I just wanted a feedback on the approach, if this is the right direction
to solve the problem.
Thanks
Roberto
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
> EVM is a module for the protection of the integrity of file metadata. It
> protects security-relevant extended attributes, and some file attributes
> such as the UID and the GID. It protects their integrity with an HMAC or
> with a signature.
>
> What makes EVM different from other LSMs is that it makes a security
> decision depending on multiple pieces of information, which cannot be
> managed atomically by the system.
>
> Example: cp -a file.orig file.dest
>
> If security.selinux, security.ima and security.evm must be preserved, cp
> will invoke setxattr() for each xattr, and EVM performs a verification
> during each operation. The problem is that copying security.evm from
> file.orig to file.dest will likely break the following EVM verifications if
> some metadata still have to be copied. EVM has no visibility on the
> metadata of the source file, so it cannot determine when the copy can be
> considered complete.
>
> On the other hand, EVM has to check metadata during every operation to
> ensure that there is no transition from corrupted metadata, e.g. after an
> offline attack, to valid ones after the operation. An HMAC update would
> prevent the corruption to be detected, as the HMAC on the new values
> would
> be correct. Thus, to avoid this issue, EVM has to return an error to the
> system call so that its execution will be interrupted.
>
> A solution that would satisfy both requirements, not breaking user space
> applications and detecting corrupted metadata is to let metadata operations
> be completed successfully and to pass the result of the EVM verification
> from the pre hooks to the post hooks. In this way, the HMAC update can be
> avoided if the verification wasn't successful.
>
> This approach will bring another important benefit: it is no longer
> required that every file has a valid HMAC or signature. Instead of always
> enforcing metadata integrity, even when it is not relevant for IMA, EVM
> will let IMA decide for files selected with the appraisal policy,
> depending on the result of the requested verification.
>
> The main problem is that the result of the verification currently cannot be
> passed from the pre hooks to the post hooks, due to how the LSM API is
> defined. A possible solution would be to use integrity_iint_cache for this
> purpose, but it will increase the memory pressure, as new structures will
> be allocated also for metadata operations, not only for measurement,
> appraisal and audit. Another solution would be to extend the LSM API, but
> it seems not worthwhile as EVM would be the only module getting a benefit
> from this change.
>
> Given that pre and post hooks are called from the same system call, a more
> efficient solution seems to move the hooks outside the LSM infrastructure,
> so that the return value of the pre hooks can be passed to the post hooks.
> A predefined error (-EAGAIN) will be used to signal to the system call to
> continue the execution. Otherwise, if the pre hooks return -EPERM, the
> system calls will behave as before and will immediately return before
> metadata are changed.
>
> Overview of the changes:
>
> evm_inode_init_security() LSM (no change)
> evm_inode_setxattr() LSM -> vfs_setxattr()
> evm_inode_post_setxattr() LSM -> vfs_setxattr()
> evm_inode_removexattr() LSM -> vfs_removexattr()
> evm_inode_post_removexattr() vfs_removexattr() (no change)
> evm_inode_setattr() LSM -> vfs_setattr()
> evm_inode_post_setattr() vfs_setattr() (no change)
> evm_verifyxattr() outside LSM (no change)
>
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
> fs/attr.c | 5 ++++-
> fs/xattr.c | 17 +++++++++++++++--
> security/security.c | 18 +++---------------
> 3 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index b4bbdbd4c8ca..8f26d7d2e3b4 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -224,7 +224,7 @@ int notify_change(struct dentry * dentry, struct iattr
> * attr, struct inode **de
> {
> struct inode *inode = dentry->d_inode;
> umode_t mode = inode->i_mode;
> - int error;
> + int error, evm_error;
> struct timespec64 now;
> unsigned int ia_valid = attr->ia_valid;
>
> @@ -328,6 +328,9 @@ int notify_change(struct dentry * dentry, struct iattr
> * attr, struct inode **de
> error = security_inode_setattr(dentry, attr);
> if (error)
> return error;
> + evm_error = evm_inode_setattr(dentry, attr);
> + if (evm_error)
> + return evm_error;
> error = try_break_deleg(inode, delegated_inode);
> if (error)
> return error;
> diff --git a/fs/xattr.c b/fs/xattr.c
> index e13265e65871..3b323b75b741 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -183,6 +183,7 @@ int __vfs_setxattr_noperm(struct dentry *dentry,
> const char *name,
> fsnotify_xattr(dentry);
> security_inode_post_setxattr(dentry, name, value,
> size, flags);
> + evm_inode_post_setxattr(dentry, name, value,
> size);
> }
> } else {
> if (unlikely(is_bad_inode(inode)))
> @@ -210,7 +211,7 @@ vfs_setxattr(struct dentry *dentry, const char
> *name, const void *value,
> size_t size, int flags)
> {
> struct inode *inode = dentry->d_inode;
> - int error;
> + int error, evm_error;
>
> error = xattr_permission(inode, name, MAY_WRITE);
> if (error)
> @@ -221,6 +222,12 @@ vfs_setxattr(struct dentry *dentry, const char
> *name, const void *value,
> if (error)
> goto out;
>
> + evm_error = evm_inode_setxattr(dentry, name, value, size);
> + if (evm_error) {
> + error = evm_error;
> + goto out;
> + }
> +
> error = __vfs_setxattr_noperm(dentry, name, value, size, flags);
>
> out:
> @@ -382,7 +389,7 @@ int
> vfs_removexattr(struct dentry *dentry, const char *name)
> {
> struct inode *inode = dentry->d_inode;
> - int error;
> + int error, evm_error;
>
> error = xattr_permission(inode, name, MAY_WRITE);
> if (error)
> @@ -393,6 +400,12 @@ vfs_removexattr(struct dentry *dentry, const char
> *name)
> if (error)
> goto out;
>
> + evm_error = evm_inode_removexattr(dentry, name);
> + if (evm_error) {
> + error = evm_error;
> + goto out;
> + }
> +
> error = __vfs_removexattr(dentry, name);
>
> if (!error) {
> diff --git a/security/security.c b/security/security.c
> index 7fed24b9d57e..e1368ab34cee 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1255,14 +1255,9 @@ int security_inode_permission(struct inode
> *inode, int mask)
>
> int security_inode_setattr(struct dentry *dentry, struct iattr *attr)
> {
> - int ret;
> -
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> return 0;
> - ret = call_int_hook(inode_setattr, 0, dentry, attr);
> - if (ret)
> - return ret;
> - return evm_inode_setattr(dentry, attr);
> + return call_int_hook(inode_setattr, 0, dentry, attr);
> }
> EXPORT_SYMBOL_GPL(security_inode_setattr);
>
> @@ -1291,10 +1286,7 @@ int security_inode_setxattr(struct dentry *dentry,
> const char *name,
> ret = cap_inode_setxattr(dentry, name, value, size, flags);
> if (ret)
> return ret;
> - ret = ima_inode_setxattr(dentry, name, value, size);
> - if (ret)
> - return ret;
> - return evm_inode_setxattr(dentry, name, value, size);
> + return ima_inode_setxattr(dentry, name, value, size);
> }
>
> void security_inode_post_setxattr(struct dentry *dentry, const char
> *name,
> @@ -1303,7 +1295,6 @@ void security_inode_post_setxattr(struct dentry
> *dentry, const char *name,
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> return;
> call_void_hook(inode_post_setxattr, dentry, name, value, size,
> flags);
> - evm_inode_post_setxattr(dentry, name, value, size);
> }
>
> int security_inode_getxattr(struct dentry *dentry, const char *name)
> @@ -1335,10 +1326,7 @@ int security_inode_removexattr(struct dentry
> *dentry, const char *name)
> ret = cap_inode_removexattr(dentry, name);
> if (ret)
> return ret;
> - ret = ima_inode_removexattr(dentry, name);
> - if (ret)
> - return ret;
> - return evm_inode_removexattr(dentry, name);
> + return ima_inode_removexattr(dentry, name);
> }
>
> int security_inode_need_killpriv(struct dentry *dentry)
> --
> 2.17.1
