linux-kernel - Re: [PATCH] apparmor: Restore Y/N in /sys for apparmor's "enabled"

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a7216d87-df56-8112-fd71-e9a4ff827804@canonical.com>
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>