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

[Roberto Sassu]

> From: Casey Schaufler [mailto:casey@xxxxxxxxxxxxxxxx]
> Sent: Tuesday, April 27, 2021 6:40 PM
> On 4/27/2021 8:57 AM, Roberto Sassu wrote:
> >> From: Mimi Zohar [mailto:zohar@xxxxxxxxxxxxx]
> >> 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@xxxxxxxxxxxxx]
> >>>> 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