[RESEND,PATCH 0/7] Improvements and fixes
Resending due to conflicts with existing code. This patch set provides some improvements and fixes. The first three patches solve a permission problem caused by EVM. EVM denies xattr operations also for files that are not appraised by IMA. If only executables are appraised, xattr operations on the other files should be allowed, even if metadata verification fails (for example due to missing security.evm). At the moment, in openEuler we use EVM_ALLOW_METADATA_WRITES to avoid this problem (EVM does not check metadata integrity), but it would be useful to do the verification for example to prevent accidental changes on immutable metadata. The fourth patch enables the choice of the algorithm for the HMAC and ensures that the parameters passed to the functions which handle the HMAC are consistent with the algorithm chosen. The last three patches are simple bug fixes. Roberto Sassu (7): evm: Move hooks outside LSM infrastructure evm: Extend API of post hooks to pass the result of pre hooks evm: Return -EAGAIN to ignore verification failures evm: Propagate choice of HMAC algorithm in evm_crypto.c ima: Fix datalen check in ima_write_data() evm: Fix validation of fake xattr passed by IMA evm: Initialize saved_evm_status fs/attr.c | 7 ++- fs/xattr.c | 65 +++++++++++++++++--------- include/linux/evm.h | 18 +++++--- security/integrity/evm/Kconfig | 32 +++++++++++++ security/integrity/evm/evm.h | 1 + security/integrity/evm/evm_crypto.c | 15 ++++-- security/integrity/evm/evm_main.c | 71 +++++++++++++++++++++-------- security/integrity/ima/ima_fs.c | 2 +- security/integrity/integrity.h | 2 +- security/security.c | 18 ++------ 10 files changed, 159 insertions(+), 72 deletions(-) -- 2.27.GIT
hulk inclusion category: feature feature: digest-lists --------------------------- 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@huawei.com> --- 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 d22e8187477f..322e5e887ece 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -227,7 +227,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; @@ -326,6 +326,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 470ee0af3200..a0e4e77f78b1 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -182,6 +182,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))) @@ -221,7 +222,7 @@ __vfs_setxattr_locked(struct dentry *dentry, const char *name, struct inode **delegated_inode) { struct inode *inode = dentry->d_inode; - int error; + int error, evm_error; error = xattr_permission(inode, name, MAY_WRITE); if (error) @@ -231,6 +232,12 @@ __vfs_setxattr_locked(struct dentry *dentry, const char *name, if (error) goto out; + evm_error = evm_inode_setxattr(dentry, name, value, size); + if (evm_error) { + error = evm_error; + goto out; + } + error = try_break_deleg(inode, delegated_inode); if (error) goto out; @@ -428,7 +435,7 @@ __vfs_removexattr_locked(struct dentry *dentry, const char *name, struct inode **delegated_inode) { struct inode *inode = dentry->d_inode; - int error; + int error, evm_error; error = xattr_permission(inode, name, MAY_WRITE); if (error) @@ -438,6 +445,12 @@ __vfs_removexattr_locked(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 = try_break_deleg(inode, delegated_inode); if (error) goto out; diff --git a/security/security.c b/security/security.c index 9478444bf93f..17e2bed11bf7 100644 --- a/security/security.c +++ b/security/security.c @@ -706,14 +706,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); @@ -742,10 +737,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, @@ -754,7 +746,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) @@ -786,10 +777,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.27.GIT
hulk inclusion category: feature feature: digest-lists --------------------------- This patch extends the API of post hooks to pass the result returned by the pre hooks. Given that now this information is available, post hooks can stop before updating the HMAC if the result of the pre hook is not zero. They still reset the result of the last verification, stored in the integrity_iint_cache, to ensure that file metadata are re-evaluated after update. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- fs/attr.c | 2 +- fs/xattr.c | 50 ++++++++++++++++++------------- include/linux/evm.h | 18 +++++++---- security/integrity/evm/evm_main.c | 21 +++++++++++-- 4 files changed, 60 insertions(+), 31 deletions(-) diff --git a/fs/attr.c b/fs/attr.c index 322e5e887ece..9a632198c07c 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -341,7 +341,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de if (!error) { fsnotify_change(dentry, ia_valid); ima_inode_post_setattr(dentry); - evm_inode_post_setattr(dentry, ia_valid); + evm_inode_post_setattr(dentry, ia_valid, evm_error); } return error; diff --git a/fs/xattr.c b/fs/xattr.c index a0e4e77f78b1..64ebf7b7fe89 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -150,24 +150,8 @@ __vfs_setxattr(struct dentry *dentry, struct inode *inode, const char *name, } EXPORT_SYMBOL(__vfs_setxattr); -/** - * __vfs_setxattr_noperm - perform setxattr operation without performing - * permission checks. - * - * @dentry - object to perform setxattr on - * @name - xattr name to set - * @value - value to set @name to - * @size - size of @value - * @flags - flags to pass into filesystem operations - * - * returns the result of the internal setxattr or setsecurity operations. - * - * This function requires the caller to lock the inode's i_mutex before it - * is executed. It also assumes that the caller will make the appropriate - * permission checks. - */ -int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, - const void *value, size_t size, int flags) +static int __vfs_setxattr_noperm_evm(struct dentry *dentry, const char *name, + const void *value, size_t size, int flags, int evm_error) { struct inode *inode = dentry->d_inode; int error = -EAGAIN; @@ -182,7 +166,8 @@ 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); + evm_inode_post_setxattr(dentry, name, value, size, + evm_error); } } else { if (unlikely(is_bad_inode(inode))) @@ -204,6 +189,28 @@ int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, return error; } +/** + * __vfs_setxattr_noperm - perform setxattr operation without performing + * permission checks. + * + * @dentry - object to perform setxattr on + * @name - xattr name to set + * @value - value to set @name to + * @size - size of @value + * @flags - flags to pass into filesystem operations + * + * returns the result of the internal setxattr or setsecurity operations. + * + * This function requires the caller to lock the inode's i_mutex before it + * is executed. It also assumes that the caller will make the appropriate + * permission checks. + */ +int __vfs_setxattr_noperm(struct dentry *dentry, const char *name, + const void *value, size_t size, int flags) +{ + return __vfs_setxattr_noperm_evm(dentry, name, value, size, flags, 0); +} + /** * __vfs_setxattr_locked: set an extended attribute while holding the inode * lock @@ -242,7 +249,8 @@ __vfs_setxattr_locked(struct dentry *dentry, const char *name, if (error) goto out; - error = __vfs_setxattr_noperm(dentry, name, value, size, flags); + error = __vfs_setxattr_noperm_evm(dentry, name, value, size, flags, + evm_error); out: return error; @@ -459,7 +467,7 @@ __vfs_removexattr_locked(struct dentry *dentry, const char *name, if (!error) { fsnotify_xattr(dentry); - evm_inode_post_removexattr(dentry, name); + evm_inode_post_removexattr(dentry, name, evm_error); } out: diff --git a/include/linux/evm.h b/include/linux/evm.h index 8302bc29bb35..cc23d2248d3e 100644 --- a/include/linux/evm.h +++ b/include/linux/evm.h @@ -22,16 +22,19 @@ extern enum integrity_status evm_verifyxattr(struct dentry *dentry, size_t xattr_value_len, struct integrity_iint_cache *iint); extern int evm_inode_setattr(struct dentry *dentry, struct iattr *attr); -extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid); +extern void evm_inode_post_setattr(struct dentry *dentry, int ia_valid, + int evm_pre_error); extern int evm_inode_setxattr(struct dentry *dentry, const char *name, const void *value, size_t size); extern void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, const void *xattr_value, - size_t xattr_value_len); + size_t xattr_value_len, + int evm_pre_error); extern int evm_inode_removexattr(struct dentry *dentry, const char *xattr_name); extern void evm_inode_post_removexattr(struct dentry *dentry, - const char *xattr_name); + const char *xattr_name, + int evm_pre_error); extern int evm_inode_init_security(struct inode *inode, const struct xattr *xattr_array, struct xattr *evm); @@ -66,7 +69,8 @@ static inline int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) return 0; } -static inline void evm_inode_post_setattr(struct dentry *dentry, int ia_valid) +static inline void evm_inode_post_setattr(struct dentry *dentry, int ia_valid, + int evm_pre_error) { return; } @@ -80,7 +84,8 @@ static inline int evm_inode_setxattr(struct dentry *dentry, const char *name, static inline void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, const void *xattr_value, - size_t xattr_value_len) + size_t xattr_value_len, + int evm_pre_error) { return; } @@ -92,7 +97,8 @@ static inline int evm_inode_removexattr(struct dentry *dentry, } static inline void evm_inode_post_removexattr(struct dentry *dentry, - const char *xattr_name) + const char *xattr_name, + int evm_pre_error) { return; } diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 138c0ab419fa..f0b515334171 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -576,6 +576,7 @@ static void evm_reset_status(struct inode *inode, int bit) * @xattr_name: pointer to the affected extended attribute name * @xattr_value: pointer to the new extended attribute value * @xattr_value_len: pointer to the new extended attribute value length + * @evm_pre_error: error returned by evm_inode_setxattr() * * Update the HMAC stored in 'security.evm' to reflect the change. * @@ -584,7 +585,8 @@ static void evm_reset_status(struct inode *inode, int bit) * i_mutex lock. */ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, - const void *xattr_value, size_t xattr_value_len) + const void *xattr_value, size_t xattr_value_len, + int evm_pre_error) { int is_evm = !strcmp(xattr_name, XATTR_NAME_EVM); @@ -597,6 +599,9 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, if (is_evm) return; + if (evm_pre_error) + return; + evm_update_evmxattr(dentry, xattr_name, xattr_value, xattr_value_len); } @@ -604,13 +609,15 @@ void evm_inode_post_setxattr(struct dentry *dentry, const char *xattr_name, * evm_inode_post_removexattr - update 'security.evm' after removing the xattr * @dentry: pointer to the affected dentry * @xattr_name: pointer to the affected extended attribute name + * @evm_pre_error: error returned by evm_inode_removexattr() * * Update the HMAC stored in 'security.evm' to reflect removal of the xattr. * * No need to take the i_mutex lock here, as this function is called from * vfs_removexattr() which takes the i_mutex. */ -void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name) +void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name, + int evm_pre_error) { int is_evm = !strcmp(xattr_name, XATTR_NAME_EVM); @@ -622,6 +629,9 @@ void evm_inode_post_removexattr(struct dentry *dentry, const char *xattr_name) if (is_evm) return; + if (evm_pre_error) + return; + evm_update_evmxattr(dentry, xattr_name, NULL, 0); } @@ -681,6 +691,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) * evm_inode_post_setattr - update 'security.evm' after modifying metadata * @dentry: pointer to the affected dentry * @ia_valid: for the UID and GID status + * @evm_pre_error: error returned by evm_inode_setattr() * * For now, update the HMAC stored in 'security.evm' to reflect UID/GID * changes. @@ -688,13 +699,17 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) * This function is called from notify_change(), which expects the caller * to lock the inode's i_mutex. */ -void evm_inode_post_setattr(struct dentry *dentry, int ia_valid) +void evm_inode_post_setattr(struct dentry *dentry, int ia_valid, + int evm_pre_error) { if (!evm_key_loaded()) return; evm_reset_status(dentry->d_inode, IMA_CHANGE_ATTR); + if (evm_pre_error) + return; + if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) evm_update_evmxattr(dentry, NULL, NULL, 0); } -- 2.27.GIT
hulk inclusion category: feature feature: digest-lists --------------------------- By default, EVM maintains the same behavior as before hooks were moved outside the LSM infrastructure. When EVM returns -EPERM, callers stop their execution and return the error to user space. This patch introduces a new mode, called ignore, that changes the return value of the pre hooks from -EPERM to -EAGAIN. It also modifies the callers of pre and post hooks to continue the execution if -EAGAIN is returned. The error is then handled by the post hooks. The only error that is not ignored is when user space is trying to modify immutable metadata. Once that signature has been validated with the current values of metadata, there is no valid reason to change them. From user space perspective, operations on corrupted metadata are successfully performed but post hooks didn't update the HMAC. At the next IMA verification, when evm_verifyxattr() is called, corruption will be detected and access will be denied. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- fs/attr.c | 2 +- fs/xattr.c | 4 ++-- security/integrity/evm/evm_main.c | 24 ++++++++++++++++++------ 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/fs/attr.c b/fs/attr.c index 9a632198c07c..a96871b6ba7d 100644 --- a/fs/attr.c +++ b/fs/attr.c @@ -327,7 +327,7 @@ int notify_change(struct dentry * dentry, struct iattr * attr, struct inode **de if (error) return error; evm_error = evm_inode_setattr(dentry, attr); - if (evm_error) + if (evm_error && evm_error != -EAGAIN) return evm_error; error = try_break_deleg(inode, delegated_inode); if (error) diff --git a/fs/xattr.c b/fs/xattr.c index 64ebf7b7fe89..adb11cb82be5 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -240,7 +240,7 @@ __vfs_setxattr_locked(struct dentry *dentry, const char *name, goto out; evm_error = evm_inode_setxattr(dentry, name, value, size); - if (evm_error) { + if (evm_error && evm_error != -EAGAIN) { error = evm_error; goto out; } @@ -454,7 +454,7 @@ __vfs_removexattr_locked(struct dentry *dentry, const char *name, goto out; evm_error = evm_inode_removexattr(dentry, name); - if (evm_error) { + if (evm_error && evm_error != -EAGAIN) { error = evm_error; goto out; } diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index f0b515334171..181f4758e3bd 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -61,7 +61,7 @@ static struct xattr_list evm_config_default_xattrnames[] = { LIST_HEAD(evm_config_xattrnames); -static int evm_fixmode; +static int evm_fixmode, evm_ignoremode __ro_after_init; static int __init evm_set_param(char *str) { if (strncmp(str, "fix", 3) == 0) @@ -70,6 +70,8 @@ static int __init evm_set_param(char *str) evm_initialized |= EVM_INIT_X509; else if (strncmp(str, "allow_metadata_writes", 21) == 0) evm_initialized |= EVM_ALLOW_METADATA_WRITES; + else if (strncmp(str, "ignore", 6) == 0) + evm_ignoremode = 1; return 0; } __setup("evm=", evm_set_param); @@ -450,6 +452,7 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, const void *xattr_value, size_t xattr_value_len) { enum integrity_status evm_status; + int rc = -EPERM; if (strcmp(xattr_name, XATTR_NAME_EVM) == 0) { if (!capable(CAP_SYS_ADMIN)) @@ -494,12 +497,17 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name, !evm_xattr_change(dentry, xattr_name, xattr_value, xattr_value_len)) return 0; - if (evm_status != INTEGRITY_PASS) + if (evm_status != INTEGRITY_PASS) { + if (evm_ignoremode && evm_status != INTEGRITY_PASS_IMMUTABLE) + rc = -EAGAIN; + integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata", integrity_status_msg[evm_status], - -EPERM, 0); - return evm_status == INTEGRITY_PASS ? 0 : -EPERM; + rc, 0); + } + + return evm_status == INTEGRITY_PASS ? 0 : rc; } /** @@ -659,6 +667,7 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) { unsigned int ia_valid = attr->ia_valid; enum integrity_status evm_status; + int rc = -EPERM; /* Policy permits modification of the protected attrs even though * there's no HMAC key loaded @@ -681,10 +690,13 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr) !evm_attr_change(dentry, attr)) return 0; + if (evm_ignoremode && evm_status != INTEGRITY_PASS_IMMUTABLE) + rc = -EAGAIN; + integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry), dentry->d_name.name, "appraise_metadata", - integrity_status_msg[evm_status], -EPERM, 0); - return -EPERM; + integrity_status_msg[evm_status], rc, 0); + return rc; } /** -- 2.27.GIT
hulk inclusion category: bugfix bugzilla: 3007 CVE: NA --------------------------- Commit 5feeb61183dd ("evm: Allow non-SHA1 digital signatures") introduced the possibility to use different hash algorithm for signatures, but kept the algorithm for the HMAC hard-coded (SHA1). Switching to a different algorithm for HMAC would require to change the code in different places. This patch introduces a new global variable called evm_hash_algo, and consistently uses it whenever EVM perform HMAC-related operations. It also introduces a new kernel configuration option called CONFIG_EVM_DEFAULT_HASH so that evm_hash_algo can be defined at kernel compilation time. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/evm/Kconfig | 32 +++++++++++++++++++++++++++++ security/integrity/evm/evm.h | 1 + security/integrity/evm/evm_crypto.c | 15 +++++++++----- security/integrity/evm/evm_main.c | 21 +++++++++++-------- security/integrity/integrity.h | 2 +- 5 files changed, 56 insertions(+), 15 deletions(-) diff --git a/security/integrity/evm/Kconfig b/security/integrity/evm/Kconfig index 60221852b26a..9ccfa4df58ed 100644 --- a/security/integrity/evm/Kconfig +++ b/security/integrity/evm/Kconfig @@ -12,6 +12,38 @@ config EVM If you are unsure how to answer this question, answer N. +choice + prompt "Default EVM hash algorithm" + default EVM_DEFAULT_HASH_SHA256 + depends on EVM + help + Select the default hash algorithm used for the HMAC. + + config EVM_DEFAULT_HASH_SHA1 + bool "SHA1 (default)" + depends on CRYPTO_SHA1=y + + config EVM_DEFAULT_HASH_SHA256 + bool "SHA256" + depends on CRYPTO_SHA256=y + + config EVM_DEFAULT_HASH_SHA512 + bool "SHA512" + depends on CRYPTO_SHA512=y + + config EVM_DEFAULT_HASH_WP512 + bool "WP512" + depends on CRYPTO_WP512=y +endchoice + +config EVM_DEFAULT_HASH + string + depends on EVM + default "sha1" if EVM_DEFAULT_HASH_SHA1 + default "sha256" if EVM_DEFAULT_HASH_SHA256 + default "sha512" if EVM_DEFAULT_HASH_SHA512 + default "wp512" if EVM_DEFAULT_HASH_WP512 + config EVM_ATTR_FSUUID bool "FSUUID (version 2)" default y diff --git a/security/integrity/evm/evm.h b/security/integrity/evm/evm.h index c3f437f5db10..9afbe0f3254a 100644 --- a/security/integrity/evm/evm.h +++ b/security/integrity/evm/evm.h @@ -36,6 +36,7 @@ struct xattr_list { }; extern int evm_initialized; +extern enum hash_algo evm_hash_algo; #define EVM_ATTR_FSUUID 0x0001 diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index 61082bff1645..917a08e06b62 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -38,7 +38,7 @@ static DEFINE_MUTEX(mutex); static unsigned long evm_set_key_flags; -static char * const evm_hmac = "hmac(sha256)"; +enum hash_algo evm_hash_algo __ro_after_init = HASH_ALGO_SHA256; /** * evm_set_key() - set EVM HMAC key from the kernel @@ -79,8 +79,12 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo) long rc; const char *algo; struct crypto_shash **tfm, *tmp_tfm; + char evm_hmac[CRYPTO_MAX_ALG_NAME]; struct shash_desc *desc; + snprintf(evm_hmac, sizeof(evm_hmac), "hmac(%s)", + CONFIG_EVM_DEFAULT_HASH); + if (type == EVM_XATTR_HMAC) { if (!(evm_initialized & EVM_INIT_HMAC)) { pr_err_once("HMAC key is not set\n"); @@ -324,14 +328,15 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name, if (rc) return -EPERM; - data.hdr.algo = HASH_ALGO_SHA1; + data.hdr.algo = evm_hash_algo; rc = evm_calc_hmac(dentry, xattr_name, xattr_value, xattr_value_len, &data); if (rc == 0) { data.hdr.xattr.sha1.type = EVM_XATTR_HMAC; rc = __vfs_setxattr_noperm(dentry, XATTR_NAME_EVM, &data.hdr.xattr.data[1], - SHA1_DIGEST_SIZE + 1, 0); + hash_digest_size[evm_hash_algo] + 1, + 0); } else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) { rc = __vfs_removexattr(dentry, XATTR_NAME_EVM); } @@ -343,7 +348,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr, { struct shash_desc *desc; - desc = init_desc(EVM_XATTR_HMAC, HASH_ALGO_SHA1); + desc = init_desc(EVM_XATTR_HMAC, evm_hash_algo); if (IS_ERR(desc)) { pr_info("init_desc failed\n"); return PTR_ERR(desc); @@ -356,7 +361,7 @@ int evm_init_hmac(struct inode *inode, const struct xattr *lsm_xattr, } /* - * Get the key from the TPM for the SHA1-HMAC + * Get the key from the TPM for the HMAC */ int evm_init_key(void) { diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 181f4758e3bd..61172bd96780 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -196,21 +196,19 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, /* check value type */ switch (xattr_data->type) { case EVM_XATTR_HMAC: - if (xattr_len < SHA256_DIGEST_SIZE) { + if (xattr_len != hash_digest_size[evm_hash_algo] + 1) { + evm_status = INTEGRITY_FAIL; rc = -EINVAL; break; } - if (xattr_len != sizeof(struct evm_ima_xattr_data)) { - evm_status = INTEGRITY_FAIL; - goto out; - } - digest.hdr.algo = HASH_ALGO_SHA1; + + digest.hdr.algo = evm_hash_algo; rc = evm_calc_hmac(dentry, xattr_name, xattr_value, xattr_value_len, &digest); if (rc) break; rc = crypto_memneq(xattr_data->digest, digest.digest, - SHA1_DIGEST_SIZE); + hash_digest_size[evm_hash_algo]); if (rc) rc = -EINVAL; break; @@ -750,7 +748,7 @@ int evm_inode_init_security(struct inode *inode, goto out; evm_xattr->value = xattr_data; - evm_xattr->value_len = sizeof(*xattr_data); + evm_xattr->value_len = hash_digest_size[evm_hash_algo] + 1; evm_xattr->name = XATTR_EVM_SUFFIX; return 0; out: @@ -772,10 +770,15 @@ void __init evm_load_x509(void) static int __init init_evm(void) { - int error; + int error, i; struct list_head *pos, *q; struct xattr_list *xattr; + i = match_string(hash_algo_name, HASH_ALGO__LAST, + CONFIG_EVM_DEFAULT_HASH); + if (i >= 0) + evm_hash_algo = i; + evm_init_config(); error = integrity_init_keyring(INTEGRITY_KEYRING_EVM); diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h index 2828661c6097..82008b9dda57 100644 --- a/security/integrity/integrity.h +++ b/security/integrity/integrity.h @@ -83,7 +83,7 @@ enum evm_ima_xattr_type { struct evm_ima_xattr_data { u8 type; - u8 digest[SHA256_DIGEST_SIZE]; + u8 digest[SHA512_DIGEST_SIZE]; } __packed; #define IMA_MAX_DIGEST_SIZE 64 -- 2.27.GIT
hulk inclusion category: feature feature: digest-lists --------------------------- This patch avoids a possible overflow of datalen when it is checked at the beginning of ima_write_data(). Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/ima/ima_fs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c index 8c28ff907aa7..a5f87fcdf731 100644 --- a/security/integrity/ima/ima_fs.c +++ b/security/integrity/ima/ima_fs.c @@ -361,7 +361,7 @@ static ssize_t ima_write_data(struct file *file, const char __user *buf, goto out; result = -EFBIG; - if (datalen + 1 > 64 * 1024 * 1024) + if (datalen > 64 * 1024 * 1024 - 1) goto out; result = -ENOMEM; -- 2.27.GIT
hulk inclusion category: feature feature: digest-lists --------------------------- This patch ensures that xattr_data_len passed by IMA is greater than 2 bytes, so that the hash algorithm can be retrieved from xattr_value. The fake xattr type is always IMA_XATTR_DIGEST_NG. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/evm/evm_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index 61172bd96780..d4363cad7696 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -178,7 +178,7 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, /* IMA added a fake xattr, set also EVM fake xattr */ if (!ima_present && xattr_name && !strcmp(xattr_name, XATTR_NAME_IMA) && - xattr_value_len >= sizeof(struct evm_ima_xattr_data)) { + xattr_value_len > 2) { evm_fake_xattr.hash_algo = ((struct evm_ima_xattr_data *)xattr_value)->digest[0]; xattr_data = -- 2.27.GIT
hulk inclusion category: feature feature: digest-lists --------------------------- This patch avoids a compilation error by initializing the saved_evm_status variable. Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com> --- security/integrity/evm/evm_main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index d4363cad7696..5155ff4c4ef2 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -146,7 +146,8 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry, { struct evm_ima_xattr_data *xattr_data = NULL; struct signature_v2_hdr *hdr; - enum integrity_status evm_status = INTEGRITY_PASS, saved_evm_status; + enum integrity_status evm_status = INTEGRITY_PASS; + enum integrity_status saved_evm_status = INTEGRITY_UNKNOWN; struct evm_digest digest; struct ima_digest *found_digest; struct inode *inode; -- 2.27.GIT
Applied. On 2020/8/18 17:27, Roberto Sassu wrote:
Resending due to conflicts with existing code.
This patch set provides some improvements and fixes. The first three patches solve a permission problem caused by EVM. EVM denies xattr operations also for files that are not appraised by IMA. If only executables are appraised, xattr operations on the other files should be allowed, even if metadata verification fails (for example due to missing security.evm).
At the moment, in openEuler we use EVM_ALLOW_METADATA_WRITES to avoid this problem (EVM does not check metadata integrity), but it would be useful to do the verification for example to prevent accidental changes on immutable metadata.
The fourth patch enables the choice of the algorithm for the HMAC and ensures that the parameters passed to the functions which handle the HMAC are consistent with the algorithm chosen.
The last three patches are simple bug fixes.
Roberto Sassu (7): evm: Move hooks outside LSM infrastructure evm: Extend API of post hooks to pass the result of pre hooks evm: Return -EAGAIN to ignore verification failures evm: Propagate choice of HMAC algorithm in evm_crypto.c ima: Fix datalen check in ima_write_data() evm: Fix validation of fake xattr passed by IMA evm: Initialize saved_evm_status
fs/attr.c | 7 ++- fs/xattr.c | 65 +++++++++++++++++--------- include/linux/evm.h | 18 +++++--- security/integrity/evm/Kconfig | 32 +++++++++++++ security/integrity/evm/evm.h | 1 + security/integrity/evm/evm_crypto.c | 15 ++++-- security/integrity/evm/evm_main.c | 71 +++++++++++++++++++++-------- security/integrity/ima/ima_fs.c | 2 +- security/integrity/integrity.h | 2 +- security/security.c | 18 ++------ 10 files changed, 159 insertions(+), 72 deletions(-)
participants (2)
-
Roberto Sassu -
Yang Yingliang