Message-ID: <ead81edf-ca8f-9e97-96ca-984202e7d8ac@schaufler-ca.com>
Date:   Mon, 15 Nov 2021 13:42:54 -0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH] block: Check ADMIN before NICE for IOPRIO_CLASS_RT
Content-Language: en-US
To:     Alistair Delva <adelva@google.com>,
        Ondrej Mosnacek <omosnace@redhat.com>
Cc:     Linux kernel mailing list <linux-kernel@vger.kernel.org>,
        Khazhismel Kumykov <khazhy@google.com>,
        Bart Van Assche <bvanassche@acm.org>,
        Serge Hallyn <serge@hallyn.com>, Jens Axboe <axboe@kernel.dk>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Paul Moore <paul@paul-moore.com>,
        SElinux list <selinux@vger.kernel.org>,
        Linux Security Module list 
        <linux-security-module@vger.kernel.org>,
        "Cc: Android Kernel" <kernel-team@android.com>,
        Linux Stable maillist <stable@vger.kernel.org>,
        Casey Schaufler <casey@schaufler-ca.com>
References: <20211115173850.3598768-1-adelva@google.com>
 <CAFqZXNvVHv8Oje-WV6MWMF96kpR6epTsbc-jv-JF+YJw=55i1w@mail.gmail.com>
 <CANDihLEFZAz8DwkkMGiDJnDMjxiUuSCanYsJtkRwa9RoyruLFA@mail.gmail.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
In-Reply-To: <CANDihLEFZAz8DwkkMGiDJnDMjxiUuSCanYsJtkRwa9RoyruLFA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 11/15/2021 11:08 AM, Alistair Delva wrote:
> On Mon, Nov 15, 2021 at 11:04 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>> On Mon, Nov 15, 2021 at 7:14 PM Alistair Delva <adelva@google.com> wrote:
>>> Booting to Android userspace on 5.14 or newer triggers the following
>>> SELinux denial:
>>>
>>> avc: denied { sys_nice } for comm="init" capability=23
>>>       scontext=u:r:init:s0 tcontext=u:r:init:s0 tclass=capability
>>>       permissive=0
>>>
>>> Init is PID 0 running as root, so it already has CAP_SYS_ADMIN. For
>>> better compatibility with older SEPolicy, check ADMIN before NICE.
>> But with this patch you in turn punish the new/better policies that
>> try to avoid giving domains CAP_SYS_ADMIN unless necessary (using only
>> the more granular capabilities wherever possible), which may now get a
>> bogus sys_admin denial. IMHO the order is better as it is, as it
>> motivates the "good" policy writing behavior - i.e. spelling out the
>> capability permissions more explicitly and avoiding CAP_SYS_ADMIN.
>>
>> IOW, if you domain does CAP_SYS_NICE things, and you didn't explicitly
>> grant it that (and instead rely on the CAP_SYS_ADMIN fallback), then
>> the denial correctly flags it as an issue in your policy and
>> encourages you to add that sys_nice permission to the domain. Then
>> when one beautiful hypothetical day the CAP_SYS_ADMIN fallback is
>> removed, your policy will be ready for that and things will keep
>> working.
>>
>> Feel free to carry that patch downstream if patching the kernel is
>> easier for you than fixing the policy, but for the upstream kernel
>> this is just a step in the wrong direction.
> I'm personally fine with this position, but I am curious why "never
> break userspace" doesn't apply to SELinux policies.

Because SELinux policy is configuration data, not system code.
One is free to modify SELinux policy to suit one's whims without
making any change to the Linux kernel or its APIs.

>   At the end of the
> day, booting 5.13 or older, we don't get a denial, and there's nothing
> for the sysadmin to do. On 5.14 and newer, we get denials. This is a
> common pattern we see each year: some new capability or permission is
> required where it wasn't required before, and there's no compatibility
> mechanism to grandfather in old policies.

This is an artifact of separating policy from mechanism. The
capability mechanism does not suffer from this issue because
it embodies its policy. SELinux, Smack, AppArmor and "containers"
are vulnerable to it because they explicitly deny the kernel and
kernel developers permission to make assumptions about how they
define "policy".

>   So, we have to touch
> userspace. If this is just how things are, I can certainly update our
> init.te definitions.

If SELinux was a required kernel mechanism and the policy was
included in the kernel tree you might be able to argue that
kernel developers are responsible for changes to SELinux policy.
But it ain't, and it isn't.   By design.

>
>>> Fixes: 9d3a39a5f1e4 ("block: grant IOPRIO_CLASS_RT to CAP_SYS_NICE")
>>> Signed-off-by: Alistair Delva <adelva@google.com>
>>> Cc: Khazhismel Kumykov <khazhy@google.com>
>>> Cc: Bart Van Assche <bvanassche@acm.org>
>>> Cc: Serge Hallyn <serge@hallyn.com>
>>> Cc: Jens Axboe <axboe@kernel.dk>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Paul Moore <paul@paul-moore.com>
>>> Cc: selinux@vger.kernel.org
>>> Cc: linux-security-module@vger.kernel.org
>>> Cc: kernel-team@android.com
>>> Cc: stable@vger.kernel.org # v5.14+
>>> ---
>>>   block/ioprio.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/block/ioprio.c b/block/ioprio.c
>>> index 0e4ff245f2bf..4d59c559e057 100644
>>> --- a/block/ioprio.c
>>> +++ b/block/ioprio.c
>>> @@ -69,7 +69,7 @@ int ioprio_check_cap(int ioprio)
>>>
>>>          switch (class) {
>>>                  case IOPRIO_CLASS_RT:
>>> -                       if (!capable(CAP_SYS_NICE) && !capable(CAP_SYS_ADMIN))
>>> +                       if (!capable(CAP_SYS_ADMIN) && !capable(CAP_SYS_NICE))
>>>                                  return -EPERM;
>>>                          fallthrough;
>>>                          /* rt has prio field too */
>>> --
>>> 2.34.0.rc1.387.gb447b232ab-goog
>>>
>> --
>> Ondrej Mosnacek
>> Software Engineer, Linux Security - SELinux kernel
>> Red Hat, Inc.
>>