diff mbox

[v2,1/2] tiocsti-restrict : Add owner user namespace to tty_struct

Message ID 20170423072457.27120-1-matt@nmatt.com (mailing list archive)
State New, archived
Headers show

Commit Message

Matt Brown April 23, 2017, 7:24 a.m. UTC
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@nmatt.com>
---
 drivers/tty/tty_io.c | 1 +
 include/linux/tty.h  | 2 ++
 2 files changed, 3 insertions(+)

Comments

Jann Horn April 23, 2017, 5:02 p.m. UTC | #1
On Sun, Apr 23, 2017 at 9:24 AM, Matt Brown <matt@nmatt.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@nmatt.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?
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Matt Brown April 23, 2017, 8:23 p.m. UTC | #2
On 04/23/2017 01:02 PM, Jann Horn wrote:
> On Sun, Apr 23, 2017 at 9:24 AM, Matt Brown <matt@nmatt.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@nmatt.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?"
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jann Horn April 23, 2017, 8:53 p.m. UTC | #3
On Sun, Apr 23, 2017 at 10:23 PM, Matt Brown <matt@nmatt.com> wrote:
> On 04/23/2017 01:02 PM, Jann Horn wrote:
>>
>> On Sun, Apr 23, 2017 at 9:24 AM, Matt Brown <matt@nmatt.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@nmatt.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().
--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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();
 
 	return tty;
 }
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 1017e904..d902d42 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -12,6 +12,7 @@ 
 #include <uapi/linux/tty.h>
 #include <linux/rwsem.h>
 #include <linux/llist.h>
+#include <linux/user_namespace.h>
 
 
 /*
@@ -333,6 +334,7 @@  struct tty_struct {
 	/* If the tty has a pending do_SAK, queue it here - akpm */
 	struct work_struct SAK_work;
 	struct tty_port *port;
+	struct user_namespace *owner_user_ns;
 };
 
 /* Each of a tty's open files has private_data pointing to tty_file_private */