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 <[email protected]>
Cc: [email protected]
---
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];
}
--
2.17.1
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 <[email protected]>
Cc: [email protected]
---
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);
+ }
}
/*
--
2.17.1
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 <[email protected]>
Cc: [email protected]
---
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);
}
--
2.17.1
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 <[email protected]>
> Cc: [email protected]
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);
> + }
> }
>
> /*
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 <[email protected]>
> Cc: [email protected]
> ---
> 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 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 <[email protected]>
> Cc: [email protected]
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];
> }
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 <[email protected]>
>> Cc: [email protected]
>> ---
>> 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
--
HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Bo PENG, Jian LI, Yanli SHI