| Message ID | 20210305151923.29039-5-roberto.sassu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | evm: Improve usability of portable signatures | expand |
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); > } >
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
> 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
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
> 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
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
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
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
> 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
> 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
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
> 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
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 --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); }
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(-)