Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <e3604476-5255-109e-994f-01e09b5340c0@linux.ibm.com>
Date:   Wed, 19 Jan 2022 07:45:22 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v8 19/19] ima: Enable IMA namespaces
Content-Language: en-US
To:     Christian Brauner <brauner@kernel.org>
Cc:     Stefan Berger <stefanb@linux.vnet.ibm.com>,
        linux-integrity@vger.kernel.org, zohar@linux.ibm.com,
        serge@hallyn.com, christian.brauner@ubuntu.com,
        containers@lists.linux.dev, dmitry.kasatkin@gmail.com,
        ebiederm@xmission.com, krzysztof.struczynski@huawei.com,
        roberto.sassu@huawei.com, mpeters@redhat.com, lhinds@redhat.com,
        lsturman@redhat.com, puiterwi@redhat.com, jejb@linux.ibm.com,
        jamjoom@us.ibm.com, linux-kernel@vger.kernel.org,
        paul@paul-moore.com, rgb@redhat.com,
        linux-security-module@vger.kernel.org, jmorris@namei.org
References: <20220104170416.1923685-1-stefanb@linux.vnet.ibm.com>
 <20220104170416.1923685-20-stefanb@linux.vnet.ibm.com>
 <20220114144515.vbler7ae3jqebhec@wittgenstein>
 <8f7e0bcc-cd7c-723d-c544-300b5e8f3873@linux.ibm.com>
 <20220119094613.cxxxmz5qbuehd7c3@wittgenstein>
From:   Stefan Berger <stefanb@linux.ibm.com>
In-Reply-To: <20220119094613.cxxxmz5qbuehd7c3@wittgenstein>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/19/22 04:46, Christian Brauner wrote:
> On Tue, Jan 18, 2022 at 01:09:12PM -0500, Stefan Berger wrote:
>> On 1/14/22 09:45, Christian Brauner wrote:
>>> On Tue, Jan 04, 2022 at 12:04:16PM -0500, Stefan Berger wrote:
>>>> From: Stefan Berger <stefanb@linux.ibm.com>
>>>>
>>>> Introduce the IMA_NS in Kconfig for IMA namespace enablement.
>>>>
>>>> Enable the lazy initialization of an IMA namespace when a user mounts
>>>> SecurityFS. Now a user_namespace will get a pointer to an ima_namespace
>>>> and therefore add an implementation of get_current_ns() that returns this
>>>> pointer.
>>>>
>>>> get_current_ns() may now return a NULL pointer for as long as the IMA
>>>> namespace hasn't been created, yet. Therefore, return early from those
>>>> functions that may now get a NULL pointer from this call. The NULL
>>>> pointer can typically be treated similar to not having an IMA policy set
>>>> and simply return early from a function.
>>>>
>>>> Implement ima_ns_from_file() for SecurityFS-related files where we can
>>>> now get the IMA namespace via the user namespace pointer associated
>>>> with the superblock of the SecurityFS filesystem instance. Since
>>>> the functions using ima_ns_from_file() will only be called after an
>>>> ima_namesapce has been allocated they will never get a NULL pointer
>>>> for the ima_namespace.
>>>>
>>>> Switch access to userns->ima_ns to use acquire/release semantics to ensure
>>>> that a newly created ima_namespace structure is fully visible upon access.
>>>>
>>>> Replace usage of current_user_ns() with ima_ns_from_user_ns() that
>>>> implements a method to derive the user_namespace from the given
>>>> ima_namespace. It leads to the same result.
>>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> ---
>> [...]
>>>> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
>>>> index b7dbc687b6ff..5a9b511ebbae 100644
>>>> --- a/security/integrity/ima/ima_policy.c
>>>> +++ b/security/integrity/ima/ima_policy.c
>>>> @@ -1333,6 +1333,7 @@ static unsigned int ima_parse_appraise_algos(char *arg)
>>>>    static int ima_parse_rule(struct ima_namespace *ns,
>>>>    			  char *rule, struct ima_rule_entry *entry)
>>>>    {
>>>> +	struct user_namespace *user_ns = ima_ns_to_user_ns(ns);
>>> So I think ima_policy_write() and therefore ima_parse_rule() can
>>> legitimately be reached at least from an ancestor userns but also from a
>>> completely unrelated userns via securityfs. Sorry, I didn't see this
>>> earlier. Think of the following two scenarios:
>>>
>>> * userns1: unshare -U --map-root --mount
>>> -----------------------------------------
>>>      mount -t securityfs securityfs /userns1_securityfs
>>>      fd_in_userns1 = open("/userns1_securityfs/ima_file, O_RDWR);
>>>
>>>      /* I _think_ that sending of fds here should work but I haven't
>>>       * bothered to recheck the scm code as I need to do some driving in a
>>>       * little bit so I'm running out of time...
>>>       */
>>>      send_fd_scm_rights(fd_in_userns1, task_in_userns2);
>>>
>>> * userns2: unshare -U --map-root --mount
>>> -----------------------------------------
>>>      fd_from_userns1 = receive_fd_scm_rights();
>>>      write_policy(fd_from_userns1, "my fancy policy");
>> Passing an fd around like this presumably indicates that you intend to let
>> the recipient read/write to it.
> Yes.
>
>>
>>> It also means that if you inherit an fd from a more privileged imans
>>> instance you can write to it:
>> Now in this example we have to assume that root is making a mistake passing
>> the file descriptor around?
>>
>> # ls -l /sys/kernel/security/ima/
>> total 0
>> -r--r-----. 1 root root 0 Jan 18 12:48 ascii_runtime_measurements
>> -r--r-----. 1 root root 0 Jan 18 12:48 binary_runtime_measurements
>> -rw-------. 1 root root 0 Jan 18 12:48 policy
>> -r--r-----. 1 root root 0 Jan 18 12:48 runtime_measurements_count
>> -r--r-----. 1 root root 0 Jan 18 12:48 violations
>>
>>> * initial_userns:
>>
>> So that's the host, right? And this is a 2nd independent example from the
>> first.
> Yes, these are just two examples to give a more complete idea of the
> semantics by specifying two cases and how ima would behave.
>
>>> ------------------
>>>      mount -t securityfs securityfs /initial_securityfs
>>>
>>>      fd_in_initial_securityfs = open("/initial_securityfs/ima_file, O_RDWR);
>>>
>>>      pid = fork():
>>>      if (pid == 0) {
>>> 	unshare(CLONE_NEWUSER);
>>> 	/* write idmapping for yourself */
>>>
>>> 	write_policy(fd_in_initial_securityfs, "my fancy policy");
>>>      }
>>>
>>> would allow an unprivileged caller to alter the host's ima policy (as
>>> you can see the example requires cooperation).
>> Sorry, not currently following. Root is the only one being able to open that
>> IMA files on the host, right? Is this a mistake here where root passed the
> Yes.
>
>> fd onto the child and that child is not trusted to mess with the fd
>> including passing it on further?
> This is just an example what the current semantics mean in practice.
> The above code snippet is neither good nor bad by itself as that depends
> on context:
>
> 1) Let's say for whatever reason you would like to let unprivileged
>     containers add policy rules (sorry in case I'm using the wrong ima
>     vernacular) for themselves to the initial ima namespace during
>     startup. That can be a rather valid and important use-case. Then this
>     code snipped above where root opens a policy fd in the host
>     securityfs instance and then let's the container access it across
>     fork() + post namespace creation is "good" as it will allow the
>     container to write the rules during setup while e.g. letting the
>     container manager process (the code prior to fork) continue doing
>     other stuff.

I would agree to supporting passing the fd to other containers to have 
them add rules to the policy, if that's what is intended.


>
> 2) If you only want to ever allow container manager on the host write
>     rules for the container in the initial ima ns but never the container
>     setup process itself then the above code is "bad". The policy fd
>     should've been closed before the fork() and definitely be opened
>     o-cloexec.

I would treat the IMA files' file descriptors like those of fd = 
open("/top/secret/file", O_RDWR) assuming the programmer knows the 
implications of passing the fd around, including knowing that open fds 
are inherited by child processes... I don't see that there's anything 
wrong with that.


>
> The examples really were just trying to make obvious what the semantics
> are that you're buying.