Opened 11 years ago

Last modified 7 years ago

#848 closed defect (Fixed)

Fix switch_id to properly drop euid/uid/egid/gid

Reported by: ioerror Owned by:
Priority: High Milestone:
Component: Core Tor/Tor Version: 0.2.0.31
Severity: Keywords:
Cc: ioerror, arma, sjmurdoch, nickm Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I spent the day with Theo De Raadt and he pointed out that we incorrectly try to drop uid/gid. I'll attach a patch that fixes this.

[Automatically added by flyspray2trac: Operating System: All]

Child Tickets

Attachments (9)

tor_switch_id.patch (4.1 KB) - added by ioerror 11 years ago.
Patch to fix switch_id()
tor_switch_id-02.patch (5.1 KB) - added by ioerror 11 years ago.
A second try for style and function points.
tor_switch_id-03.patch (5.1 KB) - added by ioerror 11 years ago.
Proper patch without whitespace error
combined.patch (6.9 KB) - added by ioerror 11 years ago.
Patch with logging of all egid/euid/uid/gid information and a proper switch_id() by sjmurdoc
change-uid-and-log.patch (10.8 KB) - added by sjmurdoch 11 years ago.
Patch queue based on tor_switch_id-03.patch to log changes
tor_switch_id-04.patch (7.8 KB) - added by ioerror 11 years ago.
Patch based on the last patch by Steven Murdoch. We're getting closer. We need to test this on *BSD.
tor_switch_id-05.patch (9.5 KB) - added by sjmurdoch 11 years ago.
MQ patch queue, re-ordering changes and restoring OpenBSD comment
tor_switch_id-06.patch (10.8 KB) - added by sjmurdoch 11 years ago.
Now handles lack of getres(u|g)id on MacOS X and confirming that seteuid(0) fails after priv-drop
handle-double-changes.patch (1.5 KB) - added by sjmurdoch 11 years ago.
Patch on r17207 to allow multiple changes of user name

Download all attachments as: .zip

Change History (32)

Changed 11 years ago by ioerror

Attachment: tor_switch_id.patch added

Patch to fix switch_id()

comment:1 Changed 11 years ago by ioerror

The specific order and code was inspired by the code in OpenBSD's ntp.c:
http://www.openbsd.org/cgi-bin/cvsweb/~checkout~/src/usr.sbin/ntpd/ntp.c?rev=1.108;content-type=text%2Fplain

This was at the suggestion by Theo. The patch will only allow a user to switch to a group that is their primary group for their uid.

comment:2 Changed 11 years ago by arma

Style comments on the patch:

1)
+ *msg = tor_strdup("Problem with User value. "

"See logs for details.");

these can be put onto the same line, now that the first one is shorter

2)
+/ Call setuid and setgid to run as <b>user</b> and only switch to their
+ * primary <b>group</b>. Return 0 on success. On failure, log and return -1.
now that 'group' isn't an argument to the function, it shouldn't be in b and /b

3)
+ log_warn(LD_CONFIG,"Error setting configured user: '%s' not found.", user);
is long enough that it should be line-wrapped. You can notice this sort of this
with 'make check-spaces'

4)
Now the user arg to switch_id() is mandatory. You should just tor_assert() it
at the beginning of the function, rather than warning and returning -1 if it's null.

5)
the indentation is non-standard in the new setgroups().... clause.

6)
in the ifdef windows case, it still says (void)group; even though the group arg
is gone now.

comment:3 Changed 11 years ago by arma

As you mentioned on #tor-dev, at some point you should

7) add a changelog entry

8) patch the man page

now that i mention it, 9) there's another place in config.c where Group is
mentioned, and should be removed

comment:4 Changed 11 years ago by ioerror

Ok, I've made an update to the patch to address issues 1-9 that you've mentioned. It appears that 'make check-spaces' is

now happy with the files I've touched. The man page now removes the 'group' option and better explains the behavior of

the 'user' option. I'm not totally sure if you'll be happy with how I reformatted for 5, perhaps you can give me some
more feedback on that?

My change log is as follows:

"I spent the day with Theo De Raadt in Calgary. He pointed out that we incorrectly try to drop uid/gid. We came up with
the following patch to address the issue. Its logic comes from the OpenBSD ntpd.c code as he felt that it correctly
handled the issue well. This patch changes switch_id() to now only take a single argument, the username. From that
username, we derive the primary group for said user. The function then does the following dance of setsgroups(),
setegid(), setgid(), seteuid() and finally it does a setuid(). If it has any issues with any of those calls, it bails out
and fails to drop privs."

Changed 11 years ago by ioerror

Attachment: tor_switch_id-02.patch added

A second try for style and function points.

Changed 11 years ago by ioerror

Attachment: tor_switch_id-03.patch added

Proper patch without whitespace error

comment:5 Changed 11 years ago by ioerror

sj made a nice combined patch and I'll attach it here. It's working properly on linux and logging nicely to the terminal (before we init logging).

Changed 11 years ago by ioerror

Attachment: combined.patch added

Patch with logging of all egid/euid/uid/gid information and a proper switch_id() by sjmurdoc

Changed 11 years ago by sjmurdoch

Attachment: change-uid-and-log.patch added

Patch queue based on tor_switch_id-03.patch to log changes

comment:6 Changed 11 years ago by ioerror

I've modified Sj's patch to be more verbose about which system calls might fail. This will help us as we try to debug this on other platforms. Here's what it looks like now when Tor starts:

Nov 04 18:18:07.724 [notice] Tor v0.2.1.6-alpha-dev (r17188). This is experimental software. Do not rely on it for strong anonymity. (Running on Linux i686)
Nov 04 18:18:07.724 [warn] Skipping obsolete configuration option 'Group'
Nov 04 18:18:07.725 [notice] Initialized libevent version 1.1a using method epoll. Good.
Nov 04 18:18:07.725 [notice] Opening Socks listener on 127.0.0.1:9050
Nov 04 18:18:07.725 [warn] UID is 0 (real), 0 (effective), 0 (saved)
Nov 04 18:18:07.725 [warn] GID is 0 (real), 0 (effective), 0 (saved)
Nov 04 18:18:07.725 [warn] Supplementary groups are: 0
Nov 04 18:18:07.726 [warn] UID is 110 (real), 110 (effective), 110 (saved)
Nov 04 18:18:07.726 [warn] GID is 113 (real), 113 (effective), 113 (saved)
Nov 04 18:18:07.726 [warn] Supplementary groups are: 113

I'll upload the patch now.

Changed 11 years ago by ioerror

Attachment: tor_switch_id-04.patch added

Patch based on the last patch by Steven Murdoch. We're getting closer. We need to test this on *BSD.

comment:7 Changed 11 years ago by ioerror

Bill Paul tested this for us on FreeBSD 7 and it appears to work as expected with one issue. It appears that we don't reset the environment (thus Tor tries to do something expected but incorrect) and Tor attempts to open /root/.tor rather than the ~/ of the uid we drop to:

<Evil-Bill> In util.c:expand_filename(), it says:
<Evil-Bill> home = getenv("HOME");
<Evil-Bill> if (!home) {
<Evil-Bill> log_warn(LD_CONFIG, "Couldn't find $HOME environment variable while "
<Evil-Bill> "expanding \"%s\"", filename);
<Evil-Bill> return NULL;
<Evil-Bill> Thing is, the environment doesn't change when you switch UIDs.
<Evil-Bill> I would get the home directory using getpwent().
<Evil-Bill> Oh, wait... it does have code to do that.

Here's the output of his attempts to drop privs (this part works, yay):
u/wpaul/tor/tor/src/common:mini{48}% uname -a
FreeBSD mini 7.0-RELEASE FreeBSD 7.0-RELEASE #4: Sun Oct 19 02:31:45 PDT 2008 root@mini:/usr/src/sys/i386/compile/MINI i386

Nov 04 22:02:27.675 [notice] Tor v0.2.1.6-alpha-dev (r17188). This is experimental software. Do not rely on it for strong anonymity. (Running on FreeBSD i386)
Nov 04 22:02:27.686 [warn] Skipping obsolete configuration option 'Group'
Nov 04 22:02:27.690 [notice] Initialized libevent version 1.3d using method kqueue. Good.
Nov 04 22:02:27.690 [notice] Opening Socks listener on 127.0.0.1:9050
Nov 04 22:02:27.691 [warn] UID is 0 (real), 0 (effective), 0 (saved)
Nov 04 22:02:27.691 [warn] GID is 0 (real), 0 (effective), 0 (saved)
Nov 04 22:02:27.692 [warn] Supplementary groups are: 0 0 5
Nov 04 22:02:27.705 [warn] UID is 1001 (real), 1001 (effective), 1001 (saved)
Nov 04 22:02:27.705 [warn] GID is 1001 (real), 1001 (effective), 1001 (saved)
Nov 04 22:02:27.705 [warn] Supplementary groups are: 1001
Nov 04 22:02:29.144 [notice] I learned some more directory information, but not enough to build a circuit: We have no network-status consensus.
Nov 04 22:02:29.144 [notice] Bootstrapped 5%: Connecting to directory server.
Nov 04 22:02:29.340 [notice] Bootstrapped 10%: Finishing handshake with directory server.
Nov 04 22:02:31.717 [notice] Bootstrapped 15%: Establishing an encrypted directory connection.
Nov 04 22:02:33.678 [notice] Bootstrapped 20%: Asking for networkstatus consensus.
Nov 04 22:02:33.891 [notice] Bootstrapped 25%: Loading networkstatus consensus.
Nov 04 22:02:43.596 [notice] This version of Tor (0.2.1.6-alpha-dev) is newer than any recommended version, according to the directory authorities. Recommended versions are: 0.1.2.17,0.1.2.18,0.1.2.19,0.2.0.26-rc,0.2.0.27-rc,0.2.0.28-rc,0.2.0.29-rc,0.2.0.30,0.2.0.31,0.2.1.1-alpha,0.2.1.2-alpha,0.2.1.4-alpha,0.2.1.5-alpha,0.2.1.6-alpha
Nov 04 22:02:43.598 [notice] Bootstrapped 45%: Asking for relay descriptors.
Nov 04 22:02:43.598 [notice] I learned some more directory information, but not enough to build a circuit: We have only 0/1102 usable descriptors.
Nov 04 22:02:46.450 [notice] Bootstrapped 50%: Loading relay descriptors.
Nov 04 22:02:51.482 [notice] Bootstrapped 60%: Loading relay descriptors.
Nov 04 22:02:51.483 [notice] I learned some more directory information, but not enough to build a circuit: We have only 96/1102 usable descriptors.
Nov 04 22:02:53.306 [notice] Bootstrapped 69%: Loading relay descriptors.
Nov 04 22:02:53.307 [notice] I learned some more directory information, but not enough to build a circuit: We have only 186/1102 usable descriptors.
Nov 04 22:02:56.321 [notice] We now have enough directory information to build circuits.
Nov 04 22:02:56.321 [notice] Bootstrapped 80%: Connecting to the Tor network.
Nov 04 22:02:56.751 [notice] Bootstrapped 85%: Finishing handshake with first hop.
Nov 04 22:02:58.034 [notice] Bootstrapped 90%: Establishing a Tor circuit.
Nov 04 22:03:01.375 [notice] Tor has successfully opened a circuit. Looks like client functionality is working.
Nov 04 22:03:01.375 [notice] Bootstrapped 100%: Done.

comment:8 Changed 11 years ago by sjmurdoch

I chatted to Robert Watson about this bug, and he commented that it was really hard to get this right. It's even harder

to get it right on multiple platforms. The problem is that the OS has many different types of privileged credentials,

in addition to UIDs and GIDs. For example, there are open file handles, resource limits, data mapped into memory. These
are set up properly on exec(), so a more robust solution would be to have a privileged program which opens the socket,
then exec()s Tor as an unprivileged user, and passes it the open socket.

That said, we can do better than what Tor currently does. So I think we're on the right track for fixing the privilege
dropping code. But we should be wary of recommending it as a robust solution. I think for the moment, starting Tor as
non-root, and using firewall rules to redirect the low ports, sounds a better recommended approach.

comment:9 Changed 11 years ago by sjmurdoch

Tests on FreeBSD 7.0
====================

Original Tor


Running Tor as root, and changing to user/group nobody/nobody, the original Tor code correctly changes all the UIDs and GIDs, but leaves root's groups (root and operator) in the supplementary groups.

If the Tor executable is setuid, it retains the supplementary groups of the user which launched Tor. It still correctly changes all the UIDs and GIDs.

Patched Tor


With the patch (which compiles cleanly) Tor does the right thing:

$ tor --User nobody --Group nobody
...
Nov 05 14:33:09.679 [notice] Opening Socks listener on 127.0.0.1:9050
Nov 05 14:33:09.681 [notice] UID is 0 (real), 0 (effective), 0 (saved)
Nov 05 14:33:09.681 [notice] GID is 0 (real), 0 (effective), 0 (saved)
Nov 05 14:33:09.682 [notice] Supplementary groups are: 0 0 5
Nov 05 14:33:09.682 [notice] Changing user and groups
Nov 05 14:33:09.692 [notice] UID is 65534 (real), 65534 (effective), 65534 (saved)
Nov 05 14:33:09.693 [notice] GID is 65534 (real), 65534 (effective), 65534 (saved)
Nov 05 14:33:09.693 [notice] Supplementary groups are: 65534
Nov 05 14:33:09.695 [notice] Parsing GEOIP file.
...

comment:10 Changed 11 years ago by sjmurdoch

Tests on OpenBSD 4.3
====================

Original Tor


Same has FreeBSD for both running as root and setuid

Patched Tor


Same as FreeBSD:

$ tor --User nobody --Group nobody
...
Nov 05 17:19:57.144 [notice] Opening Socks listener on 127.0.0.1:9050
Nov 05 17:19:57.145 [notice] UID is 0 (real), 0 (effective), 0 (saved)
Nov 05 17:19:57.145 [notice] GID is 0 (real), 0 (effective), 0 (saved)
Nov 05 17:19:57.146 [notice] Supplementary groups are: 0 2 3 4 5 20 31
Nov 05 17:19:57.146 [notice] Changing user and groups
Nov 05 17:19:57.148 [notice] UID is 32767 (real), 32767 (effective), 32767 (saved)
Nov 05 17:19:57.148 [notice] GID is 32767 (real), 32767 (effective), 32767 (saved)
Nov 05 17:19:57.148 [notice] Supplementary groups are: 32767
Nov 05 17:19:57.150 [notice] Parsing GEOIP file.
...

Changed 11 years ago by sjmurdoch

Attachment: tor_switch_id-05.patch added

MQ patch queue, re-ordering changes and restoring OpenBSD comment

comment:11 Changed 11 years ago by sjmurdoch

I've just uploaded a new version of the patch -- tor_switch_id-05.patch -- as tested in the two comments above.

It now:

  • uses log_notice for debugging information (the lowest log level printed at this stage in application startup)
  • prints "Changing user and groups"
  • restores the ordering of setuid and seteuid in the OpenBSD comment
  • splits the patch up more logically

I've got a couple of questions

  • Do you know why OpenBSD does seteuid first, then setuid? The other order seems to work on Linux, FreeBSD and OpenBSD.
  • Do you know why letting users specify a group is a bad idea?

comment:12 Changed 11 years ago by ioerror

The patch appears to break for Mac OS X leopard builds (it looks like getresuid() is missing):

gcc -DHAVE_CONFIG_H -I. -I../.. -I../common -g -O2 -Wall -g -O2 -MT compat.o -MD -MP -MF .deps/compat.Tpo -c -o compat.o compat.c
compat.c: In function 'log_credential_status':
compat.c:945: warning: implicit declaration of function 'getresuid'
compat.c:953: warning: implicit declaration of function 'getresgid'
mv -f .deps/compat.Tpo .deps/compat.Po

[...]

Undefined symbols:

"_getresgid", referenced from:

_log_credential_status in libor.a(compat.o)

"_getresuid", referenced from:

_log_credential_status in libor.a(compat.o)

ld: symbol(s) not found
collect2: ld returned 1 exit status
make[3]: * [tor] Error 1
make[2]:
* [all-recursive] Error 1
make[1]: * [all-recursive] Error 1
make:
* [all] Error 2

comment:13 Changed 11 years ago by ioerror

I talked at length with Theo again today and he suggests that we look at the uidswap.c code in portable OpenSSH. I think we'll really need to emulate what they do if we want to be portable...

Changed 11 years ago by sjmurdoch

Attachment: tor_switch_id-06.patch added

Now handles lack of getres(u|g)id on MacOS X and confirming that seteuid(0) fails after priv-drop

comment:14 Changed 11 years ago by sjmurdoch

I've uploaded a new version (tor_switch_id-06.patch) which handles the lack of getres(u|g)id.
As far as I can tell, there's no way to get access to the saved UID/GID under MacOS X.

The patch also does a simple sanity check, of trying to do setuid(0), seteuid(0), and making sure it
fails. We should probably include more of the sanity checking and portability code from
uidswap.c.

comment:15 Changed 11 years ago by sjmurdoch

I looked through more of OpenSSH, and the code in uidswap.c is partially superseded by
setusercontext() -- a BSD only function which does a lot more than setuid/setgid. The
file session.c calls the functions in uidswap.c only if setusercontext() is unavailable.

We should also note that uidswap.c doesn't set the supplementary groups.

Currently our patch sets the supplementary groups to the primary group of the selected user.
This could lead to non-intuitive behavior. For example, suppose the user "tor" is a member of
groups "tor" and "test". A file has owner test, group test, and permissions -r-x---r-x.

A user who has been su'ed as "tor" cannot read the file, but "tor --User tor" can read the file
since it is not part of the "test" group.

I could be persuaded by the argument that anyone with such a setup is asking for trouble, but
we should bear it in mind.

Changed 11 years ago by sjmurdoch

Attachment: handle-double-changes.patch added

Patch on r17207 to allow multiple changes of user name

comment:16 Changed 11 years ago by sjmurdoch

I've uploaded another patch I wrote because I thought Tor supported changing the UID of a
running process. It turns out that Tor will exit with an error if this is attempted.

We should probably not apply the patch, as it doesn't do anything useful now. It might
be useful later, should Tor's behavior change. That said, it isn't hugely useful to
permit changing the UID a second time, because it would only work if Tor is running as
root the first time.

comment:17 Changed 11 years ago by mouradovski

thank you very mutch

comment:18 Changed 11 years ago by nickm

I wouldn't object to checking for setusercontext() with autoconf and using it where available.

I agree that having -r-x---r-x permissions on anything is a little nutty.

comment:19 Changed 11 years ago by nickm

Can we call this fixed now? It seems that the remaining issues fall under the heading of "people who do
bizarre things with permissions will sometimes be surprised with the results", rather than practical
security problems.

comment:20 Changed 11 years ago by ioerror

I think that this is as fixed as it's going to get. At some point we may want to revisit this and make a real
priv-sep Tor but that would be a totally different issue, I think. I'd close the bug but my account doesn't have
the ability to do so. :-)

comment:21 Changed 11 years ago by nickm

Closing. Please mark for reopen if I'm wrong to do so.

comment:22 Changed 11 years ago by nickm

flyspray2trac: bug closed.

comment:23 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.