Re: Patch to authenticate by uid/gid on ControlSocket

[Nick Mathewson <nickm@xxxxxxxxxxxxxx>]

On Sun, Mar 01, 2009 at 05:37:09AM -0500, Michael Gold wrote:
> Hi all,
> 
> The attached patch allows tor (0.2.0.34) to automatically authenticate
> the client of a Unix-domain control socket, by asking the kernel for the
> uid and gid of the remote user.  There are still some unresolved issues,
> but feedback would be appreciated.

Hi, Michael!

A few issues here: first off, we try not to add new features in the
stable series.  Since the development series is in feature freeze too
(we're trying to stabilize it), we'd have to hold off on applying this
until 0.2.2.x-alpha forks off.

Secondly, UID/GID-based authentication is potentially problematic in
the general case, and I think we should think hard about whether it's
safe in the particular case here.  For TCP sockets, it would be a
totally bad idea: it's way to easy for a user's web browser (which
runs as their own UID/GID, after all) to get tricked into making a
connection to a local socket, saying "AUTHENTICATE\r\n", and then
running whatever anonymity-breaking control commands the attacker
wants.  (That's why we recommend passwords on all control sockets.)
Now, I don't _think_ the same attack works for unix sockets, but we
should try to figure out if there is a similar attack here.  I'd guess
offhand that this is probably safe, but I'd like to have more than a
guess to go on here.
 
I'm also worried about the change that seems (if I understand
correctly) to make the initial AUTHENTICATE command optional when
you're using UID/GID authentication.  Some of the earliest attacks on
the control port protocol worked by embedding the payload inside
another protocol, confident that any amount of garbage would get
ignored if it ended with \r\n.  That's why the current protocol closes
the connection immediately if the first command is anything but
AUTHENTICATE or PROTOCOLINFO.  Accepting anything besides
AUTHENTICATE, PROTOCOLINFO, or QUIT as a first command is
nonconformant to control-spec.txt, and probably shouldn't be allowed.

On the permissions issue: In Tor today, I believe only the main thread
creates any files, so it ought to be safe to change the umask
temporarily.


> diff --git a/configure.in b/configure.in
> index e733ad9..d7c0a25 100644

[Nice to see somebody besides me using git. ;) ] 

> --- a/configure.in
> +++ b/configure.in
> @@ -18,7 +18,7 @@ fi
>  
>  # Not a no-op; we want to make sure that CPPFLAGS is set before we use
>  # the += operator on it in src/or/Makefile.am
> -CPPFLAGS="$CPPFLAGS -I../common"
> +CPPFLAGS="$CPPFLAGS -I\${top_srcdir}/src/common"

This seems like an unrelated change for cross-compiling, yeah?  If so,
maybe it should go into its own patch.  


>    if (options->CookieAuthentication) {
>      int also_password = options->HashedControlPassword != NULL ||
>        options->HashedControlSessionPassword != NULL;
> @@ -2642,6 +2642,179 @@ connection_control_reached_eof(control_connection_t *conn)
>    return 0;
>  }
>  
> +/** Called to initialise a new control socket. */
> +int
> +connection_control_init_accepted_conn(control_connection_t *conn)
> +{

Wow, there's a lot of code here.  Any way to break this into
functions?  It seems like it might be easier to parse the user/group
lists when the option is set, rather than when the connection happens,
too.

yrs,
-- 
Nick