search

Scribery / tlog

Terminal I/O logger
http://scribery.github.io/tlog/
GNU General Public License v2.0
311 stars 52 forks source link

Implement privilege separation in tlog-rec #5

Closed spbnick closed 7 years ago

spbnick commented 8 years ago

Make tlog-rec executable SUID/SGID to a dedicated user/group. Drop back to the original user/group before exec'ing the shell in the fork. This will keep tlog-rec safe from the modification by the recorded user and would also allow authenticated filtering of the log messages.

Upon RPM installation, a special user and group both named tlog should be created, and tlog-rec should be owned by them and marked SUID and SGID.

After forking the shell process, tlog-rec should drop to the real UID/GID with setuid(getuid()) and setgid(getgid()).

For additional security, consider retreiving the effective user/group IDs and setting them to the real IDs with seteuid/setegid right after starting, passing them around (say as euid/egid) and only setting them back with seteuid/setegid when necessary. I.e. when reading the configuration and when opening the logging socket/file.

Also consider calling setreuid(euid, euid)/setregid(egid, egid) in the parent process after forking the shell, so the parent (recording) process would not be able to regain the user's real IDs. However, ignore EPERM, perhaps producing a warning, as POSIX says that might not be permitted, even though it works on Linux and some BSDs.

To verify:

lslebodn commented 8 years ago

You might want to drop supplementary groups with "setgroups()". It is especially useful for increasing privileges. We already have some code in sssd. You might inspire. https://github.com/SSSD/sssd/blob/master/src/util/become_user.c

It would be good to to clear/reduce environment variables "clearenv()" as a first think in binaries with SUID adn SGID. It is also good to use restrictive umask for such binaries.

I am not sure how much time do you have but it would be good to use linux capabilities as a hardening. For changing UID/GID it should be enough to have (CAP_SETGID and CAP_SETUID). I can be achieved with libcap/libcap-ng or it can be done in distribution specific packaging. The same as ping on fedora. BTW *BSD or platforms can still relay on SUID and SGID;

sh$ getcap /usr/bin/ping /usr/bin/ping = cap_net_admin,cap_net_raw+ep

It's possible to find more info in man 7 capabilities

spbnick commented 8 years ago

Thanks a lot for review, Lukas!

Yes, we can drop the supplementary groups in the original (recording) process after forking the child, good idea.

We cannot clear the environment variables as the first thing, because we need to pass them to the child, recorded shell. However, we can remove any tlog-specific variables in the fork, before executing the child, and we can remove everything unnecessary in the parent after forking the child. We also have issue #3 tracking that.

Since we're started in place of the user shell, we're started with the target user's credentials and we cannot set any capabilities in general. That's why tlog-rec must be SUID/SGID to a special user to access special resources and be protected from the recorded user. So, if I understand it correctly, we don't need to change capabilities, as we don't need, and don't get any. Also, tlog-rec shouldn't ever run as a superuser, unless it is set SUID/SGID root, or the logging-in user is root, but that's an exception and shouldn't happen.

However, thanks for the mention of the capabilities, I used that opportunity to learn more about them :)

spbnick commented 7 years ago

Ah, right, tlog can't drop the supplementary groups with setgroups, because it doesn't run as root.

lslebodn commented 7 years ago

Ah, right, tlog can't drop the supplementary groups with setgroups, because it doesn't run as root.

hmm, I'm confused. It can call setresgid and setresuid but it cannot call setgroups. That looks suspicious.

spbnick commented 7 years ago

Ah, yeah, setresgid/uid can do that. From their manpage:

An unprivileged process may change its real UID, effective UID, and saved set-user-ID, each to one of: the current real UID, the current effective UID or the current saved set-user-ID.

lslebodn commented 7 years ago

The man page for setresuid/gid says that it require capabilities CAP_SETGID and CAP_SETUID. The man page for setgroups says that it requires capability CAP_SETGID,

And with suid/sgid bit you are roll with all calabilities

spbnick commented 7 years ago

Hmm, sorry, I'm not sure what you mean. Could you please elaborate? Thank you.

lslebodn commented 7 years ago

I confused as well. As I already eplained tlog has enough rights to run setgroups. So could you explain where is the problem?

spbnick commented 7 years ago

Hmm, perhaps I ran setgroups with EUID/EGID set to RUID/RGID and that's why setgroups returned EPERM. I'll check if it works with original EUID/EGID.

lslebodn commented 7 years ago

On (19/01/17 07:55), Nikolai Kondrashov wrote:

Hmm, perhaps I ran setgroups with EUID/EGID set to RUID/RGID and that's why setgroups returned EPERM. I'll check if it works with original EUID/EGID.

Yes, it should be called before setresuid/gid

LS

spbnick commented 7 years ago

Nope, EPERM, even if called right at the start of main(), no matter SUID/SGID, or not.

spbnick commented 7 years ago

Oh, and yeah, I'm calling setgroups(0, NULL).

lslebodn commented 7 years ago

On (19/01/17 08:20), Nikolai Kondrashov wrote:

Oh, and yeah, I'm calling setgroups(0, NULL).

That's weird. We call setgroups(0, NULL) in sssd in setuid process.

/usr/libexec/sssd/krb5_child (via become_user) and it does not cause any problem. https://git.fedorahosted.org/cgit/sssd.git/tree/src/providers/krb5/krb5_become_user.c?id=bdf63b2c329f12b4cdfcc04122f4547aad6bfa35#n43

I am out of ideas.

spbnick commented 7 years ago

Which UIDs/GIDs krb5_child has when it invokes setgroups?

lslebodn commented 7 years ago

On (19/01/17 10:24), Nikolai Kondrashov wrote:

Which UIDs/GIDs krb5_child has when it invokes setgroups?

sssd_be is running as non-privileged user "sssd". It spawns the krb5_child which has suid bit %attr(4750,root,sssd)

Does it answer your question?

LS

spbnick commented 7 years ago

Right, so it's suid-root, which is different from suid to a regular user, and that's why I assume setgroups works there and not in tlog. Yeah, you answered my question, thank you :)

spbnick commented 7 years ago

Once I change the owner of tlog-rec to root, setgroups works fine.