linux-kernel - Re: [PATCH v2 1/2] tiocsti-restrict : Add owner user namespace to tty

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] [day] [month] [year] [list]

Message-ID: <CAG48ez1r_ORVvCFy5F5k=uU7LnLEez8o7oZrx6B4A8D6w9Jm3A@mail.gmail.com>
Date:   Sun, 23 Apr 2017 22:53:16 +0200
From:   Jann Horn <jannh@...gle.com>
To:     Matt Brown <matt@...tt.com>
Cc:     serge@...lyn.com, jmorris@...ei.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        jslaby@...e.com, corbet@....net, Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        kernel-hardening@...ts.openwall.com,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH v2 1/2] tiocsti-restrict : Add owner user namespace to tty_struct

On Sun, Apr 23, 2017 at 10:23 PM, Matt Brown <matt@...tt.com> wrote:
> On 04/23/2017 01:02 PM, Jann Horn wrote:
>>
>> On Sun, Apr 23, 2017 at 9:24 AM, Matt Brown <matt@...tt.com> wrote:
>>>
>>> This patch adds struct user_namespace *owner_user_ns to the tty_struct.
>>> Then it is set to current_user_ns() in the alloc_tty_struct function.
>>>
>>> This is done to facilitate capability checks against the original user
>>> namespace that allocated the tty.
>>>
>>> E.g. ns_capable(tty->owner_user_ns,CAP_SYS_ADMIN)
>>>
>>> This combined with the use of user namespace's will allow hardening
>>> protections to be built to mitigate container escapes that utilize TTY
>>> ioctls such as TIOCSTI.
>>>
>>> See: https://bugzilla.redhat.com/show_bug.cgi?id=1411256
>>>
>>> Signed-off-by: Matt Brown <matt@...tt.com>
>>> ---
>>>  drivers/tty/tty_io.c | 1 +
>>>  include/linux/tty.h  | 2 ++
>>>  2 files changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>> index e6d1a65..e774385 100644
>>> --- a/drivers/tty/tty_io.c
>>> +++ b/drivers/tty/tty_io.c
>>> @@ -3191,6 +3191,7 @@ struct tty_struct *alloc_tty_struct(struct
>>> tty_driver *driver, int idx)
>>>         tty->index = idx;
>>>         tty_line_name(driver, idx, tty->name);
>>>         tty->dev = tty_get_device(tty);
>>> +       tty->owner_user_ns = current_user_ns();
>>
>>
>> Why are you not taking a reference to the userns?
>>
>
> current_user_ns() returns the user namespace of the current task. What
> do you mean "not taking a reference to the userns?"

For managing the lifetime of various object types, including
struct user_namespace, the kernel uses reference counting.
You can see that struct user_namespace has a member named
"count" of type atomic_t. This member tracks how many
references to a user namespace exist in the kernel, not counting
some types of temporary references.
Basically, when this counter reaches zero, the kernel assumes that
nothing uses the namespace anymore and frees the
struct user_namespace. Attempting to use the struct user_namespace
after that would cause a use-after-free bug.

This means that when you store a reference to a
user_namespace in a tty_struct, you need to increment the "count"
member of the user_namespace ("take a reference"), and when the
tty_struct is released, you need to decrement the "count" member of
the user_namespace back down ("drop a reference"). To change the
count, you should use the helper functions get_user_ns() and
put_user_ns().