This patch prevents memory access beyond the evm_tfm array by checking the validity of the index (hash algorithm) passed to init_desc(). The hash algorithm can be arbitrarily set if the security.ima xattr type is not EVM_XATTR_HMAC.
Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Cc: stable@vger.kernel.org --- security/integrity/evm/evm_crypto.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index e11564eb645b..82a38e801ee4 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo) tfm = &hmac_tfm; algo = evm_hmac; } else { + if (hash_algo >= HASH_ALGO__LAST) + return ERR_PTR(-EINVAL); + tfm = &evm_tfm[hash_algo]; algo = hash_algo_name[hash_algo]; }
This patch adds a call to evm_reset_status() in evm_inode_post_setattr(), before security.evm is updated. The same is done in the other evm_inode_post_* functions.
Fixes: 523b74b16bcbb ("evm: reset EVM status when file attributes change") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Cc: stable@vger.kernel.org --- security/integrity/evm/evm_main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index b6d9f14bc234..b41c2d8a8834 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -512,8 +512,11 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid) if (!evm_key_loaded()) return;
- if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) + if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) { + evm_reset_status(dentry->d_inode); + evm_update_evmxattr(dentry, NULL, NULL, 0); + } }
/*
On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
This patch adds a call to evm_reset_status() in evm_inode_post_setattr(), before security.evm is updated. The same is done in the other evm_inode_post_* functions.
Fixes: 523b74b16bcbb ("evm: reset EVM status when file attributes change") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Cc: stable@vger.kernel.org
Why all of a sudden do we also need to clear the EVM cached status when modifying the file attributes? The HMAC is being recalculated. If the reason is because of EVM portable and immutable signatures, then the "Fixes" tag is incorrect.
Mimi
security/integrity/evm/evm_main.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c index b6d9f14bc234..b41c2d8a8834 100644 --- a/security/integrity/evm/evm_main.c +++ b/security/integrity/evm/evm_main.c @@ -512,8 +512,11 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid) if (!evm_key_loaded()) return;
- if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
- if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID)) {
evm_reset_status(dentry->d_inode);- evm_update_evmxattr(dentry, NULL, NULL, 0);
- }
} /*
Currently, ima_appraise_measurement() ignores the EVM status when evm_verifyxattr() returns INTEGRITY_UNKNOWN. If a file has a valid security.ima xattr with type IMA_XATTR_DIGEST or IMA_XATTR_DIGEST_NG, ima_appraise_measurement() returns INTEGRITY_PASS regardless of the EVM status. The problem is that the EVM status is overwritten with the appraisal status.
This patch mitigates the issue by selecting signature verification as the only method allowed for appraisal when EVM is not initialized. Since the new behavior might break user space, it must be turned on by adding ima_appraise_req_evm to the kernel command line.
Fixes: 2fe5d6def1672 ("ima: integrity appraisal extension") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Cc: stable@vger.kernel.org --- Documentation/admin-guide/kernel-parameters.txt | 3 +++ security/integrity/ima/ima_appraise.c | 12 ++++++++++++ 2 files changed, 15 insertions(+)
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 52e6fbb042cc..80e1c233656b 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1588,6 +1588,9 @@ Format: { "off" | "enforce" | "fix" | "log" } default: "enforce"
+ ima_appraise_req_evm + [IMA] require EVM for appraisal with file digests. + ima_appraise_tcb [IMA] Deprecated. Use ima_policy= instead. The builtin appraise policy appraises all files owned by uid=0. diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index 5fb7127bbe68..a32ed5d7afd1 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -33,6 +33,14 @@ static int __init default_appraise_setup(char *str)
__setup("ima_appraise=", default_appraise_setup);
+static bool ima_appraise_req_evm; +static int __init appraise_req_evm_setup(char *str) +{ + ima_appraise_req_evm = true; + return 1; +} +__setup("ima_appraise_req_evm", appraise_req_evm_setup); + /* * is_ima_appraise_enabled - return appraise status * @@ -245,7 +253,11 @@ int ima_appraise_measurement(enum ima_hooks func, switch (status) { case INTEGRITY_PASS: case INTEGRITY_PASS_IMMUTABLE: + break; case INTEGRITY_UNKNOWN: + if (ima_appraise_req_evm && + xattr_value->type != EVM_IMA_XATTR_DIGSIG) + goto out; break; case INTEGRITY_NOXATTRS: /* No EVM protected xattrs. */ case INTEGRITY_NOLABEL: /* No security.evm xattr. */
On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 52e6fbb042cc..80e1c233656b 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1588,6 +1588,9 @@ Format: { "off" | "enforce" | "fix" | "log" } default: "enforce"
- ima_appraise_req_evm
[IMA] require EVM for appraisal with file digests.
As much as possible we want to limit the number of new boot command line options as possible. Is there a reason for not extending "ima_appraise=" with "require-evm" or "enforce-evm"?
Mimi
- ima_appraise_tcb [IMA] Deprecated. Use ima_policy= instead. The builtin appraise policy appraises all files owned by uid=0.
On 5/20/2019 11:20 PM, Mimi Zohar wrote:
On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 52e6fbb042cc..80e1c233656b 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1588,6 +1588,9 @@ Format: { "off" | "enforce" | "fix" | "log" } default: "enforce"
- ima_appraise_req_evm
[IMA] require EVM for appraisal with file digests.As much as possible we want to limit the number of new boot command line options as possible. Is there a reason for not extending "ima_appraise=" with "require-evm" or "enforce-evm"?
ima-appraise= can be disabled with CONFIG_IMA_APPRAISE_BOOTPARAM, which probably is done when the system is in production.
Should I allow to use ima-appraise=require-evm even if CONFIG_IMA_APPRAISE_BOOTPARAM=n?
Thanks
Roberto
On Tue, 2019-05-21 at 09:26 +0200, Roberto Sassu wrote:
On 5/20/2019 11:20 PM, Mimi Zohar wrote:
On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index 52e6fbb042cc..80e1c233656b 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -1588,6 +1588,9 @@ Format: { "off" | "enforce" | "fix" | "log" } default: "enforce"
- ima_appraise_req_evm
[IMA] require EVM for appraisal with file digests.As much as possible we want to limit the number of new boot command line options as possible. Is there a reason for not extending "ima_appraise=" with "require-evm" or "enforce-evm"?
ima-appraise= can be disabled with CONFIG_IMA_APPRAISE_BOOTPARAM, which probably is done when the system is in production.
Should I allow to use ima-appraise=require-evm even if CONFIG_IMA_APPRAISE_BOOTPARAM=n?
Yes, that should be fine. It's making "ima_appraise" stricter.
Mimi
This patch ensures that integrity_audit_msg() is called only when the status is not INTEGRITY_PASS.
Fixes: 8606404fa555c ("ima: digital signature verification support") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Cc: stable@vger.kernel.org --- security/integrity/ima/ima_appraise.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index a32ed5d7afd1..f5f4506bcb8e 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -359,8 +359,9 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_PASS; }
- integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename, - op, cause, rc, 0); + if (status != INTEGRITY_PASS) + integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, + filename, op, cause, rc, 0); } else { ima_cache_flags(iint, func); }
On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
This patch ensures that integrity_audit_msg() is called only when the status is not INTEGRITY_PASS.
Fixes: 8606404fa555c ("ima: digital signature verification support") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Cc: stable@vger.kernel.org
security/integrity/ima/ima_appraise.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index a32ed5d7afd1..f5f4506bcb8e 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -359,8 +359,9 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_PASS; }
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,op, cause, rc, 0);
if (status != INTEGRITY_PASS)integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,filename, op, cause, rc, 0);
For some reason, the integrity verification has failed. In some specific cases, we'll let it pass, but do we really want to remove any indication that it failed in all cases?
Mimi
} else { ima_cache_flags(iint, func); }
On 5/20/2019 11:20 PM, Mimi Zohar wrote:
On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
This patch ensures that integrity_audit_msg() is called only when the status is not INTEGRITY_PASS.
Fixes: 8606404fa555c ("ima: digital signature verification support") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Cc: stable@vger.kernel.org
security/integrity/ima/ima_appraise.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c index a32ed5d7afd1..f5f4506bcb8e 100644 --- a/security/integrity/ima/ima_appraise.c +++ b/security/integrity/ima/ima_appraise.c @@ -359,8 +359,9 @@ int ima_appraise_measurement(enum ima_hooks func, status = INTEGRITY_PASS; }
integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,op, cause, rc, 0);
if (status != INTEGRITY_PASS)integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode,filename, op, cause, rc, 0);For some reason, the integrity verification has failed. In some specific cases, we'll let it pass, but do we really want to remove any indication that it failed in all cases?
Ok. It is fine for me to discard the patch.
Roberto
On Thu, 2019-05-16 at 18:12 +0200, Roberto Sassu wrote:
This patch prevents memory access beyond the evm_tfm array by checking the validity of the index (hash algorithm) passed to init_desc(). The hash algorithm can be arbitrarily set if the security.ima xattr type is not EVM_XATTR_HMAC.
Fixes: 5feeb61183dde ("evm: Allow non-SHA1 digital signatures") Signed-off-by: Roberto Sassu roberto.sassu@huawei.com Cc: stable@vger.kernel.org
Thanks!
security/integrity/evm/evm_crypto.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c index e11564eb645b..82a38e801ee4 100644 --- a/security/integrity/evm/evm_crypto.c +++ b/security/integrity/evm/evm_crypto.c @@ -89,6 +89,9 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo) tfm = &hmac_tfm; algo = evm_hmac; } else {
if (hash_algo >= HASH_ALGO__LAST)return ERR_PTR(-EINVAL);- tfm = &evm_tfm[hash_algo]; algo = hash_algo_name[hash_algo]; }
linux-stable-mirror@lists.linaro.org
-
Mimi Zohar -
Roberto Sassu