On Mon, Mar 02, 2009 at 13:58:21 -0500, Nick Mathewson wrote:
> 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.
It doesn't look like the relevant code has changed much between the two
series, so this shouldn't be a problem.
> 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 grepped the Debian iceweasel-3.0.6 source package for AF_UNIX,
AF_LOCAL, and AF_FILE. It looks like Unix sockets are only used for IPC
(using a fixed path), asynchronous DNS lookups, and some socketpair
calls. I've never heard of any browser allowing users to connect to
Unix domain sockets, or any web server that listens on them -- so a
similar attack seems unlikely.
Also, on my system there are already some important programs listening
on Unix sockets, such as X11, dbus, ssh-agent, and cups; dbus and
ssh-agent both use getpeereid/SO_PEERCRED for authentication (with no
additional credentials required, AFAICT).
> 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.
Is it enough to require an AUTHENTICATE command, or should we also try
to verify that there's two-way communication? (e.g., by including some
random string in PROTOCOLINFO and having the client echo it back.)
I guess I should add a new AuthMethod to PROTOCOLINFO too; probably
something like "UID".
> 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.
Good, that makes things simple.
> > +/** 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?
Sure, both the ControlAllowUsers and ControlAllowGroups sections (at
least) could be easily broken out.
> 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.
Maybe, and it would reveal configuration errors earlier too -- but users
and groups can be dynamically changed, and reading it each time ensures
we have the latest info. For example, someone authenticating via a
group membership should have their access revoked immediately when they
are removed from the group.
Thanks for your comments,
-- Michael
Attachment:
signature.asc
Description: Digital signature