| Message ID | 20170419034526.18565-1-matt@nmatt.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: > This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity > project in-kernel. > > This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding > sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI > ioctl calls from non CAP_SYS_ADMIN users. > > Possible effects on userland: > > There could be a few user programs that would be effected by this > change. > See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> > notable programs are: agetty, csh, xemacs and tcsh > > However, I still believe that this change is worth it given that the > Kconfig defaults to n. This will be a feature that is turned on for the It's not worthless, but note that for instance before this was fixed in lxc, this patch would not have helped with escapes from privileged containers. > same reason that people activate it when using grsecurity. Users of this > opt-in feature will realize that they are choosing security over some OS > features like unprivileged TIOCSTI ioctls, as should be clear in the > Kconfig help message. > > Threat Model/Patch Rational: > > >From grsecurity's config for GRKERNSEC_HARDEN_TTY. > > | There are very few legitimate uses for this functionality and it > | has made vulnerabilities in several 'su'-like programs possible in > | the past. Even without these vulnerabilities, it provides an > | attacker with an easy mechanism to move laterally among other > | processes within the same user's compromised session. > > So if one process within a tty session becomes compromised it can follow > that additional processes, that are thought to be in different security > boundaries, can be compromised as a result. When using a program like su > or sudo, these additional processes could be in a tty session where TTY file > descriptors are indeed shared over privilege boundaries. > > This is also an excellent writeup about the issue: > <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> > > Signed-off-by: Matt Brown <matt@nmatt.com> > --- > drivers/tty/tty_io.c | 4 ++++ > include/linux/tty.h | 2 ++ > kernel/sysctl.c | 12 ++++++++++++ > security/Kconfig | 13 +++++++++++++ > 4 files changed, 31 insertions(+) > > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > index e6d1a65..31894e8 100644 > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) > * FIXME: may race normal receive processing > */ > > +int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); > + > static int tiocsti(struct tty_struct *tty, char __user *p) > { > char ch, mbz = 0; > struct tty_ldisc *ld; > > + if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) > + return -EPERM; > if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) > return -EPERM; > if (get_user(ch, p)) > diff --git a/include/linux/tty.h b/include/linux/tty.h > index 1017e904..7011102 100644 > --- a/include/linux/tty.h > +++ b/include/linux/tty.h > @@ -342,6 +342,8 @@ struct tty_file_private { > struct list_head list; > }; > > +extern int tiocsti_restrict; > + > /* tty magic number */ > #define TTY_MAGIC 0x5401 > > diff --git a/kernel/sysctl.c b/kernel/sysctl.c > index acf0a5a..68d1363 100644 > --- a/kernel/sysctl.c > +++ b/kernel/sysctl.c > @@ -67,6 +67,7 @@ > #include <linux/kexec.h> > #include <linux/bpf.h> > #include <linux/mount.h> > +#include <linux/tty.h> > > #include <linux/uaccess.h> > #include <asm/processor.h> > @@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { > .extra2 = &two, > }, > #endif > +#if defined CONFIG_TTY > + { > + .procname = "tiocsti_restrict", > + .data = &tiocsti_restrict, > + .maxlen = sizeof(int), > + .mode = 0644, > + .proc_handler = proc_dointvec_minmax_sysadmin, > + .extra1 = &zero, > + .extra2 = &one, > + }, > +#endif > { > .procname = "ngroups_max", > .data = &ngroups_max, > diff --git a/security/Kconfig b/security/Kconfig > index 3ff1bf9..7d13331 100644 > --- a/security/Kconfig > +++ b/security/Kconfig > @@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT > > If you are unsure how to answer this question, answer N. > > +config SECURITY_TIOCSTI_RESTRICT This is an odd way to name this. Shouldn't the name reflect that it is setting the default, rather than enabling the feature? Besides that, I'm ok with the patch. > + bool "Restrict unprivileged use of tiocsti command injection" > + default n > + help > + This enforces restrictions on unprivileged users injecting commands > + into other processes which share a tty session using the TIOCSTI > + ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. > + > + If this option is not selected, no restrictions will be enforced > + unless the tiocsti_restrict sysctl is explicitly set to (1). > + > + If you are unsure how to answer this question, answer N. > + > config SECURITY > bool "Enable different security models" > depends on SYSFS > -- > 2.10.2
On Tue, Apr 18, 2017 at 9:58 PM, Serge E. Hallyn <serge@hallyn.com> wrote: > On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: >> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >> project in-kernel. >> >> This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding >> sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI >> ioctl calls from non CAP_SYS_ADMIN users. >> >> Possible effects on userland: >> >> There could be a few user programs that would be effected by this >> change. >> See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> >> notable programs are: agetty, csh, xemacs and tcsh >> >> However, I still believe that this change is worth it given that the >> Kconfig defaults to n. This will be a feature that is turned on for the > > It's not worthless, but note that for instance before this was fixed > in lxc, this patch would not have helped with escapes from privileged > containers. > >> same reason that people activate it when using grsecurity. Users of this >> opt-in feature will realize that they are choosing security over some OS >> features like unprivileged TIOCSTI ioctls, as should be clear in the >> Kconfig help message. >> >> Threat Model/Patch Rational: >> >> >From grsecurity's config for GRKERNSEC_HARDEN_TTY. >> >> | There are very few legitimate uses for this functionality and it >> | has made vulnerabilities in several 'su'-like programs possible in >> | the past. Even without these vulnerabilities, it provides an >> | attacker with an easy mechanism to move laterally among other >> | processes within the same user's compromised session. >> >> So if one process within a tty session becomes compromised it can follow >> that additional processes, that are thought to be in different security >> boundaries, can be compromised as a result. When using a program like su >> or sudo, these additional processes could be in a tty session where TTY file >> descriptors are indeed shared over privilege boundaries. >> >> This is also an excellent writeup about the issue: >> <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> >> >> Signed-off-by: Matt Brown <matt@nmatt.com> Thanks for working on this! I think it'll be nice to have available. >> --- >> drivers/tty/tty_io.c | 4 ++++ >> include/linux/tty.h | 2 ++ >> kernel/sysctl.c | 12 ++++++++++++ >> security/Kconfig | 13 +++++++++++++ >> 4 files changed, 31 insertions(+) >> >> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c >> index e6d1a65..31894e8 100644 >> --- a/drivers/tty/tty_io.c >> +++ b/drivers/tty/tty_io.c >> @@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) >> * FIXME: may race normal receive processing >> */ >> >> +int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); >> + >> static int tiocsti(struct tty_struct *tty, char __user *p) >> { >> char ch, mbz = 0; >> struct tty_ldisc *ld; >> >> + if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) >> + return -EPERM; I wonder if it might be worth adding a pr_warn_ratelimited() here to help people identify either programs that want to use this feature or actual attacks? >> if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >> return -EPERM; >> if (get_user(ch, p)) >> diff --git a/include/linux/tty.h b/include/linux/tty.h >> index 1017e904..7011102 100644 >> --- a/include/linux/tty.h >> +++ b/include/linux/tty.h >> @@ -342,6 +342,8 @@ struct tty_file_private { >> struct list_head list; >> }; >> >> +extern int tiocsti_restrict; >> + >> /* tty magic number */ >> #define TTY_MAGIC 0x5401 >> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index acf0a5a..68d1363 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -67,6 +67,7 @@ >> #include <linux/kexec.h> >> #include <linux/bpf.h> >> #include <linux/mount.h> >> +#include <linux/tty.h> >> >> #include <linux/uaccess.h> >> #include <asm/processor.h> >> @@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { >> .extra2 = &two, >> }, >> #endif >> +#if defined CONFIG_TTY >> + { >> + .procname = "tiocsti_restrict", >> + .data = &tiocsti_restrict, Since this is a new sysctl, it'll need to get documented in Documentation/sysctl/kernel.txt as part of this patch. >> + .maxlen = sizeof(int), >> + .mode = 0644, >> + .proc_handler = proc_dointvec_minmax_sysadmin, >> + .extra1 = &zero, >> + .extra2 = &one, >> + }, >> +#endif >> { >> .procname = "ngroups_max", >> .data = &ngroups_max, >> diff --git a/security/Kconfig b/security/Kconfig >> index 3ff1bf9..7d13331 100644 >> --- a/security/Kconfig >> +++ b/security/Kconfig >> @@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT >> >> If you are unsure how to answer this question, answer N. >> >> +config SECURITY_TIOCSTI_RESTRICT > > This is an odd way to name this. Shouldn't the name reflect that it > is setting the default, rather than enabling the feature? This is modeled after SECURITY_DMESG_RESTRICT. I think the Kconfig name is fine (given the other one), but it'd be worth maybe reorganizing the commit message to say "this introduces tiocsti_restrict sysctl, whose default is controlled via CONFIG_SECURITY_TIOCSTI_RESTRICT" or similar. Right now the commit message doesn't, I don't think, make clear what the config does. > > Besides that, I'm ok with the patch. > >> + bool "Restrict unprivileged use of tiocsti command injection" >> + default n >> + help >> + This enforces restrictions on unprivileged users injecting commands >> + into other processes which share a tty session using the TIOCSTI >> + ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. >> + >> + If this option is not selected, no restrictions will be enforced >> + unless the tiocsti_restrict sysctl is explicitly set to (1). >> + >> + If you are unsure how to answer this question, answer N. >> + >> config SECURITY >> bool "Enable different security models" >> depends on SYSFS >> -- >> 2.10.2 -Kees
On Tue, 18 Apr 2017, Matt Brown wrote: > This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity > project in-kernel. It seems like an ugly hack to an ugly feature (CAP_SYS_ADMIN barely makes sense here), and rather than sprinkling these types of things throughout the kernel, I wonder if it might be better to implement it via LSM, in the YAMA module. - James
On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: > On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: >> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >> project in-kernel. >> >> This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding >> sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI >> ioctl calls from non CAP_SYS_ADMIN users. >> >> Possible effects on userland: >> >> There could be a few user programs that would be effected by this >> change. >> See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> >> notable programs are: agetty, csh, xemacs and tcsh >> >> However, I still believe that this change is worth it given that the >> Kconfig defaults to n. This will be a feature that is turned on for the > > It's not worthless, but note that for instance before this was fixed > in lxc, this patch would not have helped with escapes from privileged > containers. > I assume you are talking about this CVE: https://bugzilla.redhat.com/show_bug.cgi?id=1411256 In retrospect, is there any way that an escape from a privileged container with the this bug could have been prevented? >> same reason that people activate it when using grsecurity. Users of this >> opt-in feature will realize that they are choosing security over some OS >> features like unprivileged TIOCSTI ioctls, as should be clear in the >> Kconfig help message. >> >> Threat Model/Patch Rational: >> >> >From grsecurity's config for GRKERNSEC_HARDEN_TTY. >> >> | There are very few legitimate uses for this functionality and it >> | has made vulnerabilities in several 'su'-like programs possible in >> | the past. Even without these vulnerabilities, it provides an >> | attacker with an easy mechanism to move laterally among other >> | processes within the same user's compromised session. >> >> So if one process within a tty session becomes compromised it can follow >> that additional processes, that are thought to be in different security >> boundaries, can be compromised as a result. When using a program like su >> or sudo, these additional processes could be in a tty session where TTY file >> descriptors are indeed shared over privilege boundaries. >> >> This is also an excellent writeup about the issue: >> <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> >> >> Signed-off-by: Matt Brown <matt@nmatt.com> >> --- >> drivers/tty/tty_io.c | 4 ++++ >> include/linux/tty.h | 2 ++ >> kernel/sysctl.c | 12 ++++++++++++ >> security/Kconfig | 13 +++++++++++++ >> 4 files changed, 31 insertions(+) >> >> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c >> index e6d1a65..31894e8 100644 >> --- a/drivers/tty/tty_io.c >> +++ b/drivers/tty/tty_io.c >> @@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) >> * FIXME: may race normal receive processing >> */ >> >> +int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); >> + >> static int tiocsti(struct tty_struct *tty, char __user *p) >> { >> char ch, mbz = 0; >> struct tty_ldisc *ld; >> >> + if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) >> + return -EPERM; >> if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >> return -EPERM; >> if (get_user(ch, p)) >> diff --git a/include/linux/tty.h b/include/linux/tty.h >> index 1017e904..7011102 100644 >> --- a/include/linux/tty.h >> +++ b/include/linux/tty.h >> @@ -342,6 +342,8 @@ struct tty_file_private { >> struct list_head list; >> }; >> >> +extern int tiocsti_restrict; >> + >> /* tty magic number */ >> #define TTY_MAGIC 0x5401 >> >> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> index acf0a5a..68d1363 100644 >> --- a/kernel/sysctl.c >> +++ b/kernel/sysctl.c >> @@ -67,6 +67,7 @@ >> #include <linux/kexec.h> >> #include <linux/bpf.h> >> #include <linux/mount.h> >> +#include <linux/tty.h> >> >> #include <linux/uaccess.h> >> #include <asm/processor.h> >> @@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { >> .extra2 = &two, >> }, >> #endif >> +#if defined CONFIG_TTY >> + { >> + .procname = "tiocsti_restrict", >> + .data = &tiocsti_restrict, >> + .maxlen = sizeof(int), >> + .mode = 0644, >> + .proc_handler = proc_dointvec_minmax_sysadmin, >> + .extra1 = &zero, >> + .extra2 = &one, >> + }, >> +#endif >> { >> .procname = "ngroups_max", >> .data = &ngroups_max, >> diff --git a/security/Kconfig b/security/Kconfig >> index 3ff1bf9..7d13331 100644 >> --- a/security/Kconfig >> +++ b/security/Kconfig >> @@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT >> >> If you are unsure how to answer this question, answer N. >> >> +config SECURITY_TIOCSTI_RESTRICT > > This is an odd way to name this. Shouldn't the name reflect that it > is setting the default, rather than enabling the feature? > > Besides that, I'm ok with the patch. > >> + bool "Restrict unprivileged use of tiocsti command injection" >> + default n >> + help >> + This enforces restrictions on unprivileged users injecting commands >> + into other processes which share a tty session using the TIOCSTI >> + ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. >> + >> + If this option is not selected, no restrictions will be enforced >> + unless the tiocsti_restrict sysctl is explicitly set to (1). >> + >> + If you are unsure how to answer this question, answer N. >> + >> config SECURITY >> bool "Enable different security models" >> depends on SYSFS >> -- >> 2.10.2
On 04/19/2017 01:20 AM, Kees Cook wrote: > On Tue, Apr 18, 2017 at 9:58 PM, Serge E. Hallyn <serge@hallyn.com> wrote: >> On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: >>> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >>> project in-kernel. >>> >>> This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding >>> sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI >>> ioctl calls from non CAP_SYS_ADMIN users. >>> >>> Possible effects on userland: >>> >>> There could be a few user programs that would be effected by this >>> change. >>> See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> >>> notable programs are: agetty, csh, xemacs and tcsh >>> >>> However, I still believe that this change is worth it given that the >>> Kconfig defaults to n. This will be a feature that is turned on for the >> >> It's not worthless, but note that for instance before this was fixed >> in lxc, this patch would not have helped with escapes from privileged >> containers. >> >>> same reason that people activate it when using grsecurity. Users of this >>> opt-in feature will realize that they are choosing security over some OS >>> features like unprivileged TIOCSTI ioctls, as should be clear in the >>> Kconfig help message. >>> >>> Threat Model/Patch Rational: >>> >>> >From grsecurity's config for GRKERNSEC_HARDEN_TTY. >>> >>> | There are very few legitimate uses for this functionality and it >>> | has made vulnerabilities in several 'su'-like programs possible in >>> | the past. Even without these vulnerabilities, it provides an >>> | attacker with an easy mechanism to move laterally among other >>> | processes within the same user's compromised session. >>> >>> So if one process within a tty session becomes compromised it can follow >>> that additional processes, that are thought to be in different security >>> boundaries, can be compromised as a result. When using a program like su >>> or sudo, these additional processes could be in a tty session where TTY file >>> descriptors are indeed shared over privilege boundaries. >>> >>> This is also an excellent writeup about the issue: >>> <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> >>> >>> Signed-off-by: Matt Brown <matt@nmatt.com> > > Thanks for working on this! I think it'll be nice to have available. > >>> --- >>> drivers/tty/tty_io.c | 4 ++++ >>> include/linux/tty.h | 2 ++ >>> kernel/sysctl.c | 12 ++++++++++++ >>> security/Kconfig | 13 +++++++++++++ >>> 4 files changed, 31 insertions(+) >>> >>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c >>> index e6d1a65..31894e8 100644 >>> --- a/drivers/tty/tty_io.c >>> +++ b/drivers/tty/tty_io.c >>> @@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) >>> * FIXME: may race normal receive processing >>> */ >>> >>> +int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); >>> + >>> static int tiocsti(struct tty_struct *tty, char __user *p) >>> { >>> char ch, mbz = 0; >>> struct tty_ldisc *ld; >>> >>> + if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) >>> + return -EPERM; > > I wonder if it might be worth adding a pr_warn_ratelimited() here to > help people identify either programs that want to use this feature or > actual attacks? > I can include that in the next version of the patch. Any suggestions on what the warning ought to say? >>> if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >>> return -EPERM; >>> if (get_user(ch, p)) >>> diff --git a/include/linux/tty.h b/include/linux/tty.h >>> index 1017e904..7011102 100644 >>> --- a/include/linux/tty.h >>> +++ b/include/linux/tty.h >>> @@ -342,6 +342,8 @@ struct tty_file_private { >>> struct list_head list; >>> }; >>> >>> +extern int tiocsti_restrict; >>> + >>> /* tty magic number */ >>> #define TTY_MAGIC 0x5401 >>> >>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >>> index acf0a5a..68d1363 100644 >>> --- a/kernel/sysctl.c >>> +++ b/kernel/sysctl.c >>> @@ -67,6 +67,7 @@ >>> #include <linux/kexec.h> >>> #include <linux/bpf.h> >>> #include <linux/mount.h> >>> +#include <linux/tty.h> >>> >>> #include <linux/uaccess.h> >>> #include <asm/processor.h> >>> @@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { >>> .extra2 = &two, >>> }, >>> #endif >>> +#if defined CONFIG_TTY >>> + { >>> + .procname = "tiocsti_restrict", >>> + .data = &tiocsti_restrict, > > Since this is a new sysctl, it'll need to get documented in > Documentation/sysctl/kernel.txt as part of this patch. Will add in next patch version. > >>> + .maxlen = sizeof(int), >>> + .mode = 0644, >>> + .proc_handler = proc_dointvec_minmax_sysadmin, >>> + .extra1 = &zero, >>> + .extra2 = &one, >>> + }, >>> +#endif >>> { >>> .procname = "ngroups_max", >>> .data = &ngroups_max, >>> diff --git a/security/Kconfig b/security/Kconfig >>> index 3ff1bf9..7d13331 100644 >>> --- a/security/Kconfig >>> +++ b/security/Kconfig >>> @@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT >>> >>> If you are unsure how to answer this question, answer N. >>> >>> +config SECURITY_TIOCSTI_RESTRICT >> >> This is an odd way to name this. Shouldn't the name reflect that it >> is setting the default, rather than enabling the feature? > > This is modeled after SECURITY_DMESG_RESTRICT. I think the Kconfig > name is fine (given the other one), but it'd be worth maybe > reorganizing the commit message to say "this introduces > tiocsti_restrict sysctl, whose default is controlled via > CONFIG_SECURITY_TIOCSTI_RESTRICT" or similar. Right now the commit > message doesn't, I don't think, make clear what the config does. > I will reorganize the commit message as you suggested. As for the Kconfig name, I'm open to calling it something else. However, I thought basing it off of SECURITY_DMESG_RESTRICT made sense. >> >> Besides that, I'm ok with the patch. >> >>> + bool "Restrict unprivileged use of tiocsti command injection" >>> + default n >>> + help >>> + This enforces restrictions on unprivileged users injecting commands >>> + into other processes which share a tty session using the TIOCSTI >>> + ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. >>> + >>> + If this option is not selected, no restrictions will be enforced >>> + unless the tiocsti_restrict sysctl is explicitly set to (1). >>> + >>> + If you are unsure how to answer this question, answer N. >>> + >>> config SECURITY >>> bool "Enable different security models" >>> depends on SYSFS >>> -- >>> 2.10.2 > > -Kees >
Quoting Matt Brown (matt@nmatt.com): > On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: > >On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: > >>This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity > >>project in-kernel. > >> > >>This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding > >>sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI > >>ioctl calls from non CAP_SYS_ADMIN users. > >> > >>Possible effects on userland: > >> > >>There could be a few user programs that would be effected by this > >>change. > >>See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> > >>notable programs are: agetty, csh, xemacs and tcsh > >> > >>However, I still believe that this change is worth it given that the > >>Kconfig defaults to n. This will be a feature that is turned on for the > > > >It's not worthless, but note that for instance before this was fixed > >in lxc, this patch would not have helped with escapes from privileged > >containers. > > > > I assume you are talking about this CVE: > https://bugzilla.redhat.com/show_bug.cgi?id=1411256 > > In retrospect, is there any way that an escape from a privileged > container with the this bug could have been prevented? I don't know, that's what I was probing for. Detecting that the pgrp or session - heck, the pid namespace - has changed would seem like a good indicator that it shouldn't be able to push. > >>same reason that people activate it when using grsecurity. Users of this > >>opt-in feature will realize that they are choosing security over some OS > >>features like unprivileged TIOCSTI ioctls, as should be clear in the > >>Kconfig help message. > >> > >>Threat Model/Patch Rational: > >> > >>>From grsecurity's config for GRKERNSEC_HARDEN_TTY. > >> > >> | There are very few legitimate uses for this functionality and it > >> | has made vulnerabilities in several 'su'-like programs possible in > >> | the past. Even without these vulnerabilities, it provides an > >> | attacker with an easy mechanism to move laterally among other > >> | processes within the same user's compromised session. > >> > >>So if one process within a tty session becomes compromised it can follow > >>that additional processes, that are thought to be in different security > >>boundaries, can be compromised as a result. When using a program like su > >>or sudo, these additional processes could be in a tty session where TTY file > >>descriptors are indeed shared over privilege boundaries. > >> > >>This is also an excellent writeup about the issue: > >><http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> > >> > >>Signed-off-by: Matt Brown <matt@nmatt.com> > >>--- > >> drivers/tty/tty_io.c | 4 ++++ > >> include/linux/tty.h | 2 ++ > >> kernel/sysctl.c | 12 ++++++++++++ > >> security/Kconfig | 13 +++++++++++++ > >> 4 files changed, 31 insertions(+) > >> > >>diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > >>index e6d1a65..31894e8 100644 > >>--- a/drivers/tty/tty_io.c > >>+++ b/drivers/tty/tty_io.c > >>@@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) > >> * FIXME: may race normal receive processing > >> */ > >> > >>+int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); > >>+ > >> static int tiocsti(struct tty_struct *tty, char __user *p) > >> { > >> char ch, mbz = 0; > >> struct tty_ldisc *ld; > >> > >>+ if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) > >>+ return -EPERM; > >> if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) > >> return -EPERM; > >> if (get_user(ch, p)) > >>diff --git a/include/linux/tty.h b/include/linux/tty.h > >>index 1017e904..7011102 100644 > >>--- a/include/linux/tty.h > >>+++ b/include/linux/tty.h > >>@@ -342,6 +342,8 @@ struct tty_file_private { > >> struct list_head list; > >> }; > >> > >>+extern int tiocsti_restrict; > >>+ > >> /* tty magic number */ > >> #define TTY_MAGIC 0x5401 > >> > >>diff --git a/kernel/sysctl.c b/kernel/sysctl.c > >>index acf0a5a..68d1363 100644 > >>--- a/kernel/sysctl.c > >>+++ b/kernel/sysctl.c > >>@@ -67,6 +67,7 @@ > >> #include <linux/kexec.h> > >> #include <linux/bpf.h> > >> #include <linux/mount.h> > >>+#include <linux/tty.h> > >> > >> #include <linux/uaccess.h> > >> #include <asm/processor.h> > >>@@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { > >> .extra2 = &two, > >> }, > >> #endif > >>+#if defined CONFIG_TTY > >>+ { > >>+ .procname = "tiocsti_restrict", > >>+ .data = &tiocsti_restrict, > >>+ .maxlen = sizeof(int), > >>+ .mode = 0644, > >>+ .proc_handler = proc_dointvec_minmax_sysadmin, > >>+ .extra1 = &zero, > >>+ .extra2 = &one, > >>+ }, > >>+#endif > >> { > >> .procname = "ngroups_max", > >> .data = &ngroups_max, > >>diff --git a/security/Kconfig b/security/Kconfig > >>index 3ff1bf9..7d13331 100644 > >>--- a/security/Kconfig > >>+++ b/security/Kconfig > >>@@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT > >> > >> If you are unsure how to answer this question, answer N. > >> > >>+config SECURITY_TIOCSTI_RESTRICT > > > >This is an odd way to name this. Shouldn't the name reflect that it > >is setting the default, rather than enabling the feature? > > > >Besides that, I'm ok with the patch. > > > >>+ bool "Restrict unprivileged use of tiocsti command injection" > >>+ default n > >>+ help > >>+ This enforces restrictions on unprivileged users injecting commands > >>+ into other processes which share a tty session using the TIOCSTI > >>+ ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. > >>+ > >>+ If this option is not selected, no restrictions will be enforced > >>+ unless the tiocsti_restrict sysctl is explicitly set to (1). > >>+ > >>+ If you are unsure how to answer this question, answer N. > >>+ > >> config SECURITY > >> bool "Enable different security models" > >> depends on SYSFS > >>-- > >>2.10.2
On 04/19/2017 07:18 AM, James Morris wrote: > On Tue, 18 Apr 2017, Matt Brown wrote: > >> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >> project in-kernel. > > It seems like an ugly hack to an ugly feature (CAP_SYS_ADMIN barely makes > sense here), and rather than sprinkling these types of things throughout > the kernel, I wonder if it might be better to implement it via LSM, in the > YAMA module. > > CAP_SYS_ADMIN is already used in the TIOCSTI TTY code to allow character insertion into TTYs other than the caller's controlling terminal. This is done because different TTYs indicate a security boundary that should only be able to be crossed by a privileged process. This patch would merely extend this security boundary protection to include unprivileged processes from utilizing a common TTY to step across a security boundary. > > - James >
On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: > Quoting Matt Brown (matt@nmatt.com): >> On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: >>> On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: >>>> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >>>> project in-kernel. >>>> >>>> This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding >>>> sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI >>>> ioctl calls from non CAP_SYS_ADMIN users. >>>> >>>> Possible effects on userland: >>>> >>>> There could be a few user programs that would be effected by this >>>> change. >>>> See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> >>>> notable programs are: agetty, csh, xemacs and tcsh >>>> >>>> However, I still believe that this change is worth it given that the >>>> Kconfig defaults to n. This will be a feature that is turned on for the >>> >>> It's not worthless, but note that for instance before this was fixed >>> in lxc, this patch would not have helped with escapes from privileged >>> containers. >>> >> >> I assume you are talking about this CVE: >> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 >> >> In retrospect, is there any way that an escape from a privileged >> container with the this bug could have been prevented? > > I don't know, that's what I was probing for. Detecting that the pgrp > or session - heck, the pid namespace - has changed would seem like a > good indicator that it shouldn't be able to push. > pgrp and session won't do because in the case we are discussing current->signal->tty is the same as tty. This is the current check that is already in place: | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) | return -EPERM; The only thing I could find to detect the tty message coming from a container is as follows: | task_active_pid_ns(current)->level This will be zero when run on the host, but 1 when run inside a container. However this is very much a hack and could probably break some userland stuff where there are multiple levels of namespaces. The real problem is that there are no TTY namespaces. I don't think we can solve this problem for CAP_SYS_ADMIN containers unless we want to introduce a config that allows one to override normal CAP_SYS_ADMIN functionality by denying TIOCSTI ioctls for processes whom task_active_pid_ns(current)->level is equal to 0. In the mean time, I think we can go ahead with this feature to give people the ability to lock down non CAP_SYS_ADMIN containers/processes. >>>> same reason that people activate it when using grsecurity. Users of this >>>> opt-in feature will realize that they are choosing security over some OS >>>> features like unprivileged TIOCSTI ioctls, as should be clear in the >>>> Kconfig help message. >>>> >>>> Threat Model/Patch Rational: >>>> >>>> >From grsecurity's config for GRKERNSEC_HARDEN_TTY. >>>> >>>> | There are very few legitimate uses for this functionality and it >>>> | has made vulnerabilities in several 'su'-like programs possible in >>>> | the past. Even without these vulnerabilities, it provides an >>>> | attacker with an easy mechanism to move laterally among other >>>> | processes within the same user's compromised session. >>>> >>>> So if one process within a tty session becomes compromised it can follow >>>> that additional processes, that are thought to be in different security >>>> boundaries, can be compromised as a result. When using a program like su >>>> or sudo, these additional processes could be in a tty session where TTY file >>>> descriptors are indeed shared over privilege boundaries. >>>> >>>> This is also an excellent writeup about the issue: >>>> <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> >>>> >>>> Signed-off-by: Matt Brown <matt@nmatt.com> >>>> --- >>>> drivers/tty/tty_io.c | 4 ++++ >>>> include/linux/tty.h | 2 ++ >>>> kernel/sysctl.c | 12 ++++++++++++ >>>> security/Kconfig | 13 +++++++++++++ >>>> 4 files changed, 31 insertions(+) >>>> >>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c >>>> index e6d1a65..31894e8 100644 >>>> --- a/drivers/tty/tty_io.c >>>> +++ b/drivers/tty/tty_io.c >>>> @@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) >>>> * FIXME: may race normal receive processing >>>> */ >>>> >>>> +int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); >>>> + >>>> static int tiocsti(struct tty_struct *tty, char __user *p) >>>> { >>>> char ch, mbz = 0; >>>> struct tty_ldisc *ld; >>>> >>>> + if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) >>>> + return -EPERM; >>>> if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >>>> return -EPERM; >>>> if (get_user(ch, p)) >>>> diff --git a/include/linux/tty.h b/include/linux/tty.h >>>> index 1017e904..7011102 100644 >>>> --- a/include/linux/tty.h >>>> +++ b/include/linux/tty.h >>>> @@ -342,6 +342,8 @@ struct tty_file_private { >>>> struct list_head list; >>>> }; >>>> >>>> +extern int tiocsti_restrict; >>>> + >>>> /* tty magic number */ >>>> #define TTY_MAGIC 0x5401 >>>> >>>> diff --git a/kernel/sysctl.c b/kernel/sysctl.c >>>> index acf0a5a..68d1363 100644 >>>> --- a/kernel/sysctl.c >>>> +++ b/kernel/sysctl.c >>>> @@ -67,6 +67,7 @@ >>>> #include <linux/kexec.h> >>>> #include <linux/bpf.h> >>>> #include <linux/mount.h> >>>> +#include <linux/tty.h> >>>> >>>> #include <linux/uaccess.h> >>>> #include <asm/processor.h> >>>> @@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { >>>> .extra2 = &two, >>>> }, >>>> #endif >>>> +#if defined CONFIG_TTY >>>> + { >>>> + .procname = "tiocsti_restrict", >>>> + .data = &tiocsti_restrict, >>>> + .maxlen = sizeof(int), >>>> + .mode = 0644, >>>> + .proc_handler = proc_dointvec_minmax_sysadmin, >>>> + .extra1 = &zero, >>>> + .extra2 = &one, >>>> + }, >>>> +#endif >>>> { >>>> .procname = "ngroups_max", >>>> .data = &ngroups_max, >>>> diff --git a/security/Kconfig b/security/Kconfig >>>> index 3ff1bf9..7d13331 100644 >>>> --- a/security/Kconfig >>>> +++ b/security/Kconfig >>>> @@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT >>>> >>>> If you are unsure how to answer this question, answer N. >>>> >>>> +config SECURITY_TIOCSTI_RESTRICT >>> >>> This is an odd way to name this. Shouldn't the name reflect that it >>> is setting the default, rather than enabling the feature? >>> >>> Besides that, I'm ok with the patch. >>> >>>> + bool "Restrict unprivileged use of tiocsti command injection" >>>> + default n >>>> + help >>>> + This enforces restrictions on unprivileged users injecting commands >>>> + into other processes which share a tty session using the TIOCSTI >>>> + ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. >>>> + >>>> + If this option is not selected, no restrictions will be enforced >>>> + unless the tiocsti_restrict sysctl is explicitly set to (1). >>>> + >>>> + If you are unsure how to answer this question, answer N. >>>> + >>>> config SECURITY >>>> bool "Enable different security models" >>>> depends on SYSFS >>>> -- >>>> 2.10.2
Quoting Matt Brown (matt@nmatt.com): > On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: > >Quoting Matt Brown (matt@nmatt.com): > >>On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: > >>>On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: > >>>>This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity > >>>>project in-kernel. > >>>> > >>>>This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding > >>>>sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI > >>>>ioctl calls from non CAP_SYS_ADMIN users. > >>>> > >>>>Possible effects on userland: > >>>> > >>>>There could be a few user programs that would be effected by this > >>>>change. > >>>>See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> > >>>>notable programs are: agetty, csh, xemacs and tcsh > >>>> > >>>>However, I still believe that this change is worth it given that the > >>>>Kconfig defaults to n. This will be a feature that is turned on for the > >>> > >>>It's not worthless, but note that for instance before this was fixed > >>>in lxc, this patch would not have helped with escapes from privileged > >>>containers. > >>> > >> > >>I assume you are talking about this CVE: > >>https://bugzilla.redhat.com/show_bug.cgi?id=1411256 > >> > >>In retrospect, is there any way that an escape from a privileged > >>container with the this bug could have been prevented? > > > >I don't know, that's what I was probing for. Detecting that the pgrp > >or session - heck, the pid namespace - has changed would seem like a > >good indicator that it shouldn't be able to push. > > > > pgrp and session won't do because in the case we are discussing > current->signal->tty is the same as tty. > > This is the current check that is already in place: > | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) > | return -EPERM; Yeah... > The only thing I could find to detect the tty message coming from a > container is as follows: > | task_active_pid_ns(current)->level > > This will be zero when run on the host, but 1 when run inside a > container. However this is very much a hack and could probably break > some userland stuff where there are multiple levels of namespaces. Yes. This is also however why I don't like the current patch, because capable() will never be true in a container, so nested containers break. What does current->signal->tty->pgrp actually point to? Can we take it to be the pgrp which opened it? Could we check ns_capable(current_pid_ns()->user_ns, CAP_whatever) and get a meaningful answer? > The real problem is that there are no TTY namespaces. I don't think we > can solve this problem for CAP_SYS_ADMIN containers unless we want to > introduce a config that allows one to override normal CAP_SYS_ADMIN > functionality by denying TIOCSTI ioctls for processes whom > task_active_pid_ns(current)->level is equal to 0. > > In the mean time, I think we can go ahead with this feature to give > people the ability to lock down non CAP_SYS_ADMIN containers/processes. > > >>>>same reason that people activate it when using grsecurity. Users of this > >>>>opt-in feature will realize that they are choosing security over some OS > >>>>features like unprivileged TIOCSTI ioctls, as should be clear in the > >>>>Kconfig help message. > >>>> > >>>>Threat Model/Patch Rational: > >>>> > >>>>>From grsecurity's config for GRKERNSEC_HARDEN_TTY. > >>>> > >>>>| There are very few legitimate uses for this functionality and it > >>>>| has made vulnerabilities in several 'su'-like programs possible in > >>>>| the past. Even without these vulnerabilities, it provides an > >>>>| attacker with an easy mechanism to move laterally among other > >>>>| processes within the same user's compromised session. > >>>> > >>>>So if one process within a tty session becomes compromised it can follow > >>>>that additional processes, that are thought to be in different security > >>>>boundaries, can be compromised as a result. When using a program like su > >>>>or sudo, these additional processes could be in a tty session where TTY file > >>>>descriptors are indeed shared over privilege boundaries. > >>>> > >>>>This is also an excellent writeup about the issue: > >>>><http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> > >>>> > >>>>Signed-off-by: Matt Brown <matt@nmatt.com> > >>>>--- > >>>>drivers/tty/tty_io.c | 4 ++++ > >>>>include/linux/tty.h | 2 ++ > >>>>kernel/sysctl.c | 12 ++++++++++++ > >>>>security/Kconfig | 13 +++++++++++++ > >>>>4 files changed, 31 insertions(+) > >>>> > >>>>diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > >>>>index e6d1a65..31894e8 100644 > >>>>--- a/drivers/tty/tty_io.c > >>>>+++ b/drivers/tty/tty_io.c > >>>>@@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) > >>>> * FIXME: may race normal receive processing > >>>> */ > >>>> > >>>>+int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); > >>>>+ > >>>>static int tiocsti(struct tty_struct *tty, char __user *p) > >>>>{ > >>>> char ch, mbz = 0; > >>>> struct tty_ldisc *ld; > >>>> > >>>>+ if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) > >>>>+ return -EPERM; > >>>> if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) > >>>> return -EPERM; > >>>> if (get_user(ch, p)) > >>>>diff --git a/include/linux/tty.h b/include/linux/tty.h > >>>>index 1017e904..7011102 100644 > >>>>--- a/include/linux/tty.h > >>>>+++ b/include/linux/tty.h > >>>>@@ -342,6 +342,8 @@ struct tty_file_private { > >>>> struct list_head list; > >>>>}; > >>>> > >>>>+extern int tiocsti_restrict; > >>>>+ > >>>>/* tty magic number */ > >>>>#define TTY_MAGIC 0x5401 > >>>> > >>>>diff --git a/kernel/sysctl.c b/kernel/sysctl.c > >>>>index acf0a5a..68d1363 100644 > >>>>--- a/kernel/sysctl.c > >>>>+++ b/kernel/sysctl.c > >>>>@@ -67,6 +67,7 @@ > >>>>#include <linux/kexec.h> > >>>>#include <linux/bpf.h> > >>>>#include <linux/mount.h> > >>>>+#include <linux/tty.h> > >>>> > >>>>#include <linux/uaccess.h> > >>>>#include <asm/processor.h> > >>>>@@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { > >>>> .extra2 = &two, > >>>> }, > >>>>#endif > >>>>+#if defined CONFIG_TTY > >>>>+ { > >>>>+ .procname = "tiocsti_restrict", > >>>>+ .data = &tiocsti_restrict, > >>>>+ .maxlen = sizeof(int), > >>>>+ .mode = 0644, > >>>>+ .proc_handler = proc_dointvec_minmax_sysadmin, > >>>>+ .extra1 = &zero, > >>>>+ .extra2 = &one, > >>>>+ }, > >>>>+#endif > >>>> { > >>>> .procname = "ngroups_max", > >>>> .data = &ngroups_max, > >>>>diff --git a/security/Kconfig b/security/Kconfig > >>>>index 3ff1bf9..7d13331 100644 > >>>>--- a/security/Kconfig > >>>>+++ b/security/Kconfig > >>>>@@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT > >>>> > >>>> If you are unsure how to answer this question, answer N. > >>>> > >>>>+config SECURITY_TIOCSTI_RESTRICT > >>> > >>>This is an odd way to name this. Shouldn't the name reflect that it > >>>is setting the default, rather than enabling the feature? > >>> > >>>Besides that, I'm ok with the patch. > >>> > >>>>+ bool "Restrict unprivileged use of tiocsti command injection" > >>>>+ default n > >>>>+ help > >>>>+ This enforces restrictions on unprivileged users injecting commands > >>>>+ into other processes which share a tty session using the TIOCSTI > >>>>+ ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. > >>>>+ > >>>>+ If this option is not selected, no restrictions will be enforced > >>>>+ unless the tiocsti_restrict sysctl is explicitly set to (1). > >>>>+ > >>>>+ If you are unsure how to answer this question, answer N. > >>>>+ > >>>>config SECURITY > >>>> bool "Enable different security models" > >>>> depends on SYSFS > >>>>-- > >>>>2.10.2
Quoting Serge E. Hallyn (serge@hallyn.com): > Quoting Matt Brown (matt@nmatt.com): > > On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: > > >Quoting Matt Brown (matt@nmatt.com): > > >>On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: > > >>>On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: > > >>>>This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity > > >>>>project in-kernel. > > >>>> > > >>>>This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding > > >>>>sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI > > >>>>ioctl calls from non CAP_SYS_ADMIN users. > > >>>> > > >>>>Possible effects on userland: > > >>>> > > >>>>There could be a few user programs that would be effected by this > > >>>>change. > > >>>>See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> > > >>>>notable programs are: agetty, csh, xemacs and tcsh > > >>>> > > >>>>However, I still believe that this change is worth it given that the > > >>>>Kconfig defaults to n. This will be a feature that is turned on for the > > >>> > > >>>It's not worthless, but note that for instance before this was fixed > > >>>in lxc, this patch would not have helped with escapes from privileged > > >>>containers. > > >>> > > >> > > >>I assume you are talking about this CVE: > > >>https://bugzilla.redhat.com/show_bug.cgi?id=1411256 > > >> > > >>In retrospect, is there any way that an escape from a privileged > > >>container with the this bug could have been prevented? > > > > > >I don't know, that's what I was probing for. Detecting that the pgrp > > >or session - heck, the pid namespace - has changed would seem like a > > >good indicator that it shouldn't be able to push. > > > > > > > pgrp and session won't do because in the case we are discussing > > current->signal->tty is the same as tty. > > > > This is the current check that is already in place: > > | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) > > | return -EPERM; > > Yeah... > > > The only thing I could find to detect the tty message coming from a > > container is as follows: > > | task_active_pid_ns(current)->level > > > > This will be zero when run on the host, but 1 when run inside a > > container. However this is very much a hack and could probably break > > some userland stuff where there are multiple levels of namespaces. > > Yes. This is also however why I don't like the current patch, because > capable() will never be true in a container, so nested containers break. > > What does current->signal->tty->pgrp actually point to? Can we take > it to be the pgrp which opened it? Could we check > ns_capable(current_pid_ns()->user_ns, CAP_whatever) and get a meaningful > answer? > Ok I see that's meaningless, you can't get pidns from pid. We could instead add a user_ns *owner to the struct tty and store the user_ns of the task which opened it. It's more invasive, but also more meaningful.
On 2017-04-20 11:19, Serge E. Hallyn wrote: > Quoting Matt Brown (matt@nmatt.com): >> On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: >> >Quoting Matt Brown (matt@nmatt.com): >> >>On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: >> >>>On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: >> >>>>This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >> >>>>project in-kernel. >> >>>> >> >>>>This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding >> >>>>sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI >> >>>>ioctl calls from non CAP_SYS_ADMIN users. >> >>>> >> >>>>Possible effects on userland: >> >>>> >> >>>>There could be a few user programs that would be effected by this >> >>>>change. >> >>>>See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> >> >>>>notable programs are: agetty, csh, xemacs and tcsh >> >>>> >> >>>>However, I still believe that this change is worth it given that the >> >>>>Kconfig defaults to n. This will be a feature that is turned on for the >> >>> >> >>>It's not worthless, but note that for instance before this was fixed >> >>>in lxc, this patch would not have helped with escapes from privileged >> >>>containers. >> >>> >> >> >> >>I assume you are talking about this CVE: >> >>https://bugzilla.redhat.com/show_bug.cgi?id=1411256 >> >> >> >>In retrospect, is there any way that an escape from a privileged >> >>container with the this bug could have been prevented? >> > >> >I don't know, that's what I was probing for. Detecting that the pgrp >> >or session - heck, the pid namespace - has changed would seem like a >> >good indicator that it shouldn't be able to push. >> > >> >> pgrp and session won't do because in the case we are discussing >> current->signal->tty is the same as tty. >> >> This is the current check that is already in place: >> | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >> | return -EPERM; > > Yeah... > >> The only thing I could find to detect the tty message coming from a >> container is as follows: >> | task_active_pid_ns(current)->level >> >> This will be zero when run on the host, but 1 when run inside a >> container. However this is very much a hack and could probably break >> some userland stuff where there are multiple levels of namespaces. > > Yes. This is also however why I don't like the current patch, because > capable() will never be true in a container, so nested containers > break. > What do you mean by "capable() will never be true in a container"? My understanding is that if a container is given CAP_SYS_ADMIN then capable(CAP_SYS_ADMIN) will return true? I agree the hack I mentioned above would be a bad idea because it would break nested containers, but the current patch would not IMO. A better version of the hack could involve a config CONFIG_TIOCSTI_MAX_NS_LEVEL where a check would be performed to ensure that task_active_pid_ns(current)->level is not greater than the config value(an integer that is >= 0) . Again, I think we both would agree that this is not the best solution. The clear downside is that you could have multiple container layers where the desired security boundaries happened to fall at different levels. Just throwing ideas around. > What does current->signal->tty->pgrp actually point to? Can we take > it to be the pgrp which opened it? Could we check > ns_capable(current_pid_ns()->user_ns, CAP_whatever) and get a > meaningful > answer? > >> The real problem is that there are no TTY namespaces. I don't think we >> can solve this problem for CAP_SYS_ADMIN containers unless we want to >> introduce a config that allows one to override normal CAP_SYS_ADMIN >> functionality by denying TIOCSTI ioctls for processes whom >> task_active_pid_ns(current)->level is equal to 0. >> >> In the mean time, I think we can go ahead with this feature to give >> people the ability to lock down non CAP_SYS_ADMIN >> containers/processes. >> >> >>>>same reason that people activate it when using grsecurity. Users of this >> >>>>opt-in feature will realize that they are choosing security over some OS >> >>>>features like unprivileged TIOCSTI ioctls, as should be clear in the >> >>>>Kconfig help message. >> >>>> >> >>>>Threat Model/Patch Rational: >> >>>> >> >>>>>From grsecurity's config for GRKERNSEC_HARDEN_TTY. >> >>>> >> >>>>| There are very few legitimate uses for this functionality and it >> >>>>| has made vulnerabilities in several 'su'-like programs possible in >> >>>>| the past. Even without these vulnerabilities, it provides an >> >>>>| attacker with an easy mechanism to move laterally among other >> >>>>| processes within the same user's compromised session. >> >>>> >> >>>>So if one process within a tty session becomes compromised it can follow >> >>>>that additional processes, that are thought to be in different security >> >>>>boundaries, can be compromised as a result. When using a program like su >> >>>>or sudo, these additional processes could be in a tty session where TTY file >> >>>>descriptors are indeed shared over privilege boundaries. >> >>>> >> >>>>This is also an excellent writeup about the issue: >> >>>><http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> >> >>>> >> >>>>Signed-off-by: Matt Brown <matt@nmatt.com> >> >>>>--- >> >>>>drivers/tty/tty_io.c | 4 ++++ >> >>>>include/linux/tty.h | 2 ++ >> >>>>kernel/sysctl.c | 12 ++++++++++++ >> >>>>security/Kconfig | 13 +++++++++++++ >> >>>>4 files changed, 31 insertions(+) >> >>>> >> >>>>diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c >> >>>>index e6d1a65..31894e8 100644 >> >>>>--- a/drivers/tty/tty_io.c >> >>>>+++ b/drivers/tty/tty_io.c >> >>>>@@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) >> >>>> * FIXME: may race normal receive processing >> >>>> */ >> >>>> >> >>>>+int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); >> >>>>+ >> >>>>static int tiocsti(struct tty_struct *tty, char __user *p) >> >>>>{ >> >>>> char ch, mbz = 0; >> >>>> struct tty_ldisc *ld; >> >>>> >> >>>>+ if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) >> >>>>+ return -EPERM; >> >>>> if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >> >>>> return -EPERM; >> >>>> if (get_user(ch, p)) >> >>>>diff --git a/include/linux/tty.h b/include/linux/tty.h >> >>>>index 1017e904..7011102 100644 >> >>>>--- a/include/linux/tty.h >> >>>>+++ b/include/linux/tty.h >> >>>>@@ -342,6 +342,8 @@ struct tty_file_private { >> >>>> struct list_head list; >> >>>>}; >> >>>> >> >>>>+extern int tiocsti_restrict; >> >>>>+ >> >>>>/* tty magic number */ >> >>>>#define TTY_MAGIC 0x5401 >> >>>> >> >>>>diff --git a/kernel/sysctl.c b/kernel/sysctl.c >> >>>>index acf0a5a..68d1363 100644 >> >>>>--- a/kernel/sysctl.c >> >>>>+++ b/kernel/sysctl.c >> >>>>@@ -67,6 +67,7 @@ >> >>>>#include <linux/kexec.h> >> >>>>#include <linux/bpf.h> >> >>>>#include <linux/mount.h> >> >>>>+#include <linux/tty.h> >> >>>> >> >>>>#include <linux/uaccess.h> >> >>>>#include <asm/processor.h> >> >>>>@@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { >> >>>> .extra2 = &two, >> >>>> }, >> >>>>#endif >> >>>>+#if defined CONFIG_TTY >> >>>>+ { >> >>>>+ .procname = "tiocsti_restrict", >> >>>>+ .data = &tiocsti_restrict, >> >>>>+ .maxlen = sizeof(int), >> >>>>+ .mode = 0644, >> >>>>+ .proc_handler = proc_dointvec_minmax_sysadmin, >> >>>>+ .extra1 = &zero, >> >>>>+ .extra2 = &one, >> >>>>+ }, >> >>>>+#endif >> >>>> { >> >>>> .procname = "ngroups_max", >> >>>> .data = &ngroups_max, >> >>>>diff --git a/security/Kconfig b/security/Kconfig >> >>>>index 3ff1bf9..7d13331 100644 >> >>>>--- a/security/Kconfig >> >>>>+++ b/security/Kconfig >> >>>>@@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT >> >>>> >> >>>> If you are unsure how to answer this question, answer N. >> >>>> >> >>>>+config SECURITY_TIOCSTI_RESTRICT >> >>> >> >>>This is an odd way to name this. Shouldn't the name reflect that it >> >>>is setting the default, rather than enabling the feature? >> >>> >> >>>Besides that, I'm ok with the patch. >> >>> >> >>>>+ bool "Restrict unprivileged use of tiocsti command injection" >> >>>>+ default n >> >>>>+ help >> >>>>+ This enforces restrictions on unprivileged users injecting commands >> >>>>+ into other processes which share a tty session using the TIOCSTI >> >>>>+ ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. >> >>>>+ >> >>>>+ If this option is not selected, no restrictions will be enforced >> >>>>+ unless the tiocsti_restrict sysctl is explicitly set to (1). >> >>>>+ >> >>>>+ If you are unsure how to answer this question, answer N. >> >>>>+ >> >>>>config SECURITY >> >>>> bool "Enable different security models" >> >>>> depends on SYSFS >> >>>>-- >> >>>>2.10.2
Quoting matt@nmatt.com (matt@nmatt.com): > On 2017-04-20 11:19, Serge E. Hallyn wrote: > >Quoting Matt Brown (matt@nmatt.com): > >>On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: > >>>Quoting Matt Brown (matt@nmatt.com): > >>>>On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: > >>>>>On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: > >>>>>>This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity > >>>>>>project in-kernel. > >>>>>> > >>>>>>This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding > >>>>>>sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI > >>>>>>ioctl calls from non CAP_SYS_ADMIN users. > >>>>>> > >>>>>>Possible effects on userland: > >>>>>> > >>>>>>There could be a few user programs that would be effected by this > >>>>>>change. > >>>>>>See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> > >>>>>>notable programs are: agetty, csh, xemacs and tcsh > >>>>>> > >>>>>>However, I still believe that this change is worth it given that the > >>>>>>Kconfig defaults to n. This will be a feature that is turned on for the > >>>>> > >>>>>It's not worthless, but note that for instance before this was fixed > >>>>>in lxc, this patch would not have helped with escapes from privileged > >>>>>containers. > >>>>> > >>>> > >>>>I assume you are talking about this CVE: > >>>>https://bugzilla.redhat.com/show_bug.cgi?id=1411256 > >>>> > >>>>In retrospect, is there any way that an escape from a privileged > >>>>container with the this bug could have been prevented? > >>> > >>>I don't know, that's what I was probing for. Detecting that the pgrp > >>>or session - heck, the pid namespace - has changed would seem like a > >>>good indicator that it shouldn't be able to push. > >>> > >> > >>pgrp and session won't do because in the case we are discussing > >>current->signal->tty is the same as tty. > >> > >>This is the current check that is already in place: > >> | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) > >> | return -EPERM; > > > >Yeah... > > > >>The only thing I could find to detect the tty message coming from a > >>container is as follows: > >> | task_active_pid_ns(current)->level > >> > >>This will be zero when run on the host, but 1 when run inside a > >>container. However this is very much a hack and could probably break > >>some userland stuff where there are multiple levels of namespaces. > > > >Yes. This is also however why I don't like the current patch, because > >capable() will never be true in a container, so nested containers > >break. > > > > What do you mean by "capable() will never be true in a container"? > My understanding > is that if a container is given CAP_SYS_ADMIN then > capable(CAP_SYS_ADMIN) will return > true? No, capable(X) checks for X with respect to the initial user namespace. So for root-owned containers it will be true, but containers running in non-initial user namespaces cannot pass that check. To check for privilege with respect to another user namespace, you need to use ns_capable. But for that you need a user_ns to target. > I agree the hack I mentioned above would be a bad idea because > it would break > nested containers, but the current patch would not IMO. > > A better version of the hack could involve a config > CONFIG_TIOCSTI_MAX_NS_LEVEL where > a check would be performed to ensure that > task_active_pid_ns(current)->level is not > greater than the config value(an integer that is >= 0) . Yeah. That would break a different set of cases than the capable check, I assume. A smaller set, I think. > Again, I think we both would agree that this is not the best > solution. The clear > downside is that you could have multiple container layers where the > desired security > boundaries happened to fall at different levels. Just throwing ideas > around. Yup, appreciated.
On 04/20/2017 01:41 PM, Serge E. Hallyn wrote: > Quoting matt@nmatt.com (matt@nmatt.com): >> On 2017-04-20 11:19, Serge E. Hallyn wrote: >>> Quoting Matt Brown (matt@nmatt.com): >>>> On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: >>>>> Quoting Matt Brown (matt@nmatt.com): >>>>>> On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: >>>>>>> On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: >>>>>>>> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >>>>>>>> project in-kernel. >>>>>>>> >>>>>>>> This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding >>>>>>>> sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI >>>>>>>> ioctl calls from non CAP_SYS_ADMIN users. >>>>>>>> >>>>>>>> Possible effects on userland: >>>>>>>> >>>>>>>> There could be a few user programs that would be effected by this >>>>>>>> change. >>>>>>>> See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> >>>>>>>> notable programs are: agetty, csh, xemacs and tcsh >>>>>>>> >>>>>>>> However, I still believe that this change is worth it given that the >>>>>>>> Kconfig defaults to n. This will be a feature that is turned on for the >>>>>>> >>>>>>> It's not worthless, but note that for instance before this was fixed >>>>>>> in lxc, this patch would not have helped with escapes from privileged >>>>>>> containers. >>>>>>> >>>>>> >>>>>> I assume you are talking about this CVE: >>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 >>>>>> >>>>>> In retrospect, is there any way that an escape from a privileged >>>>>> container with the this bug could have been prevented? >>>>> >>>>> I don't know, that's what I was probing for. Detecting that the pgrp >>>>> or session - heck, the pid namespace - has changed would seem like a >>>>> good indicator that it shouldn't be able to push. >>>>> >>>> >>>> pgrp and session won't do because in the case we are discussing >>>> current->signal->tty is the same as tty. >>>> >>>> This is the current check that is already in place: >>>> | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >>>> | return -EPERM; >>> >>> Yeah... >>> >>>> The only thing I could find to detect the tty message coming from a >>>> container is as follows: >>>> | task_active_pid_ns(current)->level >>>> >>>> This will be zero when run on the host, but 1 when run inside a >>>> container. However this is very much a hack and could probably break >>>> some userland stuff where there are multiple levels of namespaces. >>> >>> Yes. This is also however why I don't like the current patch, because >>> capable() will never be true in a container, so nested containers >>> break. >>> >> >> What do you mean by "capable() will never be true in a container"? >> My understanding >> is that if a container is given CAP_SYS_ADMIN then >> capable(CAP_SYS_ADMIN) will return >> true? > > No, capable(X) checks for X with respect to the initial user namespace. > So for root-owned containers it will be true, but containers running in > non-initial user namespaces cannot pass that check. > > To check for privilege with respect to another user namespace, you need > to use ns_capable. But for that you need a user_ns to target. > How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ? current_user_ns() was found in include/linux/cred.h >> I agree the hack I mentioned above would be a bad idea because >> it would break >> nested containers, but the current patch would not IMO. >> >> A better version of the hack could involve a config >> CONFIG_TIOCSTI_MAX_NS_LEVEL where >> a check would be performed to ensure that >> task_active_pid_ns(current)->level is not >> greater than the config value(an integer that is >= 0) . > > Yeah. That would break a different set of cases than the capable > check, I assume. A smaller set, I think. > >> Again, I think we both would agree that this is not the best >> solution. The clear >> downside is that you could have multiple container layers where the >> desired security >> boundaries happened to fall at different levels. Just throwing ideas >> around. > > Yup, appreciated. >
On Fri, Apr 21, 2017 at 01:09:59AM -0400, Matt Brown wrote: > On 04/20/2017 01:41 PM, Serge E. Hallyn wrote: > >Quoting matt@nmatt.com (matt@nmatt.com): > >>On 2017-04-20 11:19, Serge E. Hallyn wrote: > >>>Quoting Matt Brown (matt@nmatt.com): > >>>>On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: > >>>>>Quoting Matt Brown (matt@nmatt.com): > >>>>>>On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: > >>>>>>>On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: > >>>>>>>>This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity > >>>>>>>>project in-kernel. > >>>>>>>> > >>>>>>>>This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding > >>>>>>>>sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI > >>>>>>>>ioctl calls from non CAP_SYS_ADMIN users. > >>>>>>>> > >>>>>>>>Possible effects on userland: > >>>>>>>> > >>>>>>>>There could be a few user programs that would be effected by this > >>>>>>>>change. > >>>>>>>>See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> > >>>>>>>>notable programs are: agetty, csh, xemacs and tcsh > >>>>>>>> > >>>>>>>>However, I still believe that this change is worth it given that the > >>>>>>>>Kconfig defaults to n. This will be a feature that is turned on for the > >>>>>>> > >>>>>>>It's not worthless, but note that for instance before this was fixed > >>>>>>>in lxc, this patch would not have helped with escapes from privileged > >>>>>>>containers. > >>>>>>> > >>>>>> > >>>>>>I assume you are talking about this CVE: > >>>>>>https://bugzilla.redhat.com/show_bug.cgi?id=1411256 > >>>>>> > >>>>>>In retrospect, is there any way that an escape from a privileged > >>>>>>container with the this bug could have been prevented? > >>>>> > >>>>>I don't know, that's what I was probing for. Detecting that the pgrp > >>>>>or session - heck, the pid namespace - has changed would seem like a > >>>>>good indicator that it shouldn't be able to push. > >>>>> > >>>> > >>>>pgrp and session won't do because in the case we are discussing > >>>>current->signal->tty is the same as tty. > >>>> > >>>>This is the current check that is already in place: > >>>>| if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) > >>>>| return -EPERM; > >>> > >>>Yeah... > >>> > >>>>The only thing I could find to detect the tty message coming from a > >>>>container is as follows: > >>>>| task_active_pid_ns(current)->level > >>>> > >>>>This will be zero when run on the host, but 1 when run inside a > >>>>container. However this is very much a hack and could probably break > >>>>some userland stuff where there are multiple levels of namespaces. > >>> > >>>Yes. This is also however why I don't like the current patch, because > >>>capable() will never be true in a container, so nested containers > >>>break. > >>> > >> > >>What do you mean by "capable() will never be true in a container"? > >>My understanding > >>is that if a container is given CAP_SYS_ADMIN then > >>capable(CAP_SYS_ADMIN) will return > >>true? > > > >No, capable(X) checks for X with respect to the initial user namespace. > >So for root-owned containers it will be true, but containers running in > >non-initial user namespaces cannot pass that check. > > > >To check for privilege with respect to another user namespace, you need > >to use ns_capable. But for that you need a user_ns to target. > > > > How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ? > > current_user_ns() was found in include/linux/cred.h Any user can create a new user namespace and pass the above check. What we want is to find the user namespace which opened the tty.
On Thu, Apr 20, 2017 at 10:24 PM, Serge E. Hallyn <serge@hallyn.com> wrote: > On Fri, Apr 21, 2017 at 01:09:59AM -0400, Matt Brown wrote: >> On 04/20/2017 01:41 PM, Serge E. Hallyn wrote: >> >Quoting matt@nmatt.com (matt@nmatt.com): >> >>On 2017-04-20 11:19, Serge E. Hallyn wrote: >> >>>Quoting Matt Brown (matt@nmatt.com): >> >>>>On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: >> >>>>>Quoting Matt Brown (matt@nmatt.com): >> >>>>>>On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: >> >>>>>>>On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: >> >>>>>>>>This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >> >>>>>>>>project in-kernel. >> >>>>>>>> >> >>>>>>>>This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding >> >>>>>>>>sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI >> >>>>>>>>ioctl calls from non CAP_SYS_ADMIN users. >> >>>>>>>> >> >>>>>>>>Possible effects on userland: >> >>>>>>>> >> >>>>>>>>There could be a few user programs that would be effected by this >> >>>>>>>>change. >> >>>>>>>>See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> >> >>>>>>>>notable programs are: agetty, csh, xemacs and tcsh >> >>>>>>>> >> >>>>>>>>However, I still believe that this change is worth it given that the >> >>>>>>>>Kconfig defaults to n. This will be a feature that is turned on for the >> >>>>>>> >> >>>>>>>It's not worthless, but note that for instance before this was fixed >> >>>>>>>in lxc, this patch would not have helped with escapes from privileged >> >>>>>>>containers. >> >>>>>>> >> >>>>>> >> >>>>>>I assume you are talking about this CVE: >> >>>>>>https://bugzilla.redhat.com/show_bug.cgi?id=1411256 >> >>>>>> >> >>>>>>In retrospect, is there any way that an escape from a privileged >> >>>>>>container with the this bug could have been prevented? >> >>>>> >> >>>>>I don't know, that's what I was probing for. Detecting that the pgrp >> >>>>>or session - heck, the pid namespace - has changed would seem like a >> >>>>>good indicator that it shouldn't be able to push. >> >>>>> >> >>>> >> >>>>pgrp and session won't do because in the case we are discussing >> >>>>current->signal->tty is the same as tty. >> >>>> >> >>>>This is the current check that is already in place: >> >>>>| if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >> >>>>| return -EPERM; >> >>> >> >>>Yeah... >> >>> >> >>>>The only thing I could find to detect the tty message coming from a >> >>>>container is as follows: >> >>>>| task_active_pid_ns(current)->level >> >>>> >> >>>>This will be zero when run on the host, but 1 when run inside a >> >>>>container. However this is very much a hack and could probably break >> >>>>some userland stuff where there are multiple levels of namespaces. >> >>> >> >>>Yes. This is also however why I don't like the current patch, because >> >>>capable() will never be true in a container, so nested containers >> >>>break. >> >>> >> >> >> >>What do you mean by "capable() will never be true in a container"? >> >>My understanding >> >>is that if a container is given CAP_SYS_ADMIN then >> >>capable(CAP_SYS_ADMIN) will return >> >>true? >> > >> >No, capable(X) checks for X with respect to the initial user namespace. >> >So for root-owned containers it will be true, but containers running in >> >non-initial user namespaces cannot pass that check. >> > >> >To check for privilege with respect to another user namespace, you need >> >to use ns_capable. But for that you need a user_ns to target. >> > >> >> How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ? >> >> current_user_ns() was found in include/linux/cred.h > > Any user can create a new user namespace and pass the above check. What we > want is to find the user namespace which opened the tty. Can we use file->cred->user_ns? Hm, but I see there isn't really a single file associated with tty_struct. -Kees
On 04/21/2017 01:24 AM, Serge E. Hallyn wrote: > On Fri, Apr 21, 2017 at 01:09:59AM -0400, Matt Brown wrote: >> On 04/20/2017 01:41 PM, Serge E. Hallyn wrote: >>> Quoting matt@nmatt.com (matt@nmatt.com): >>>> On 2017-04-20 11:19, Serge E. Hallyn wrote: >>>>> Quoting Matt Brown (matt@nmatt.com): >>>>>> On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: >>>>>>> Quoting Matt Brown (matt@nmatt.com): >>>>>>>> On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: >>>>>>>>> On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: >>>>>>>>>> This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity >>>>>>>>>> project in-kernel. >>>>>>>>>> >>>>>>>>>> This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding >>>>>>>>>> sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI >>>>>>>>>> ioctl calls from non CAP_SYS_ADMIN users. >>>>>>>>>> >>>>>>>>>> Possible effects on userland: >>>>>>>>>> >>>>>>>>>> There could be a few user programs that would be effected by this >>>>>>>>>> change. >>>>>>>>>> See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> >>>>>>>>>> notable programs are: agetty, csh, xemacs and tcsh >>>>>>>>>> >>>>>>>>>> However, I still believe that this change is worth it given that the >>>>>>>>>> Kconfig defaults to n. This will be a feature that is turned on for the >>>>>>>>> >>>>>>>>> It's not worthless, but note that for instance before this was fixed >>>>>>>>> in lxc, this patch would not have helped with escapes from privileged >>>>>>>>> containers. >>>>>>>>> >>>>>>>> >>>>>>>> I assume you are talking about this CVE: >>>>>>>> https://bugzilla.redhat.com/show_bug.cgi?id=1411256 >>>>>>>> >>>>>>>> In retrospect, is there any way that an escape from a privileged >>>>>>>> container with the this bug could have been prevented? >>>>>>> >>>>>>> I don't know, that's what I was probing for. Detecting that the pgrp >>>>>>> or session - heck, the pid namespace - has changed would seem like a >>>>>>> good indicator that it shouldn't be able to push. >>>>>>> >>>>>> >>>>>> pgrp and session won't do because in the case we are discussing >>>>>> current->signal->tty is the same as tty. >>>>>> >>>>>> This is the current check that is already in place: >>>>>> | if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) >>>>>> | return -EPERM; >>>>> >>>>> Yeah... >>>>> >>>>>> The only thing I could find to detect the tty message coming from a >>>>>> container is as follows: >>>>>> | task_active_pid_ns(current)->level >>>>>> >>>>>> This will be zero when run on the host, but 1 when run inside a >>>>>> container. However this is very much a hack and could probably break >>>>>> some userland stuff where there are multiple levels of namespaces. >>>>> >>>>> Yes. This is also however why I don't like the current patch, because >>>>> capable() will never be true in a container, so nested containers >>>>> break. >>>>> >>>> >>>> What do you mean by "capable() will never be true in a container"? >>>> My understanding >>>> is that if a container is given CAP_SYS_ADMIN then >>>> capable(CAP_SYS_ADMIN) will return >>>> true? >>> >>> No, capable(X) checks for X with respect to the initial user namespace. >>> So for root-owned containers it will be true, but containers running in >>> non-initial user namespaces cannot pass that check. >>> >>> To check for privilege with respect to another user namespace, you need >>> to use ns_capable. But for that you need a user_ns to target. >>> >> >> How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ? >> >> current_user_ns() was found in include/linux/cred.h > > Any user can create a new user namespace and pass the above check. What we > want is to find the user namespace which opened the tty. > I believe I have a working solution that I can show in the next version of the patch later today, but I just want to run the logic by you first. I added: "struct user_namespace *owner_user_ns;" as a field in tty_struct (include/linux/tty.h) Note: I am totally open to suggestions for a better name. Then I added "tty->owner_user_ns = current_user_ns();" to the alloc_tty_struct function. (drivers/tty/tty_io.c) When testing with a docker container, running in a different user namespace, I printed out current_user_ns()->level, which returned 1, and tty->owner_user_ns->level, which returned 0. This seems to prove that I am correctly storing the user namespace which opened the tty. Please let me know if there are any edge cases that I am missing with this approach.
Quoting Matt Brown (matt@nmatt.com): > On 04/21/2017 01:24 AM, Serge E. Hallyn wrote: > >On Fri, Apr 21, 2017 at 01:09:59AM -0400, Matt Brown wrote: > >>On 04/20/2017 01:41 PM, Serge E. Hallyn wrote: > >>>Quoting matt@nmatt.com (matt@nmatt.com): > >>>>On 2017-04-20 11:19, Serge E. Hallyn wrote: > >>>>>Quoting Matt Brown (matt@nmatt.com): > >>>>>>On 04/19/2017 07:53 PM, Serge E. Hallyn wrote: > >>>>>>>Quoting Matt Brown (matt@nmatt.com): > >>>>>>>>On 04/19/2017 12:58 AM, Serge E. Hallyn wrote: > >>>>>>>>>On Tue, Apr 18, 2017 at 11:45:26PM -0400, Matt Brown wrote: > >>>>>>>>>>This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity > >>>>>>>>>>project in-kernel. > >>>>>>>>>> > >>>>>>>>>>This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding > >>>>>>>>>>sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI > >>>>>>>>>>ioctl calls from non CAP_SYS_ADMIN users. > >>>>>>>>>> > >>>>>>>>>>Possible effects on userland: > >>>>>>>>>> > >>>>>>>>>>There could be a few user programs that would be effected by this > >>>>>>>>>>change. > >>>>>>>>>>See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> > >>>>>>>>>>notable programs are: agetty, csh, xemacs and tcsh > >>>>>>>>>> > >>>>>>>>>>However, I still believe that this change is worth it given that the > >>>>>>>>>>Kconfig defaults to n. This will be a feature that is turned on for the > >>>>>>>>> > >>>>>>>>>It's not worthless, but note that for instance before this was fixed > >>>>>>>>>in lxc, this patch would not have helped with escapes from privileged > >>>>>>>>>containers. > >>>>>>>>> > >>>>>>>> > >>>>>>>>I assume you are talking about this CVE: > >>>>>>>>https://bugzilla.redhat.com/show_bug.cgi?id=1411256 > >>>>>>>> > >>>>>>>>In retrospect, is there any way that an escape from a privileged > >>>>>>>>container with the this bug could have been prevented? > >>>>>>> > >>>>>>>I don't know, that's what I was probing for. Detecting that the pgrp > >>>>>>>or session - heck, the pid namespace - has changed would seem like a > >>>>>>>good indicator that it shouldn't be able to push. > >>>>>>> > >>>>>> > >>>>>>pgrp and session won't do because in the case we are discussing > >>>>>>current->signal->tty is the same as tty. > >>>>>> > >>>>>>This is the current check that is already in place: > >>>>>>| if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) > >>>>>>| return -EPERM; > >>>>> > >>>>>Yeah... > >>>>> > >>>>>>The only thing I could find to detect the tty message coming from a > >>>>>>container is as follows: > >>>>>>| task_active_pid_ns(current)->level > >>>>>> > >>>>>>This will be zero when run on the host, but 1 when run inside a > >>>>>>container. However this is very much a hack and could probably break > >>>>>>some userland stuff where there are multiple levels of namespaces. > >>>>> > >>>>>Yes. This is also however why I don't like the current patch, because > >>>>>capable() will never be true in a container, so nested containers > >>>>>break. > >>>>> > >>>> > >>>>What do you mean by "capable() will never be true in a container"? > >>>>My understanding > >>>>is that if a container is given CAP_SYS_ADMIN then > >>>>capable(CAP_SYS_ADMIN) will return > >>>>true? > >>> > >>>No, capable(X) checks for X with respect to the initial user namespace. > >>>So for root-owned containers it will be true, but containers running in > >>>non-initial user namespaces cannot pass that check. > >>> > >>>To check for privilege with respect to another user namespace, you need > >>>to use ns_capable. But for that you need a user_ns to target. > >>> > >> > >>How about: ns_capable(current_user_ns(),CAP_SYS_ADMIN) ? > >> > >>current_user_ns() was found in include/linux/cred.h > > > >Any user can create a new user namespace and pass the above check. What we > >want is to find the user namespace which opened the tty. > > > > I believe I have a working solution that I can show in the next version > of the patch later today, but I just want to run the logic by you first. > > I added: "struct user_namespace *owner_user_ns;" as a field in > tty_struct (include/linux/tty.h) Note: I am totally open to suggestions > for a better name. > > Then I added "tty->owner_user_ns = current_user_ns();" to the > alloc_tty_struct function. (drivers/tty/tty_io.c) That's what I was hoping could work. Then you can check ns_capable with respect to that. You'll want to grab a reference to the user_ns, and drop it on final close, but otherwise this sounds good to me. I don't really know the tty layer well though so we'll need some sanity checking from someone who does. > When testing with a docker container, running in a different user > namespace, I printed out current_user_ns()->level, which returned 1, > and tty->owner_user_ns->level, which returned 0. This seems to prove > that I am correctly storing the user namespace which opened the tty. > > Please let me know if there are any edge cases that I am missing with > this approach. Thanks for posting this! This seems like the best solution to me.
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c index e6d1a65..31894e8 100644 --- a/drivers/tty/tty_io.c +++ b/drivers/tty/tty_io.c @@ -2296,11 +2296,15 @@ static int tty_fasync(int fd, struct file *filp, int on) * FIXME: may race normal receive processing */ +int tiocsti_restrict = IS_ENABLED(CONFIG_SECURITY_TIOCSTI_RESTRICT); + static int tiocsti(struct tty_struct *tty, char __user *p) { char ch, mbz = 0; struct tty_ldisc *ld; + if (tiocsti_restrict && !capable(CAP_SYS_ADMIN)) + return -EPERM; if ((current->signal->tty != tty) && !capable(CAP_SYS_ADMIN)) return -EPERM; if (get_user(ch, p)) diff --git a/include/linux/tty.h b/include/linux/tty.h index 1017e904..7011102 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -342,6 +342,8 @@ struct tty_file_private { struct list_head list; }; +extern int tiocsti_restrict; + /* tty magic number */ #define TTY_MAGIC 0x5401 diff --git a/kernel/sysctl.c b/kernel/sysctl.c index acf0a5a..68d1363 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -67,6 +67,7 @@ #include <linux/kexec.h> #include <linux/bpf.h> #include <linux/mount.h> +#include <linux/tty.h> #include <linux/uaccess.h> #include <asm/processor.h> @@ -833,6 +834,17 @@ static struct ctl_table kern_table[] = { .extra2 = &two, }, #endif +#if defined CONFIG_TTY + { + .procname = "tiocsti_restrict", + .data = &tiocsti_restrict, + .maxlen = sizeof(int), + .mode = 0644, + .proc_handler = proc_dointvec_minmax_sysadmin, + .extra1 = &zero, + .extra2 = &one, + }, +#endif { .procname = "ngroups_max", .data = &ngroups_max, diff --git a/security/Kconfig b/security/Kconfig index 3ff1bf9..7d13331 100644 --- a/security/Kconfig +++ b/security/Kconfig @@ -18,6 +18,19 @@ config SECURITY_DMESG_RESTRICT If you are unsure how to answer this question, answer N. +config SECURITY_TIOCSTI_RESTRICT + bool "Restrict unprivileged use of tiocsti command injection" + default n + help + This enforces restrictions on unprivileged users injecting commands + into other processes which share a tty session using the TIOCSTI + ioctl. This option makes TIOCSTI use require CAP_SYS_ADMIN. + + If this option is not selected, no restrictions will be enforced + unless the tiocsti_restrict sysctl is explicitly set to (1). + + If you are unsure how to answer this question, answer N. + config SECURITY bool "Enable different security models" depends on SYSFS
This patch reproduces GRKERNSEC_HARDEN_TTY functionality from the grsecurity project in-kernel. This will create the Kconfig SECURITY_TIOCSTI_RESTRICT and the corresponding sysctl kernel.tiocsti_restrict that, when activated, restrict all TIOCSTI ioctl calls from non CAP_SYS_ADMIN users. Possible effects on userland: There could be a few user programs that would be effected by this change. See: <https://codesearch.debian.net/search?q=ioctl%5C%28.*TIOCSTI> notable programs are: agetty, csh, xemacs and tcsh However, I still believe that this change is worth it given that the Kconfig defaults to n. This will be a feature that is turned on for the same reason that people activate it when using grsecurity. Users of this opt-in feature will realize that they are choosing security over some OS features like unprivileged TIOCSTI ioctls, as should be clear in the Kconfig help message. Threat Model/Patch Rational: From grsecurity's config for GRKERNSEC_HARDEN_TTY. | There are very few legitimate uses for this functionality and it | has made vulnerabilities in several 'su'-like programs possible in | the past. Even without these vulnerabilities, it provides an | attacker with an easy mechanism to move laterally among other | processes within the same user's compromised session. So if one process within a tty session becomes compromised it can follow that additional processes, that are thought to be in different security boundaries, can be compromised as a result. When using a program like su or sudo, these additional processes could be in a tty session where TTY file descriptors are indeed shared over privilege boundaries. This is also an excellent writeup about the issue: <http://www.halfdog.net/Security/2012/TtyPushbackPrivilegeEscalation/> Signed-off-by: Matt Brown <matt@nmatt.com> --- drivers/tty/tty_io.c | 4 ++++ include/linux/tty.h | 2 ++ kernel/sysctl.c | 12 ++++++++++++ security/Kconfig | 13 +++++++++++++ 4 files changed, 31 insertions(+)