Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#2972 closed enhancement (fixed)

Allow ControlSocket to be group writable

Reported by: lunar Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: tor-client
Cc: lunar@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

This is an attempt to move <http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=552556> foward.

Right now, ControlPort + CookieAuthFileGroupReadable offers a way for specific users (members of the same group as the Tor process) to controel a system-wide Tor daemon. It would be great to have a similar access control mechanism for ControlSocket.

The attached patch is an attempt to implement such behaviour. It adds a new configuration option, UnixSocketsGroupWritable, which when enabled, will make socket g+rw upon creation.

Child Tickets

Attachments (4)

tor-0.2.2.19-alpha-820-g94401f0+UnixSocketsGroupWritable.diff (3.7 KB) - added by lunar 9 years ago.
Patch implementing UnixSocketsGroupWritable option
demo_un.c (2.0 KB) - added by nickm 8 years ago.
demo_un_dir.c (2.2 KB) - added by nickm 8 years ago.
un_tests.tgz (1.5 KB) - added by nickm 8 years ago.

Download all attachments as: .zip

Change History (25)

Changed 9 years ago by lunar

Patch implementing UnixSocketsGroupWritable option

comment:1 Changed 9 years ago by nickm

Milestone: Tor: 0.2.2.x-final
Status: newneeds_review

comment:2 Changed 9 years ago by Sebastian

Git am doesn't work for me on that patch as uploaded. Tried to recreate it in my branch bug2972 ( https://gitweb.torproject.org/sebastian/tor.git/commitdiff/bug2972 ). Lunar: please verify that is how it is supposed to look?

comment:3 Changed 9 years ago by Sebastian

For comments on the patch itself: I think we shouldn't mix ControlSocket and UnixSocket in our option names, because that might lead to confusion about what the option is supposed to do.

I made the change in the branch mentioned above, and also added a changes file.

comment:4 Changed 9 years ago by rransom

+        log_warn(LD_FS,"Unable to make %s group-writeable.", address);

s/writeable/writable/

comment:5 Changed 9 years ago by Sebastian

thanks, fixed

comment:6 Changed 8 years ago by lunar

Cc: lunar@… added

When I started to code, the option was actually called ControlSocketsGroupWritable.

But while digging in the code to see where to implement this, I realised that the easiest way would affect all unix sockets created by Tor. Right now, unix sockets are only used by the ControlSockets option, so the rename is not false either, but care will need to be taken if any other unix socket is added.

Is strange that git-am failed... Thanks for taking care of this patch! :)

comment:7 Changed 8 years ago by nickm

I like this idea, but think that depending on the default group seems error-prone. Perhaps instead of a boolean, it could take the name of a group, and chgrp the socket before doing the chmod? That seems less likely to wind up with surprising results.

Also, could we use fchmod here? I don't _think_ we have a race-condition to worry about here, but it's best to err on the side of safety.

Finally, the linux unix(7) manpage says:

Connecting  to  the
       socket  object  requires  read/write permission.  This behavior differs
       from many BSD-derived systems which ignore permissions for  Unix  sock‐
       ets.  Portable programs should not rely on this feature for security.

Is this true nowadays? If so, we shouldn't give people a false sense of security by allowing this option where it won't work.

comment:8 in reply to:  7 ; Changed 8 years ago by Sebastian

Replying to nickm:

I like this idea, but think that depending on the default group seems error-prone. Perhaps instead of a boolean, it could take the name of a group, and chgrp the socket before doing the chmod? That seems less likely to wind up with surprising results.

Do you think the same applies to the case of cookie auth?

Finally, the linux unix(7) manpage says:

Connecting  to  the
       socket  object  requires  read/write permission.  This behavior differs
       from many BSD-derived systems which ignore permissions for  Unix  sock‐
       ets.  Portable programs should not rely on this feature for security.

Is this true nowadays? If so, we shouldn't give people a false sense of security by allowing this option where it won't work.

We should probably disable the ControlSocket option altogether on such systems, or at least warn loudly when it is used?

comment:9 in reply to:  8 ; Changed 8 years ago by rransom

Replying to Sebastian:

Replying to nickm:

I like this idea, but think that depending on the default group seems error-prone. Perhaps instead of a boolean, it could take the name of a group, and chgrp the socket before doing the chmod? That seems less likely to wind up with surprising results.

Do you think the same applies to the case of cookie auth?

I don't think relying on the default group is error-prone at all. Tor's documentation states that it when it is started as root and the User option is specified, it changes to that user ID and the default group associated with that user ID; that appears to work correctly on Linux 2.6.x with glibc and FreeBSD 8.2-RELEASE-p1.

Finally, the linux unix(7) manpage says:

Connecting  to  the
       socket  object  requires  read/write permission.  This behavior differs
       from many BSD-derived systems which ignore permissions for  Unix  sock‐
       ets.  Portable programs should not rely on this feature for security.

Is this true nowadays? If so, we shouldn't give people a false sense of security by allowing this option where it won't work.

Part of the reason I started using FreeBSD was so that I could test that fact. I haven't tested it or RTFSed yet, but the unix(4) and connect(2) man pages seem to say that FreeBSD currently does not allow users without write access to a local socket to connect to it.

We should probably disable the ControlSocket option altogether on such systems, or at least warn loudly when it is used?

There is enough FUD out there about whether filesystem permissions on local sockets are enforced that I would recommend removing the ControlSocket option. It isn't even reliably possible to determine which kernel a compiled executable is running on anymore.

comment:10 in reply to:  8 Changed 8 years ago by nickm

Replying to Sebastian:

Replying to nickm:

I like this idea, but think that depending on the default group seems error-prone. Perhaps instead of a boolean, it could take the name of a group, and chgrp the socket before doing the chmod? That seems less likely to wind up with surprising results.

Do you think the same applies to the case of cookie auth?

Yes, I think so.

comment:11 in reply to:  9 Changed 8 years ago by nickm

Replying to rransom:

Replying to Sebastian:

Replying to nickm:

I like this idea, but think that depending on the default group seems error-prone. Perhaps instead of a boolean, it could take the name of a group, and chgrp the socket before doing the chmod? That seems less likely to wind up with surprising results.

Do you think the same applies to the case of cookie auth?

I don't think relying on the default group is error-prone at all. Tor's documentation states that it when it is started as root and the User option is specified, it changes to that user ID and the default group associated with that user ID; that appears to work correctly on Linux 2.6.x with glibc and FreeBSD 8.2-RELEASE-p1.

I'm not saying it's error-prone because of a lack of documentation; I'm saying it's error-prone because it's not too hard for people to get confused about which group is in fact the default group for a given user at a given time. If you think you've set it up so that you're running as user=tor group=tor but in fact you're running as user=tor group=users, then you're in deep trouble. Specifying the group explicitly makes this kind of error basically impossible to do.

Finally, the linux unix(7) manpage says:

Connecting  to  the
       socket  object  requires  read/write permission.  This behavior differs
       from many BSD-derived systems which ignore permissions for  Unix  sock‐
       ets.  Portable programs should not rely on this feature for security.

Is this true nowadays? If so, we shouldn't give people a false sense of security by allowing this option where it won't work.

Part of the reason I started using FreeBSD was so that I could test that fact. I haven't tested it or RTFSed yet, but the unix(4) and connect(2) man pages seem to say that FreeBSD currently does not allow users without write access to a local socket to connect to it.

We should probably disable the ControlSocket option altogether on such systems, or at least warn loudly when it is used?

There is enough FUD out there about whether filesystem permissions on local sockets are enforced that I would recommend removing the ControlSocket option. It isn't even reliably possible to determine which kernel a compiled executable is running on anymore.

Well, we could always make a socket at runtime, give it permissions 000, and then try to write to it. :/

Personally I want to know which live systems actually have this problem before we remove a feature based on some kernels not doing it right.

I'm attaching a pair of test programs that should compile on most unixes. One verifies that we honor mode 000 on AF_UNIX sockets; the other verifies that we check permissions in the directory path when connecting to AF_UNIX sockets. If we can find extant tor-supported platforms where the answer is "no", that's something we should act on.

Changed 8 years ago by nickm

Attachment: demo_un.c added

Changed 8 years ago by nickm

Attachment: demo_un_dir.c added

comment:12 Changed 8 years ago by nickm

Priority: normalmajor

comment:13 Changed 8 years ago by nickm

Uploading a slightly streamlined version of those tests in un_tests.tgz

Changed 8 years ago by nickm

Attachment: un_tests.tgz added

comment:14 Changed 8 years ago by nickm

So, we found that at least one platform (SunOS 5.11 snv_90 sun4v sparc SUNW,T5240), the variant that does a chmod 000 on the socket achieves nothing to keep people from accessing it, but the variant that does chmod 000 on the directory containing the socket successfully prevents access to the socket.

Do we believe that there are unixes that matter where the permissions on a directory containing a unix socket aren't checked on attempts to open the socket?

Also, fchmod works on unix sockets on some platforms but not others.

comment:15 Changed 8 years ago by nickm

Let's move this ahead.

Given that the authentication cookie file's group-readability support already does not support specifying a group, I'm okay with leaving off support for specifying a particular group to 0.2.3.x.

On the security issue: We should check the permissions on the directory containing the socket. We should warn if it's world-w or world-rx. We should warn if it's group-w or group-rx and the option to make the socket group-accessible is not set or the group that owns the directory is not the same group as is getting rights on the socket. (The check_private_dir() function in util.c can already do some of this.)

I believe we can tell whether our host system implements file permissions properly on sockets by the trick of doing a chmod 000 then trying to connect to the socket. But that's potentially tricky to get right, and I'd rather just warn everywhere if people are leaving the socket in a visible directory.

comment:16 Changed 8 years ago by nickm

Okay, I've added the code to make sure that the permissions on any directory holding a unix socket are right. This should help keep people safe even on Solaris-like systems that don't check socket permissions.

See branch "bug2972" in my public repository. Somebody give this branch another review? I'd like to merge it soon.

comment:17 Changed 8 years ago by nickm

Updated based on comments from rransom on IRC.

comment:18 Changed 8 years ago by rransom

s/at the the end/at the end/

Other than that, bug2972 looks good now.

comment:19 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Great, merging!

comment:20 Changed 7 years ago by nickm

Keywords: tor-client added

comment:21 Changed 7 years ago by nickm

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