Re: TOMOYO's pull request for v6.12

[Kees Cook <kees@kernel.org>]

On Sat, Oct 05, 2024 at 08:41:06AM +0900, Tetsuo Handa wrote:
> On 2024/10/05 5:54, Kees Cook wrote:
> > - tomoyo_register_hooks() becomes an exploitation gadget that could be
> >   used to bypass the LSM as a whole.
> 
> tomoyo_register_hooks() is enabled only if "CONFIG_SECURITY_TOMOYO_LKM is
> included while building the kernel" && "security=tomoyo is specified or
> tomoyo is included in the lsm= kernel command line options".
> 
> Therefore, those who are building kernels with CONFIG_SECURITY_TOMOYO_LKM=n are
> not affected.

Sure, but my point is that convincing RedHat that this is acceptable is
likely to be an uphill battle considering their effort to gain full
ro_after_init coverage for SELinux.

> Even if kernels are built with CONFIG_SECURITY_TOMOYO_LKM=y, callbacks
> registered by tomoyo_register_hooks() won't be called unless "security=tomoyo
> is specified or tomoyo is included in the lsm= kernel command line options", for
> the proxy callbacks that use static call tables are not registered.

This part I overlooked. I forgot that Tomoyo is still not fully stackable,
so it isn't getting included in CONFIG_LSM= for the distros that do
build it.

> Even if kernels are built with CONFIG_SECURITY_TOMOYO_LKM=y, and "security=tomoyo
> is specified or tomoyo is included in the lsm= kernel command line options",
> tomoyo_register_hooks() can be called only once.

An attacker with a read/write primitive would be able to locate and
write to "registered" (since it is not read-only), allowing them to call
tomoyo_register_hooks() multiple times.

> And tomoyo.ko is loaded by the
> time /sbin/init (nowadays /usr/lib/systemd/systemd) starts. That is, by the time
> an attacker can login from console or can start attacking via network,
> tomoyo_register_hooks() is no longer callable.

See above -- calling tomoyo_register_hooks() after boot is entirely
feasible given a read/write attack primitive.

> Therefore, the only problem with tomoyo.ko approach is that the static call tables
> for tomoyo_register_hooks() are currently not marked as __ro_after_init. But it will
> be possible to make the static call tables read-only if the static call tables

The tables actually don't matter as much -- an attacker could construct
their own table anywhere in kernel memory and pass that as an argument
for their call to tomoyo_register_hooks().

(This is actually one of the reasons I have pushed to have sensitive
functions like that be able to check that their passed-in argument is
contained in a read-only area, but that didn't get much traction[1].)

> are aligned in a page boundary and an architecture-dependent kernel API that changes
> given page's attribute to read-only is called. (This is why __ro_after_init can work,
> isn't it?)

The __ro_after_init section is immediately neighboring the .rodata
section, so when .rodata is marked read-only (after __init has
finished), the kernel marks both sections read-only. (Except for, I
think, s390, which does two passes: .rodata is read-only before __init,
and then __ro_after_init is marked read-only after __init.)

> As a whole, I don't think tomoyo.ko approach is unacceptably dangerous.

I agree, this implementation is safer than I initial assessed (due to the
LSM's view of the hooks being skipped due to lsm= not including tomoyo).
I still think how this patch ended up in Linus's tree was a big mistake,
though.

Regardless, my opinion is unchanged: I think it will be harder to convince
RedHat to build in _this_ version of tomoyo compared to the stock version.

(Out of curiosity, does RedHat build in AppArmor?)

-Kees

[1] https://lore.kernel.org/lkml/202408052100.74A2316C27@keescook/

-- 
Kees Cook