Harden dhcp by checking for /sbin/dhclient

[kris7t]

In order to avoid picking up some random executable from $PATH and potentially running it root privileges, dhcp.c should check for /sbin/dhclient or /usr/sbin/dhclient directly. This might cause problems on distributions where dhclient is located elsewhere, but as far as I know, /sbin is the default location (and even Arch, which uses only /usr/bin, symlinks /sbin to /usr/bin). This is the same approach that netfilter.c takes when running iptables-restore.

I formulated the check for root privileges according to https://github.com/netblue30/firejail/blob/master/src/firejail/x11.c#L1103, which allows the binary to either be owned by root, or the root group. A more paranoid version would use open, fstat and fexecve to make sure ownership of the binary does not change between the stat and exec calls. On the other hand, manipulation of a binary already owned by root means the system is already compromised, so there's probably not much point to be careful about TOCTOU here. netfiler.c even skips the check altogether, and relies on /sbin being only writable by root.

Hopefully, we don't blindly execute a binary with root privileges anywhere else in the codebase.

A more meta question: what is the policy with running external executable from firejail with root privilages (even if capability and seccomp sandboxed)? The potential for introducing setuid-related vulnerabilities is high. If we want to integrate additional external tools (like xdg-dbus-proxy), we should try to keep it reasonably safe.

[smitsohu]

Looks good. Thanks for finding this!

[smitsohu]

IMHO sbox is not really in the position to figure out what to execute. Maybe we should replace the execvp with execv?

[kris7t]

Yeah, execv sounds much safer. We could even move the uid check to sbox, since it would receive the absolute path of the executable.

[smitsohu]

Hopefully, we don't blindly execute a binary with root privileges anywhere else in the codebase.

Quickly checked and there seem to be no other instances. Also means that replacing execvp with execv is indeed a trivial thing.

We could even move the uid check to sbox, since it would receive the absolute path of the executable.

👍

[smitsohu]

Merged, thanks!

[smitsohu]

I'm going to remove the execvp in a separate patch.

[kris7t]

@smitsohu Oh, I am also trying to put together a patch for that -- with some extra checking. Playing around with some symlinks, I think the paranoia with fecexve might be warranted, although I am still figuring out how to use it properly.

[smitsohu]

@kris7t Go ahead! I did not start yet :)