Thread (30 messages) 30 messages, 4 authors, 2021-05-03

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

From: Casey Schaufler <casey@schaufler-ca.com>
Date: 2021-04-27 16:40:05
Also in: linux-fsdevel, linux-security-module, lkml

On 4/27/2021 8:57 AM, Roberto Sassu wrote:
quoted
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:
quoted
quoted
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:
quoted
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.
quoted
quoted
quoted
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.
quoted
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.
quoted
quoted
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.
quoted
quoted
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
  
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help