diff mbox series

[03/11] evm: Refuse EVM_ALLOW_METADATA_WRITES only if the HMAC key is loaded

Message ID 20200618160133.937-3-roberto.sassu@huawei.com (mailing list archive)
State New, archived
Headers show
Series [01/11] evm: Execute evm_inode_init_security() only when the HMAC key is loaded | expand

Commit Message

Roberto Sassu June 18, 2020, 4:01 p.m. UTC
Granting metadata write is safe if the HMAC key is not loaded, as it won't
let an attacker obtain a valid HMAC from corrupted xattrs. evm_write_key()
however does not allow it if any key is loaded, including a public key,
which should not be a problem.

This patch allows setting EVM_ALLOW_METADATA_WRITES if the EVM_INIT_HMAC
flag is not set.

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_secfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Mimi Zohar Aug. 21, 2020, 8:14 p.m. UTC | #1
Hi Roberto,

On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> Granting metadata write is safe if the HMAC key is not loaded, as it won't
> let an attacker obtain a valid HMAC from corrupted xattrs. evm_write_key()
> however does not allow it if any key is loaded, including a public key,
> which should not be a problem.
> 

Why is the existing hebavior a problem?  What is the problem being
solved?

> This patch allows setting EVM_ALLOW_METADATA_WRITES if the EVM_INIT_HMAC
> flag is not set.
> 
> 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_secfs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
> index cfc3075769bb..92fe26ace797 100644
> --- a/security/integrity/evm/evm_secfs.c
> +++ b/security/integrity/evm/evm_secfs.c
> @@ -84,7 +84,7 @@ static ssize_t evm_write_key(struct file *file, const char __user *buf,
>  	 * keys are loaded.
>  	 */
>  	if ((i & EVM_ALLOW_METADATA_WRITES) &&
> -	    ((evm_initialized & EVM_KEY_MASK) != 0) &&
> +	    ((evm_initialized & EVM_INIT_HMAC) != 0) &&
>  	    !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
>  		return -EPERM;

>  

Documentation/ABI/testing/evm needs to be updated as well.

thanks,

Mimi
Roberto Sassu Aug. 31, 2020, 8:24 a.m. UTC | #2
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, August 21, 2020 10:15 PM
> Hi Roberto,
> 
> On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> > Granting metadata write is safe if the HMAC key is not loaded, as it won't
> > let an attacker obtain a valid HMAC from corrupted xattrs.
> evm_write_key()
> > however does not allow it if any key is loaded, including a public key,
> > which should not be a problem.
> >
> 
> Why is the existing hebavior a problem?  What is the problem being
> solved?

Hi Mimi

currently it is not possible to set EVM_ALLOW_METADATA_WRITES when
only a public key is loaded and the HMAC key is not. The patch removes
this limitation.

> > This patch allows setting EVM_ALLOW_METADATA_WRITES if the
> EVM_INIT_HMAC
> > flag is not set.
> >
> > 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_secfs.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/evm/evm_secfs.c
> b/security/integrity/evm/evm_secfs.c
> > index cfc3075769bb..92fe26ace797 100644
> > --- a/security/integrity/evm/evm_secfs.c
> > +++ b/security/integrity/evm/evm_secfs.c
> > @@ -84,7 +84,7 @@ static ssize_t evm_write_key(struct file *file, const
> char __user *buf,
> >  	 * keys are loaded.
> >  	 */
> >  	if ((i & EVM_ALLOW_METADATA_WRITES) &&
> > -	    ((evm_initialized & EVM_KEY_MASK) != 0) &&
> > +	    ((evm_initialized & EVM_INIT_HMAC) != 0) &&
> >  	    !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
> >  		return -EPERM;
> 
> >
> 
> Documentation/ABI/testing/evm needs to be updated as well.

Ok.

Thanks

Roberto

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

> thanks,
> 
> Mimi
> 
>
Mimi Zohar Aug. 31, 2020, 9:31 p.m. UTC | #3
On Mon, 2020-08-31 at 08:24 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Friday, August 21, 2020 10:15 PM
> > Hi Roberto,
> > 
> > On Thu, 2020-06-18 at 18:01 +0200, Roberto Sassu wrote:
> > > Granting metadata write is safe if the HMAC key is not loaded, as it won't
> > > let an attacker obtain a valid HMAC from corrupted xattrs.
> > evm_write_key()
> > > however does not allow it if any key is loaded, including a public key,
> > > which should not be a problem.
> > >
> > 
> > Why is the existing hebavior a problem?  What is the problem being
> > solved?
> 
> Hi Mimi
> 
> currently it is not possible to set EVM_ALLOW_METADATA_WRITES when
> only a public key is loaded and the HMAC key is not. The patch removes
> this limitation.

Yes, I understand.  You're describing "what" the problem is, not "why"
this is a problem.  Support for loading EVM HMAC and x509 certificates
isn't new.  Please add a line or two prior to this paragraph providing
the context for why this is now a problem.

Is the problem related to previoulsy not beginning EVM verification
until after the EVM HMAC key was loaded?  Or perhaps EVM signatures
were not that common since they weren't portable.   Now, with portable
and immutable signatures loading x509 certificates is more common.

thanks,

Mimi
diff mbox series

Patch

diff --git a/security/integrity/evm/evm_secfs.c b/security/integrity/evm/evm_secfs.c
index cfc3075769bb..92fe26ace797 100644
--- a/security/integrity/evm/evm_secfs.c
+++ b/security/integrity/evm/evm_secfs.c
@@ -84,7 +84,7 @@  static ssize_t evm_write_key(struct file *file, const char __user *buf,
 	 * keys are loaded.
 	 */
 	if ((i & EVM_ALLOW_METADATA_WRITES) &&
-	    ((evm_initialized & EVM_KEY_MASK) != 0) &&
+	    ((evm_initialized & EVM_INIT_HMAC) != 0) &&
 	    !(evm_initialized & EVM_ALLOW_METADATA_WRITES))
 		return -EPERM;