diff mbox series

[v5,06/12] evm: Ignore INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS if conditions are safe

Message ID 20210407105252.30721-7-roberto.sassu@huawei.com (mailing list archive)
State New, archived
Headers show
Series evm: Improve usability of portable signatures | expand

Commit Message

Roberto Sassu April 7, 2021, 10:52 a.m. UTC 
When a file is being created, LSMs can set the initial label with the
inode_init_security hook. If no HMAC key is loaded, the new file will have
LSM xattrs but not the HMAC. It is also possible that the file remains
without protected xattrs after creation if no active LSM provided it.

Unfortunately, EVM will deny any further metadata operation on new files,
as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error, or
INTEGRITY_NOXATTRS if no protected xattrs exist. This would limit the
usability of EVM when only a public key is loaded, as commands such as cp
or tar with the option to preserve xattrs won't work.

This patch ignores these errors when they won't be an issue, if no HMAC key
is loaded and cannot be loaded in the future (which can be enforced by
setting the EVM_SETUP_COMPLETE initialization flag).

Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_main.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

Comments

Mimi Zohar May 3, 2021, 12:12 a.m. UTC | #1 
Hi Roberto,

On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> When a file is being created, LSMs can set the initial label with the
> inode_init_security hook. If no HMAC key is loaded, the new file will have
> LSM xattrs but not the HMAC. It is also possible that the file remains
> without protected xattrs after creation if no active LSM provided it.
> 
> Unfortunately, EVM will deny any further metadata operation on new files,
> as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error, or
> INTEGRITY_NOXATTRS if no protected xattrs exist. This would limit the
> usability of EVM when only a public key is loaded, as commands such as cp
> or tar with the option to preserve xattrs won't work.
> 
> This patch ignores these errors when they won't be an issue, if no HMAC key
> is loaded and cannot be loaded in the future (which can be enforced by
> setting the EVM_SETUP_COMPLETE initialization flag).
> 
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/evm/evm_main.c | 23 ++++++++++++++++++++++-
>  1 file changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
> index 998818283fda..6556e8c22da9 100644
> --- a/security/integrity/evm/evm_main.c
> +++ b/security/integrity/evm/evm_main.c
> @@ -90,6 +90,24 @@ static bool evm_key_loaded(void)
>  	return (bool)(evm_initialized & EVM_KEY_MASK);
>  }
>  
> +/*
> + * Ignoring INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS is safe if no HMAC key
> + * is loaded and the EVM_SETUP_COMPLETE initialization flag is set.
> + */
> +static bool evm_ignore_error_safe(enum integrity_status evm_status)
> +{
> +	if (evm_initialized & EVM_INIT_HMAC)
> +		return false;
> +
> +	if (!(evm_initialized & EVM_SETUP_COMPLETE))
> +		return false;
> +
> +	if (evm_status != INTEGRITY_NOLABEL && evm_status != INTEGRITY_NOXATTRS)
> +		return false;
> +
> +	return true;
> +}
> +
>  static int evm_find_protected_xattrs(struct dentry *dentry)
>  {
>  	struct inode *inode = d_backing_inode(dentry);
> @@ -354,6 +372,8 @@ static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
>  				    -EPERM, 0);
>  	}
>  out:
> +	if (evm_ignore_error_safe(evm_status))
> +		return 0;

I agree with the concept, but the function name doesn't provide enough
context.  Perhaps defining a function more along the lines of
"evm_hmac_disabled()" would be more appropriate and at the same time
self documenting.

>  	if (evm_status != INTEGRITY_PASS)
>  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>  				    dentry->d_name.name, "appraise_metadata",
> @@ -515,7 +535,8 @@ int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
>  		return 0;
>  	evm_status = evm_verify_current_integrity(dentry);
>  	if ((evm_status == INTEGRITY_PASS) ||
> -	    (evm_status == INTEGRITY_NOXATTRS))
> +	    (evm_status == INTEGRITY_NOXATTRS) ||
> +	    (evm_ignore_error_safe(evm_status)))

It would also remove the INTEGRITY_NOXATTRS test duplication here.

thanks,

Mimi

>  		return 0;
>  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
>  			    dentry->d_name.name, "appraise_metadata",
Roberto Sassu May 3, 2021, 7:55 a.m. UTC | #2 
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 2:13 AM
> Hi Roberto,
> 
> On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > When a file is being created, LSMs can set the initial label with the
> > inode_init_security hook. If no HMAC key is loaded, the new file will have
> > LSM xattrs but not the HMAC. It is also possible that the file remains
> > without protected xattrs after creation if no active LSM provided it.
> >
> > Unfortunately, EVM will deny any further metadata operation on new files,
> > as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error, or
> > INTEGRITY_NOXATTRS if no protected xattrs exist. This would limit the
> > usability of EVM when only a public key is loaded, as commands such as cp
> > or tar with the option to preserve xattrs won't work.
> >
> > This patch ignores these errors when they won't be an issue, if no HMAC
> key
> > is loaded and cannot be loaded in the future (which can be enforced by
> > setting the EVM_SETUP_COMPLETE initialization flag).
> >
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  security/integrity/evm/evm_main.c | 23 ++++++++++++++++++++++-
> >  1 file changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/integrity/evm/evm_main.c
> b/security/integrity/evm/evm_main.c
> > index 998818283fda..6556e8c22da9 100644
> > --- a/security/integrity/evm/evm_main.c
> > +++ b/security/integrity/evm/evm_main.c
> > @@ -90,6 +90,24 @@ static bool evm_key_loaded(void)
> >  	return (bool)(evm_initialized & EVM_KEY_MASK);
> >  }
> >
> > +/*
> > + * Ignoring INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS is safe if no HMAC
> key
> > + * is loaded and the EVM_SETUP_COMPLETE initialization flag is set.
> > + */
> > +static bool evm_ignore_error_safe(enum integrity_status evm_status)
> > +{
> > +	if (evm_initialized & EVM_INIT_HMAC)
> > +		return false;
> > +
> > +	if (!(evm_initialized & EVM_SETUP_COMPLETE))
> > +		return false;
> > +
> > +	if (evm_status != INTEGRITY_NOLABEL && evm_status !=
> INTEGRITY_NOXATTRS)
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> >  static int evm_find_protected_xattrs(struct dentry *dentry)
> >  {
> >  	struct inode *inode = d_backing_inode(dentry);
> > @@ -354,6 +372,8 @@ static int evm_protect_xattr(struct dentry *dentry,
> const char *xattr_name,
> >  				    -EPERM, 0);
> >  	}
> >  out:
> > +	if (evm_ignore_error_safe(evm_status))
> > +		return 0;
> 
> I agree with the concept, but the function name doesn't provide enough
> context.  Perhaps defining a function more along the lines of
> "evm_hmac_disabled()" would be more appropriate and at the same time
> self documenting.

Since the function checks if the passed error can be ignored,
would evm_ignore_error_hmac_disabled() also be ok?

> >  	if (evm_status != INTEGRITY_PASS)
> >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> >  				    dentry->d_name.name,
> "appraise_metadata",
> > @@ -515,7 +535,8 @@ int evm_inode_setattr(struct dentry *dentry, struct
> iattr *attr)
> >  		return 0;
> >  	evm_status = evm_verify_current_integrity(dentry);
> >  	if ((evm_status == INTEGRITY_PASS) ||
> > -	    (evm_status == INTEGRITY_NOXATTRS))
> > +	    (evm_status == INTEGRITY_NOXATTRS) ||
> > +	    (evm_ignore_error_safe(evm_status)))
> 
> It would also remove the INTEGRITY_NOXATTRS test duplication here.

Ok.

Thanks

Roberto

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

> thanks,
> 
> Mimi
> 
> >  		return 0;
> >  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> d_backing_inode(dentry),
> >  			    dentry->d_name.name, "appraise_metadata",
Mimi Zohar May 3, 2021, 12:07 p.m. UTC | #3 
On Mon, 2021-05-03 at 07:55 +0000, Roberto Sassu wrote:
> 
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c

> > > @@ -354,6 +372,8 @@ static int evm_protect_xattr(struct dentry *dentry,
> > const char *xattr_name,
> > >  				    -EPERM, 0);
> > >  	}
> > >  out:
> > > +	if (evm_ignore_error_safe(evm_status))
> > > +		return 0;
> > 
> > I agree with the concept, but the function name doesn't provide enough
> > context.  Perhaps defining a function more along the lines of
> > "evm_hmac_disabled()" would be more appropriate and at the same time
> > self documenting.
> 
> Since the function checks if the passed error can be ignored,
> would evm_ignore_error_hmac_disabled() also be ok?

The purpose of evm_protect_xattr() is to prevent allowing an invalid
security.evm xattr from being re-calculated and updated, making it
valid.   Refer to the first line of the function description.  That
hasn't changed.

One of the reasons for defining a new function is to avoid code
duplication, but it should not come at the expense of clear and easily
understood code.   In this case, the reason for "ignoring" certain
return codes needs to be highlighted, not hidden.  
(is_)evm_hmac_disabled() makes this very clear.

Please update the function description to include the reason why making
an exception is safe.

thanks,

Mimi

> > >  	if (evm_status != INTEGRITY_PASS)
> > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > >  				    dentry->d_name.name,
> > "appraise_metadata",
Roberto Sassu May 3, 2021, 2:15 p.m. UTC | #4 
> From: Roberto Sassu [mailto:roberto.sassu@huawei.com]
> Sent: Monday, May 3, 2021 9:55 AM
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 2:13 AM
> > Hi Roberto,
> >
> > On Wed, 2021-04-07 at 12:52 +0200, Roberto Sassu wrote:
> > > When a file is being created, LSMs can set the initial label with the
> > > inode_init_security hook. If no HMAC key is loaded, the new file will have
> > > LSM xattrs but not the HMAC. It is also possible that the file remains
> > > without protected xattrs after creation if no active LSM provided it.
> > >
> > > Unfortunately, EVM will deny any further metadata operation on new
> files,
> > > as evm_protect_xattr() will always return the INTEGRITY_NOLABEL error,
> or
> > > INTEGRITY_NOXATTRS if no protected xattrs exist. This would limit the
> > > usability of EVM when only a public key is loaded, as commands such as
> cp
> > > or tar with the option to preserve xattrs won't work.
> > >
> > > This patch ignores these errors when they won't be an issue, if no HMAC
> > key
> > > is loaded and cannot be loaded in the future (which can be enforced by
> > > setting the EVM_SETUP_COMPLETE initialization flag).
> > >
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  security/integrity/evm/evm_main.c | 23 ++++++++++++++++++++++-
> > >  1 file changed, 22 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/security/integrity/evm/evm_main.c
> > b/security/integrity/evm/evm_main.c
> > > index 998818283fda..6556e8c22da9 100644
> > > --- a/security/integrity/evm/evm_main.c
> > > +++ b/security/integrity/evm/evm_main.c
> > > @@ -90,6 +90,24 @@ static bool evm_key_loaded(void)
> > >  	return (bool)(evm_initialized & EVM_KEY_MASK);
> > >  }
> > >
> > > +/*
> > > + * Ignoring INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS is safe if no
> HMAC
> > key
> > > + * is loaded and the EVM_SETUP_COMPLETE initialization flag is set.
> > > + */
> > > +static bool evm_ignore_error_safe(enum integrity_status evm_status)
> > > +{
> > > +	if (evm_initialized & EVM_INIT_HMAC)
> > > +		return false;
> > > +
> > > +	if (!(evm_initialized & EVM_SETUP_COMPLETE))
> > > +		return false;
> > > +
> > > +	if (evm_status != INTEGRITY_NOLABEL && evm_status !=
> > INTEGRITY_NOXATTRS)
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > >  static int evm_find_protected_xattrs(struct dentry *dentry)
> > >  {
> > >  	struct inode *inode = d_backing_inode(dentry);
> > > @@ -354,6 +372,8 @@ static int evm_protect_xattr(struct dentry
> *dentry,
> > const char *xattr_name,
> > >  				    -EPERM, 0);
> > >  	}
> > >  out:
> > > +	if (evm_ignore_error_safe(evm_status))
> > > +		return 0;
> >
> > I agree with the concept, but the function name doesn't provide enough
> > context.  Perhaps defining a function more along the lines of
> > "evm_hmac_disabled()" would be more appropriate and at the same time
> > self documenting.
> 
> Since the function checks if the passed error can be ignored,
> would evm_ignore_error_hmac_disabled() also be ok?
> 
> > >  	if (evm_status != INTEGRITY_PASS)
> > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > >  				    dentry->d_name.name,
> > "appraise_metadata",
> > > @@ -515,7 +535,8 @@ int evm_inode_setattr(struct dentry *dentry,
> struct
> > iattr *attr)
> > >  		return 0;
> > >  	evm_status = evm_verify_current_integrity(dentry);
> > >  	if ((evm_status == INTEGRITY_PASS) ||
> > > -	    (evm_status == INTEGRITY_NOXATTRS))
> > > +	    (evm_status == INTEGRITY_NOXATTRS) ||
> > > +	    (evm_ignore_error_safe(evm_status)))
> >
> > It would also remove the INTEGRITY_NOXATTRS test duplication here.
> 
> Ok.

Actually, it does not seem a duplication. Currently, INTEGRITY_NOXATTRS
is ignored also when the HMAC key is loaded.

Roberto

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

> Thanks
> 
> Roberto
> 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli
> 
> > thanks,
> >
> > Mimi
> >
> > >  		return 0;
> > >  	integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > d_backing_inode(dentry),
> > >  			    dentry->d_name.name, "appraise_metadata",
Mimi Zohar May 3, 2021, 2:34 p.m. UTC | #5 
On Mon, 2021-05-03 at 14:15 +0000, Roberto Sassu wrote:

> > > >  	if (evm_status != INTEGRITY_PASS)
> > > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > d_backing_inode(dentry),
> > > >  				    dentry->d_name.name,
> > > "appraise_metadata",
> > > > @@ -515,7 +535,8 @@ int evm_inode_setattr(struct dentry *dentry,
> > struct
> > > iattr *attr)
> > > >  		return 0;
> > > >  	evm_status = evm_verify_current_integrity(dentry);
> > > >  	if ((evm_status == INTEGRITY_PASS) ||
> > > > -	    (evm_status == INTEGRITY_NOXATTRS))
> > > > +	    (evm_status == INTEGRITY_NOXATTRS) ||
> > > > +	    (evm_ignore_error_safe(evm_status)))
> > >
> > > It would also remove the INTEGRITY_NOXATTRS test duplication here.
> > 
> > Ok.
> 
> Actually, it does not seem a duplication. Currently, INTEGRITY_NOXATTRS
> is ignored also when the HMAC key is loaded.

The existing INTEGRITY_NOXATTRS exemption is more general and includes
the new case of when EVM HMAC is disabled.  The additional exemption is
only needed for INTEGRITY_NOLABEL, when EVM HMAC is disabled.

Mimi
Roberto Sassu May 4, 2021, 1:16 p.m. UTC | #6 
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, May 3, 2021 4:35 PM
> On Mon, 2021-05-03 at 14:15 +0000, Roberto Sassu wrote:
> 
> > > > >  	if (evm_status != INTEGRITY_PASS)
> > > > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > d_backing_inode(dentry),
> > > > >  				    dentry->d_name.name,
> > > > "appraise_metadata",
> > > > > @@ -515,7 +535,8 @@ int evm_inode_setattr(struct dentry *dentry,
> > > struct
> > > > iattr *attr)
> > > > >  		return 0;
> > > > >  	evm_status = evm_verify_current_integrity(dentry);
> > > > >  	if ((evm_status == INTEGRITY_PASS) ||
> > > > > -	    (evm_status == INTEGRITY_NOXATTRS))
> > > > > +	    (evm_status == INTEGRITY_NOXATTRS) ||
> > > > > +	    (evm_ignore_error_safe(evm_status)))
> > > >
> > > > It would also remove the INTEGRITY_NOXATTRS test duplication here.
> > >
> > > Ok.
> >
> > Actually, it does not seem a duplication. Currently, INTEGRITY_NOXATTRS
> > is ignored also when the HMAC key is loaded.
> 
> The existing INTEGRITY_NOXATTRS exemption is more general and includes
> the new case of when EVM HMAC is disabled.  The additional exemption is
> only needed for INTEGRITY_NOLABEL, when EVM HMAC is disabled.

Unfortunately, evm_ignore_error_safe() is called by both evm_protect_xattr()
and evm_inode_setattr(). The former requires an exemption also for
INTEGRITY_NOXATTRS.

I would keep the function as it is. In the worst case, when the status is
INTEGRITY_NOXATTRS in evm_inode_setattr(), the function will not
be called.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
Mimi Zohar May 4, 2021, 1:45 p.m. UTC | #7 
On Tue, 2021-05-04 at 13:16 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, May 3, 2021 4:35 PM
> > On Mon, 2021-05-03 at 14:15 +0000, Roberto Sassu wrote:
> > 
> > > > > >  	if (evm_status != INTEGRITY_PASS)
> > > > > >  		integrity_audit_msg(AUDIT_INTEGRITY_METADATA,
> > > > > d_backing_inode(dentry),
> > > > > >  				    dentry->d_name.name,
> > > > > "appraise_metadata",
> > > > > > @@ -515,7 +535,8 @@ int evm_inode_setattr(struct dentry *dentry,
> > > > struct
> > > > > iattr *attr)
> > > > > >  		return 0;
> > > > > >  	evm_status = evm_verify_current_integrity(dentry);
> > > > > >  	if ((evm_status == INTEGRITY_PASS) ||
> > > > > > -	    (evm_status == INTEGRITY_NOXATTRS))
> > > > > > +	    (evm_status == INTEGRITY_NOXATTRS) ||
> > > > > > +	    (evm_ignore_error_safe(evm_status)))
> > > > >
> > > > > It would also remove the INTEGRITY_NOXATTRS test duplication here.
> > > >
> > > > Ok.
> > >
> > > Actually, it does not seem a duplication. Currently, INTEGRITY_NOXATTRS
> > > is ignored also when the HMAC key is loaded.
> > 
> > The existing INTEGRITY_NOXATTRS exemption is more general and includes
> > the new case of when EVM HMAC is disabled.  The additional exemption is
> > only needed for INTEGRITY_NOLABEL, when EVM HMAC is disabled.
> 
> Unfortunately, evm_ignore_error_safe() is called by both evm_protect_xattr()
> and evm_inode_setattr(). The former requires an exemption also for
> INTEGRITY_NOXATTRS.
> 
> I would keep the function as it is. In the worst case, when the status is
> INTEGRITY_NOXATTRS in evm_inode_setattr(), the function will not
> be called.

Right, which is another reason for replacing evm_ignore_eror_safe()
with (is_)evm_hmac_disabled() and inlining the error tests.

thanks,

Mimi
diff mbox series

Patch

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 998818283fda..6556e8c22da9 100644
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -90,6 +90,24 @@  static bool evm_key_loaded(void)
 	return (bool)(evm_initialized & EVM_KEY_MASK);
 }
 
+/*
+ * Ignoring INTEGRITY_NOLABEL/INTEGRITY_NOXATTRS is safe if no HMAC key
+ * is loaded and the EVM_SETUP_COMPLETE initialization flag is set.
+ */
+static bool evm_ignore_error_safe(enum integrity_status evm_status)
+{
+	if (evm_initialized & EVM_INIT_HMAC)
+		return false;
+
+	if (!(evm_initialized & EVM_SETUP_COMPLETE))
+		return false;
+
+	if (evm_status != INTEGRITY_NOLABEL && evm_status != INTEGRITY_NOXATTRS)
+		return false;
+
+	return true;
+}
+
 static int evm_find_protected_xattrs(struct dentry *dentry)
 {
 	struct inode *inode = d_backing_inode(dentry);
@@ -354,6 +372,8 @@  static int evm_protect_xattr(struct dentry *dentry, const char *xattr_name,
 				    -EPERM, 0);
 	}
 out:
+	if (evm_ignore_error_safe(evm_status))
+		return 0;
 	if (evm_status != INTEGRITY_PASS)
 		integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 				    dentry->d_name.name, "appraise_metadata",
@@ -515,7 +535,8 @@  int evm_inode_setattr(struct dentry *dentry, struct iattr *attr)
 		return 0;
 	evm_status = evm_verify_current_integrity(dentry);
 	if ((evm_status == INTEGRITY_PASS) ||
-	    (evm_status == INTEGRITY_NOXATTRS))
+	    (evm_status == INTEGRITY_NOXATTRS) ||
+	    (evm_ignore_error_safe(evm_status)))
 		return 0;
 	integrity_audit_msg(AUDIT_INTEGRITY_METADATA, d_backing_inode(dentry),
 			    dentry->d_name.name, "appraise_metadata",