spkr-beep / beep

beep is a command line tool for linux that beeps the PC speaker
https://github.com/spkr-beep/beep
GNU General Public License v2.0
68 stars 11 forks source link

Running under sudo is not synonymous with the program having extra privileges #5

Closed lighterowl closed 5 years ago

lighterowl commented 5 years ago

In my /etc/rc.local, I start a tmux session via the following command :

sudo -u daniel -- tmux new-session -d -s irc /usr/bin/irssi

The tmux daemon is thus started and permanently running while managing the irc session, which runs /usr/bin/irssi. However, this also means that that session - and every new session I create, since tmux apparently fills the sessions' environment with the same variables that the tmux daemon had while it started - contains the following variables :

$ printenv | grep SUDO
SUDO_GID=0
SUDO_COMMAND=/usr/bin/tmux new-session -d -s irc /usr/bin/irssi
SUDO_USER=root
SUDO_UID=0

This immediately means that running beep in any of these tmux sessions is impossible due to it checking the existence of every one of these variables.

Since sudo is used with -u in this case, which actually limits the privileges of the created process and ties the tmux session to the proper user, what is the danger related to disallowing running beep?

Thanks.

ndim commented 5 years ago

Thanks for pointing this out.

The intention of the beep setuid-root and sudo checks is to avoid running any potentially risky code if beep was invoked by some user with beep having gained elevated priviledges via setuid-root or sudo.

I have written the setuid-root and sudo checks quickly and without code reviews to prevent users from being exposed to the risks of running beep with elevated priviledges pointed out by CVE-2018-0492 and CVE-2018-1000532. So I wanted those checks simple and obvious to avoid introducing more potentially exploitable bugs, and to avoid any more delays in releasing a beep release with the two CVEs fixed, while still catching all existing beep users (and packagers) who used to use setuid-root and sudo-root routinely as those were the recommended way to run beep back in the day.

Although I want to avoid making the setuid and sudo checks in beep more complicated (they are supposed to run as root without exposing any more attack surface than absolutely necessary), allowing a few more cases of beep being run via sudo could make sense:

I am tending to the latter. Any comments?

FWIW, I personally still have my user's tmux session start from a script run from said user's crontab every few minutes. This has the additional advantage that my user does not require any elevated priviledges to change anything about it:

if tmux has-session -t auto-user-session > /dev/null 2>&1; then
        :
else
        tmux new-session -d -s auto-user-session -n irssi irssi
        tmux new-window  -d -t auto-user-session
fi

(I have not read up on systemd user sessions and services and stuff to a degree which could allow me to replace that hack.)

lighterowl commented 5 years ago

As far as I'm concerned, every environment variable is brittle by definition. If somebody's very inclined to run the program via sudo, they can just as well unexport these variables and beep will never have a chance to realise what's going on. So it sounds to me like protecting the program from people who know how fairly uncomplicated it is to bypass these checks. The second idea definitely sounds much better to me.

I have to admit that I wasn't aware of the old console API requiring root access, but that explains why the program doesn't just quit if geteuid/getegid return zero, indicating root. Perhaps these checks could become a part of the driver API, like a new function called is_allowed or something? This way you could entirely disallow running as root with the evdev API, and disallow setuid only in the console API - without those two getting in the way of one another. If a new function doesn't sound good, these checks could just as well go into driver_detect.

What do you think?

ndim commented 5 years ago

I want a properly logged in root user to be able to run beep, so always quitting if geteuid()==0 or getegid()==0 is not an option.

Also, I definitely want to avoid the added complexity of moving those checks to a place later in the program (and FWIW, I would like to remove the old API in the longer run).

So I am making the SUDO_* check conditional on running as root. That will allow beep to run as non-root user when sudo is involved, but it still catches those old installations which might have just updated beep without updating their setuid-root or sudo-root setup for running beep.

On a less beep related note, which is still related to your original problem: IMHO, the fact that your tmux session has strange variables like SUDO_* set indicates to me that this is not a very clean way to start a tmux session. There might be other issues with that session not being a proper user session.