Opened 4 years ago

Closed 4 years ago

#15220 closed enhancement (implemented)

Allow SocksSockets writable by arbitrary user

Reported by: sysrqb Owned by:
Priority: High Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: 027-triaged-1-in, TorCoreTeam201507
Cc: mcs, gk, ioerror, andrea, dgoulet, yawning, nickm Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description (last modified by sysrqb)

#12585 implemented SOCKSPort unix socket support, thus allowing proxying inet connections over a unix socket. Unfortunately, the config options only this SOCKSPort to be accessable for the Tor user, or at best, the Tor group (i.e. debian-tor). This makes it quite unuseful for normal users who aren't usually members of that group. Perhaps a new config option should be added which specifies the file socket ownership.

(dgoulet reminded me SocksSocket was merged into SocksPort)

Child Tickets

Change History (17)

comment:1 Changed 4 years ago by sysrqb

Type: defectenhancement

Not really a defect, but it means other applications which are run as a different user than tor can't take advantage of this, which is super sad.

comment:2 Changed 4 years ago by mcs

Cc: mcs added

comment:3 Changed 4 years ago by gk

Cc: gk added

comment:4 Changed 4 years ago by sysrqb

Cc: ioerror andrea dgoulet yawning nickm added

This issue doesn't affect the Tor Browser use-case, or the situation where the user is a member of the tor group (or is the tor user), but in most other cases this is a serious usability problem. So, do we leave this (mostly) useless in 0.2.6 and fix this in 0.2.7? Or, do we tweak the implementation and make it usable in 0.2.6 and then finish fixing it in 0.2.7?

In this specific case, I dont seem the harm of defaulting the unix socket to 0666, the INET SocksPort is no different (unless restricted by a firewall or somesuch). This also negates the use of SocksSocketsGroupWritable. But now that we're in the freeze, it seems too late for us to start changing config option behavior. Thoughts?

comment:5 Changed 4 years ago by sysrqb

Description: modified (diff)

comment:6 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-final

I think this is 0.2.7 material by default; It's neither a security hole nor a regression.

comment:7 in reply to:  6 Changed 4 years ago by dgoulet

Replying to nickm:

I think this is 0.2.7 material by default; It's neither a security hole nor a regression.

Adding torrc options to set the user/group for the unix socket is out of the question for 0.2.6 for sure. However, right now without a change from 660 to 666 (world open), this feature is unusable unless the user puts itself in the tor system group (ex: debian-tor) or chmod the socket. This means that anyone using torsocks out of the box won't be able to use this feature nor people using nginx Unix socket support for instance.

Isn't the point of SocksPort to be world usable (like an inet socket)? If you really want it not world open, set the socket path to be in a directory you only control. Would that be a middle ground for inclusion in 0.2.6?

If we don't get this in 0.2.6, I would advocate for an extra documentation somewhere explaining how to access the socket else that feature is dead until 0.2.7 imo.

comment:8 Changed 4 years ago by nickm

Documentation is fine here IMO; but I'm not so sure I am not comfortable having the defaults be different for different kinds of unix sockets. (Like, having SocksPort open by default but having ControlPort be private by default.) I also worry that making this open-by-default will bite somebody, somehow, somewhere along the line.

comment:9 Changed 4 years ago by nickm

bug15220_026 has an implementation of what I think the Right Thing would look like here. It's untested, though, and looks a little big. What do we think?

comment:10 in reply to:  9 Changed 4 years ago by yawning

Replying to nickm:

bug15220_026 has an implementation of what I think the Right Thing would look like here. It's untested, though, and looks a little big. What do we think?

Per the discussion from IRC, I think this is the right way to make things less restrictive, with the caveat that something should happen if the user attempts to configure a world-writable control socket, without any authentication methods specified.

As for the "something" that happens when this crazy config is specified:

  • Warning (bare minimum, IMO). There are single user systems out there for which an unauthenticated control socket is ok.
  • Error (what I personally favor). Said single user systems out there still have groups, and all the code out there that can talk to the controller should/does support authentication.

The general feeling is that this is feels a bit big for 0.2.6.x, though no one came up with a better interim solution.

(If I missed anything in my summary please let me know.)

comment:11 Changed 4 years ago by sysrqb

Ah, I see I had the same thought as Yawning, with respect to the control socket. I tried to write a patch for that, which takes advantage of the warning we emit when ControlPort_set is set without any authentication. Sadly I couldn't find an elegant way to do it, it seems like we'd need to reparse the ControlSocket line again specifically to check if WorldWritable was there. An alternative is adding the warning in options_act_reversible() after configured_ports is set, but that is relatively late in the startup sequence for this.

It's tested and it works, with a minor tweak. Overall, it does seem a little large, but it's not very intrusive. I think if there is an easy way to add a warning when the control socket is world readable, then it will be beneficial to merge this. If adding the warning is too difficult, then I think no merge.

comment:12 Changed 4 years ago by sysrqb

And, with that, I've pushed a branch, bug15220_026_sysrqb, it has two additional commits. The first (5ce5527823) has the minor fix. The second (61dcd926aa8) is an ugly attempt to provide a warning. I hope there is a better way to do it.

comment:13 Changed 4 years ago by dgoulet

Status: newneeds_review

comment:14 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:15 Changed 4 years ago by isabela

Points: small
Priority: normalmajor
Version: Tor: 0.2.7

comment:16 Changed 4 years ago by nickm

Keywords: TorCoreTeam201507 added

comment:17 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks plausible at this point. I agree the warning code is yucky, and I'd welcome a patch.

Note: See TracTickets for help on using tickets.