install-setuid target

[haampie]

Convenience target to make ste setuid:

make
sudo make install-setuid prefix=...

Depends on #11

[fabiensanglard]

Can you please squash all your changes in one commit? Some commit changes each others and it is difficult to review.

[haampie]

done ^

[denilsonsa]

Please check if it behaves as expected and if it doesn't introduce any vulnerability when installing as setuid.

When requiring sudo, only users who already have super-user access can run the tool. However, installing as setuid means any user can run this tool as super-user, and since this tool essentially launches any arbitrary command, it can allow any user to run any arbitrary command as super-user. Yikes!

Let's look at the code:

st/code/src/utils.cpp

Lines 36 to 39 in 8e680b6

	if (sudo_uid == nullptr || sudo_gid == nullptr) {
	// Not running with sudo, user is root and nothing we can drop here.
	return;
	}

The code checks for a couple of sudo-related env variables. If they are not there, the code doesn't try dropping any root privileges, which is precisely the vulnerability I was talking about. In fact, we can test it right now:

$ sudo ./ste-without-setuid whoami
EXEC: [ whoami]
denilsonsa             ← Good
Num threads = 1
Num process = 1
…

$ ./ste-with-setuid whoami
EXEC: [ whoami]
root                   ← DANGER
Num threads = 1
Num process = 1
…

So, before merging this PR, the code must be updated to properly drop privileges when launched without sudo.

[haampie]

To be honest I did not really read the code that dropped privileges.

Does the latest commit address your issue? Same code path likely works for sudo and for setuid

[denilsonsa]

I'm not super-familiar with the APIs, so I can't judge if this is the best approach.

However, this latest commit changed the sudo behavior. Not sure if it's good or bad. Look:

$ whoami
denilsonsa

$ sudo whoami
root

$ ./ste-setuid whoami
EXEC: [ whoami]
denilsonsa
…

$ sudo ./ste whoami
EXEC: [ whoami]
root
…

This means the command will be running as root, when ste is invoked from sudo, and that wasn't @fabiensanglard's original behavior. I can't decide if it's better. It looks more consistent, though.

---

Maybe a better approach could be to detect when it's going to run the child process as root and only proceed if a new --run-as-root parameter is present. My rationale is to provide with sane and safe defaults, and prevent people from shooting themselves in the foot.

[haampie]

To me this behavior looks better. If you run a command under sudo, I would expect the subprocesses to run as root too. It's confusing that sudo mkdir /root/x works but sudo ste mkdir /root/x does not.

[fabiensanglard]

As of d086dfd there are four ways to run ste.

• No root privileges -> ste fails because it needs it.
• Using sudo -> ste drops privileges to run the command.
• Using set-user-id -> ste drops privileges to run the command.
• In the rare case user needs to run a command as root, they can sudo bash to make sure privileges can't be dropped.

I base this behavior on two factors.

1/ I don't think it is common for an user to want a command to run as root.
2/ I prefer to protect users from the common case where ste is run via sudo or when it is installed with set-user-id.

[fabiensanglard]

If you want to submit an individual PR for the Makefile install with set-user-id, I would gladly merge it.

[glitsj16]

As of d086dfd there are four ways to run ste.
No root privileges -> ste fails because it needs it.
Using sudo -> ste drops privileges to run the command.
Using set-user-id -> ste drops privileges to run the command.
In the rare case user needs to run a command as root, they can sudo bash to make sure privileges can't be dropped.

An alternative approach to mitigate potential setuid vulnerabilities is to use a second group on the ste binary:

instead of

chown root:root /usr/bin/ste
chmod 4755 /usr/bin/ste

implement like

chown root:ste /usr/bin/ste
chmod 4750 /usr/bin/ste

That way admins have a way to limit access to the setuid binary via standard groupadd procedure.