| 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
| ||
|
Message-ID: <6f310cfc1a0f505cf5e07885728d8cbf783cd644.camel@huaweicloud.com>
Date: Fri, 17 Jan 2025 18:06:06 +0100
From: Roberto Sassu <roberto.sassu@...weicloud.com>
To: Mimi Zohar <zohar@...ux.ibm.com>, viro@...iv.linux.org.uk,
brauner@...nel.org, jack@...e.cz, dmitry.kasatkin@...il.com,
eric.snowberg@...cle.com, paul@...l-moore.com, jmorris@...ei.org,
serge@...lyn.com
Cc: linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-integrity@...r.kernel.org, linux-security-module@...r.kernel.org,
Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PATCH v2 5/7] ima: Set security.ima on file close when
ima_appraise=fix
On Wed, 2025-01-15 at 08:46 -0500, Mimi Zohar wrote:
> Please use "__fput()" rather than "file close". Perhaps update the subject line to
> something like "ima: Defer fixing security.ima to __fput()".
>
> On Thu, 2024-11-28 at 11:06 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu@...wei.com>
> >
> > IMA-Appraisal implements a fix mode, selectable from the kernel command
> > line by specifying ima_appraise=fix.
> >
> > The fix mode is meant to be used in a TOFU (trust on first use) model,
> > where systems are supposed to work under controlled conditions before the
> > real enforcement starts.
> >
> > Since the systems are under controlled conditions, it is assumed that the
> > files are not corrupted, and thus their current data digest can be trusted,
> > and written to security.ima.
> >
> > When IMA-Appraisal is switched to enforcing mode, the security.ima value
> > collected during the fix mode is used as a reference value, and a mismatch
> > with the current value cause the access request to be denied.
> >
> > However, since fixing security.ima is placed in ima_appraise_measurement()
> > during the integrity check, it requires the inode lock to be taken in
> > process_measurement(), in addition to ima_update_xattr() invoked at file
> > close.
> >
> > Postpone the security.ima update to ima_check_last_writer(), by setting the
> > new atomic flag IMA_UPDATE_XATTR_FIX in the inode integrity metadata, in
> > ima_appraise_measurement(), if security.ima needs to be fixed. In this way,
> > the inode lock can be removed from process_measurement(). Also, set the
> > cause appropriately for the fix operation and for allowing access to new
> > and empty signed files.
> >
> > Finally, update security.ima when IMA_UPDATE_XATTR_FIX is set, and when
> > there wasn't a previous security.ima update, which occurs if the process
> > closing the file descriptor is the last writer.
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@...wei.com>
>
> Roberto, I really like the idea of removing the inode_lock in process_measurement()
> needed for writing xattrs, but I'm concerned about the delay being introduced. For
> example, does it interfere with labeling the filesystem with file signatures
> (with/without EVM enabled)?
There will be a difference when EVM is enabled, and inode metadata are
corrupted.
In that case, currently IMA in fix mode is able to fix inode metadata
as well, by writing security.ima. That happens because IMA ignores the
result of evm_verifyxattr() and writes the xattr directly, causing EVM
to update the HMAC to a valid one.
With the new patch, the EVM HMAC remains invalid until file close,
meaning that it will not be possible for example to set xattr on the
opened file descriptor. It works after closing the file though.
Setting other LSMs xattrs will fail as well, if the EVM HMAC is
invalid.
If the problem is EVM, I would recommend setting evm=fix as well, so
that inode metadata can be properly fixed.
I will update the documentation to describe the limitation introduced
by this patch, and to suggest to use evm=fix.
Thanks
Roberto
> > ---
> > security/integrity/ima/ima.h | 1 +
> > security/integrity/ima/ima_appraise.c | 7 +++++--
> > security/integrity/ima/ima_main.c | 18 +++++++++++-------
> > 3 files changed, 17 insertions(+), 9 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index b4eeab48f08a..22c3b87cfcac 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -179,6 +179,7 @@ struct ima_kexec_hdr {
> > #define IMA_CHANGE_ATTR 2
> > #define IMA_DIGSIG 3
> > #define IMA_MUST_MEASURE 4
> > +#define IMA_UPDATE_XATTR_FIX 5
> >
> > /* IMA integrity metadata associated with an inode */
> > struct ima_iint_cache {
> > diff --git a/security/integrity/ima/ima_appraise.c
> > b/security/integrity/ima/ima_appraise.c
> > index 656c709b974f..94401de8b805 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -576,8 +576,10 @@ int ima_appraise_measurement(enum ima_hooks func, struct
> > ima_iint_cache *iint,
> > if ((ima_appraise & IMA_APPRAISE_FIX) && !try_modsig &&
> > (!xattr_value ||
> > xattr_value->type != EVM_IMA_XATTR_DIGSIG)) {
> > - if (!ima_fix_xattr(dentry, iint))
> > - status = INTEGRITY_PASS;
> > + /* Fix by setting security.ima on file close. */
> > + set_bit(IMA_UPDATE_XATTR_FIX, &iint->atomic_flags);
> > + status = INTEGRITY_PASS;
> > + cause = "fix";
> > }
> >
> > /*
> > @@ -587,6 +589,7 @@ int ima_appraise_measurement(enum ima_hooks func, struct
> > ima_iint_cache *iint,
> > if (inode->i_size == 0 && iint->flags & IMA_NEW_FILE &&
> > test_bit(IMA_DIGSIG, &iint->atomic_flags)) {
> > status = INTEGRITY_PASS;
> > + cause = "new-signed-file";
> > }
> >
> > integrity_audit_msg(AUDIT_INTEGRITY_DATA, inode, filename,
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 1e474ff6a777..50b37420ea2c 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -158,13 +158,16 @@ static void ima_check_last_writer(struct ima_iint_cache
> > *iint,
> > struct inode *inode, struct file *file)
> > {
> > fmode_t mode = file->f_mode;
> > - bool update;
> > + bool update = false, update_fix;
> >
> > - if (!(mode & FMODE_WRITE))
> > + update_fix = test_and_clear_bit(IMA_UPDATE_XATTR_FIX,
> > + &iint->atomic_flags);
> > +
> > + if (!(mode & FMODE_WRITE) && !update_fix)
> > return;
> >
> > ima_iint_lock(inode);
> > - if (atomic_read(&inode->i_writecount) == 1) {
> > + if (atomic_read(&inode->i_writecount) == 1 && (mode & FMODE_WRITE)) {
>
> Probably better to reverse the "mode & FMODE_WRITE" and atomic_read() test order.
>
> Mimi
>
> > struct kstat stat;
> >
> > update = test_and_clear_bit(IMA_UPDATE_XATTR,
> > @@ -181,6 +184,10 @@ static void ima_check_last_writer(struct ima_iint_cache *iint,
> > ima_update_xattr(iint, file);
> > }
> > }
> > +
> > + if (!update && update_fix)
> > + ima_update_xattr(iint, file);
> > +
> > ima_iint_unlock(inode);
> > }
> >
> > @@ -378,13 +385,10 @@ static int process_measurement(struct file *file, const
> > struct cred *cred,
> > template_desc);
> > if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
> > rc = ima_check_blacklist(iint, modsig, pcr);
> > - if (rc != -EPERM) {
> > - inode_lock(inode);
> > + if (rc != -EPERM)
> > rc = ima_appraise_measurement(func, iint, file,
> > pathname, xattr_value,
> > xattr_len, modsig);
> > - inode_unlock(inode);
> > - }
> > if (!rc)
> > rc = mmap_violation_check(func, file, &pathbuf,
> > &pathname, filename);
Powered by blists - more mailing lists