| Message ID | 20170423072457.27120-1-matt@nmatt.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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?"
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().
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 */
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(+)