Fixing su + runuser vulnerability CVE-2016-2779

Fixing su + runuser vulnerability CVE-2016-2779

[Stanislav Brabec]

As you may know, there are reported vulnerabilities for several Linux
utilities using TIOCSTI ioctl terminal injection.

I am just thinking about a best fix for them. However possible fixes
are easy (see below), all of user space fixes have side effects.

There is also questionable, whether it is issue of util-linux at all,
and whether it should be restricted in the kernel.

For now, I am attaching a simple reproduced from the Debian bug.
It also contains references.

To Federico Bento: Could you confirm that you are author and approve
the license in the test_tiocsti.c, please?


In following examples, I never entered "id -u -n" manually. They were
injected.

Bad:
util-linux # ./runuser -u nobody ./test_tiocsti
id -u -n
util-linux # id -u -n
root

OK:
util-linux # su nobody -c ./test_tiocsti
util-linux #

Bad:
util-linux # su - nobody -s $PWD/test_tiocsti
id -u -n
util-linux # id -u -n
root
util-linux # 

Bad:
util-linux # su - nobody --session-command $PWD/test_tiocsti
id -u -n
util-linux # id -u -n
root
util-linux #

Not only util-linux is affected:
util-linux # cp -a test_tiocsti /openSUSE-root/
util-linux # chroot /openSUSE-root /test_tiocsti
id -u -n
util-linux # id -u -n
root
util-linux # sudo -u nobody ./test_tiocsti
id -u -n
util-linux # id -u -n
root


There are some controversial things with the straightforward fix:

setsid() prevents TIOCSTI attack described in the report (easy to
reproduce), but it has side effects: It disconnects the task from job
control. With setsid(), ^Z cannot be used for sending the application
to background any more (easy to reproduce by calling setsid()
unconditionally in the same place).

su-common.c now calls setsid() only if new session is requested.


Another possible fixes would be:

- Request redirection of all I/O channels. (I. e. documentation fix
  plus possible command line option to make it simpler.)

- Or create custom pty container (like script does).

- Or create a kernel level fix restricting TIOCSTI and let utilities as
  they are.

First two will have side effects, third seems to be a right way to me.


Additionally, https://bugzilla.redhat.com/show_bug.cgi?id=173008 says,
that even it does not handle all possible attacks, because attacker can
still read and write to the terminal:

==== steal.sh ====
#!/bin/sh
(
sleep 3
exec 0>&1
echo "Hallo" >/dev/stdout
cat >/tmp/nobody-savefile
) &
==================

~/util-linux # ./runuser -u nobody ./steal.sh
~/util-linux # Hallo

To prevent unwanted reading from a terminal, one would:

- Either request redirection of all I/O channels. (I. e. documentation
  fix plus possible command line option to make it simpler.)

- Or kill slave processes holding the terminal after exiting. It
  prevents it in 100%, but it has important side effects.

- Or create custom pty container (like script does).

- Or, finally introduce some type of revoke() syscall in the kernel.

First is de facto not a fix, second has side effects, third would be a
bit fragile, and fourth does not exist yet.


I don't have a test case yet. The test environment inside a test suite
needs a custom pty with shell. I tries to use script for it but failed:
$TS_CMD_SCRIPT" -c "$TS_CMD_RUNUSER -u nobody $TS_HELPER_TIOCSTI" $TS_OUTDIR/${TS_TESTNAME}-typescript


>From ce590b57ac1c54011a007839ac939afefec942a3 Mon Sep 17 00:00:00 2001
From: Stanislav Brabec <sbrabec@suse.cz>
Date: Wed, 2 Mar 2016 19:46:34 +0100
Subject: [PATCH] Add helper for TIOCSTI exploit

This helper/exploit injects "id -u -n\n" to the vulnerable calling terminal.

Use id -u -n to get a reproducible output of test cases based on it.

What can happen:

Nothing, no exploit: pty is not accessible, sedsid() disconnected the task from
pty, TIOCSTI failed.

The command is injected to the unprivileged environment pty, and you see e. g.
"nobody": This is acceptable.

The command is injected to the caller (privileged) pty, and you see "root" (or
caller uid name): This is not acceptable and has security implications.

References:

CVE-2016-2779
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2016-2779
http://seclists.org/oss-sec/2016/q1/448
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=815922
https://bugzilla.redhat.com/show_bug.cgi?id=173008
https://bugzilla.suse.com/show_bug.cgi?id=968674
https://bugzilla.suse.com/show_bug.cgi?id=968675

CVE-2016-2781
http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2016-2781
http://seclists.org/oss-sec/2016/q1/452

Signed-off-by: Stanislav Brabec <sbrabec@suse.cz>
Cc: Federico Bento <up201407890@alunos.dcc.fc.up.pt>
---
 tests/commands.sh            |  2 ++
 tests/helpers/Makemodule.am  |  3 +++
 tests/helpers/test_tiocsti.c | 27 +++++++++++++++++++++++++++
 3 files changed, 32 insertions(+)
 create mode 100644 tests/helpers/test_tiocsti.c

diff --git a/tests/commands.sh b/tests/commands.sh
index 5518dc3..3327f66 100644
--- a/tests/commands.sh
+++ b/tests/commands.sh
@@ -30,6 +30,7 @@ TS_HELPER_SCRIPT="$top_builddir/test_script"
 TS_HELPER_SIGRECEIVE="$top_builddir/test_sigreceive"
 TS_HELPER_STRUTILS="$top_builddir/test_strutils"
 TS_HELPER_SYSINFO="$top_builddir/test_sysinfo"
+TS_HELPER_TIOCSTI="$top_builddir/test_tiocsti"
 TS_HELPER_UUID_PARSER="$top_builddir/test_uuid_parser"
 
 # paths to commands
@@ -73,6 +74,7 @@ TS_CMD_MOUNTPOINT=${TS_CMD_MOUNTPOINT:-"$top_builddir/mountpoint"}
 TS_CMD_NAMEI=${TS_CMD_NAMEI-"$top_builddir/namei"}
 TS_CMD_PARTX=${TS_CMD_PARTX-"$top_builddir/partx"}
 TS_CMD_RENAME=${TS_CMD_RENAME-"$top_builddir/rename"}
+TS_CMD_RUNUSER=${TS_CMD_RUNUSER-"$top_builddir/runuser"}
 TS_CMD_REV=${TS_CMD_REV:-"$top_builddir/rev"}
 TS_CMD_SCRIPT=${TS_CMD_SCRIPT-"$top_builddir/script"}
 TS_CMD_SCRIPTREPLAY=${TS_CMD_SCRIPTREPLAY-"$top_builddir/scriptreplay"}
diff --git a/tests/helpers/Makemodule.am b/tests/helpers/Makemodule.am
index 0618e75..3070a8b 100644
--- a/tests/helpers/Makemodule.am
+++ b/tests/helpers/Makemodule.am
@@ -15,3 +15,6 @@ check_PROGRAMS += test_sigreceive
 test_sigreceive_SOURCES = tests/helpers/test_sigreceive.c
 test_sigreceive_LDADD = $(LDADD) libcommon.la
 
+check_PROGRAMS += test_tiocsti
+test_tiocsti_SOURCES = tests/helpers/test_tiocsti.c
+
diff --git a/tests/helpers/test_tiocsti.c b/tests/helpers/test_tiocsti.c
new file mode 100644
index 0000000..c269dc0
--- /dev/null
+++ b/tests/helpers/test_tiocsti.c
@@ -0,0 +1,27 @@
+/*
+ * test_tiocsti - test security of TIOCSTI
+ *
+ * Written by Federico Bento <up201407890@alunos.dcc.fc.up.pt>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <sys/ioctl.h>
+
+int main(void)
+{
+  char *cmd = "id -u -n\n";
+  while(*cmd)
+   ioctl(0, TIOCSTI, cmd++);
+}
-- 
2.7.2

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Ángel González]

On 02/03/16 20:35, Stanislav Brabec wrote:
> Another possible fixes would be:
>
> - Request redirection of all I/O channels. (I. e. documentation fix
>    plus possible command line option to make it simpler.)
>
> - Or create custom pty container (like script does).
>
> - Or create a kernel level fix restricting TIOCSTI and let utilities as
>    they are.
>
> First two will have side effects, third seems to be a right way to me.

+1

IMHO a process without CAP_SYS_ADMIN (or similar) shouldn't be able to 
fake input¹ into a terminal owned² by a different user.


¹ yes, that's the goal of TIOCSTI)
² Not a complete solution, since you could have:
$ su root su $USER -s ./test_tiocsti

but if you are the owner of the terminal, it could do all kind of nasty 
things all the way down anyway.



> Additionally, https://bugzilla.redhat.com/show_bug.cgi?id=173008 says,
> that even it does not handle all possible attacks, because attacker can
> still read and write to the terminal:
>
> ==== steal.sh ====
> #!/bin/sh
> (
> sleep 3
> exec 0>&1
> echo "Hallo">/dev/stdout
> cat>/tmp/nobody-savefile
> )&
> ==================
>
> ~/util-linux # ./runuser -u nobody ./steal.sh
> ~/util-linux # Hallo

Nice use of background process with what is otherwise expected.
The is that the user is tricked into thinking that the child process 
[tree] has finished while it hasn't.

However, it doesn't seem work here:
> ./steal.sh: line 5: /dev/stdout: Permission denied
> cat: -: Input/output error

Re: Fixing su + runuser vulnerability CVE-2016-2779

[up201407890]

Quoting "Stanislav Brabec" <sbrabec@suse.cz>:


> To Federico Bento: Could you confirm that you are author and approve
> the license in the test_tiocsti.c, please?

There were vulnerabilities in the past regarding the same issue for  
different programs ("su" for example, CVE-2005-4890), that when  
reported, used the same technique. The program is the most basic you  
can get to demonstrate this, so i'm pretty sure i wasn't the only one  
that used something similar.
At the time, though, i wrote it off my head. This doesn't mean, as I  
said, that no one wrote something very similar, so i'm not really sure  
if I should be credited for something that has a high probability of  
having been written before.


On another note, grsecurity recently released a new feature named  
GRKERNSEC_HARDEN_TTY that disallows the use of TIOCSTI to unprivileged  
users unless the caller has CAP_SYS_ADMIN. Brad Spengler (spender)  
said that looking into it, he didn't find legitimate uses of such  
ioctl, and no wide usage of writevt.

https://gitweb.gentoo.org/proj/hardened-patchset.git/commit/?id=d47ea9080b76a7445e8a36545c539b2a62c31faa

Check out gr_handle_tiocsti()


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Stanislav Brabec]

On Mar 3, 2016 at 01:37 up201407890@alunos.dcc.fc.up.pt wrote:

> On another note, grsecurity recently released a new feature named
> GRKERNSEC_HARDEN_TTY that disallows the use of TIOCSTI to unprivileged
> users unless the caller has CAP_SYS_ADMIN.

This will fix all util-linux issues, but not chroot. There root inside 
the chroot escapes from chroot and calls commands outside.

I can imagine yet another kernel level solution:

Implement a way to disallow TIOCSTI, eventually revoke terminal R/W access.

This would need application level fixes:
- Before calling the restricted process, disallow TIOCSTI.
- After returning from the restricted process, revoke terminal R/W.

> Brad Spengler (spender) said
> that looking into it, he didn't find legitimate uses of such ioctl, and
> no wide usage of writevt.

Some old systems had tiocsti(1) utility, probably used like a 
predecessor of readline.

Just for curiosity, I just ran grep for TIOCSTI ioctl() over all 
openSUSE sources. I got about 60 matches.

I analyzed use of some cases:

util-linux: used in agetty in wait_for_term_input()
kbd: contrib utility sti equal to tiocsti utility.
irda: Used by handle_scancode() to emulate input.
tcsh: Used in ed mode and in pushback().
emacs: Used in stuff_char() (putting char to be read from terminal)
...

It seems that TIOCSTI is used for:
- Read character, and if it does not match, put it back.
- Wait for character, than put it back for processing.
- Implementing a simple line editing.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Stanislav Brabec]

On Mar 3, 2016 at 17:21 Stanislav Brabec wrote:
> On Mar 3, 2016 at 01:37 up201407890@alunos.dcc.fc.up.pt wrote:
> 
>> On another note, grsecurity recently released a new feature named
>> GRKERNSEC_HARDEN_TTY that disallows the use of TIOCSTI to unprivileged
>> users unless the caller has CAP_SYS_ADMIN.
> 
> This will fix all util-linux issues, but not chroot. There root inside 
> the chroot escapes from chroot and calls commands outside.
> 
We had a talk about this bug, and we found, that there is no quick and
100% safe fix.

Here are possibilities:

1) Quick kernel fix disabling TIOCSTI ioctl() for non-root, if the PID
of the terminal owner is not equal to PID of the calling process,
eventually use capabilities for the same.

Pros:
+ Fix in one place.
+ Fix all possible future abuses.

Cons:
- Many utilities are potentially affected and need testing.
- Some custom code could be affected. (I can imagine for example bar
  code reader running with a dedicated UID, and pushing bar code to the
  terminal. Such code will break for sure.)


2) Per utility fix using setsid().

Pros:
+ Prevents the exploit without uncertain side effects.

Cons:
- Each affected utility needs fix.
- Loss of job control will affect working style of many people.


Conclusion:

We need a different solution:

3) Introduce new terminal ioctl() or flag in the kernel. This flag will
block TIOCSTI (and possibly other dangerous actions). It will allow to
implement something like setsid(), but without side effects of job
control loss.

Pros:
+ No unwanted side effects at all.

Cons:
- Each affected utility needs fix.


We think, that only 3 will be safe and have no side effects.


Note:

Fixing character stealing described in previous mails is not covered by
any of these solutions. This could be possible safely only with a new
syscall revoke(), which was not yet accepted to the kernel.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: Fixing su + runuser vulnerability CVE-2016-2779

[up201407890]

Quoting "Stanislav Brabec" <sbrabec@suse.cz>:

> On Mar 3, 2016 at 17:21 Stanislav Brabec wrote:
>> On Mar 3, 2016 at 01:37 up201407890@alunos.dcc.fc.up.pt wrote:
>>
>>> On another note, grsecurity recently released a new feature named
>>> GRKERNSEC_HARDEN_TTY that disallows the use of TIOCSTI to unprivileged
>>> users unless the caller has CAP_SYS_ADMIN.
>>
>> This will fix all util-linux issues, but not chroot. There root inside
>> the chroot escapes from chroot and calls commands outside.

Haven't looked into it, but grsecurity also has several chroot  
hardening features, so maybe this is why HARDEN_TTY was enough.

> Here are possibilities:
>
> 1) Quick kernel fix disabling TIOCSTI ioctl() for non-root, if the PID
> of the terminal owner is not equal to PID of the calling process,
> eventually use capabilities for the same.
>
> Pros:
> + Fix in one place.
> + Fix all possible future abuses.
>
> Cons:
> - Many utilities are potentially affected and need testing.
> - Some custom code could be affected. (I can imagine for example bar
>   code reader running with a dedicated UID, and pushing bar code to the
>   terminal. Such code will break for sure.)
>

Definitely needs testing.

>
> 2) Per utility fix using setsid().
>
> Pros:
> + Prevents the exploit without uncertain side effects.
>
> Cons:
> - Each affected utility needs fix.
> - Loss of job control will affect working style of many people.

Most weren't fixed because of this reason, it's likely they won't  
change it now.

> Conclusion:
>
> We need a different solution:
>
> 3) Introduce new terminal ioctl() or flag in the kernel. This flag will
> block TIOCSTI (and possibly other dangerous actions). It will allow to
> implement something like setsid(), but without side effects of job
> control loss.
>
> Pros:
> + No unwanted side effects at all.
>
> Cons:
> - Each affected utility needs fix.

Seems reasonable to me.

>
> We think, that only 3 will be safe and have no side effects.
>
>
> Note:
>
> Fixing character stealing described in previous mails is not covered by
> any of these solutions. This could be possible safely only with a new
> syscall revoke(), which was not yet accepted to the kernel.


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Ángel González]

I was thinking about this and the problem is actually that runuser 
returns (and control is returned to the privileged parent) while there's 
an unprivileged descendant with a handle to the tty.
Thus, it seems that it could be solved by having runuser run the child 
into a new cgroup and refusing to return while there's any remaining 
process there.


Although depending on the exact way that people is expecting to use job 
control, that might still interefere despite not changing the session 
leader. Do we know actual usages that should continue working?

Regards

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Stanislav Brabec]

On Mar 5, 2016 at 00:50 Ángel González wrote:
> I was thinking about this and the problem is actually that runuser
> returns (and control is returned to the privileged parent) while there's
> an unprivileged descendant with a handle to the tty.
> Thus, it seems that it could be solved by having runuser run the child
> into a new cgroup and refusing to return while there's any remaining
> process there.

In general, it is a good idea.

But from compatibility aspect, it is a bad idea to change it to the 
default behavior. Imagine all those poorly written legacy helpers that 
run daemons without proper disconnecting from the terminal. All those 
will be stalled.

I can imagine new --term-wait option, eventually --term-kill. But I am 
not sure, whether it is no over-complicated.

> Although depending on the exact way that people is expecting to use job
> control, that might still interefere despite not changing the session
> leader. Do we know actual usages that should continue working?
>
There is not exact list what will will break.

But imagine that many users use daily things like simple example below. 
(In a real life, you will not want to run sleep, but something more 
sophisticated (e. g. build process inside chroot that needs sudo).)

# sudo -u nobody sleep 10 &
[1] 28327
~ # fg
sudo -u nobody sleep 10
^Z
[1]+  Stopped                 sudo -u nobody sleep 10
~ # bg
[1]+ sudo -u nobody sleep 10 &
~ #
~ #
[1]+  Done                    sudo -u nobody sleep 10
~ #

It will be not possible any more with setsid().

You will get:
# sudo -u nobody sleep 10 &
[1] 28390
~ # fg
sudo -u nobody sleep 10
^Z^Z^Z^Z^Z^Z^Z^Z

You are stuck, ^Z does not work, and you cannot switch the task back to 
background. You can only press ^C.

Exactly the same will affect
runuser --command "sleep 10"

Additionally, both su and runuser already offer two variants. You can 
test the difference yourself:

Without job control:
su nobody --command "sleep 10" &
vs.
With job control:
su nobody --session-command "sleep 10" &

After calling it, type:
fg
and then Control-Z.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Karel Zak]

On Wed, Mar 02, 2016 at 08:35:54PM +0100, Stanislav Brabec wrote:
> There are some controversial things with the straightforward fix:
> 
> setsid() prevents TIOCSTI attack described in the report (easy to
> reproduce), but it has side effects: It disconnects the task from job
> control. With setsid(), ^Z cannot be used for sending the application
> to background any more (easy to reproduce by calling setsid()
> unconditionally in the same place).
> 
> su-common.c now calls setsid() only if new session is requested.

Yes, it's pretty stupid situation. 

We have exactly specified setsid() use-cases and now TIOCSTI ioctl
forces us to modify the things (and maybe introduce regressions),
because the crazy ioctl is not possible to disable by any another
way...

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Stanislav Brabec]

On Mar 7, 2016 at 14:13 Karel Zak wrote:
> On Wed, Mar 02, 2016 at 08:35:54PM +0100, Stanislav Brabec wrote:
>> There are some controversial things with the straightforward fix:
>>
>> setsid() prevents TIOCSTI attack described in the report (easy to
>> reproduce), but it has side effects: It disconnects the task from job
>> control. With setsid(), ^Z cannot be used for sending the application
>> to background any more (easy to reproduce by calling setsid()
>> unconditionally in the same place).
>>
>> su-common.c now calls setsid() only if new session is requested.
>
> Yes, it's pretty stupid situation.
>
> We have exactly specified setsid() use-cases and now TIOCSTI ioctl
> forces us to modify the things (and maybe introduce regressions),
> because the crazy ioctl is not possible to disable by any another
> way...

I would like to see a kernel support for selective disabling of TIOCSTI
without side effects like setsid() has.

setsid() fallback would be used for kernels that don't support it.

I am not sure, how complicated would be adding of such feature to the
kernel.

-- 
Best Regards / S pozdravem,

Stanislav Brabec
software developer
---------------------------------------------------------------------
SUSE LINUX, s. r. o.                         e-mail: sbrabec@suse.com
Lihovarská 1060/12                            tel: +49 911 7405384547
190 00 Praha 9                                 fax:  +420 284 084 001
Czech Republic                                    http://www.suse.cz/
PGP: 830B 40D5 9E05 35D8 5E27 6FA3 717C 209F A04F CD76

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Karel Zak]

On Tue, Mar 08, 2016 at 05:02:44PM +0100, Stanislav Brabec wrote:
> On Mar 7, 2016 at 14:13 Karel Zak wrote:
> > On Wed, Mar 02, 2016 at 08:35:54PM +0100, Stanislav Brabec wrote:
> > > There are some controversial things with the straightforward fix:
> > > 
> > > setsid() prevents TIOCSTI attack described in the report (easy to
> > > reproduce), but it has side effects: It disconnects the task from job
> > > control. With setsid(), ^Z cannot be used for sending the application
> > > to background any more (easy to reproduce by calling setsid()
> > > unconditionally in the same place).
> > > 
> > > su-common.c now calls setsid() only if new session is requested.
> > 
> > Yes, it's pretty stupid situation.
> > 
> > We have exactly specified setsid() use-cases and now TIOCSTI ioctl
> > forces us to modify the things (and maybe introduce regressions),
> > because the crazy ioctl is not possible to disable by any another
> > way...
> 
> I would like to see a kernel support for selective disabling of TIOCSTI
> without side effects like setsid() has.
> 
> setsid() fallback would be used for kernels that don't support it.
> 
> I am not sure, how complicated would be adding of such feature to the
> kernel.

I have applied patch based on libseccomp syscall filter:

   https://github.com/karelzak/util-linux/commit/8e4925016875c6a4f2ab4f833ba66f0fc57396a2

it works as expected, but IMHO it's workaround for our stupid kernel...

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Florian Weimer]

* Karel Zak:

> I have applied patch based on libseccomp syscall filter:
>
>    https://github.com/karelzak/util-linux/commit/8e4925016875c6a4f2ab4f833ba66f0fc57396a2
>
> it works as expected, but IMHO it's workaround for our stupid kernel...

How does this work?

Isn't it possible to pass the descriptor to another, unrestricted
process (perhaps spawned from cron) and then run the ioctl from there?

I'd also be concerned that the seccomp filters keep stacking up if you
do it this way.

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Karel Zak]

On Sun, Oct 02, 2016 at 03:16:00PM +0200, Florian Weimer wrote:
> * Karel Zak:
> 
> > I have applied patch based on libseccomp syscall filter:
> >
> >    https://github.com/karelzak/util-linux/commit/8e4925016875c6a4f2ab4f833ba66f0fc57396a2
> >
> > it works as expected, but IMHO it's workaround for our stupid kernel...
> 
> How does this work?
> 
> Isn't it possible to pass the descriptor to another, unrestricted
> process (perhaps spawned from cron) and then run the ioctl from there?

 Good point, I don't know. The question is how secure is TIOCSTI
 itself, does it work for arbitrary another process which without
 any relation to the original tty processes?

 The ioctl should be fired in the hell... :-)

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Karel Zak]

On Mon, Oct 03, 2016 at 12:28:30PM +0200, Karel Zak wrote:
> On Sun, Oct 02, 2016 at 03:16:00PM +0200, Florian Weimer wrote:
> > * Karel Zak:
> > 
> > > I have applied patch based on libseccomp syscall filter:
> > >
> > >    https://github.com/karelzak/util-linux/commit/8e4925016875c6a4f2ab4f833ba66f0fc57396a2
> > >
> > > it works as expected, but IMHO it's workaround for our stupid kernel...
> > 
> > How does this work?
> > 
> > Isn't it possible to pass the descriptor to another, unrestricted
> > process (perhaps spawned from cron) and then run the ioctl from there?
> 
>  Good point, I don't know. The question is how secure is TIOCSTI

 I have tried to send tty FD to another process by unix socket and the 
 ioctl result is EPERM. See the test_tiocsti below. It seems only root
 can do it (try suid the test program).

 session A:
    ./test_tiocsti --receive
 session B:
    runuser -u kzak -- ./test_tiocsti --send

>  The ioctl should be fired in the hell... :-)

 This is still true.

    Karel


diff --git a/tests/helpers/test_tiocsti.c b/tests/helpers/test_tiocsti.c
index c269dc0..5450b64 100644
--- a/tests/helpers/test_tiocsti.c
+++ b/tests/helpers/test_tiocsti.c
@@ -18,10 +18,213 @@
  */
 
 #include <sys/ioctl.h>
+#include <err.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <getopt.h>
+#include <unistd.h>
+#include <sys/socket.h>
+#include <sys/un.h>
 
-int main(void)
+#include "c.h"
+
+#define SOCKET_NAME "/tmp/TIOCSTI.socket"
+int connection_socket, data_socket;
+
+
+static void do_tiocsti(int fd, const char *cmd)
+{
+	printf("writing \"%s\" to file descriptor %d\n", cmd, fd);
+	while (*cmd) {
+		if (ioctl(fd, TIOCSTI, cmd++) < 0)
+			err(EXIT_FAILURE, "ioctl failed");
+	}
+}
+
+static int receive_tty_fd(void)
 {
-  char *cmd = "id -u -n\n";
-  while(*cmd)
-   ioctl(0, TIOCSTI, cmd++);
+	int fd;
+	struct sockaddr_un name;
+	struct msghdr msg;
+	struct iovec iov[1];
+	struct cmsghdr  *cmptr;
+	char buf[BUFSIZ];
+	ssize_t n;
+	union {
+		struct cmsghdr cm;
+		char control[CMSG_SPACE(sizeof(int))];
+	} control_un;
+
+	unlink(SOCKET_NAME);
+
+	connection_socket = socket(AF_UNIX, SOCK_SEQPACKET, 0);
+	if (connection_socket < 0)
+		err(EXIT_FAILURE, "socked failed");
+
+	memset(&name, 0, sizeof(struct sockaddr_un));
+	name.sun_family = AF_UNIX;
+	strncpy(name.sun_path, SOCKET_NAME, sizeof(name.sun_path) - 1);
+
+	if (bind(connection_socket, (const struct sockaddr *) &name,
+			sizeof(struct sockaddr_un)) < 0)
+		err(EXIT_FAILURE, "bind failed");
+
+	if (listen(connection_socket, 20) < 0)
+		err(EXIT_FAILURE, "listen failed");
+
+	data_socket = accept(connection_socket, NULL, NULL);
+	if (data_socket < 0)
+		err(EXIT_FAILURE, "accept failed");
+
+	msg.msg_control = control_un.control;
+	msg.msg_controllen = sizeof(control_un.control);
+	msg.msg_name = NULL;
+	msg.msg_namelen = 0;
+
+	iov[0].iov_base = buf;
+	iov[0].iov_len = sizeof(buf);
+	msg.msg_iov = iov;
+	msg.msg_iovlen = 1;
+
+	if ((n = recvmsg(data_socket, &msg, 0)) <= 0)
+		err(EXIT_FAILURE, "receive fd failed");
+
+	cmptr = CMSG_FIRSTHDR(&msg);
+
+	if (cmptr && cmptr->cmsg_len == CMSG_LEN(sizeof(int))) {
+
+		if (cmptr->cmsg_level != SOL_SOCKET)
+			err(EXIT_FAILURE, "control level != SOL_SOCKET");
+		if (cmptr->cmsg_type != SCM_RIGHTS)
+			err(EXIT_FAILURE, "control type != SCM_RIGHTS");
+		memcpy(&fd, CMSG_DATA(cmptr), sizeof(int));
+	} else
+		err(EXIT_FAILURE, "failed to parse message");
+
+	return fd;
+}
+
+static void send_tty_fd(int fd)
+{
+	struct sockaddr_un addr;
+	char buf[BUFSIZ];
+	struct msghdr   msg;
+	struct iovec    iov[1];
+	union {
+		struct cmsghdr    cm;
+		char              control[CMSG_SPACE(sizeof(int))];
+	} control_un;
+	struct cmsghdr  *cmptr;
+
+
+	data_socket = socket(AF_UNIX, SOCK_SEQPACKET, 0);
+	if (data_socket < 0)
+		err(EXIT_FAILURE, "socked failed");
+
+	memset(&addr, 0, sizeof(struct sockaddr_un));
+
+	addr.sun_family = AF_UNIX;
+	strncpy(addr.sun_path, SOCKET_NAME, sizeof(addr.sun_path) - 1);
+
+	if (connect(data_socket, (const struct sockaddr *) &addr,
+			sizeof(struct sockaddr_un)) < 0)
+		err(EXIT_FAILURE, "connect failed");
+
+	msg.msg_control = control_un.control;
+	msg.msg_controllen = sizeof(control_un.control);
+
+	cmptr = CMSG_FIRSTHDR(&msg);
+	cmptr->cmsg_len = CMSG_LEN(sizeof(int));
+	cmptr->cmsg_level = SOL_SOCKET;
+	cmptr->cmsg_type = SCM_RIGHTS;
+	memcpy(CMSG_DATA(cmptr), &fd, sizeof(int));
+
+	msg.msg_name = NULL;
+	msg.msg_namelen = 0;
+
+	iov[0].iov_base = buf;
+	iov[0].iov_len = sizeof(buf);
+	msg.msg_iov = iov;
+	msg.msg_iovlen = 1;
+
+	if (sendmsg(data_socket, &msg, 0) < 0)
+		err(EXIT_FAILURE, "sendmsg failed");
+}
+
+static void __attribute__((__noreturn__)) help(FILE *out)
+{
+	fprintf(out, " %s [options]\n", program_invocation_short_name);
+	fputs(" -t, --tty <path>   do TIOCSTI on the tty\n", out);
+	fputs(" -h, --help         this help\n", out);
+	fputs(" -r, --receive      listen and then do TIOCSTI on foreign FD\n", out);
+	fputs(" -s, --send         send tty FD\n", out);
+
+	exit(out == stderr ? EXIT_FAILURE : EXIT_SUCCESS);
+}
+
+enum {
+	ACT_DOIT,
+	ACT_SEND,
+	ACT_RECEIVE
+};
+
+int main(int argc, char *argv[])
+{
+	const char *ttyname = NULL;
+	int c, fd = 0, act = ACT_DOIT;
+	const char *cmd = "id -u -n\n";
+
+	static const struct option longopts[] = {
+		{ "tty",	1, 0, 't' },
+		{ "help",       0, 0, 'h' },
+		{ "receive",    0, 0, 'r' },
+		{ "send",       0, 0, 's' },
+		{ NULL, 0, 0, 0 },
+	};
+
+	while ((c = getopt_long(argc, argv, "rst:h", longopts, NULL)) != -1) {
+		switch(c) {
+		case 'r':
+			act = ACT_RECEIVE;
+			break;
+		case 's':
+			act = ACT_SEND;
+			break;
+		case 't':
+			ttyname = optarg;
+			break;
+		case 'h':
+			help(stdout);
+			break;
+		default:
+			help(stderr);
+		}
+	}
+
+
+	switch (act) {
+	case ACT_RECEIVE:
+		fd = receive_tty_fd();
+		do_tiocsti(fd, cmd);
+		close(data_socket);
+		close(connection_socket);
+		break;
+	case ACT_SEND:
+		send_tty_fd(0);
+		sleep(20);
+		break;
+	case ACT_DOIT:
+		if (ttyname) {
+			fd = open(ttyname, O_RDWR);
+			if (fd < 0)
+				err(EXIT_FAILURE, "%s: open failed", ttyname);
+		}
+		do_tiocsti(fd, cmd);
+		break;
+	}
+
+	return EXIT_SUCCESS;
 }

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Florian Weimer]

* Karel Zak:

>  I have tried to send tty FD to another process by unix socket and the 
>  ioctl result is EPERM. See the test_tiocsti below. It seems only root
>  can do it (try suid the test program).
>
>  session A:
>     ./test_tiocsti --receive

I think the recipient has to give up its controlling terminal, become
a session leader, and then reopen the passed terminal (from
/proc/self/fd, if it's not reachable from /dev/pts).  This way, you
should be able to do away with the root requirement.

>  session B:
>     runuser -u kzak -- ./test_tiocsti --send
>
>>  The ioctl should be fired in the hell... :-)
>
>  This is still true.

The kernel implementation doesn't even do error checking.  Surely it
can fail if there isn't enough memory in the destination buffer ...

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Karel Zak]

On Thu, Sep 29, 2016 at 04:40:15PM +0200, Karel Zak wrote:
> On Tue, Mar 08, 2016 at 05:02:44PM +0100, Stanislav Brabec wrote:
> > On Mar 7, 2016 at 14:13 Karel Zak wrote:
> > > On Wed, Mar 02, 2016 at 08:35:54PM +0100, Stanislav Brabec wrote:
> > > > There are some controversial things with the straightforward fix:
> > > > 
> > > > setsid() prevents TIOCSTI attack described in the report (easy to
> > > > reproduce), but it has side effects: It disconnects the task from job
> > > > control. With setsid(), ^Z cannot be used for sending the application
> > > > to background any more (easy to reproduce by calling setsid()
> > > > unconditionally in the same place).
> > > > 
> > > > su-common.c now calls setsid() only if new session is requested.
> > > 
> > > Yes, it's pretty stupid situation.
> > > 
> > > We have exactly specified setsid() use-cases and now TIOCSTI ioctl
> > > forces us to modify the things (and maybe introduce regressions),
> > > because the crazy ioctl is not possible to disable by any another
> > > way...
> > 
> > I would like to see a kernel support for selective disabling of TIOCSTI
> > without side effects like setsid() has.
> > 
> > setsid() fallback would be used for kernels that don't support it.
> > 
> > I am not sure, how complicated would be adding of such feature to the
> > kernel.
> 
> I have applied patch based on libseccomp syscall filter:
> 
>    https://github.com/karelzak/util-linux/commit/8e4925016875c6a4f2ab4f833ba66f0fc57396a2
> 
> it works as expected, but IMHO it's workaround for our stupid kernel...

 Reverted. We need something else, something better.

 I'll try to play su/runuser pty container to fix this issue, it seems
 sudo also support this use-case by use_pty flag.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Pádraig Brady]

On 03/10/16 16:04, Karel Zak wrote:
> On Thu, Sep 29, 2016 at 04:40:15PM +0200, Karel Zak wrote:
>> On Tue, Mar 08, 2016 at 05:02:44PM +0100, Stanislav Brabec wrote:
>>> On Mar 7, 2016 at 14:13 Karel Zak wrote:
>>>> On Wed, Mar 02, 2016 at 08:35:54PM +0100, Stanislav Brabec wrote:
>>>>> There are some controversial things with the straightforward fix:
>>>>>
>>>>> setsid() prevents TIOCSTI attack described in the report (easy to
>>>>> reproduce), but it has side effects: It disconnects the task from job
>>>>> control. With setsid(), ^Z cannot be used for sending the application
>>>>> to background any more (easy to reproduce by calling setsid()
>>>>> unconditionally in the same place).
>>>>>
>>>>> su-common.c now calls setsid() only if new session is requested.
>>>>
>>>> Yes, it's pretty stupid situation.
>>>>
>>>> We have exactly specified setsid() use-cases and now TIOCSTI ioctl
>>>> forces us to modify the things (and maybe introduce regressions),
>>>> because the crazy ioctl is not possible to disable by any another
>>>> way...
>>>
>>> I would like to see a kernel support for selective disabling of TIOCSTI
>>> without side effects like setsid() has.
>>>
>>> setsid() fallback would be used for kernels that don't support it.
>>>
>>> I am not sure, how complicated would be adding of such feature to the
>>> kernel.
>>
>> I have applied patch based on libseccomp syscall filter:
>>
>>    https://github.com/karelzak/util-linux/commit/8e4925016875c6a4f2ab4f833ba66f0fc57396a2
>>
>> it works as expected, but IMHO it's workaround for our stupid kernel...
> 
>  Reverted. We need something else, something better.
> 
>  I'll try to play su/runuser pty container to fix this issue, it seems
>  sudo also support this use-case by use_pty flag.

I was wondering would the secomp solution be (more) appropriate for runcon(1),
since that's selinux specific?

cheers,
pádraig

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Karel Zak]

On Mon, Oct 03, 2016 at 04:48:56PM +0100, Pádraig Brady wrote:
> On 03/10/16 16:04, Karel Zak wrote:
> > On Thu, Sep 29, 2016 at 04:40:15PM +0200, Karel Zak wrote:
> >> On Tue, Mar 08, 2016 at 05:02:44PM +0100, Stanislav Brabec wrote:
> >>> On Mar 7, 2016 at 14:13 Karel Zak wrote:
> >>>> On Wed, Mar 02, 2016 at 08:35:54PM +0100, Stanislav Brabec wrote:
> >>>>> There are some controversial things with the straightforward fix:
> >>>>>
> >>>>> setsid() prevents TIOCSTI attack described in the report (easy to
> >>>>> reproduce), but it has side effects: It disconnects the task from job
> >>>>> control. With setsid(), ^Z cannot be used for sending the application
> >>>>> to background any more (easy to reproduce by calling setsid()
> >>>>> unconditionally in the same place).
> >>>>>
> >>>>> su-common.c now calls setsid() only if new session is requested.
> >>>>
> >>>> Yes, it's pretty stupid situation.
> >>>>
> >>>> We have exactly specified setsid() use-cases and now TIOCSTI ioctl
> >>>> forces us to modify the things (and maybe introduce regressions),
> >>>> because the crazy ioctl is not possible to disable by any another
> >>>> way...
> >>>
> >>> I would like to see a kernel support for selective disabling of TIOCSTI
> >>> without side effects like setsid() has.
> >>>
> >>> setsid() fallback would be used for kernels that don't support it.
> >>>
> >>> I am not sure, how complicated would be adding of such feature to the
> >>> kernel.
> >>
> >> I have applied patch based on libseccomp syscall filter:
> >>
> >>    https://github.com/karelzak/util-linux/commit/8e4925016875c6a4f2ab4f833ba66f0fc57396a2
> >>
> >> it works as expected, but IMHO it's workaround for our stupid kernel...
> > 
> >  Reverted. We need something else, something better.
> > 
> >  I'll try to play su/runuser pty container to fix this issue, it seems
> >  sudo also support this use-case by use_pty flag.
> 
> I was wondering would the secomp solution be (more) appropriate for runcon(1),
> since that's selinux specific?

For me seccomp is ugly solution, because it does not resolve the core
of the issue. It's extra barrier and nothing more. It's the same like
ignore security bugs in network daemons, because you have iptables...

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: Fixing su + runuser vulnerability CVE-2016-2779

[Karel Zak]

On Wed, Mar 02, 2016 at 08:35:54PM +0100, Stanislav Brabec wrote:
> Another possible fixes would be:
> - Or create custom pty container (like script does).

note that for pty container you have to call setsid() otherwise in
session running shell has to control the terminal.

...
> To prevent unwanted reading from a terminal, one would:
> - Or create custom pty container (like script does).

 I'd like to cleanup all su(1) code, use signalfd and then add new
 features (for v2.30), because the current code is horrible from my
 point of view. 
 
 Anyway, I have tried to implement experimental --pty option to the
 current code. See su-pty branch on github. (It's very raw...).
 
 The problem is again (like script(1)) non-terminal stdin 
 and pty session mix

    echo AxA | su - kzak --pty -c "sed 's/x/A/'"

 this use-case is disabled for now.

 # tty
 /dev/pts/0
 
 # runuser -u kzak tty 
 /dev/pts/0

 # runuser -u kzak --pty tty 
 /dev/pts/4

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com