| Message ID | 20170423235417.12809-2-matt@nmatt.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Matt Brown (matt@nmatt.com): > 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 | 4 ++++ > include/linux/tty.h | 2 ++ > 2 files changed, 6 insertions(+) > > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > index e6d1a65..03d5ea2 100644 > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -161,6 +161,7 @@ static void release_tty(struct tty_struct *tty, int idx); > * @tty: tty struct to free > * > * Free the write buffers, tty queue and tty memory itself. > + * Decrement the owner_user_ns count. > * > * Locking: none. Must be called after tty is definitely unused > */ > @@ -171,6 +172,7 @@ static void free_tty_struct(struct tty_struct *tty) > put_device(tty->dev); > kfree(tty->write_buf); > tty->magic = 0xDEADDEAD; > + atomic_dec(&tty->owner_user_ns->count); > kfree(tty); > } > > @@ -3191,6 +3193,8 @@ 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(); > + atomic_inc(&tty->owner_user_ns->count); Hi, CONFIG_USER_NS is an option, so unfortunately you need to do tty->owner_user_ns = get_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 */ > -- > 2.10.2
On 04/23/2017 09:09 PM, Serge E. Hallyn wrote: > Quoting Matt Brown (matt@nmatt.com): >> 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 | 4 ++++ >> include/linux/tty.h | 2 ++ >> 2 files changed, 6 insertions(+) >> >> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c >> index e6d1a65..03d5ea2 100644 >> --- a/drivers/tty/tty_io.c >> +++ b/drivers/tty/tty_io.c >> @@ -161,6 +161,7 @@ static void release_tty(struct tty_struct *tty, int idx); >> * @tty: tty struct to free >> * >> * Free the write buffers, tty queue and tty memory itself. >> + * Decrement the owner_user_ns count. >> * >> * Locking: none. Must be called after tty is definitely unused >> */ >> @@ -171,6 +172,7 @@ static void free_tty_struct(struct tty_struct *tty) >> put_device(tty->dev); >> kfree(tty->write_buf); >> tty->magic = 0xDEADDEAD; >> + atomic_dec(&tty->owner_user_ns->count); >> kfree(tty); >> } >> >> @@ -3191,6 +3193,8 @@ 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(); >> + atomic_inc(&tty->owner_user_ns->count); > > Hi, > > CONFIG_USER_NS is an option, so unfortunately you need to do > > tty->owner_user_ns = get_user_ns(current_user_ns()); > Does this mean I need to also use put_user_ns(tty->owner_user_ns) in free_tty_struct instead of atomic_dec(&tty->owner_user_ns->count) ? >> >> 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 */ >> -- >> 2.10.2
Quoting Matt Brown (matt@nmatt.com): > On 04/23/2017 09:09 PM, Serge E. Hallyn wrote: > >Quoting Matt Brown (matt@nmatt.com): > >>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 | 4 ++++ > >> include/linux/tty.h | 2 ++ > >> 2 files changed, 6 insertions(+) > >> > >>diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > >>index e6d1a65..03d5ea2 100644 > >>--- a/drivers/tty/tty_io.c > >>+++ b/drivers/tty/tty_io.c > >>@@ -161,6 +161,7 @@ static void release_tty(struct tty_struct *tty, int idx); > >> * @tty: tty struct to free > >> * > >> * Free the write buffers, tty queue and tty memory itself. > >>+ * Decrement the owner_user_ns count. > >> * > >> * Locking: none. Must be called after tty is definitely unused > >> */ > >>@@ -171,6 +172,7 @@ static void free_tty_struct(struct tty_struct *tty) > >> put_device(tty->dev); > >> kfree(tty->write_buf); > >> tty->magic = 0xDEADDEAD; > >>+ atomic_dec(&tty->owner_user_ns->count); > >> kfree(tty); > >> } > >> > >>@@ -3191,6 +3193,8 @@ 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(); > >>+ atomic_inc(&tty->owner_user_ns->count); > > > >Hi, > > > >CONFIG_USER_NS is an option, so unfortunately you need to do > > > > tty->owner_user_ns = get_user_ns(current_user_ns()); > > > > Does this mean I need to also use put_user_ns(tty->owner_user_ns) in > free_tty_struct instead of atomic_dec(&tty->owner_user_ns->count) ? Yes, and since in theory the tty could outlive all the tasks in the user namespace it needs to be put_user_ns() anyway so as to free it on last put.
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index e6d1a65..03d5ea2 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -161,6 +161,7 @@ static void release_tty(struct tty_struct *tty, int idx); * @tty: tty struct to free * * Free the write buffers, tty queue and tty memory itself. + * Decrement the owner_user_ns count. * * Locking: none. Must be called after tty is definitely unused */ @@ -171,6 +172,7 @@ static void free_tty_struct(struct tty_struct *tty) put_device(tty->dev); kfree(tty->write_buf); tty->magic = 0xDEADDEAD; + atomic_dec(&tty->owner_user_ns->count); kfree(tty); } @@ -3191,6 +3193,8 @@ 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(); + atomic_inc(&tty->owner_user_ns->count); 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 | 4 ++++ include/linux/tty.h | 2 ++ 2 files changed, 6 insertions(+)