linux-kernel - Re: [PATCH 3/5] ima: Reset EVM status upon detecting changes to overlay backing file

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Wed, 31 Jan 2024 09:46:24 -0500
From: Stefan Berger <stefanb@...ux.ibm.com>
To: Amir Goldstein <amir73il@...il.com>
Cc: linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org,
        linux-unionfs@...r.kernel.org, linux-kernel@...r.kernel.org,
        paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
        zohar@...ux.ibm.com, roberto.sassu@...wei.com, miklos@...redi.hu,
        Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH 3/5] ima: Reset EVM status upon detecting changes to
 overlay backing file



On 1/31/24 08:56, Amir Goldstein wrote:
> On Tue, Jan 30, 2024 at 11:46 PM Stefan Berger <stefanb@...ux.ibm.com> wrote:
>>
>> To avoid caching effects to take effect reset the EVM status upon
>> detecting changes to the overlay backing files. This prevents a not-yet-
>> copied-up file on the overlay from executing if for example the
>> security.evm xattr on the file on the 'lower' layer has been removed.
>>
> 
> And what is expected to happen when file is executed after copy up?

The copy-up may be triggered by changing file content or file metadata.
For EVM file metadata (file attributes and xattrs) are important and if 
they change EVM would re-evaluate the file, meaning that it would 
determine the file mode bits, uid, gid and xattrs and calculate a hash 
over them and compare this hash against the signature in security.evm.

> Doesn't this change also protect the same threat after copy up?

 From what I remember from my testing is that file attribute or extended 
attribute changes on an already copied-up file were already handled 
correctly, meaning they caused the re-evaluation of the file as 
described above.

> 
>> Signed-off-by: Stefan Berger <stefanb@...ux.ibm.com>
>> ---
>>   include/linux/evm.h               | 8 ++++++++
>>   security/integrity/evm/evm_main.c | 7 +++++++
>>   security/integrity/ima/ima_main.c | 2 ++
>>   3 files changed, 17 insertions(+)
>>
>> diff --git a/include/linux/evm.h b/include/linux/evm.h
>> index d8c0343436b8..e7d6742eee9d 100644
>> --- a/include/linux/evm.h
>> +++ b/include/linux/evm.h
>> @@ -66,6 +66,8 @@ extern int evm_protected_xattr_if_enabled(const char *req_xattr_name);
>>   extern int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>>                                       int buffer_size, char type,
>>                                       bool canonical_fmt);
>> +extern void evm_reset_cache_status(struct dentry *dentry,
>> +                                  struct integrity_iint_cache *iint);
>>   #ifdef CONFIG_FS_POSIX_ACL
>>   extern int posix_xattr_acl(const char *xattrname);
>>   #else
>> @@ -189,5 +191,11 @@ static inline int evm_read_protected_xattrs(struct dentry *dentry, u8 *buffer,
>>          return -EOPNOTSUPP;
>>   }
>>
>> +static inline void evm_reset_cache_status(struct dentry *dentry,
>> +                                         struct integrity_iint_cache *iint)
>> +{
>> +       return;
>> +}
>> +
>>   #endif /* CONFIG_EVM */
>>   #endif /* LINUX_EVM_H */
>> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
>> index 22a5e26860ea..e96d127b48a2 100644
>> --- a/security/integrity/evm/evm_main.c
>> +++ b/security/integrity/evm/evm_main.c
>> @@ -721,6 +721,13 @@ static void evm_reset_status(struct inode *inode)
>>                  iint->evm_status = INTEGRITY_UNKNOWN;
>>   }
>>
>> +void evm_reset_cache_status(struct dentry *dentry,
>> +                           struct integrity_iint_cache *iint)
>> +{
>> +       if (d_real_inode(dentry) != d_backing_inode(dentry))
>> +               iint->evm_status = INTEGRITY_UNKNOWN;
>> +}
>> +
>>   /**
>>    * evm_revalidate_status - report whether EVM status re-validation is necessary
>>    * @xattr_name: pointer to the affected extended attribute name
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index cc1217ac2c6f..84bdc6e58329 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -26,6 +26,7 @@
>>   #include <linux/ima.h>
>>   #include <linux/fs.h>
>>   #include <linux/iversion.h>
>> +#include <linux/evm.h>
>>
>>   #include "ima.h"
>>
>> @@ -295,6 +296,7 @@ static int process_measurement(struct file *file, const struct cred *cred,
>>                      !inode_eq_iversion(backing_inode, iint->version)) {
>>                          iint->flags &= ~IMA_DONE_MASK;
>>                          iint->measured_pcrs = 0;
>> +                       evm_reset_cache_status(file_dentry(file), iint);
>>                  }
>>          }
> 
> Make sense.
> Unrelated to your change, I now noticed something odd about Mimi's change:
> 
>          backing_inode = d_real_inode(file_dentry(file));
> 
> I find the choice of variable name to be quite confusing, because ima/evm code
> uses  d_backing_inode() all over the place and d_real_inode() !=
> d_backing_inode().
> 
> First of all, there is never any reason to use d_backing_inode() and its name is
> quite confusing in the first place, but it will be a big cleanup to
> remove them all.
> 
> Suggest to rename the variable to real_inode, same as in
> ima_collect_measurement()
> to be consistent and reduce confusion factor, which is already high enough ;)
> 
> Thanks,
> Amir.