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: <CAGXu5j+k86VtegoRxV1s++YZdMxXEhQhqx1Yh7eu+YKHn0wNZA@mail.gmail.com>
Date:   Mon, 8 Apr 2019 10:25:28 -0700
From:   Kees Cook <keescook@...omium.org>
To:     John Johansen <john.johansen@...onical.com>
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 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.)

Should I send a v2 without the "initialized" check or is this okay to
leave as-is with the redundant check?

-- 
Kees Cook