Opened 3 years ago

Closed 3 years ago

#20906 closed enhancement (wontfix)

SocksPorts and ControlPorts should be stored in a set, not a list

Reported by: arthuredelstein Owned by: nickm
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tbb-wants
Cc: brade, mcs, gk Actual Points:
Parent ID: Points: 2
Reviewer: Sponsor:

Description

When using domain sockets, Tor Launcher needs to launch tor with a specific unix-domain SocksPort. The SocksPort is initially not in the torrc file (as the full path is not known before installation), so Tor Launcher passes the SocksPort setting in to the tor process via the tor command line.

Tor Launcher also calls saveconfig later in the session, such that the SocksPort will be saved to the torrc. So the next time Tor Launcher launches, passing the SocksPort to the command line is a duplicate of the previous SocksPort, now loaded in from the torrc. Tor then shows an error.

For an example, see ticket:20761#comment:11. More background is in #20761 as well.

I think a solution to this would be for tor to store SocksPorts (and similarly, ControlPorts) in a set rather than a list. So if the command line or a torrc or a setconf invocation specifies a redundant SocksPort setting, no error is thrown but the redundant setting is gracefully ignored. And only a single copy of a given SocksPort setting should be saved to the torrc when saveconfig is invoked.

Child Tickets

Change History (15)

comment:1 Changed 3 years ago by mcs

Cc: brade mcs added

comment:2 Changed 3 years ago by gk

Cc: gk added

comment:3 Changed 3 years ago by gk

Keywords: tbb-wants added

comment:4 Changed 3 years ago by dgoulet

Milestone: Tor: 0.3.0.x-final
Points: 0.2

Ok, once we've parsed a port, we need to validate if it's already in the list. Moving to a set might require quite the patch but validating that we have a duplicate in the list could be shorter path.

Putting this one in 030 but it could be deferred if we are a bit overwhelmed by the amount of things that goes in 030. What is the consequence of differing to 031 for TBB?

Last edited 3 years ago by arma (previous) (diff)

comment:5 in reply to:  4 Changed 3 years ago by arthuredelstein

Replying to dgoulet:

Ok, once we've parsed a port, we need to validate if it's already in the list. Moving to a set might require quite the patch but validating that we have a duplicate in the list could be shorter path.

Putting this one in 030 but it could be differed if we are a bit overwhelmed by the amount of things that goes in 030. What is the consequence of differing to 031 for TBB?

The main thing is that we are backing off using domain sockets by default (#20761) until this issue is fixed. I thing we'll want to be using domain sockets by default in TBB 7.0-stable, which comes out no later than 2017-06-13 (hopefully earlier).

comment:6 Changed 3 years ago by dgoulet

Priority: MediumHigh

As I understand it, it's a blocker for TBB to have this awesome important security feature to let's fix it asap.

comment:7 in reply to:  6 Changed 3 years ago by mcs

Replying to dgoulet:

As I understand it, it's a blocker for TBB to have this awesome important security feature to let's fix it asap.

Thanks! I want to clarify that even after this ticket is fixed, it will still be tricky for Tor Browser/Tor Launcher to cleanly switch between Unix domain port listeners and TCP listeners. Why? Because any SocksPort and ControlPort config that is passed on the tor command line is written to torrc after a SAVECONF command, and because the we cannot Tor Browser's hardcode the Unix domain socket paths we cannot avoid passing via the command line.

Is the tor team open to the idea that config that is passed via the command line should not be written to torrc? Or at least are you open to providing a way to specify that config is ephemeral and should not be persisted to torrc? We could introduce a new prefix character to indicate that, e.g., dot as in .+SocksPort unix:PATH or similar. If this is something the network team would be willing to consider, I will open a new ticket. I am not trying to sign anyone else up for this work; it might even be something for which the browser team could contribute patches.

comment:8 Changed 3 years ago by dgoulet

I think this is totally reasonable idea! And TBB seems to have an important use case for it. I would propose that you open a ticket about it and put it in 030 milestone. We'll start discussion from there on how to proceed with this.

comment:9 Changed 3 years ago by nickm

I think that a "Don't write this to torrc" flag might be workable, but I also think it's maybe less easy to implement than you might think -- especially if it needs to work for things other than LineList options. If it's LineList only, it has a reasonable chance of being easy to implement.

comment:10 in reply to:  8 Changed 3 years ago by mcs

Replying to dgoulet:

I think this is totally reasonable idea! And TBB seems to have an important use case for it. I would propose that you open a ticket about it and put it in 030 milestone. We'll start discussion from there on how to proceed with this.

I created #20956.

comment:11 Changed 3 years ago by nickm

Points: 0.22

comment:12 Changed 3 years ago by nickm

Type: defectenhancement

batch modify: I think these are "enhancement", though I could be wrong about some.

comment:13 Changed 3 years ago by nickm

Owner: set to nickm
Status: newaccepted

comment:14 Changed 3 years ago by nickm

I'm currently planning to wontfix this in favor of 20956.

comment:15 Changed 3 years ago by nickm

Resolution: wontfix
Status: acceptedclosed
Note: See TracTickets for help on using tickets.