| Message ID | 20210322154207.6802-2-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ima: don't access a file's integrity status before an IMA policy is loaded | expand |
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
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
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
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
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/
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/
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()").
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
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
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;
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(+)