diff mbox series

[v4,04/11] ima: Move ima_reset_appraise_flags() call to post hooks

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

Commit Message

Roberto Sassu March 5, 2021, 3:19 p.m. UTC
ima_inode_setxattr() and ima_inode_removexattr() hooks are called before an
operation is performed. Thus, ima_reset_appraise_flags() should not be
called there, as flags might be unnecessarily reset if the operation is
denied.

This patch introduces the post hooks ima_inode_post_setxattr() and
ima_inode_post_removexattr(), and adds the call to
ima_reset_appraise_flags() in the new functions.

Cc: Casey Schaufler <casey@schaufler-ca.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 fs/xattr.c                            |  2 ++
 include/linux/ima.h                   | 18 ++++++++++++++++++
 security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++++++++---
 security/security.c                   |  1 +
 4 files changed, 43 insertions(+), 3 deletions(-)

Comments

Casey Schaufler March 5, 2021, 5:30 p.m. UTC | #1
On 3/5/2021 7:19 AM, Roberto Sassu wrote:
> ima_inode_setxattr() and ima_inode_removexattr() hooks are called before an
> operation is performed. Thus, ima_reset_appraise_flags() should not be
> called there, as flags might be unnecessarily reset if the operation is
> denied.
>
> This patch introduces the post hooks ima_inode_post_setxattr() and
> ima_inode_post_removexattr(), and adds the call to
> ima_reset_appraise_flags() in the new functions.

I don't see anything wrong with this patch in light of the way
IMA and EVM have been treated to date.

However ...

The special casing of IMA and EVM in security.c is getting out of
hand, and appears to be unnecessary. By my count there are 9 IMA
hooks and 5 EVM hooks that have been hard coded. Adding this IMA
hook makes 10. It would be really easy to register IMA and EVM as
security modules. That would remove the dependency they currently
have on security sub-system approval for changes like this one.
I know there has been resistance to "IMA as an LSM" in the past,
but it's pretty hard to see how it wouldn't be a win.

Short of that ...

Many of the places where IMA is invoked immediately invoke EVM
as well. Instead of:

	ima_do_stuff(x, y, z);
	evm_do_stuff(x, y, z);

how about

	integrity_do_stuff(x, y, z);


>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  fs/xattr.c                            |  2 ++
>  include/linux/ima.h                   | 18 ++++++++++++++++++
>  security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++++++++---
>  security/security.c                   |  1 +
>  4 files changed, 43 insertions(+), 3 deletions(-)
>
> diff --git a/fs/xattr.c b/fs/xattr.c
> index b3444e06cded..81847f132d26 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -16,6 +16,7 @@
>  #include <linux/namei.h>
>  #include <linux/security.h>
>  #include <linux/evm.h>
> +#include <linux/ima.h>
>  #include <linux/syscalls.h>
>  #include <linux/export.h>
>  #include <linux/fsnotify.h>
> @@ -502,6 +503,7 @@ __vfs_removexattr_locked(struct user_namespace *mnt_userns,
>  
>  	if (!error) {
>  		fsnotify_xattr(dentry);
> +		ima_inode_post_removexattr(dentry, name);
>  		evm_inode_post_removexattr(dentry, name);
>  	}
>  
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 61d5723ec303..5e059da43857 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -171,7 +171,13 @@ extern void ima_inode_post_setattr(struct user_namespace *mnt_userns,
>  				   struct dentry *dentry);
>  extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		       const void *xattr_value, size_t xattr_value_len);
> +extern void ima_inode_post_setxattr(struct dentry *dentry,
> +				    const char *xattr_name,
> +				    const void *xattr_value,
> +				    size_t xattr_value_len);
>  extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
> +extern void ima_inode_post_removexattr(struct dentry *dentry,
> +				       const char *xattr_name);
>  #else
>  static inline bool is_ima_appraise_enabled(void)
>  {
> @@ -192,11 +198,23 @@ static inline int ima_inode_setxattr(struct dentry *dentry,
>  	return 0;
>  }
>  
> +static inline void ima_inode_post_setxattr(struct dentry *dentry,
> +					   const char *xattr_name,
> +					   const void *xattr_value,
> +					   size_t xattr_value_len)
> +{
> +}
> +
>  static inline int ima_inode_removexattr(struct dentry *dentry,
>  					const char *xattr_name)
>  {
>  	return 0;
>  }
> +
> +static inline void ima_inode_post_removexattr(struct dentry *dentry,
> +					      const char *xattr_name)
> +{
> +}
>  #endif /* CONFIG_IMA_APPRAISE */
>  
>  #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 565e33ff19d0..1f029e4c8d7f 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -577,21 +577,40 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  	if (result == 1) {
>  		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
>  			return -EINVAL;
> -		ima_reset_appraise_flags(d_backing_inode(dentry),
> -			xvalue->type == EVM_IMA_XATTR_DIGSIG);
>  		result = 0;
>  	}
>  	return result;
>  }
>  
> +void ima_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
> +			     const void *xattr_value, size_t xattr_value_len)
> +{
> +	const struct evm_ima_xattr_data *xvalue = xattr_value;
> +	int result;
> +
> +	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
> +				   xattr_value_len);
> +	if (result == 1)
> +		ima_reset_appraise_flags(d_backing_inode(dentry),
> +			xvalue->type == EVM_IMA_XATTR_DIGSIG);
> +}
> +
>  int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
>  {
>  	int result;
>  
>  	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
>  	if (result == 1) {
> -		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
>  		result = 0;
>  	}
>  	return result;
>  }
> +
> +void ima_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
> +{
> +	int result;
> +
> +	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
> +	if (result == 1)
> +		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
> +}
> diff --git a/security/security.c b/security/security.c
> index 5ac96b16f8fa..efb1f874dc41 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1319,6 +1319,7 @@ void security_inode_post_setxattr(struct dentry *dentry, const char *name,
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return;
>  	call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
> +	ima_inode_post_setxattr(dentry, name, value, size);
>  	evm_inode_post_setxattr(dentry, name, value, size);
>  }
>
Mimi Zohar April 26, 2021, 7:49 p.m. UTC | #2
On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> On 3/5/2021 7:19 AM, Roberto Sassu wrote:
> > ima_inode_setxattr() and ima_inode_removexattr() hooks are called before an
> > operation is performed. Thus, ima_reset_appraise_flags() should not be
> > called there, as flags might be unnecessarily reset if the operation is
> > denied.
> >
> > This patch introduces the post hooks ima_inode_post_setxattr() and
> > ima_inode_post_removexattr(), and adds the call to
> > ima_reset_appraise_flags() in the new functions.
> 
> I don't see anything wrong with this patch in light of the way
> IMA and EVM have been treated to date.
> 
> However ...
> 
> The special casing of IMA and EVM in security.c is getting out of
> hand, and appears to be unnecessary. By my count there are 9 IMA
> hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> hook makes 10. It would be really easy to register IMA and EVM as
> security modules. That would remove the dependency they currently
> have on security sub-system approval for changes like this one.
> I know there has been resistance to "IMA as an LSM" in the past,
> but it's pretty hard to see how it wouldn't be a win.

Somehow I missed the new "lsm=" boot command line option, which
dynamically allows enabling/disabling LSMs, being upstreamed.  This
would be one of the reasons for not making IMA/EVM full LSMs.

Both IMA and EVM file data/metadata is persistent across boots.  If
either one or the other is not enabled the file data hash or file
metadata HMAC will not properly be updated, potentially preventing the
system from booting when re-enabled.  Re-enabling IMA and EVM would
require "fixing" the mutable file data hash and HMAC, without any
knowledge of what the "fixed" values should be.  Dave Safford referred
to this as "blessing" the newly calculated values.

Mimi
Roberto Sassu April 27, 2021, 9:25 a.m. UTC | #3
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Monday, April 26, 2021 9:49 PM
> On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> > On 3/5/2021 7:19 AM, Roberto Sassu wrote:
> > > ima_inode_setxattr() and ima_inode_removexattr() hooks are called before
> an
> > > operation is performed. Thus, ima_reset_appraise_flags() should not be
> > > called there, as flags might be unnecessarily reset if the operation is
> > > denied.
> > >
> > > This patch introduces the post hooks ima_inode_post_setxattr() and
> > > ima_inode_post_removexattr(), and adds the call to
> > > ima_reset_appraise_flags() in the new functions.
> >
> > I don't see anything wrong with this patch in light of the way
> > IMA and EVM have been treated to date.
> >
> > However ...
> >
> > The special casing of IMA and EVM in security.c is getting out of
> > hand, and appears to be unnecessary. By my count there are 9 IMA
> > hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> > hook makes 10. It would be really easy to register IMA and EVM as
> > security modules. That would remove the dependency they currently
> > have on security sub-system approval for changes like this one.
> > I know there has been resistance to "IMA as an LSM" in the past,
> > but it's pretty hard to see how it wouldn't be a win.
> 
> Somehow I missed the new "lsm=" boot command line option, which
> dynamically allows enabling/disabling LSMs, being upstreamed.  This
> would be one of the reasons for not making IMA/EVM full LSMs.

Hi Mimi

one could argue why IMA/EVM should receive a special
treatment. I understand that this was a necessity without
LSM stacking. Now that LSM stacking is available, I don't
see any valid reason why IMA/EVM should not be managed
by the LSM infrastructure.

> Both IMA and EVM file data/metadata is persistent across boots.  If
> either one or the other is not enabled the file data hash or file
> metadata HMAC will not properly be updated, potentially preventing the
> system from booting when re-enabled.  Re-enabling IMA and EVM would
> require "fixing" the mutable file data hash and HMAC, without any
> knowledge of what the "fixed" values should be.  Dave Safford referred
> to this as "blessing" the newly calculated values.

IMA/EVM can be easily disabled in other ways, for example
by moving the IMA policy or the EVM keys elsewhere.

Also other LSMs rely on a dynamic and persistent state
(for example for file transitions in SELinux), which cannot be
trusted anymore if LSMs are even temporarily disabled.

If IMA/EVM have to be enabled to prevent misconfiguration,
I think the same can be achieved if they are full LSMs, for
example by preventing that the list of enabled LSMs changes
at run-time.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
Mimi Zohar April 27, 2021, 3:34 p.m. UTC | #4
On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Monday, April 26, 2021 9:49 PM
> > On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:

> > > However ...
> > >
> > > The special casing of IMA and EVM in security.c is getting out of
> > > hand, and appears to be unnecessary. By my count there are 9 IMA
> > > hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> > > hook makes 10. It would be really easy to register IMA and EVM as
> > > security modules. That would remove the dependency they currently
> > > have on security sub-system approval for changes like this one.
> > > I know there has been resistance to "IMA as an LSM" in the past,
> > > but it's pretty hard to see how it wouldn't be a win.

It sholdn't be one way.  Are you willing to also make the existing
IMA/EVM hooks that are not currently security hooks, security hooks
too?   And accept any new IMA/EVM hooks would result in new security
hooks?  Are you also willing to add dependency tracking between LSMs?

> > 
> > Somehow I missed the new "lsm=" boot command line option, which
> > dynamically allows enabling/disabling LSMs, being upstreamed.  This
> > would be one of the reasons for not making IMA/EVM full LSMs.
> 
> Hi Mimi
> 
> one could argue why IMA/EVM should receive a special
> treatment. I understand that this was a necessity without
> LSM stacking. Now that LSM stacking is available, I don't
> see any valid reason why IMA/EVM should not be managed
> by the LSM infrastructure.
> 
> > Both IMA and EVM file data/metadata is persistent across boots.  If
> > either one or the other is not enabled the file data hash or file
> > metadata HMAC will not properly be updated, potentially preventing the
> > system from booting when re-enabled.  Re-enabling IMA and EVM would
> > require "fixing" the mutable file data hash and HMAC, without any
> > knowledge of what the "fixed" values should be.  Dave Safford referred
> > to this as "blessing" the newly calculated values.
> 
> IMA/EVM can be easily disabled in other ways, for example
> by moving the IMA policy or the EVM keys elsewhere.

Dynamically disabling IMA/EVM is very different than removing keys and
preventing the system from booting.  Restoring the keys should result
in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
labeling the filesystem in "fix" mode, which "blesses" any changes made
when IMA/EVM were not enabled.

> Also other LSMs rely on a dynamic and persistent state
> (for example for file transitions in SELinux), which cannot be
> trusted anymore if LSMs are even temporarily disabled.

Your argument is because this is a problem for SELinux, make it also a
problem for IMA/EVM too?!   ("Two wrongs make a right")

> If IMA/EVM have to be enabled to prevent misconfiguration,
> I think the same can be achieved if they are full LSMs, for
> example by preventing that the list of enabled LSMs changes
> at run-time.

That ship sailed when "security=" was deprecated in favor of "lsm="
support, which dynamically enables/disables LSMs at runtime.

Mimi
Roberto Sassu April 27, 2021, 3:57 p.m. UTC | #5
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, April 27, 2021 5:35 PM
> On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
> > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > Sent: Monday, April 26, 2021 9:49 PM
> > > On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> 
> > > > However ...
> > > >
> > > > The special casing of IMA and EVM in security.c is getting out of
> > > > hand, and appears to be unnecessary. By my count there are 9 IMA
> > > > hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> > > > hook makes 10. It would be really easy to register IMA and EVM as
> > > > security modules. That would remove the dependency they currently
> > > > have on security sub-system approval for changes like this one.
> > > > I know there has been resistance to "IMA as an LSM" in the past,
> > > > but it's pretty hard to see how it wouldn't be a win.
> 
> It sholdn't be one way.  Are you willing to also make the existing
> IMA/EVM hooks that are not currently security hooks, security hooks
> too?   And accept any new IMA/EVM hooks would result in new security
> hooks?  Are you also willing to add dependency tracking between LSMs?

I already have a preliminary branch where IMA/EVM are full LSMs.

Indeed, the biggest problem would be to have the new hooks
accepted. I can send the patch set for evaluation to see what
people think.

> > > Somehow I missed the new "lsm=" boot command line option, which
> > > dynamically allows enabling/disabling LSMs, being upstreamed.  This
> > > would be one of the reasons for not making IMA/EVM full LSMs.
> >
> > Hi Mimi
> >
> > one could argue why IMA/EVM should receive a special
> > treatment. I understand that this was a necessity without
> > LSM stacking. Now that LSM stacking is available, I don't
> > see any valid reason why IMA/EVM should not be managed
> > by the LSM infrastructure.
> >
> > > Both IMA and EVM file data/metadata is persistent across boots.  If
> > > either one or the other is not enabled the file data hash or file
> > > metadata HMAC will not properly be updated, potentially preventing the
> > > system from booting when re-enabled.  Re-enabling IMA and EVM would
> > > require "fixing" the mutable file data hash and HMAC, without any
> > > knowledge of what the "fixed" values should be.  Dave Safford referred
> > > to this as "blessing" the newly calculated values.
> >
> > IMA/EVM can be easily disabled in other ways, for example
> > by moving the IMA policy or the EVM keys elsewhere.
> 
> Dynamically disabling IMA/EVM is very different than removing keys and
> preventing the system from booting.  Restoring the keys should result
> in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
> labeling the filesystem in "fix" mode, which "blesses" any changes made
> when IMA/EVM were not enabled.

Uhm, I thought that if you move the HMAC key for example
and you boot the system, you invalidate all files that change,
because the HMAC is not updated.

> > Also other LSMs rely on a dynamic and persistent state
> > (for example for file transitions in SELinux), which cannot be
> > trusted anymore if LSMs are even temporarily disabled.
> 
> Your argument is because this is a problem for SELinux, make it also a
> problem for IMA/EVM too?!   ("Two wrongs make a right")

To me it seems reasonable to give the ability to people to
disable the LSMs if they want to do so, and at the same time
to try to prevent accidental disable when the LSMs should be
enabled.

> > If IMA/EVM have to be enabled to prevent misconfiguration,
> > I think the same can be achieved if they are full LSMs, for
> > example by preventing that the list of enabled LSMs changes
> > at run-time.
> 
> That ship sailed when "security=" was deprecated in favor of "lsm="
> support, which dynamically enables/disables LSMs at runtime.

Maybe this possibility can be disabled with a new kernel option.
I will think a more concrete solution.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli
Mimi Zohar April 27, 2021, 4:03 p.m. UTC | #6
On Tue, 2021-04-27 at 15:57 +0000, Roberto Sassu wrote:
> > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > Sent: Tuesday, April 27, 2021 5:35 PM
> > On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
> > > > From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> > > > Sent: Monday, April 26, 2021 9:49 PM
> > > > On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> > 
> > > > > However ...
> > > > >
> > > > > The special casing of IMA and EVM in security.c is getting out of
> > > > > hand, and appears to be unnecessary. By my count there are 9 IMA
> > > > > hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> > > > > hook makes 10. It would be really easy to register IMA and EVM as
> > > > > security modules. That would remove the dependency they currently
> > > > > have on security sub-system approval for changes like this one.
> > > > > I know there has been resistance to "IMA as an LSM" in the past,
> > > > > but it's pretty hard to see how it wouldn't be a win.
> > 
> > It sholdn't be one way.  Are you willing to also make the existing
> > IMA/EVM hooks that are not currently security hooks, security hooks
> > too?   And accept any new IMA/EVM hooks would result in new security
> > hooks?  Are you also willing to add dependency tracking between LSMs?
> 
> I already have a preliminary branch where IMA/EVM are full LSMs.
> 
> Indeed, the biggest problem would be to have the new hooks
> accepted. I can send the patch set for evaluation to see what
> people think.

Defining new security hooks is pretty straight forward.   Perhaps at
least wait until Casey responds before posting the patches.

> 
> > > > Somehow I missed the new "lsm=" boot command line option, which
> > > > dynamically allows enabling/disabling LSMs, being upstreamed.  This
> > > > would be one of the reasons for not making IMA/EVM full LSMs.
> > >
> > > Hi Mimi
> > >
> > > one could argue why IMA/EVM should receive a special
> > > treatment. I understand that this was a necessity without
> > > LSM stacking. Now that LSM stacking is available, I don't
> > > see any valid reason why IMA/EVM should not be managed
> > > by the LSM infrastructure.
> > >
> > > > Both IMA and EVM file data/metadata is persistent across boots.  If
> > > > either one or the other is not enabled the file data hash or file
> > > > metadata HMAC will not properly be updated, potentially preventing the
> > > > system from booting when re-enabled.  Re-enabling IMA and EVM would
> > > > require "fixing" the mutable file data hash and HMAC, without any
> > > > knowledge of what the "fixed" values should be.  Dave Safford referred
> > > > to this as "blessing" the newly calculated values.
> > >
> > > IMA/EVM can be easily disabled in other ways, for example
> > > by moving the IMA policy or the EVM keys elsewhere.
> > 
> > Dynamically disabling IMA/EVM is very different than removing keys and
> > preventing the system from booting.  Restoring the keys should result
> > in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
> > labeling the filesystem in "fix" mode, which "blesses" any changes made
> > when IMA/EVM were not enabled.
> 
> Uhm, I thought that if you move the HMAC key for example
> and you boot the system, you invalidate all files that change,
> because the HMAC is not updated.

More likely you wouldn't be able to boot the system without the HMAC
key.

Mimi

> 
> > > Also other LSMs rely on a dynamic and persistent state
> > > (for example for file transitions in SELinux), which cannot be
> > > trusted anymore if LSMs are even temporarily disabled.
> > 
> > Your argument is because this is a problem for SELinux, make it also a
> > problem for IMA/EVM too?!   ("Two wrongs make a right")
> 
> To me it seems reasonable to give the ability to people to
> disable the LSMs if they want to do so, and at the same time
> to try to prevent accidental disable when the LSMs should be
> enabled.
> 
> > > If IMA/EVM have to be enabled to prevent misconfiguration,
> > > I think the same can be achieved if they are full LSMs, for
> > > example by preventing that the list of enabled LSMs changes
> > > at run-time.
> > 
> > That ship sailed when "security=" was deprecated in favor of "lsm="
> > support, which dynamically enables/disables LSMs at runtime.
> 
> Maybe this possibility can be disabled with a new kernel option.
> I will think a more concrete solution.
> 
> Roberto
> 
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli
Casey Schaufler April 27, 2021, 4:39 p.m. UTC | #7
On 4/27/2021 8:57 AM, Roberto Sassu wrote:
>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
>> Sent: Tuesday, April 27, 2021 5:35 PM
>> On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
>>>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
>>>> Sent: Monday, April 26, 2021 9:49 PM
>>>> On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
>>>>> However ...
>>>>>
>>>>> The special casing of IMA and EVM in security.c is getting out of
>>>>> hand, and appears to be unnecessary. By my count there are 9 IMA
>>>>> hooks and 5 EVM hooks that have been hard coded. Adding this IMA
>>>>> hook makes 10. It would be really easy to register IMA and EVM as
>>>>> security modules. That would remove the dependency they currently
>>>>> have on security sub-system approval for changes like this one.
>>>>> I know there has been resistance to "IMA as an LSM" in the past,
>>>>> but it's pretty hard to see how it wouldn't be a win.
>> It sholdn't be one way.  Are you willing to also make the existing
>> IMA/EVM hooks that are not currently security hooks, security hooks
>> too?   And accept any new IMA/EVM hooks would result in new security
>> hooks?  Are you also willing to add dependency tracking between LSMs?
> I already have a preliminary branch where IMA/EVM are full LSMs.

I don't think that IMA/EVM need to be full LSMs to address my whinging.
I don't think that changing the existing integrity_whatever() to
security_whatever() is necessary. All that I'm really objecting to is
special cases in security.c:

{
	int ret;

	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
		return 0;
	/*
	 * SELinux and Smack integrate the cap call,
	 * so assume that all LSMs supplying this call do so.
	 */
	ret = call_int_hook(inode_removexattr, 1, mnt_userns, dentry, name);
	if (ret == 1)
		ret = cap_inode_removexattr(mnt_userns, dentry, name);
	if (ret)
		return ret;
	ret = ima_inode_removexattr(dentry, name);
	if (ret)
		return ret;
	return evm_inode_removexattr(dentry, name);
}

Not all uses of ima/evm functions in security.c make sense to convert
to LSM hooks. In fact, I could be completely wrong that it makes sense
to change anything. What I see is something that looks like it ought to
be normalized. If there's good reason (and it looks like there may be)
for it to be different there's no reason to pay attention to me.

>
> Indeed, the biggest problem would be to have the new hooks
> accepted. I can send the patch set for evaluation to see what
> people think.
>
>>>> Somehow I missed the new "lsm=" boot command line option, which
>>>> dynamically allows enabling/disabling LSMs, being upstreamed.  This
>>>> would be one of the reasons for not making IMA/EVM full LSMs.
>>> Hi Mimi
>>>
>>> one could argue why IMA/EVM should receive a special
>>> treatment. I understand that this was a necessity without
>>> LSM stacking. Now that LSM stacking is available, I don't
>>> see any valid reason why IMA/EVM should not be managed
>>> by the LSM infrastructure.
>>>
>>>> Both IMA and EVM file data/metadata is persistent across boots.  If
>>>> either one or the other is not enabled the file data hash or file
>>>> metadata HMAC will not properly be updated, potentially preventing the
>>>> system from booting when re-enabled.  Re-enabling IMA and EVM would
>>>> require "fixing" the mutable file data hash and HMAC, without any
>>>> knowledge of what the "fixed" values should be.  Dave Safford referred
>>>> to this as "blessing" the newly calculated values.
>>> IMA/EVM can be easily disabled in other ways, for example
>>> by moving the IMA policy or the EVM keys elsewhere.
>> Dynamically disabling IMA/EVM is very different than removing keys and
>> preventing the system from booting.  Restoring the keys should result
>> in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
>> labeling the filesystem in "fix" mode, which "blesses" any changes made
>> when IMA/EVM were not enabled.
> Uhm, I thought that if you move the HMAC key for example
> and you boot the system, you invalidate all files that change,
> because the HMAC is not updated.
>
>>> Also other LSMs rely on a dynamic and persistent state
>>> (for example for file transitions in SELinux), which cannot be
>>> trusted anymore if LSMs are even temporarily disabled.
>> Your argument is because this is a problem for SELinux, make it also a
>> problem for IMA/EVM too?!   ("Two wrongs make a right")
> To me it seems reasonable to give the ability to people to
> disable the LSMs if they want to do so, and at the same time
> to try to prevent accidental disable when the LSMs should be
> enabled.
>
>>> If IMA/EVM have to be enabled to prevent misconfiguration,
>>> I think the same can be achieved if they are full LSMs, for
>>> example by preventing that the list of enabled LSMs changes
>>> at run-time.
>> That ship sailed when "security=" was deprecated in favor of "lsm="
>> support, which dynamically enables/disables LSMs at runtime.

security= is still supported and works the same as ever. lsm= is
more powerful than security= but also harder to use.

> Maybe this possibility can be disabled with a new kernel option.
> I will think a more concrete solution.
>
> Roberto
>
> HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> Managing Director: Li Peng, Li Jian, Shi Yanli
Mimi Zohar April 27, 2021, 4:48 p.m. UTC | #8
Hi Casey,

On Tue, 2021-04-27 at 09:39 -0700, Casey Schaufler wrote:
> >> That ship sailed when "security=" was deprecated in favor of "lsm="
> >> support, which dynamically enables/disables LSMs at runtime.
> 
> security= is still supported and works the same as ever. lsm= is
> more powerful than security= but also harder to use.

I understand that it still exists, but the documentation says it's been
deprecated.
From Documentation/admin-guide/kernel-parameters.txt:

        security=  [SECURITY] Choose a legacy "major" security module to
                        enable at boot. This has been deprecated by the
                        "lsm=" parameter.

Mimi
Roberto Sassu April 28, 2021, 7:48 a.m. UTC | #9
> From: Casey Schaufler [mailto:casey@schaufler-ca.com]
> Sent: Tuesday, April 27, 2021 6:40 PM
> On 4/27/2021 8:57 AM, Roberto Sassu wrote:
> >> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> >> Sent: Tuesday, April 27, 2021 5:35 PM
> >> On Tue, 2021-04-27 at 09:25 +0000, Roberto Sassu wrote:
> >>>> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> >>>> Sent: Monday, April 26, 2021 9:49 PM
> >>>> On Fri, 2021-03-05 at 09:30 -0800, Casey Schaufler wrote:
> >>>>> However ...
> >>>>>
> >>>>> The special casing of IMA and EVM in security.c is getting out of
> >>>>> hand, and appears to be unnecessary. By my count there are 9 IMA
> >>>>> hooks and 5 EVM hooks that have been hard coded. Adding this IMA
> >>>>> hook makes 10. It would be really easy to register IMA and EVM as
> >>>>> security modules. That would remove the dependency they currently
> >>>>> have on security sub-system approval for changes like this one.
> >>>>> I know there has been resistance to "IMA as an LSM" in the past,
> >>>>> but it's pretty hard to see how it wouldn't be a win.
> >> It sholdn't be one way.  Are you willing to also make the existing
> >> IMA/EVM hooks that are not currently security hooks, security hooks
> >> too?   And accept any new IMA/EVM hooks would result in new security
> >> hooks?  Are you also willing to add dependency tracking between LSMs?
> > I already have a preliminary branch where IMA/EVM are full LSMs.
> 
> I don't think that IMA/EVM need to be full LSMs to address my whinging.
> I don't think that changing the existing integrity_whatever() to
> security_whatever() is necessary. All that I'm really objecting to is
> special cases in security.c:
> 
> {
> 	int ret;
> 
> 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> 		return 0;
> 	/*
> 	 * SELinux and Smack integrate the cap call,
> 	 * so assume that all LSMs supplying this call do so.
> 	 */
> 	ret = call_int_hook(inode_removexattr, 1, mnt_userns, dentry,
> name);
> 	if (ret == 1)
> 		ret = cap_inode_removexattr(mnt_userns, dentry, name);
> 	if (ret)
> 		return ret;
> 	ret = ima_inode_removexattr(dentry, name);
> 	if (ret)
> 		return ret;
> 	return evm_inode_removexattr(dentry, name);
> }
> 
> Not all uses of ima/evm functions in security.c make sense to convert
> to LSM hooks. In fact, I could be completely wrong that it makes sense
> to change anything. What I see is something that looks like it ought to
> be normalized. If there's good reason (and it looks like there may be)
> for it to be different there's no reason to pay attention to me.

You are right. When I said that I don't see any valid reason for
not moving IMA/EVM to the LSM infrastructure, I probably should
have said just that it is technically feasible. Apologies for being
too resolute in my statement.

Roberto

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

> > Indeed, the biggest problem would be to have the new hooks
> > accepted. I can send the patch set for evaluation to see what
> > people think.
> >
> >>>> Somehow I missed the new "lsm=" boot command line option, which
> >>>> dynamically allows enabling/disabling LSMs, being upstreamed.  This
> >>>> would be one of the reasons for not making IMA/EVM full LSMs.
> >>> Hi Mimi
> >>>
> >>> one could argue why IMA/EVM should receive a special
> >>> treatment. I understand that this was a necessity without
> >>> LSM stacking. Now that LSM stacking is available, I don't
> >>> see any valid reason why IMA/EVM should not be managed
> >>> by the LSM infrastructure.
> >>>
> >>>> Both IMA and EVM file data/metadata is persistent across boots.  If
> >>>> either one or the other is not enabled the file data hash or file
> >>>> metadata HMAC will not properly be updated, potentially preventing the
> >>>> system from booting when re-enabled.  Re-enabling IMA and EVM
> would
> >>>> require "fixing" the mutable file data hash and HMAC, without any
> >>>> knowledge of what the "fixed" values should be.  Dave Safford referred
> >>>> to this as "blessing" the newly calculated values.
> >>> IMA/EVM can be easily disabled in other ways, for example
> >>> by moving the IMA policy or the EVM keys elsewhere.
> >> Dynamically disabling IMA/EVM is very different than removing keys and
> >> preventing the system from booting.  Restoring the keys should result
> >> in being able to re-boot the system.  Re-enabling IMA/EVM, requires re-
> >> labeling the filesystem in "fix" mode, which "blesses" any changes made
> >> when IMA/EVM were not enabled.
> > Uhm, I thought that if you move the HMAC key for example
> > and you boot the system, you invalidate all files that change,
> > because the HMAC is not updated.
> >
> >>> Also other LSMs rely on a dynamic and persistent state
> >>> (for example for file transitions in SELinux), which cannot be
> >>> trusted anymore if LSMs are even temporarily disabled.
> >> Your argument is because this is a problem for SELinux, make it also a
> >> problem for IMA/EVM too?!   ("Two wrongs make a right")
> > To me it seems reasonable to give the ability to people to
> > disable the LSMs if they want to do so, and at the same time
> > to try to prevent accidental disable when the LSMs should be
> > enabled.
> >
> >>> If IMA/EVM have to be enabled to prevent misconfiguration,
> >>> I think the same can be achieved if they are full LSMs, for
> >>> example by preventing that the list of enabled LSMs changes
> >>> at run-time.
> >> That ship sailed when "security=" was deprecated in favor of "lsm="
> >> support, which dynamically enables/disables LSMs at runtime.
> 
> security= is still supported and works the same as ever. lsm= is
> more powerful than security= but also harder to use.
> 
> > Maybe this possibility can be disabled with a new kernel option.
> > I will think a more concrete solution.
> >
> > Roberto
> >
> > HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
> > Managing Director: Li Peng, Li Jian, Shi Yanli
Roberto Sassu April 28, 2021, 3:35 p.m. UTC | #10
> From: Roberto Sassu
> Sent: Friday, March 5, 2021 4:19 PM
> ima_inode_setxattr() and ima_inode_removexattr() hooks are called before
> an
> operation is performed. Thus, ima_reset_appraise_flags() should not be
> called there, as flags might be unnecessarily reset if the operation is
> denied.
> 
> This patch introduces the post hooks ima_inode_post_setxattr() and
> ima_inode_post_removexattr(), and adds the call to
> ima_reset_appraise_flags() in the new functions.
> 
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  fs/xattr.c                            |  2 ++
>  include/linux/ima.h                   | 18 ++++++++++++++++++
>  security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++++++++---
>  security/security.c                   |  1 +
>  4 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xattr.c b/fs/xattr.c
> index b3444e06cded..81847f132d26 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -16,6 +16,7 @@
>  #include <linux/namei.h>
>  #include <linux/security.h>
>  #include <linux/evm.h>
> +#include <linux/ima.h>
>  #include <linux/syscalls.h>
>  #include <linux/export.h>
>  #include <linux/fsnotify.h>
> @@ -502,6 +503,7 @@ __vfs_removexattr_locked(struct user_namespace
> *mnt_userns,
> 
>  	if (!error) {
>  		fsnotify_xattr(dentry);
> +		ima_inode_post_removexattr(dentry, name);
>  		evm_inode_post_removexattr(dentry, name);
>  	}
> 
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index 61d5723ec303..5e059da43857 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -171,7 +171,13 @@ extern void ima_inode_post_setattr(struct
> user_namespace *mnt_userns,
>  				   struct dentry *dentry);
>  extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
>  		       const void *xattr_value, size_t xattr_value_len);
> +extern void ima_inode_post_setxattr(struct dentry *dentry,
> +				    const char *xattr_name,
> +				    const void *xattr_value,
> +				    size_t xattr_value_len);
>  extern int ima_inode_removexattr(struct dentry *dentry, const char
> *xattr_name);
> +extern void ima_inode_post_removexattr(struct dentry *dentry,
> +				       const char *xattr_name);
>  #else
>  static inline bool is_ima_appraise_enabled(void)
>  {
> @@ -192,11 +198,23 @@ static inline int ima_inode_setxattr(struct dentry
> *dentry,
>  	return 0;
>  }
> 
> +static inline void ima_inode_post_setxattr(struct dentry *dentry,
> +					   const char *xattr_name,
> +					   const void *xattr_value,
> +					   size_t xattr_value_len)
> +{
> +}
> +
>  static inline int ima_inode_removexattr(struct dentry *dentry,
>  					const char *xattr_name)
>  {
>  	return 0;
>  }
> +
> +static inline void ima_inode_post_removexattr(struct dentry *dentry,
> +					      const char *xattr_name)
> +{
> +}
>  #endif /* CONFIG_IMA_APPRAISE */
> 
>  #if defined(CONFIG_IMA_APPRAISE) &&
> defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> diff --git a/security/integrity/ima/ima_appraise.c
> b/security/integrity/ima/ima_appraise.c
> index 565e33ff19d0..1f029e4c8d7f 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -577,21 +577,40 @@ int ima_inode_setxattr(struct dentry *dentry, const
> char *xattr_name,
>  	if (result == 1) {
>  		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
>  			return -EINVAL;
> -		ima_reset_appraise_flags(d_backing_inode(dentry),
> -			xvalue->type == EVM_IMA_XATTR_DIGSIG);
>  		result = 0;
>  	}
>  	return result;
>  }
> 
> +void ima_inode_post_setxattr(struct dentry *dentry, const char
> *xattr_name,
> +			     const void *xattr_value, size_t xattr_value_len)
> +{
> +	const struct evm_ima_xattr_data *xvalue = xattr_value;
> +	int result;
> +
> +	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
> +				   xattr_value_len);
> +	if (result == 1)
> +		ima_reset_appraise_flags(d_backing_inode(dentry),

I found an issue in this patch.

Moving ima_reset_appraise_flags() to the post hook causes this
function to be executed also when __vfs_setxattr_noperm() is
called.

The problem is that at the end of a write IMA calls
ima_collect_measurement() to recalculate the file digest and
update security.ima. ima_collect_measurement() sets
IMA_COLLECTED.

However, after that __vfs_setxattr_noperm() causes
IMA_COLLECTED to be reset, and to unnecessarily recalculate
the file digest. This wouldn't happen if ima_reset_appraise_flags()
is in the pre hook.

I solved by replacing:
	iint->flags &= ~IMA_DONE_MASK;
with:
	iint->flags &= ~(IMA_DONE_MASK & ~IMA_COLLECTED);

just when the IMA_CHANGE_XATTR bit is set. It should
not be a problem since setting an xattr does not influence
the file content.

Mimi, what do you think?

Thanks

Roberto

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

> +			xvalue->type == EVM_IMA_XATTR_DIGSIG);
> +}
> +
>  int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
>  {
>  	int result;
> 
>  	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
>  	if (result == 1) {
> -		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
>  		result = 0;
>  	}
>  	return result;
>  }
> +
> +void ima_inode_post_removexattr(struct dentry *dentry, const char
> *xattr_name)
> +{
> +	int result;
> +
> +	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
> +	if (result == 1)
> +		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
> +}
> diff --git a/security/security.c b/security/security.c
> index 5ac96b16f8fa..efb1f874dc41 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1319,6 +1319,7 @@ void security_inode_post_setxattr(struct dentry
> *dentry, const char *name,
>  	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>  		return;
>  	call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
> +	ima_inode_post_setxattr(dentry, name, value, size);
>  	evm_inode_post_setxattr(dentry, name, value, size);
>  }
> 
> --
> 2.26.2
Mimi Zohar April 30, 2021, 6 p.m. UTC | #11
On Wed, 2021-04-28 at 15:35 +0000, Roberto Sassu wrote:
> > From: Roberto Sassu
> > Sent: Friday, March 5, 2021 4:19 PM
> > ima_inode_setxattr() and ima_inode_removexattr() hooks are called before
> > an
> > operation is performed. Thus, ima_reset_appraise_flags() should not be
> > called there, as flags might be unnecessarily reset if the operation is
> > denied.
> > 
> > This patch introduces the post hooks ima_inode_post_setxattr() and
> > ima_inode_post_removexattr(), and adds the call to
> > ima_reset_appraise_flags() in the new functions.
> > 
> > Cc: Casey Schaufler <casey@schaufler-ca.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  fs/xattr.c                            |  2 ++
> >  include/linux/ima.h                   | 18 ++++++++++++++++++
> >  security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++++++++---
> >  security/security.c                   |  1 +
> >  4 files changed, 43 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index b3444e06cded..81847f132d26 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/namei.h>
> >  #include <linux/security.h>
> >  #include <linux/evm.h>
> > +#include <linux/ima.h>
> >  #include <linux/syscalls.h>
> >  #include <linux/export.h>
> >  #include <linux/fsnotify.h>
> > @@ -502,6 +503,7 @@ __vfs_removexattr_locked(struct user_namespace
> > *mnt_userns,
> > 
> >  	if (!error) {
> >  		fsnotify_xattr(dentry);
> > +		ima_inode_post_removexattr(dentry, name);
> >  		evm_inode_post_removexattr(dentry, name);
> >  	}
> > 
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 61d5723ec303..5e059da43857 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -171,7 +171,13 @@ extern void ima_inode_post_setattr(struct
> > user_namespace *mnt_userns,
> >  				   struct dentry *dentry);
> >  extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
> >  		       const void *xattr_value, size_t xattr_value_len);
> > +extern void ima_inode_post_setxattr(struct dentry *dentry,
> > +				    const char *xattr_name,
> > +				    const void *xattr_value,
> > +				    size_t xattr_value_len);
> >  extern int ima_inode_removexattr(struct dentry *dentry, const char
> > *xattr_name);
> > +extern void ima_inode_post_removexattr(struct dentry *dentry,
> > +				       const char *xattr_name);
> >  #else
> >  static inline bool is_ima_appraise_enabled(void)
> >  {
> > @@ -192,11 +198,23 @@ static inline int ima_inode_setxattr(struct dentry
> > *dentry,
> >  	return 0;
> >  }
> > 
> > +static inline void ima_inode_post_setxattr(struct dentry *dentry,
> > +					   const char *xattr_name,
> > +					   const void *xattr_value,
> > +					   size_t xattr_value_len)
> > +{
> > +}
> > +
> >  static inline int ima_inode_removexattr(struct dentry *dentry,
> >  					const char *xattr_name)
> >  {
> >  	return 0;
> >  }
> > +
> > +static inline void ima_inode_post_removexattr(struct dentry *dentry,
> > +					      const char *xattr_name)
> > +{
> > +}
> >  #endif /* CONFIG_IMA_APPRAISE */
> > 
> >  #if defined(CONFIG_IMA_APPRAISE) &&
> > defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> > diff --git a/security/integrity/ima/ima_appraise.c
> > b/security/integrity/ima/ima_appraise.c
> > index 565e33ff19d0..1f029e4c8d7f 100644
> > --- a/security/integrity/ima/ima_appraise.c
> > +++ b/security/integrity/ima/ima_appraise.c
> > @@ -577,21 +577,40 @@ int ima_inode_setxattr(struct dentry *dentry, const
> > char *xattr_name,
> >  	if (result == 1) {
> >  		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
> >  			return -EINVAL;
> > -		ima_reset_appraise_flags(d_backing_inode(dentry),
> > -			xvalue->type == EVM_IMA_XATTR_DIGSIG);
> >  		result = 0;
> >  	}
> >  	return result;
> >  }
> > 
> > +void ima_inode_post_setxattr(struct dentry *dentry, const char
> > *xattr_name,
> > +			     const void *xattr_value, size_t xattr_value_len)
> > +{
> > +	const struct evm_ima_xattr_data *xvalue = xattr_value;
> > +	int result;
> > +
> > +	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
> > +				   xattr_value_len);
> > +	if (result == 1)
> > +		ima_reset_appraise_flags(d_backing_inode(dentry),
> 
> I found an issue in this patch.
> 
> Moving ima_reset_appraise_flags() to the post hook causes this
> function to be executed also when __vfs_setxattr_noperm() is
> called.
> 
> The problem is that at the end of a write IMA calls
> ima_collect_measurement() to recalculate the file digest and
> update security.ima. ima_collect_measurement() sets
> IMA_COLLECTED.
> 
> However, after that __vfs_setxattr_noperm() causes
> IMA_COLLECTED to be reset, and to unnecessarily recalculate
> the file digest. This wouldn't happen if ima_reset_appraise_flags()
> is in the pre hook.
> 
> I solved by replacing:
> 	iint->flags &= ~IMA_DONE_MASK;
> with:
> 	iint->flags &= ~(IMA_DONE_MASK & ~IMA_COLLECTED);
> 
> just when the IMA_CHANGE_XATTR bit is set. It should
> not be a problem since setting an xattr does not influence
> the file content.
> 
> Mimi, what do you think?

Thank yor for noticing this.

Without seeing the actual change it is hard to tell.   The only place
that "iint->flags &= ~IMA_DONE_MASK;" occurs is in neither of the above
functions, but in process_measurement().  There it is a part of a
compound "if" statement.  Perhaps it would be ok to change it for just
the IMA_CHANGE_XATTR test, but definitely not for the other conditions,
like untrusted mounts.

Moving ima_reset_appraise_flags() to the post hooks is to minimize
resetting the flags unnecessarily.  That is really a performance fix,
not something necessary for making the EVM portable & immutable
signatures more usable.  As much as possible, please minimize the
changes to facilitate review and testing.

thanks,

Mimi
Roberto Sassu May 3, 2021, 7:41 a.m. UTC | #12
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Friday, April 30, 2021 8:00 PM
> On Wed, 2021-04-28 at 15:35 +0000, Roberto Sassu wrote:
> > > From: Roberto Sassu
> > > Sent: Friday, March 5, 2021 4:19 PM
> > > ima_inode_setxattr() and ima_inode_removexattr() hooks are called
> before
> > > an
> > > operation is performed. Thus, ima_reset_appraise_flags() should not be
> > > called there, as flags might be unnecessarily reset if the operation is
> > > denied.
> > >
> > > This patch introduces the post hooks ima_inode_post_setxattr() and
> > > ima_inode_post_removexattr(), and adds the call to
> > > ima_reset_appraise_flags() in the new functions.
> > >
> > > Cc: Casey Schaufler <casey@schaufler-ca.com>
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  fs/xattr.c                            |  2 ++
> > >  include/linux/ima.h                   | 18 ++++++++++++++++++
> > >  security/integrity/ima/ima_appraise.c | 25 ++++++++++++++++++++++---
> > >  security/security.c                   |  1 +
> > >  4 files changed, 43 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/xattr.c b/fs/xattr.c
> > > index b3444e06cded..81847f132d26 100644
> > > --- a/fs/xattr.c
> > > +++ b/fs/xattr.c
> > > @@ -16,6 +16,7 @@
> > >  #include <linux/namei.h>
> > >  #include <linux/security.h>
> > >  #include <linux/evm.h>
> > > +#include <linux/ima.h>
> > >  #include <linux/syscalls.h>
> > >  #include <linux/export.h>
> > >  #include <linux/fsnotify.h>
> > > @@ -502,6 +503,7 @@ __vfs_removexattr_locked(struct
> user_namespace
> > > *mnt_userns,
> > >
> > >  	if (!error) {
> > >  		fsnotify_xattr(dentry);
> > > +		ima_inode_post_removexattr(dentry, name);
> > >  		evm_inode_post_removexattr(dentry, name);
> > >  	}
> > >
> > > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > > index 61d5723ec303..5e059da43857 100644
> > > --- a/include/linux/ima.h
> > > +++ b/include/linux/ima.h
> > > @@ -171,7 +171,13 @@ extern void ima_inode_post_setattr(struct
> > > user_namespace *mnt_userns,
> > >  				   struct dentry *dentry);
> > >  extern int ima_inode_setxattr(struct dentry *dentry, const char
> *xattr_name,
> > >  		       const void *xattr_value, size_t xattr_value_len);
> > > +extern void ima_inode_post_setxattr(struct dentry *dentry,
> > > +				    const char *xattr_name,
> > > +				    const void *xattr_value,
> > > +				    size_t xattr_value_len);
> > >  extern int ima_inode_removexattr(struct dentry *dentry, const char
> > > *xattr_name);
> > > +extern void ima_inode_post_removexattr(struct dentry *dentry,
> > > +				       const char *xattr_name);
> > >  #else
> > >  static inline bool is_ima_appraise_enabled(void)
> > >  {
> > > @@ -192,11 +198,23 @@ static inline int ima_inode_setxattr(struct
> dentry
> > > *dentry,
> > >  	return 0;
> > >  }
> > >
> > > +static inline void ima_inode_post_setxattr(struct dentry *dentry,
> > > +					   const char *xattr_name,
> > > +					   const void *xattr_value,
> > > +					   size_t xattr_value_len)
> > > +{
> > > +}
> > > +
> > >  static inline int ima_inode_removexattr(struct dentry *dentry,
> > >  					const char *xattr_name)
> > >  {
> > >  	return 0;
> > >  }
> > > +
> > > +static inline void ima_inode_post_removexattr(struct dentry *dentry,
> > > +					      const char *xattr_name)
> > > +{
> > > +}
> > >  #endif /* CONFIG_IMA_APPRAISE */
> > >
> > >  #if defined(CONFIG_IMA_APPRAISE) &&
> > > defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> > > diff --git a/security/integrity/ima/ima_appraise.c
> > > b/security/integrity/ima/ima_appraise.c
> > > index 565e33ff19d0..1f029e4c8d7f 100644
> > > --- a/security/integrity/ima/ima_appraise.c
> > > +++ b/security/integrity/ima/ima_appraise.c
> > > @@ -577,21 +577,40 @@ int ima_inode_setxattr(struct dentry *dentry,
> const
> > > char *xattr_name,
> > >  	if (result == 1) {
> > >  		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
> > >  			return -EINVAL;
> > > -		ima_reset_appraise_flags(d_backing_inode(dentry),
> > > -			xvalue->type == EVM_IMA_XATTR_DIGSIG);
> > >  		result = 0;
> > >  	}
> > >  	return result;
> > >  }
> > >
> > > +void ima_inode_post_setxattr(struct dentry *dentry, const char
> > > *xattr_name,
> > > +			     const void *xattr_value, size_t xattr_value_len)
> > > +{
> > > +	const struct evm_ima_xattr_data *xvalue = xattr_value;
> > > +	int result;
> > > +
> > > +	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
> > > +				   xattr_value_len);
> > > +	if (result == 1)
> > > +		ima_reset_appraise_flags(d_backing_inode(dentry),
> >
> > I found an issue in this patch.
> >
> > Moving ima_reset_appraise_flags() to the post hook causes this
> > function to be executed also when __vfs_setxattr_noperm() is
> > called.
> >
> > The problem is that at the end of a write IMA calls
> > ima_collect_measurement() to recalculate the file digest and
> > update security.ima. ima_collect_measurement() sets
> > IMA_COLLECTED.
> >
> > However, after that __vfs_setxattr_noperm() causes
> > IMA_COLLECTED to be reset, and to unnecessarily recalculate
> > the file digest. This wouldn't happen if ima_reset_appraise_flags()
> > is in the pre hook.
> >
> > I solved by replacing:
> > 	iint->flags &= ~IMA_DONE_MASK;
> > with:
> > 	iint->flags &= ~(IMA_DONE_MASK & ~IMA_COLLECTED);
> >
> > just when the IMA_CHANGE_XATTR bit is set. It should
> > not be a problem since setting an xattr does not influence
> > the file content.
> >
> > Mimi, what do you think?
> 
> Thank yor for noticing this.
> 
> Without seeing the actual change it is hard to tell.   The only place
> that "iint->flags &= ~IMA_DONE_MASK;" occurs is in neither of the above
> functions, but in process_measurement().  There it is a part of a
> compound "if" statement.  Perhaps it would be ok to change it for just
> the IMA_CHANGE_XATTR test, but definitely not for the other conditions,
> like untrusted mounts.

Ok. Should I include this change in this patch or in a separate patch?
	
> Moving ima_reset_appraise_flags() to the post hooks is to minimize
> resetting the flags unnecessarily.  That is really a performance fix,
> not something necessary for making the EVM portable & immutable
> signatures more usable.  As much as possible, please minimize the
> changes to facilitate review and testing.

Ok.

Thanks

Roberto

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

> thanks,
> 
> Mimi
Mimi Zohar May 3, 2021, 1:21 p.m. UTC | #13
On Mon, 2021-05-03 at 07:41 +0000, Roberto Sassu wrote:
> 
> > > > diff --git a/security/integrity/ima/ima_appraise.c
> > > > b/security/integrity/ima/ima_appraise.c
> > > > index 565e33ff19d0..1f029e4c8d7f 100644
> > > > --- a/security/integrity/ima/ima_appraise.c
> > > > +++ b/security/integrity/ima/ima_appraise.c
> > > > @@ -577,21 +577,40 @@ int ima_inode_setxattr(struct dentry *dentry,
> > const
> > > > char *xattr_name,
> > > >  	if (result == 1) {
> > > >  		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
> > > >  			return -EINVAL;
> > > > -		ima_reset_appraise_flags(d_backing_inode(dentry),
> > > > -			xvalue->type == EVM_IMA_XATTR_DIGSIG);
> > > >  		result = 0;
> > > >  	}
> > > >  	return result;
> > > >  }
> > > >
> > > > +void ima_inode_post_setxattr(struct dentry *dentry, const char
> > > > *xattr_name,
> > > > +			     const void *xattr_value, size_t xattr_value_len)
> > > > +{
> > > > +	const struct evm_ima_xattr_data *xvalue = xattr_value;
> > > > +	int result;
> > > > +
> > > > +	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
> > > > +				   xattr_value_len);
> > > > +	if (result == 1)
> > > > +		ima_reset_appraise_flags(d_backing_inode(dentry),
> > >
> > > I found an issue in this patch.
> > >
> > > Moving ima_reset_appraise_flags() to the post hook causes this
> > > function to be executed also when __vfs_setxattr_noperm() is
> > > called.
> > >
> > > The problem is that at the end of a write IMA calls
> > > ima_collect_measurement() to recalculate the file digest and
> > > update security.ima. ima_collect_measurement() sets
> > > IMA_COLLECTED.
> > >
> > > However, after that __vfs_setxattr_noperm() causes
> > > IMA_COLLECTED to be reset, and to unnecessarily recalculate
> > > the file digest. This wouldn't happen if ima_reset_appraise_flags()
> > > is in the pre hook.
> > >
> > > I solved by replacing:
> > > 	iint->flags &= ~IMA_DONE_MASK;
> > > with:
> > > 	iint->flags &= ~(IMA_DONE_MASK & ~IMA_COLLECTED);
> > >
> > > just when the IMA_CHANGE_XATTR bit is set. It should
> > > not be a problem since setting an xattr does not influence
> > > the file content.
> > >
> > > Mimi, what do you think?
> > 
> > Thank yor for noticing this.
> > 
> > Without seeing the actual change it is hard to tell.   The only place
> > that "iint->flags &= ~IMA_DONE_MASK;" occurs is in neither of the above
> > functions, but in process_measurement().  There it is a part of a
> > compound "if" statement.  Perhaps it would be ok to change it for just
> > the IMA_CHANGE_XATTR test, but definitely not for the other conditions,
> > like untrusted mounts.
> 
> Ok. Should I include this change in this patch or in a separate patch?
> 	
> > Moving ima_reset_appraise_flags() to the post hooks is to minimize
> > resetting the flags unnecessarily.  That is really a performance fix,
> > not something necessary for making the EVM portable & immutable
> > signatures more usable.  As much as possible, please minimize the
> > changes to facilitate review and testing.
> 
> Ok.

I'm really sorry, but the more I'm looking at the patch set, the more
I'm realizing that a number of the changes are a result of this
"performance improvement".   Without this change, reviewing the code
would be simplified.  So yes, the patch set needs to be updated to
reflect only what is needed to support making EVM portable & immutable
signatures more usable.

thanks,

Mimi
diff mbox series

Patch

diff --git a/fs/xattr.c b/fs/xattr.c
index b3444e06cded..81847f132d26 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -16,6 +16,7 @@ 
 #include <linux/namei.h>
 #include <linux/security.h>
 #include <linux/evm.h>
+#include <linux/ima.h>
 #include <linux/syscalls.h>
 #include <linux/export.h>
 #include <linux/fsnotify.h>
@@ -502,6 +503,7 @@  __vfs_removexattr_locked(struct user_namespace *mnt_userns,
 
 	if (!error) {
 		fsnotify_xattr(dentry);
+		ima_inode_post_removexattr(dentry, name);
 		evm_inode_post_removexattr(dentry, name);
 	}
 
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 61d5723ec303..5e059da43857 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -171,7 +171,13 @@  extern void ima_inode_post_setattr(struct user_namespace *mnt_userns,
 				   struct dentry *dentry);
 extern int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		       const void *xattr_value, size_t xattr_value_len);
+extern void ima_inode_post_setxattr(struct dentry *dentry,
+				    const char *xattr_name,
+				    const void *xattr_value,
+				    size_t xattr_value_len);
 extern int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name);
+extern void ima_inode_post_removexattr(struct dentry *dentry,
+				       const char *xattr_name);
 #else
 static inline bool is_ima_appraise_enabled(void)
 {
@@ -192,11 +198,23 @@  static inline int ima_inode_setxattr(struct dentry *dentry,
 	return 0;
 }
 
+static inline void ima_inode_post_setxattr(struct dentry *dentry,
+					   const char *xattr_name,
+					   const void *xattr_value,
+					   size_t xattr_value_len)
+{
+}
+
 static inline int ima_inode_removexattr(struct dentry *dentry,
 					const char *xattr_name)
 {
 	return 0;
 }
+
+static inline void ima_inode_post_removexattr(struct dentry *dentry,
+					      const char *xattr_name)
+{
+}
 #endif /* CONFIG_IMA_APPRAISE */
 
 #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 565e33ff19d0..1f029e4c8d7f 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -577,21 +577,40 @@  int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 	if (result == 1) {
 		if (!xattr_value_len || (xvalue->type >= IMA_XATTR_LAST))
 			return -EINVAL;
-		ima_reset_appraise_flags(d_backing_inode(dentry),
-			xvalue->type == EVM_IMA_XATTR_DIGSIG);
 		result = 0;
 	}
 	return result;
 }
 
+void ima_inode_post_setxattr(struct dentry *dentry, const char *xattr_name,
+			     const void *xattr_value, size_t xattr_value_len)
+{
+	const struct evm_ima_xattr_data *xvalue = xattr_value;
+	int result;
+
+	result = ima_protect_xattr(dentry, xattr_name, xattr_value,
+				   xattr_value_len);
+	if (result == 1)
+		ima_reset_appraise_flags(d_backing_inode(dentry),
+			xvalue->type == EVM_IMA_XATTR_DIGSIG);
+}
+
 int ima_inode_removexattr(struct dentry *dentry, const char *xattr_name)
 {
 	int result;
 
 	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
 	if (result == 1) {
-		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
 		result = 0;
 	}
 	return result;
 }
+
+void ima_inode_post_removexattr(struct dentry *dentry, const char *xattr_name)
+{
+	int result;
+
+	result = ima_protect_xattr(dentry, xattr_name, NULL, 0);
+	if (result == 1)
+		ima_reset_appraise_flags(d_backing_inode(dentry), 0);
+}
diff --git a/security/security.c b/security/security.c
index 5ac96b16f8fa..efb1f874dc41 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1319,6 +1319,7 @@  void security_inode_post_setxattr(struct dentry *dentry, const char *name,
 	if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
 		return;
 	call_void_hook(inode_post_setxattr, dentry, name, value, size, flags);
+	ima_inode_post_setxattr(dentry, name, value, size);
 	evm_inode_post_setxattr(dentry, name, value, size);
 }