On Wed, 2022-12-28 at 11:02 +0800, Xiu Jianfeng wrote:
> Currently it uses __vfs_setxattr_noperm() to update "security.evm",
> however there are two lsm hooks(inode_post_setxattr and inode_setsecurity)
> being called inside this function, which don't make any sense for xattr
> "security.evm", because the handlers of these two hooks, such as selinux
> and smack, only care about their own xattr.
Updating the security.ima hash triggers re-calculating and writing the
security.evm HMAC. Refer to evm_inode_post_setxattr().
Mimi
>
> On the other hand, there is a literally rather than actually cyclical
> callchain as follows:
> security_inode_post_setxattr
> ->evm_inode_post_setxattr
> ->evm_update_evmxattr
> ->__vfs_setxattr_noperm
> ->security_inode_post_setxattr
>
> So use __vfs_setxattr() to update "security.evm".
>
> Signed-off-by: Xiu Jianfeng <[email protected]>
> ---
> security/integrity/evm/evm_crypto.c | 7 +++----
> security/integrity/ima/ima_appraise.c | 8 ++++----
> 2 files changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> index fa5ff13fa8c9..d8275dfa49ef 100644
> --- a/security/integrity/evm/evm_crypto.c
> +++ b/security/integrity/evm/evm_crypto.c
> @@ -376,10 +376,9 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> xattr_value_len, &data);
> if (rc == 0) {
> data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry,
> - XATTR_NAME_EVM,
> - &data.hdr.xattr.data[1],
> - SHA1_DIGEST_SIZE + 1, 0);
> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
> + XATTR_NAME_EVM, &data.hdr.xattr.data[1],
> + SHA1_DIGEST_SIZE + 1, 0);
> } else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
> rc = __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_EVM);
> }
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index ee6f7e237f2e..d2de9dc6c345 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -98,10 +98,10 @@ static int ima_fix_xattr(struct dentry *dentry,
> iint->ima_hash->xattr.ng.type = IMA_XATTR_DIGEST_NG;
> iint->ima_hash->xattr.ng.algo = algo;
> }
> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry, XATTR_NAME_IMA,
> - &iint->ima_hash->xattr.data[offset],
> - (sizeof(iint->ima_hash->xattr) - offset) +
> - iint->ima_hash->length, 0);
> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
> + XATTR_NAME_IMA, &iint->ima_hash->xattr.data[offset],
> + (sizeof(iint->ima_hash->xattr) - offset) +
> + iint->ima_hash->length, 0);
> return rc;
> }
>
On 2023/1/19 5:45, Mimi Zohar wrote:
> On Wed, 2022-12-28 at 11:02 +0800, Xiu Jianfeng wrote:
>> Currently it uses __vfs_setxattr_noperm() to update "security.evm",
>> however there are two lsm hooks(inode_post_setxattr and inode_setsecurity)
>> being called inside this function, which don't make any sense for xattr
>> "security.evm", because the handlers of these two hooks, such as selinux
>> and smack, only care about their own xattr.
>
> Updating the security.ima hash triggers re-calculating and writing the
> security.evm HMAC. Refer to evm_inode_post_setxattr().
>
> Mimi
Hi Mimi,
I believe what Jianfeng is trying to do is to avoid re-triggering
security_inode_post_setxattr if we are updating security.evm. I can't
think of any other xattr that could "absorb" security.evm.
>
>>
>> On the other hand, there is a literally rather than actually cyclical
>> callchain as follows:
>> security_inode_post_setxattr
>> ->evm_inode_post_setxattr
>> ->evm_update_evmxattr
>> ->__vfs_setxattr_noperm
>> ->security_inode_post_setxattr
>>
>> So use __vfs_setxattr() to update "security.evm".
>>
>> Signed-off-by: Xiu Jianfeng <[email protected]>
>> ---
>> security/integrity/evm/evm_crypto.c | 7 +++----
>> security/integrity/ima/ima_appraise.c | 8 ++++----
>> 2 files changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>> index fa5ff13fa8c9..d8275dfa49ef 100644
>> --- a/security/integrity/evm/evm_crypto.c
>> +++ b/security/integrity/evm/evm_crypto.c
>> @@ -376,10 +376,9 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>> xattr_value_len, &data);
>> if (rc == 0) {
>> data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
>> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry,
>> - XATTR_NAME_EVM,
>> - &data.hdr.xattr.data[1],
>> - SHA1_DIGEST_SIZE + 1, 0);
>> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
>> + XATTR_NAME_EVM, &data.hdr.xattr.data[1],
>> + SHA1_DIGEST_SIZE + 1, 0);
>> } else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
>> rc = __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_EVM);
>> }
>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>> index ee6f7e237f2e..d2de9dc6c345 100644
>> --- a/security/integrity/ima/ima_appraise.c
>> +++ b/security/integrity/ima/ima_appraise.c
>> @@ -98,10 +98,10 @@ static int ima_fix_xattr(struct dentry *dentry,
>> iint->ima_hash->xattr.ng.type = IMA_XATTR_DIGEST_NG;
>> iint->ima_hash->xattr.ng.algo = algo;
>> }
>> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry, XATTR_NAME_IMA,
>> - &iint->ima_hash->xattr.data[offset],
>> - (sizeof(iint->ima_hash->xattr) - offset) +
>> - iint->ima_hash->length, 0);
>> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
>> + XATTR_NAME_IMA, &iint->ima_hash->xattr.data[offset],
>> + (sizeof(iint->ima_hash->xattr) - offset) +
>> + iint->ima_hash->length, 0);
>> return rc;
>> }
>>
>
>
--
Best
GUO Zihua
On Mon, 2023-01-30 at 09:53 +0800, Guozihua (Scott) wrote:
> On 2023/1/19 5:45, Mimi Zohar wrote:
> > On Wed, 2022-12-28 at 11:02 +0800, Xiu Jianfeng wrote:
> >> Currently it uses __vfs_setxattr_noperm() to update "security.evm",
> >> however there are two lsm hooks(inode_post_setxattr and inode_setsecurity)
> >> being called inside this function, which don't make any sense for xattr
> >> "security.evm", because the handlers of these two hooks, such as selinux
> >> and smack, only care about their own xattr.
> >
> > Updating the security.ima hash triggers re-calculating and writing the
> > security.evm HMAC. Refer to evm_inode_post_setxattr().
>
> Hi Mimi,
>
> I believe what Jianfeng is trying to do is to avoid re-triggering
> security_inode_post_setxattr if we are updating security.evm. I can't
> think of any other xattr that could "absorb" security.evm.
I understand. Comments below ...
> >
> >>
> >> On the other hand, there is a literally rather than actually cyclical
> >> callchain as follows:
> >> security_inode_post_setxattr
> >> ->evm_inode_post_setxattr
> >> ->evm_update_evmxattr
> >> ->__vfs_setxattr_noperm
> >> ->security_inode_post_setxattr
> >>
> >> So use __vfs_setxattr() to update "security.evm".
> >>
> >> Signed-off-by: Xiu Jianfeng <[email protected]>
> >> ---
> >> security/integrity/evm/evm_crypto.c | 7 +++----
> >> security/integrity/ima/ima_appraise.c | 8 ++++----
> >> 2 files changed, 7 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
> >> index fa5ff13fa8c9..d8275dfa49ef 100644
> >> --- a/security/integrity/evm/evm_crypto.c
> >> +++ b/security/integrity/evm/evm_crypto.c
> >> @@ -376,10 +376,9 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
> >> xattr_value_len, &data);
> >> if (rc == 0) {
> >> data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
> >> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry,
> >> - XATTR_NAME_EVM,
> >> - &data.hdr.xattr.data[1],
> >> - SHA1_DIGEST_SIZE + 1, 0);
> >> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
> >> + XATTR_NAME_EVM, &data.hdr.xattr.data[1],
> >> + SHA1_DIGEST_SIZE + 1, 0);
Although __vfs_setxattr_noperm() doesn't do any permission checking, it
does other things - make sure the filesystem supports writing xattrs,
calls fsnotify_xattr().
> >> } else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
> >> rc = __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_EVM);
> >> }
> >> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> >> index ee6f7e237f2e..d2de9dc6c345 100644
> >> --- a/security/integrity/ima/ima_appraise.c
> >> +++ b/security/integrity/ima/ima_appraise.c
> >> @@ -98,10 +98,10 @@ static int ima_fix_xattr(struct dentry *dentry,
> >> iint->ima_hash->xattr.ng.type = IMA_XATTR_DIGEST_NG;
> >> iint->ima_hash->xattr.ng.algo = algo;
> >> }
> >> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry, XATTR_NAME_IMA,
> >> - &iint->ima_hash->xattr.data[offset],
> >> - (sizeof(iint->ima_hash->xattr) - offset) +
> >> - iint->ima_hash->length, 0);
> >> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
> >> + XATTR_NAME_IMA, &iint->ima_hash->xattr.data[offset],
> >> + (sizeof(iint->ima_hash->xattr) - offset) +
> >> + iint->ima_hash->length, 0);
To clarify, ima_fix_xattr() is either directly called when in "fix"
mode or from ima_update_xattr(). With this change, the recalculated
file hash would be written to security.ima, but security.evm would not
be updated.
> >> return rc;
> >> }
--
thanks,
Mimi
Hi,
On 2023/1/31 19:31, Mimi Zohar wrote:
> On Mon, 2023-01-30 at 09:53 +0800, Guozihua (Scott) wrote:
>> On 2023/1/19 5:45, Mimi Zohar wrote:
>>> On Wed, 2022-12-28 at 11:02 +0800, Xiu Jianfeng wrote:
>>>> Currently it uses __vfs_setxattr_noperm() to update "security.evm",
>>>> however there are two lsm hooks(inode_post_setxattr and inode_setsecurity)
>>>> being called inside this function, which don't make any sense for xattr
>>>> "security.evm", because the handlers of these two hooks, such as selinux
>>>> and smack, only care about their own xattr.
>>>
>>> Updating the security.ima hash triggers re-calculating and writing the
>>> security.evm HMAC. Refer to evm_inode_post_setxattr().
>>
>> Hi Mimi,
>>
>> I believe what Jianfeng is trying to do is to avoid re-triggering
>> security_inode_post_setxattr if we are updating security.evm. I can't
>> think of any other xattr that could "absorb" security.evm.
>
> I understand. Comments below ...
>>>
>>>>
>>>> On the other hand, there is a literally rather than actually cyclical
>>>> callchain as follows:
>>>> security_inode_post_setxattr
>>>> ->evm_inode_post_setxattr
>>>> ->evm_update_evmxattr
>>>> ->__vfs_setxattr_noperm
>>>> ->security_inode_post_setxattr
>>>>
>>>> So use __vfs_setxattr() to update "security.evm".
>>>>
>>>> Signed-off-by: Xiu Jianfeng <[email protected]>
>>>> ---
>>>> security/integrity/evm/evm_crypto.c | 7 +++----
>>>> security/integrity/ima/ima_appraise.c | 8 ++++----
>>>> 2 files changed, 7 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>>> index fa5ff13fa8c9..d8275dfa49ef 100644
>>>> --- a/security/integrity/evm/evm_crypto.c
>>>> +++ b/security/integrity/evm/evm_crypto.c
>>>> @@ -376,10 +376,9 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>>>> xattr_value_len, &data);
>>>> if (rc == 0) {
>>>> data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
>>>> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry,
>>>> - XATTR_NAME_EVM,
>>>> - &data.hdr.xattr.data[1],
>>>> - SHA1_DIGEST_SIZE + 1, 0);
>>>> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
>>>> + XATTR_NAME_EVM, &data.hdr.xattr.data[1],
>>>> + SHA1_DIGEST_SIZE + 1, 0);
>
> Although __vfs_setxattr_noperm() doesn't do any permission checking, it
> does other things - make sure the filesystem supports writing xattrs,
> calls fsnotify_xattr().
>
>>>> } else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
>>>> rc = __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_EVM);
>>>> }
>>>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>>>> index ee6f7e237f2e..d2de9dc6c345 100644
>>>> --- a/security/integrity/ima/ima_appraise.c
>>>> +++ b/security/integrity/ima/ima_appraise.c
>>>> @@ -98,10 +98,10 @@ static int ima_fix_xattr(struct dentry *dentry,
>>>> iint->ima_hash->xattr.ng.type = IMA_XATTR_DIGEST_NG;
>>>> iint->ima_hash->xattr.ng.algo = algo;
>>>> }
>>>> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry, XATTR_NAME_IMA,
>>>> - &iint->ima_hash->xattr.data[offset],
>>>> - (sizeof(iint->ima_hash->xattr) - offset) +
>>>> - iint->ima_hash->length, 0);
>>>> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
>>>> + XATTR_NAME_IMA, &iint->ima_hash->xattr.data[offset],
>>>> + (sizeof(iint->ima_hash->xattr) - offset) +
>>>> + iint->ima_hash->length, 0);
>
> To clarify, ima_fix_xattr() is either directly called when in "fix"
> mode or from ima_update_xattr(). With this change, the recalculated
> file hash would be written to security.ima, but security.evm would not
> be updated.
Thanks for you explanation, I will drop this patch.
>
>>>> return rc;
>>>> }
>
On 2023/1/31 19:31, Mimi Zohar wrote:
> On Mon, 2023-01-30 at 09:53 +0800, Guozihua (Scott) wrote:
>> On 2023/1/19 5:45, Mimi Zohar wrote:
>>> On Wed, 2022-12-28 at 11:02 +0800, Xiu Jianfeng wrote:
>>>> Currently it uses __vfs_setxattr_noperm() to update "security.evm",
>>>> however there are two lsm hooks(inode_post_setxattr and inode_setsecurity)
>>>> being called inside this function, which don't make any sense for xattr
>>>> "security.evm", because the handlers of these two hooks, such as selinux
>>>> and smack, only care about their own xattr.
>>>
>>> Updating the security.ima hash triggers re-calculating and writing the
>>> security.evm HMAC. Refer to evm_inode_post_setxattr().
>>
>> Hi Mimi,
>>
>> I believe what Jianfeng is trying to do is to avoid re-triggering
>> security_inode_post_setxattr if we are updating security.evm. I can't
>> think of any other xattr that could "absorb" security.evm.
>
> I understand. Comments below ...
>>>
>>>>
>>>> On the other hand, there is a literally rather than actually cyclical
>>>> callchain as follows:
>>>> security_inode_post_setxattr
>>>> ->evm_inode_post_setxattr
>>>> ->evm_update_evmxattr
>>>> ->__vfs_setxattr_noperm
>>>> ->security_inode_post_setxattr
>>>>
>>>> So use __vfs_setxattr() to update "security.evm".
>>>>
>>>> Signed-off-by: Xiu Jianfeng <[email protected]>
>>>> ---
>>>> security/integrity/evm/evm_crypto.c | 7 +++----
>>>> security/integrity/ima/ima_appraise.c | 8 ++++----
>>>> 2 files changed, 7 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
>>>> index fa5ff13fa8c9..d8275dfa49ef 100644
>>>> --- a/security/integrity/evm/evm_crypto.c
>>>> +++ b/security/integrity/evm/evm_crypto.c
>>>> @@ -376,10 +376,9 @@ int evm_update_evmxattr(struct dentry *dentry, const char *xattr_name,
>>>> xattr_value_len, &data);
>>>> if (rc == 0) {
>>>> data.hdr.xattr.sha1.type = EVM_XATTR_HMAC;
>>>> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry,
>>>> - XATTR_NAME_EVM,
>>>> - &data.hdr.xattr.data[1],
>>>> - SHA1_DIGEST_SIZE + 1, 0);
>>>> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
>>>> + XATTR_NAME_EVM, &data.hdr.xattr.data[1],
>>>> + SHA1_DIGEST_SIZE + 1, 0);
>
> Although __vfs_setxattr_noperm() doesn't do any permission checking, it
> does other things - make sure the filesystem supports writing xattrs,
> calls fsnotify_xattr().
Thanks for the explanation Mimi, this makes sense.
>
>>>> } else if (rc == -ENODATA && (inode->i_opflags & IOP_XATTR)) {
>>>> rc = __vfs_removexattr(&init_user_ns, dentry, XATTR_NAME_EVM);
>>>> }
>>>> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
>>>> index ee6f7e237f2e..d2de9dc6c345 100644
>>>> --- a/security/integrity/ima/ima_appraise.c
>>>> +++ b/security/integrity/ima/ima_appraise.c
>>>> @@ -98,10 +98,10 @@ static int ima_fix_xattr(struct dentry *dentry,
>>>> iint->ima_hash->xattr.ng.type = IMA_XATTR_DIGEST_NG;
>>>> iint->ima_hash->xattr.ng.algo = algo;
>>>> }
>>>> - rc = __vfs_setxattr_noperm(&init_user_ns, dentry, XATTR_NAME_IMA,
>>>> - &iint->ima_hash->xattr.data[offset],
>>>> - (sizeof(iint->ima_hash->xattr) - offset) +
>>>> - iint->ima_hash->length, 0);
>>>> + rc = __vfs_setxattr(&init_user_ns, dentry, d_inode(dentry),
>>>> + XATTR_NAME_IMA, &iint->ima_hash->xattr.data[offset],
>>>> + (sizeof(iint->ima_hash->xattr) - offset) +
>>>> + iint->ima_hash->length, 0);
>
> To clarify, ima_fix_xattr() is either directly called when in "fix"
> mode or from ima_update_xattr(). With this change, the recalculated
> file hash would be written to security.ima, but security.evm would not
> be updated.
Sorry I missed this part. I agree that it is not a good idea to alter
ima_fix_xattr().
>
>>>> return rc;
>>>> }
>
--
Best
GUO Zihua