diff mbox series

[v2,07/12] evm: Introduce EVM_RESET_STATUS atomic flag

Message ID 20200904092643.20013-3-roberto.sassu@huawei.com
State New
Headers show
Series IMA/EVM fixes | expand

Commit Message

Roberto Sassu Sept. 4, 2020, 9:26 a.m. UTC
When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
metadata. Its main purpose is to allow users to freely set metadata when
they are protected by a portable signature, until the HMAC key is loaded.

However, IMA is not notified about metadata changes and, after the first
successful appraisal, always allows access to the files without checking
metadata again.

This patch introduces the new atomic flag EVM_RESET_STATUS in
integrity_iint_cache that is set in the EVM post hooks and cleared in
evm_verify_hmac(). IMA checks the new flag in process_measurement() and if
it is set, it clears the appraisal flags.

Although the flag could be cleared also by evm_inode_setxattr() and
evm_inode_setattr() before IMA sees it, this does not happen if
EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
evm_verifyxattr(), this ensures that IMA always sees the flag set before it
is cleared.

This patch also adds a call to evm_reset_status() in
evm_inode_post_setattr() so that EVM won't return the cached status the
next time appraisal is performed.

Cc: stable@vger.kernel.org # 4.16.x
Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 17 +++++++++++++++--
 security/integrity/ima/ima_main.c |  8 ++++++--
 security/integrity/integrity.h    |  1 +
 3 files changed, 22 insertions(+), 4 deletions(-)

Comments

Mimi Zohar Sept. 17, 2020, 12:01 p.m. UTC | #1
[Cc'ing John Johansen]

Hi Roberto,

On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation on
> metadata. Its main purpose is to allow users to freely set metadata when
> they are protected by a portable signature, until the HMAC key is loaded.
> 
> However, IMA is not notified about metadata changes and, after the first
> successful appraisal, always allows access to the files without checking
> metadata again.
> 
> This patch introduces the new atomic flag EVM_RESET_STATUS in
> integrity_iint_cache that is set in the EVM post hooks and cleared in
> evm_verify_hmac(). IMA checks the new flag in process_measurement() and if
> it is set, it clears the appraisal flags.
> 
> Although the flag could be cleared also by evm_inode_setxattr() and
> evm_inode_setattr() before IMA sees it, this does not happen if
> EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> is cleared.
> 
> This patch also adds a call to evm_reset_status() in
> evm_inode_post_setattr() so that EVM won't return the cached status the
> next time appraisal is performed.
> 
> Cc: stable@vger.kernel.org # 4.16.x
> Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of EVM-protected metadata")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 17 +++++++++++++++--
>  security/integrity/ima/ima_main.c |  8 ++++++--
>  security/integrity/integrity.h    |  1 +
>  3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 4e9f5e8b21d5..05be1ad3e6f3 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -221,8 +221,15 @@ static enum integrity_status evm_verify_hmac(struct dentry *dentry,
>  		evm_status = (rc == -ENODATA) ?
>  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
>  out:
> -	if (iint)
> +	if (iint) {
> +		/*
> +		 * EVM_RESET_STATUS can be cleared only by evm_verifyxattr()
> +		 * when EVM_ALLOW_METADATA_WRITES is set. This guarantees that
> +		 * IMA sees the EVM_RESET_STATUS flag set before it is cleared.
> +		 */
> +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
>  		iint->evm_status = evm_status;

True IMA is currently the only caller of evm_verifyxattr() in the
upstreamed kernel, but it is an exported function, which may be called
from elsewhere.  The previous version crossed the boundary between EVM
& IMA with EVM modifying the IMA flag directly.  This version assumes
that IMA will be the only caller.  Otherwise, I like this version.

Mimi

> +	}
>  	kfree(xattr_data);
>  	return evm_status;
>  }
> @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
>  	struct integrity_iint_cache *iint;
>  
>  	iint = integrity_iint_find(inode);
> -	if (iint)
> +	if (iint) {
> +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> +			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> +
>  		iint->evm_status = INTEGRITY_UNKNOWN;
> +	}
>  }
>  
>  /**
> @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
>  	if (!evm_key_loaded())
>  		return;
>  
> +	evm_reset_status(dentry->d_inode);
> +
>  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
>  		evm_update_evmxattr(dentry, NULL, NULL, 0);
>  }
Roberto Sassu Sept. 17, 2020, 5:36 p.m. UTC | #2
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Thursday, September 17, 2020 2:01 PM
> [Cc'ing John Johansen]
> 
> Hi Roberto,
> 
> On Fri, 2020-09-04 at 11:26 +0200, Roberto Sassu wrote:
> > When EVM_ALLOW_METADATA_WRITES is set, EVM allows any operation
> on
> > metadata. Its main purpose is to allow users to freely set metadata when
> > they are protected by a portable signature, until the HMAC key is loaded.
> >
> > However, IMA is not notified about metadata changes and, after the first
> > successful appraisal, always allows access to the files without checking
> > metadata again.
> >
> > This patch introduces the new atomic flag EVM_RESET_STATUS in
> > integrity_iint_cache that is set in the EVM post hooks and cleared in
> > evm_verify_hmac(). IMA checks the new flag in process_measurement()
> and if
> > it is set, it clears the appraisal flags.
> >
> > Although the flag could be cleared also by evm_inode_setxattr() and
> > evm_inode_setattr() before IMA sees it, this does not happen if
> > EVM_ALLOW_METADATA_WRITES is set. Since the only remaining caller is
> > evm_verifyxattr(), this ensures that IMA always sees the flag set before it
> > is cleared.
> >
> > This patch also adds a call to evm_reset_status() in
> > evm_inode_post_setattr() so that EVM won't return the cached status
> the
> > next time appraisal is performed.
> >
> > Cc: stable@vger.kernel.org # 4.16.x
> > Fixes: ae1ba1676b88e ("EVM: Allow userland to permit modification of
> EVM-protected metadata")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/evm/evm_main.c | 17 +++++++++++++++--
> >  security/integrity/ima/ima_main.c |  8 ++++++--
> >  security/integrity/integrity.h    |  1 +
> >  3 files changed, 22 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -221,8 +221,15 @@ static enum integrity_status
> evm_verify_hmac(struct dentry *dentry,
> >  		evm_status = (rc == -ENODATA) ?
> >  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> >  out:
> > -	if (iint)
> > +	if (iint) {
> > +		/*
> > +		 * EVM_RESET_STATUS can be cleared only by
> evm_verifyxattr()
> > +		 * when EVM_ALLOW_METADATA_WRITES is set. This
> guarantees that
> > +		 * IMA sees the EVM_RESET_STATUS flag set before it is
> cleared.
> > +		 */
> > +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> >  		iint->evm_status = evm_status;
> 
> True IMA is currently the only caller of evm_verifyxattr() in the
> upstreamed kernel, but it is an exported function, which may be called
> from elsewhere.  The previous version crossed the boundary between EVM
> & IMA with EVM modifying the IMA flag directly.  This version assumes
> that IMA will be the only caller.  Otherwise, I like this version.

Ok, I think it is better, as you suggested, to export a new EVM function
that tells if evm_reset_status() will be executed in the EVM post hooks, and
to call this function from IMA. IMA would then call ima_reset_appraise_flags()
also depending on the result of the new EVM function.

ima_reset_appraise_flags() should be called in a post hook in IMA.
Should I introduce it?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli

> Mimi
> 
> > +	}
> >  	kfree(xattr_data);
> >  	return evm_status;
> >  }
> > @@ -418,8 +425,12 @@ static void evm_reset_status(struct inode *inode)
> >  	struct integrity_iint_cache *iint;
> >
> >  	iint = integrity_iint_find(inode);
> > -	if (iint)
> > +	if (iint) {
> > +		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
> > +			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > +
> >  		iint->evm_status = INTEGRITY_UNKNOWN;
> > +	}
> >  }
> >
> >  /**
> > @@ -513,6 +524,8 @@ void evm_inode_post_setattr(struct dentry
> *dentry, int ia_valid)
> >  	if (!evm_key_loaded())
> >  		return;
> >
> > +	evm_reset_status(dentry->d_inode);
> > +
> >  	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
> >  		evm_update_evmxattr(dentry, NULL, NULL, 0);
> >  }
Mimi Zohar Sept. 17, 2020, 5:47 p.m. UTC | #3
On Thu, 2020-09-17 at 17:36 +0000, Roberto Sassu wrote:
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > index 4e9f5e8b21d5..05be1ad3e6f3 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -221,8 +221,15 @@ static enum integrity_status
> > evm_verify_hmac(struct dentry *dentry,
> > >  		evm_status = (rc == -ENODATA) ?
> > >  				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
> > >  out:
> > > -	if (iint)
> > > +	if (iint) {
> > > +		/*
> > > +		 * EVM_RESET_STATUS can be cleared only by
> > evm_verifyxattr()
> > > +		 * when EVM_ALLOW_METADATA_WRITES is set. This
> > guarantees that
> > > +		 * IMA sees the EVM_RESET_STATUS flag set before it is
> > cleared.
> > > +		 */
> > > +		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
> > >  		iint->evm_status = evm_status;
> > 
> > True IMA is currently the only caller of evm_verifyxattr() in the
> > upstreamed kernel, but it is an exported function, which may be called
> > from elsewhere.  The previous version crossed the boundary between EVM
> > & IMA with EVM modifying the IMA flag directly.  This version assumes
> > that IMA will be the only caller.  Otherwise, I like this version.
> 
> Ok, I think it is better, as you suggested, to export a new EVM function
> that tells if evm_reset_status() will be executed in the EVM post hooks, and
> to call this function from IMA. IMA would then call ima_reset_appraise_flags()
> also depending on the result of the new EVM function.
> 
> ima_reset_appraise_flags() should be called in a post hook in IMA.
> Should I introduce it?

Yes, so any callers of evm_verifyxattr() will need to implement the
post hook as well.  As much as possible, please limit code duplication.

The last time I looked, there didn't seem to be a locking concern, but
please make sure.

thanks,

Mimi
diff mbox series

Patch

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 4e9f5e8b21d5..05be1ad3e6f3 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -221,8 +221,15 @@  static enum integrity_status evm_verify_hmac(struct dentry *dentry,
 		evm_status = (rc == -ENODATA) ?
 				INTEGRITY_NOXATTRS : INTEGRITY_FAIL;
 out:
-	if (iint)
+	if (iint) {
+		/*
+		 * EVM_RESET_STATUS can be cleared only by evm_verifyxattr()
+		 * when EVM_ALLOW_METADATA_WRITES is set. This guarantees that
+		 * IMA sees the EVM_RESET_STATUS flag set before it is cleared.
+		 */
+		clear_bit(EVM_RESET_STATUS, &iint->atomic_flags);
 		iint->evm_status = evm_status;
+	}
 	kfree(xattr_data);
 	return evm_status;
 }
@@ -418,8 +425,12 @@  static void evm_reset_status(struct inode *inode)
 	struct integrity_iint_cache *iint;
 
 	iint = integrity_iint_find(inode);
-	if (iint)
+	if (iint) {
+		if (evm_initialized & EVM_ALLOW_METADATA_WRITES)
+			set_bit(EVM_RESET_STATUS, &iint->atomic_flags);
+
 		iint->evm_status = INTEGRITY_UNKNOWN;
+	}
 }
 
 /**
@@ -513,6 +524,8 @@  void evm_inode_post_setattr(struct dentry *dentry, int ia_valid)
 	if (!evm_key_loaded())
 		return;
 
+	evm_reset_status(dentry->d_inode);
+
 	if (ia_valid & (ATTR_MODE | ATTR_UID | ATTR_GID))
 		evm_update_evmxattr(dentry, NULL, NULL, 0);
 }
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 8a91711ca79b..bb9976dc2b74 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -246,8 +246,12 @@  static int process_measurement(struct file *file, const struct cred *cred,
 
 	mutex_lock(&iint->mutex);
 
-	if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags))
-		/* reset appraisal flags if ima_inode_post_setattr was called */
+	if (test_and_clear_bit(IMA_CHANGE_ATTR, &iint->atomic_flags) ||
+	    test_bit(EVM_RESET_STATUS, &iint->atomic_flags))
+		/*
+		 * Reset appraisal flags if ima_inode_post_setattr was called or
+		 * EVM reset its status and metadata modification was enabled.
+		 */
 		iint->flags &= ~(IMA_APPRAISE | IMA_APPRAISED |
 				 IMA_APPRAISE_SUBMASK | IMA_APPRAISED_SUBMASK |
 				 IMA_ACTION_FLAGS);
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index 413c803c5208..2adec51c0f6e 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -70,6 +70,7 @@ 
 #define IMA_CHANGE_ATTR		2
 #define IMA_DIGSIG		3
 #define IMA_MUST_MEASURE	4
+#define EVM_RESET_STATUS	5
 
 enum evm_ima_xattr_type {
 	IMA_XATTR_DIGEST = 0x01,