lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 8 Apr 2019 10:47:49 -0700 From: John Johansen <john.johansen@...onical.com> To: Kees Cook <keescook@...omium.org> Cc: James Morris <jmorris@...ei.org>, David Rheinsberg <david.rheinsberg@...il.com>, "Serge E. Hallyn" <serge@...lyn.com>, linux-security-module <linux-security-module@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org> Subject: Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled" On 4/8/19 10:25 AM, Kees Cook wrote: > On Mon, Apr 8, 2019 at 9:58 AM John Johansen > <john.johansen@...onical.com> wrote: >>> +/* Can only be set before AppArmor is initialized (i.e. on boot cmdline). */ >>> +static int param_set_aaintbool(const char *val, const struct kernel_param *kp) >>> +{ >>> + struct kernel_param kp_local; >>> + bool value; >>> + int error; >>> + >>> + if (apparmor_initialized) >>> + return -EPERM; >>> + >> This isn't sufficient/correct. apparmor_initialized is only set after >> apparmor has gone through and completed initialization. However if >> apparmor is not selected as one of the LSMs to enable, then this check >> won't stop apparmor_enabled from being set post boot. >> >> However with the apparmor_enabled param being 0444 and the >> apparmor_enabled_setup() fn handling boot cmdline do with even need >> the set parameter fn? > > Yup, that's true. I've gone and tested this, and yes, the 0444 is > sufficient to protect the logic here (even if root chmods the inode). > So the test here is redundant. However, very early in the threads > about LSM boot cmdline enabling it was made clear that > "apparmor.enabled=..." needed to stay working, which means the "set" > op is still needed. (But I'm happy to do whatever you want here -- I > was just trying to keep the functionality as it was.) > Right, though the legacy case that most document reference is apparmor=0/1 which is handled by __apparmor_enabled_setup() still best not to break apparmor.enabled > Should I send a v2 without the "initialized" check or is this okay to > leave as-is with the redundant check? > redundant is fine, it will help protect against shooting ourselves in the foot if some one ever tries changing the 0444 you can have my Acked-by: John Johansen <john.johansen@...onical.com>
Powered by blists - more mailing lists