[PATCH 1/2] ima: don't access a file's integrity status before an IMA policy is loaded

[PATCH 1/2] ima: don't access a file's integrity status before an IMA policy is loaded

[Mimi Zohar]

Only after an IMA policy is loaded, check, save, or update the cached
file's integrity status.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/ima/ima_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 9ef748ea829f..9d1196f712e1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -606,6 +606,9 @@ void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
 	struct integrity_iint_cache *iint;
 	int must_appraise;
 
+	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+		return;
+
 	must_appraise = ima_must_appraise(mnt_userns, inode, MAY_ACCESS,
 					  FILE_CHECK);
 	if (!must_appraise)
@@ -636,6 +639,9 @@ void ima_post_path_mknod(struct user_namespace *mnt_userns,
 	struct inode *inode = dentry->d_inode;
 	int must_appraise;
 
+	if (!ima_policy_flag || !S_ISREG(inode->i_mode))
+		return;
+
 	must_appraise = ima_must_appraise(mnt_userns, inode, MAY_ACCESS,
 					  FILE_CHECK);
 	if (!must_appraise)
-- 
2.27.0

[PATCH 2/2] integrity: double check iint_cache was initialized

[Mimi Zohar]

The kernel may be built with multiple LSMs, but only a subset may be
enabled on the boot command line by specifying "lsm=".  Not including
"integrity" on the ordered LSM list may result in a NULL deref.

As reported by Dmitry Vyukov:
in qemu:
qemu-system-x86_64       -enable-kvm     -machine q35,nvdimm -cpu
max,migratable=off -smp 4       -m 4G,slots=4,maxmem=16G        -hda
wheezy.img      -kernel arch/x86/boot/bzImage   -nographic -vga std
 -soundhw all     -usb -usbdevice tablet  -bt hci -bt device:keyboard
   -net user,host=10.0.2.10,hostfwd=tcp::10022-:22 -net
nic,model=virtio-net-pci   -object
memory-backend-file,id=pmem1,share=off,mem-path=/dev/zero,size=64M
  -device nvdimm,id=nvdimm1,memdev=pmem1  -append "console=ttyS0
root=/dev/sda earlyprintk=serial rodata=n oops=panic panic_on_warn=1
panic=86400 lsm=smack numa=fake=2 nopcid dummy_hcd.num=8"   -pidfile
vm_pid -m 2G -cpu host

But it crashes on NULL deref in integrity_inode_get during boot:

Run /sbin/init as init process
BUG: kernel NULL pointer dereference, address: 000000000000001c
PGD 0 P4D 0
Oops: 0000 [#1] PREEMPT SMP KASAN
CPU: 3 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc2+ #97
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
rel-1.13.0-44-g88ab0c15525c-prebuilt.qemu.org 04/01/2014
RIP: 0010:kmem_cache_alloc+0x2b/0x370 mm/slub.c:2920
Code: 57 41 56 41 55 41 54 41 89 f4 55 48 89 fd 53 48 83 ec 10 44 8b
3d d9 1f 90 0b 65 48 8b 04 25 28 00 00 00 48 89 44 24 08 31 c0 <8b> 5f
1c 4cf
RSP: 0000:ffffc9000032f9d8 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff888017fc4f00 RCX: 0000000000000000
RDX: ffff888040220000 RSI: 0000000000000c40 RDI: 0000000000000000
RBP: 0000000000000000 R08: 0000000000000000 R09: ffff888019263627
R10: ffffffff83937cd1 R11: 0000000000000000 R12: 0000000000000c40
R13: ffff888019263538 R14: 0000000000000000 R15: 0000000000ffffff
FS:  0000000000000000(0000) GS:ffff88802d180000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000000001c CR3: 000000000b48e000 CR4: 0000000000750ee0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
 integrity_inode_get+0x47/0x260 security/integrity/iint.c:105
 process_measurement+0x33d/0x17e0 security/integrity/ima/ima_main.c:237
 ima_bprm_check+0xde/0x210 security/integrity/ima/ima_main.c:474
 security_bprm_check+0x7d/0xa0 security/security.c:845
 search_binary_handler fs/exec.c:1708 [inline]
 exec_binprm fs/exec.c:1761 [inline]
 bprm_execve fs/exec.c:1830 [inline]
 bprm_execve+0x764/0x19a0 fs/exec.c:1792
 kernel_execve+0x370/0x460 fs/exec.c:1973
 try_to_run_init_process+0x14/0x4e init/main.c:1366
 kernel_init+0x11d/0x1b8 init/main.c:1477
 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
Modules linked in:
CR2: 000000000000001c
---[ end trace 22d601a500de7d79 ]---

Since LSMs and IMA may be configured at build time, but not enabled at
run time, panic the system if "integrity" was not initialized before use.

Reported-by: Dmitry Vyukov <dvyukov@google.com>
Fixes: 79f7865d844c ("LSM: Introduce "lsm=" for boottime LSM selection")
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 security/integrity/iint.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/security/integrity/iint.c b/security/integrity/iint.c
index 1d20003243c3..0ba01847e836 100644
--- a/security/integrity/iint.c
+++ b/security/integrity/iint.c
@@ -98,6 +98,14 @@ struct integrity_iint_cache *integrity_inode_get(struct inode *inode)
 	struct rb_node *node, *parent = NULL;
 	struct integrity_iint_cache *iint, *test_iint;
 
+	/*
+	 * The integrity's "iint_cache" is initialized at security_init(),
+	 * unless it is not included in the ordered list of LSMs enabled
+	 * on the boot command line.
+	 */
+	if (!iint_cache)
+		panic("%s: lsm=integrity required.\n", __func__);
+
 	iint = integrity_iint_find(inode);
 	if (iint)
 		return iint;
-- 
2.27.0

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Eric Biggers]

On Mon, Mar 22, 2021 at 11:42:07AM -0400, Mimi Zohar wrote:
> 
> Reported-by: Dmitry Vyukov <dvyukov@google.com>
> Fixes: 79f7865d844c ("LSM: Introduce "lsm=" for boottime LSM selection")
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

Missing Cc stable?

- Eric

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Mimi Zohar]

On Mon, 2021-03-22 at 09:52 -0700, Eric Biggers wrote:
> On Mon, Mar 22, 2021 at 11:42:07AM -0400, Mimi Zohar wrote:
> > 
> > Reported-by: Dmitry Vyukov <dvyukov@google.com>
> > Fixes: 79f7865d844c ("LSM: Introduce "lsm=" for boottime LSM selection")
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> Missing Cc stable?

Yes, I was waiting for some comments/review/tags, before adding it.

Mimi

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Petr Vorel]

Hi Mimi, Tetsuo, Kees, all,

FYI this commit merged as 92063f3ca73a ("integrity: double check iint_cache was initialized")
is the reason for openSUSE distro installer going back from lsm= to deprecated
security= when filling default grub parameters because security=apparmor or
security=selinux does not break boot when used with ima_policy=tcb, unlike
using lsm.

@Kees, @Mimi sure, people who use ima_policy=tcb will just remove lsm parameter
or add "integrity" to it but I wonder whether there could be "integrity"
automatic inclusion when using ima_policy=tcb. Although the point of lsm= (and
CONFIG_LSM) is to have *ordered* list of enabled LSMs and it wouldn't be clear
on which place.

Kind regards,
Petr

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Casey Schaufler]

On 2/24/2022 6:20 AM, Petr Vorel wrote:
> Hi Mimi, Tetsuo, Kees, all,
>
> FYI this commit merged as 92063f3ca73a ("integrity: double check iint_cache was initialized")
> is the reason for openSUSE distro installer going back from lsm= to deprecated
> security= when filling default grub parameters because security=apparmor or
> security=selinux does not break boot when used with ima_policy=tcb, unlike
> using lsm.

OK, color me confused. Integrity isn't an LSM. It doesn't
call security_add_hooks().

> @Kees, @Mimi sure, people who use ima_policy=tcb will just remove lsm parameter
> or add "integrity" to it but I wonder whether there could be "integrity"
> automatic inclusion when using ima_policy=tcb. Although the point of lsm= (and
> CONFIG_LSM) is to have *ordered* list of enabled LSMs and it wouldn't be clear
> on which place.

Why would adding integrity to the lsm= make sense? It's not an LSM.

Sorry, but something is wrong here.

>
> Kind regards,
> Petr

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Petr Vorel]

Hi Casey,

> On 2/24/2022 6:20 AM, Petr Vorel wrote:
> > Hi Mimi, Tetsuo, Kees, all,

> > FYI this commit merged as 92063f3ca73a ("integrity: double check iint_cache was initialized")
> > is the reason for openSUSE distro installer going back from lsm= to deprecated
> > security= when filling default grub parameters because security=apparmor or
> > security=selinux does not break boot when used with ima_policy=tcb, unlike
> > using lsm.

> OK, color me confused. Integrity isn't an LSM. It doesn't
> call security_add_hooks().
Really: "Initially I also questioned making "integrity" an LSM.  Perhaps it's
time to reconsider." [1]

> > @Kees, @Mimi sure, people who use ima_policy=tcb will just remove lsm parameter
> > or add "integrity" to it but I wonder whether there could be "integrity"
> > automatic inclusion when using ima_policy=tcb. Although the point of lsm= (and
> > CONFIG_LSM) is to have *ordered* list of enabled LSMs and it wouldn't be clear
> > on which place.

> Why would adding integrity to the lsm= make sense? It's not an LSM.

> Sorry, but something is wrong here.
np. I explained that: try to boot with "ima_policy=tcb lsm=" or "ima_policy=tcb
lsm=whatever" (whatever != integrity).

Also have look at commit 92063f3ca73a ("integrity: double check iint_cache was
initialized") which explain why it's needed.

Kind regards,
Petr

[1] https://lore.kernel.org/linux-integrity/3ed2004413e0ac07c7bd6f10294d6b6fac6fdbf3.camel@linux.ibm.com/

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Casey Schaufler]

On 2/24/2022 9:42 AM, Petr Vorel wrote:
> Hi Casey,
>
>> On 2/24/2022 6:20 AM, Petr Vorel wrote:
>>> Hi Mimi, Tetsuo, Kees, all,
>>> FYI this commit merged as 92063f3ca73a ("integrity: double check iint_cache was initialized")
>>> is the reason for openSUSE distro installer going back from lsm= to deprecated
>>> security= when filling default grub parameters because security=apparmor or
>>> security=selinux does not break boot when used with ima_policy=tcb, unlike
>>> using lsm.
>> OK, color me confused. Integrity isn't an LSM. It doesn't
>> call security_add_hooks().
> Really: "Initially I also questioned making "integrity" an LSM.  Perhaps it's
> time to reconsider." [1]

It was always my expectation, which appears to have been poorly
communicated, that "making integrity an LSM" meant using the LSM
hook infrastructure. Just adding "integrity" to lsm= doesn't make
it an LSM to my mind.

>>> @Kees, @Mimi sure, people who use ima_policy=tcb will just remove lsm parameter
>>> or add "integrity" to it but I wonder whether there could be "integrity"
>>> automatic inclusion when using ima_policy=tcb. Although the point of lsm= (and
>>> CONFIG_LSM) is to have *ordered* list of enabled LSMs and it wouldn't be clear
>>> on which place.
>> Why would adding integrity to the lsm= make sense? It's not an LSM.
>> Sorry, but something is wrong here.
> np. I explained that: try to boot with "ima_policy=tcb lsm=" or "ima_policy=tcb
> lsm=whatever" (whatever != integrity).
>
> Also have look at commit 92063f3ca73a ("integrity: double check iint_cache was
> initialized") which explain why it's needed.

	"The mixed metaphor never boils."

What is bothering me is that IMA, which is not an LSM, depends on the
LSM mechanism for specification. Sigh, I can see that boat has sailed.

Since IMA doesn't use the LSM hook mechanisms (doesn't add hooks to the
hook lists) it shouldn't matter where in the lsm= string "integrity" is
or where in CONFIG_LSM it appears. The ordering is only relevant to the
"registered" hooks, and IMA doesn't register.

... except that it shouldn't be 1st, since "capability" is always supposed
to be 1st. And it shouldn't be last, because BPF whats to be there, and I
can't say if their user-space will handle the lsm string if it isn't.

> Kind regards,
> Petr
>
> [1] https://lore.kernel.org/linux-integrity/3ed2004413e0ac07c7bd6f10294d6b6fac6fdbf3.camel@linux.ibm.com/

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Mimi Zohar]

Hi Petr, Casey,

On Thu, 2022-02-24 at 10:51 -0800, Casey Schaufler wrote:
> On 2/24/2022 9:42 AM, Petr Vorel wrote:

> It was always my expectation, which appears to have been poorly
> communicated, that "making integrity an LSM" meant using the LSM
> hook infrastructure. Just adding "integrity" to lsm= doesn't make
> it an LSM to my mind.

Agreed.  The actual commit that introduced the change was 3d6e5f6dcf65
("LSM: Convert security_initcall() into DEFINE_LSM()").

-- 
thanks,

Mimi

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Petr Vorel]

Hi Mimi, all,

> Hi Petr, Casey,

> On Thu, 2022-02-24 at 10:51 -0800, Casey Schaufler wrote:
> > On 2/24/2022 9:42 AM, Petr Vorel wrote:

> > It was always my expectation, which appears to have been poorly
> > communicated, that "making integrity an LSM" meant using the LSM
> > hook infrastructure. Just adding "integrity" to lsm= doesn't make
> > it an LSM to my mind.

> Agreed.  The actual commit that introduced the change was 3d6e5f6dcf65
> ("LSM: Convert security_initcall() into DEFINE_LSM()").
I wonder whether we can improve things now.

Kind regards,
Petr

Re: [PATCH 2/2] integrity: double check iint_cache was initialized

[Mimi Zohar]

On Mon, 2022-02-28 at 14:44 +0100, Petr Vorel wrote:
> Hi Mimi, all,
> 
> > Hi Petr, Casey,
> 
> > On Thu, 2022-02-24 at 10:51 -0800, Casey Schaufler wrote:
> > > On 2/24/2022 9:42 AM, Petr Vorel wrote:
> 
> > > It was always my expectation, which appears to have been poorly
> > > communicated, that "making integrity an LSM" meant using the LSM
> > > hook infrastructure. Just adding "integrity" to lsm= doesn't make
> > > it an LSM to my mind.
> 
> > Agreed.  The actual commit that introduced the change was 3d6e5f6dcf65
> > ("LSM: Convert security_initcall() into DEFINE_LSM()").
> I wonder whether we can improve things now.

I'm not sure it is possible to revert the change.  Perhaps the simplest
solution would be to move integrity off the security hook.  It just
needs to be initialized before EVM and IMA.

thanks,

Mimi

Re: [PATCH 1/2] ima: don't access a file's integrity status before an IMA policy is loaded

[Eric Biggers]

On Mon, Mar 22, 2021 at 11:42:06AM -0400, Mimi Zohar wrote:
> Only after an IMA policy is loaded, check, save, or update the cached
> file's integrity status.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>

This commit message doesn't describe what the actual effect of this change is.
Is it fixing something?

- Eric

Re: [PATCH 1/2] ima: don't access a file's integrity status before an IMA policy is loaded

[Mimi Zohar]

On Mon, 2021-03-22 at 09:51 -0700, Eric Biggers wrote:
> On Mon, Mar 22, 2021 at 11:42:06AM -0400, Mimi Zohar wrote:
> > Only after an IMA policy is loaded, check, save, or update the cached
> > file's integrity status.
> > 
> > Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
> 
> This commit message doesn't describe what the actual effect of this change is.
> Is it fixing something?

No, it's just short circuiting out even earlier, but isn't needed.

Mimi