Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3076 closed enhancement (fixed)

Implement 'SocksPort auto' and 'ControlPort auto'

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

Description (last modified by mikeperry)

In order to ship Tor Browser Bundles that do not conflict with distribution-installed tor clients and tor relays, we must give TBB the ability to automatically choose a new SOCKS port and Control port if the default ports are in use. (See #2264).

This requires a few pieces to coordinate. Creating torrc options to allow tor to listen on automatically free ports is the first step.

These options also need to kick out a log line at notice level that Vidalia is capable of easily parsing (#3077).

I think we need to do this ASAP, and that it should go into 0.2.2.x, so we can release TBBs that don't break when people have another tor installed. I can code this myself it that is what it takes (but obviously I'd prefer not to :).

Child Tickets

Change History (26)

comment:1 Changed 8 years ago by mikeperry

Description: modified (diff)

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

Priority: normalmajor
Type: defectenhancement

Replying to mikeperry:

In order to ship Tor Browser Bundles that do not conflict with distribution-installed tor clients and tor relays, we must give TBB the ability to automatically choose a new SOCKS port and Control port if the default ports are in use. (See #2264).

This requires a few pieces to coordinate. Creating torrc options to allow tor to listen on automatically free ports is the first step.

These options also need to kick out a log line at notice level that Vidalia is capable of easily parsing (#3077).

An AutoPortFile option (to write out the ports Tor has chosen to an easily parsed file, much like the PidFile option) would be a much better interface than a log line.

comment:3 in reply to:  2 ; Changed 8 years ago by mikeperry

Replying to rransom:

These options also need to kick out a log line at notice level that Vidalia is capable of easily parsing (#3077).

An AutoPortFile option (to write out the ports Tor has chosen to an easily parsed file, much like the PidFile option) would be a much better interface than a log line.

Depends. Can we choose a predictable directory that Vidalia will be able to find in all cases? For TBB this is probably just $CWD, but in other cases it may be more complex.. Not sure if the other cases matter at all, though.

comment:4 in reply to:  2 Changed 8 years ago by arma

Replying to rransom:

Replying to mikeperry:

In order to ship Tor Browser Bundles that do not conflict with distribution-installed tor clients and tor relays, we must give TBB the ability to automatically choose a new SOCKS port and Control port if the default ports are in use. (See #2264).

This requires a few pieces to coordinate. Creating torrc options to allow tor to listen on automatically free ports is the first step.

These options also need to kick out a log line at notice level that Vidalia is capable of easily parsing (#3077).

An AutoPortFile option (to write out the ports Tor has chosen to an easily parsed file, much like the PidFile option) would be a much better interface than a log line.

I agree that we want some better interface than "parse the log lines until you find one you recognize". I'm hoping Nick will take a look at this.

comment:5 Changed 8 years ago by nickm

On the interface:

I can get that we need to expose the ControlPort via a way that doesn't involve using the ControlPort, but is there some reason that we can't use a GETINFO command for the other ports? Also, is there any reason to do this _only_ for socksport and controlport?

As far as possible, I want to avoid having log messages be any part of our defined interface. If we *did* do that, we'd want to make it really obvious which ones they are.

On the implementation:

It seems like "FooPort auto" ought to mean "bind the address with sin_port set to 0, then do a getsockname() on the socket, then advertise that one." That doesn't sound too hard. We would also need to tweak anything that advertises a port.

comment:6 in reply to:  5 ; Changed 8 years ago by arma

Replying to nickm:

On the interface:

I can get that we need to expose the ControlPort via a way that doesn't involve using the ControlPort, but is there some reason that we can't use a GETINFO command for the other ports?

Not that I know of.

Also, is there any reason to do this _only_ for socksport and controlport?

No.

As far as possible, I want to avoid having log messages be any part of our defined interface. If we *did* do that, we'd want to make it really obvious which ones they are.

Right.

On the implementation:

It seems like "FooPort auto" ought to mean "bind the address with sin_port set to 0, then do a getsockname() on the socket, then advertise that one." That doesn't sound too hard. We would also need to tweak anything that advertises a port.

Ok. I expect "ORPort auto" will want more smarts, for two reasons. First, we want to pick one that is reachable, so it's a more complex testing process than 'bind and see what the answer is'. Same, I guess, with "DirPort auto". Second, we might prefer relays on 443 or 80 or 143 or something -- I'd been imagining a function that returns what number we'd like to try next and the first few are random skewed toward the numbers we'd prefer and after that it goes random. Your idea of "if you meant to get a random one, let the kernel decide" is a good one. All of *that* said, an ORPort auto that simply binds to an available port would be a great start.

comment:7 in reply to:  6 Changed 8 years ago by nickm

Agreed. Also for "ORPort/DirPort auto", I think we want the setting to be a little persistent. In other words, if you wind up with your OR port as 46371, you should try 46371 the next time you start with ORPort auto, and only ask for a random port when that fails.

That said, it doesn't need to be there in the first version of this: I suggest that "prefer 443/80/143" and "retry till it's reachable" can be their own tickets that we open once this is done.

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

All but the "write the controlport to a file or the log or something" part is now implemented in branch "feature3076" in my public repo. Have a look.

Actually, the log _will_ currently note the actual controlport, but please don't use that! Log messages are explicitly not a stable interface.

As for the interface to expose the controlport...

Replying to mikeperry:

[...]

Depends. Can we choose a predictable directory that Vidalia will be able to find in all cases? For TBB this is probably just $CWD, but in other cases it may be more complex.. Not sure if the other cases matter at all, though.

It could be a filename specified on the command-line, right? If so, is there any reason that finding such a file would be harder for Vidalia than finding the log messages would be?

comment:9 Changed 8 years ago by nickm

Status: newneeds_review

comment:10 Changed 8 years ago by nickm

Wrote a patch to implement ControlPortWriteToFile; see the updated feature3076 branch in my public.

But on consideration I am worried about MITM issues here: the patch would make it easier to wind up in a situation where an attacker can listen on port X and convince the controller to connect to port X instead of to Tor... either by reading a stale file and binding to the listed port, or by overwriting the file with a new port. Should we care? Can we do anything about this?

comment:11 in reply to:  10 ; Changed 8 years ago by mikeperry

Replying to nickm:

Wrote a patch to implement ControlPortWriteToFile; see the updated feature3076 branch in my public.

It looks like you're going to create a temp file here if it already exists, and then replace the existing file due to magic in start_writing_to_file()? This may have consequences for my permissions comments below..

Also, perhaps the man page should specify that the file name can be relative to the current working directory of the tor process? Or do we want to default to the Tor data directory, or config directory?

But on consideration I am worried about MITM issues here: the patch would make it easier to wind up in a situation where an attacker can listen on port X and convince the controller to connect to port X instead of to Tor... either by reading a stale file and binding to the listed port, or by overwriting the file with a new port. Should we care? Can we do anything about this?

I think we can assume that anyone with write access to the torrc wins, so we only need to consider new vulnerabilities for installs that will use this feature by default. There are then two major use cases to consider: distribution packages, and our bundles.

Since distributions may perform their own custom least-privilege implementations, I think if we explicitly set this file to 640 in the code, that this should clue those people in to the fact that we only want tor and vidalia to be able to read this file, if they even decide to enable this option. I don't envision system tor installs needing or wanting this option, though.

If we think about our own packages, I think we also only really want this feature for the non-expert Tor Browser Bundles. I think we lose the least-privilege possibility right off the bat for them. For those bundles, the same account will run vidalia, tor and Firefox. If this is the same account that has filesystem ownership of the bundle files, that account can exploit the bundle in a lot of ways (such as altering torrc or the binaries).

If the filesystem owner account is not used to launch the bundles, we may be opening up new vulnerabilities by allowing this secondary account to access this file. But it's also not clear how well the bundles will function without write access to their data directory and CWD (where everything lives)... The state file can't be written to save guards or CBT data, for example, so this appears to be a non-recommended situation rather than a more secure one.

Am I right here? It looks to me for the cases where this will be used, it doesn't give any additional ability, so long as we make sure it is readable only by the bundle user and not o+r.

comment:12 in reply to:  11 Changed 8 years ago by rransom

Replying to mikeperry:

Replying to nickm:

Wrote a patch to implement ControlPortWriteToFile; see the updated feature3076 branch in my public.

It looks like you're going to create a temp file here if it already exists, and then replace the existing file due to magic in start_writing_to_file()? This may have consequences for my permissions comments below..

Also, perhaps the man page should specify that the file name can be relative to the current working directory of the tor process? Or do we want to default to the Tor data directory, or config directory?

We want no default value at all. Vidalia can set this option on the command line in the TBBs.

All configuration options that specify file or directory names can be relative to Tor's current working directory.

But on consideration I am worried about MITM issues here: the patch would make it easier to wind up in a situation where an attacker can listen on port X and convince the controller to connect to port X instead of to Tor... either by reading a stale file and binding to the listed port, or by overwriting the file with a new port. Should we care? Can we do anything about this?

If the filesystem owner account is not used to launch the bundles, we may be opening up new vulnerabilities by allowing this secondary account to access this file. But it's also not clear how well the bundles will function without write access to their data directory and CWD (where everything lives)... The state file can't be written to save guards or CBT data, for example, so this appears to be a non-recommended situation rather than a more secure one.

Tor won't work at all if it is not run as the user that owns its DataDirectory. See #2824 for one moderately annoying consequence.

comment:13 Changed 8 years ago by nickm

I'm particularly concerned about Windows here: on the windows versions we care about, do FS permissions work well enough to keep hostile parties from reading our datadir?

comment:14 in reply to:  13 Changed 8 years ago by rransom

Replying to nickm:

I'm particularly concerned about Windows here: on the windows versions we care about, do FS permissions work well enough to keep hostile parties from reading our datadir?

That's unlikely, even if the hostile parties aren't running with the same privileges the user has. But writing a listening port number to a file after we open the listener isn't a problem.

comment:15 Changed 8 years ago by nickm

I think it is a problem. Two attacks here:

1) If the attacker can write to the file: The attacker overwrites the listening port number before the controller reads the file. Now the controller connects to the attacker instead. The attacker learns the required AUTHENTICATE command, and now takes control of the Tor process.

2) If the attacker can only read from the file: The attacker reads the listening port number, then either kills Tor, provokes it to crash, or somehow gets into a situation where the file is still there but Tor is not still listening on that port. Now the attacker binds to that port, and the controller to connect to it. The attacker learns the required AUTHENTICATE command, and takes control of the Tor process when it eventually restarts (assuming password authentication).

comment:16 in reply to:  15 Changed 8 years ago by rransom

Replying to nickm:

I think it is a problem. Two attacks here:

1) If the attacker can write to the file: The attacker overwrites the listening port number before the controller reads the file. Now the controller connects to the attacker instead. The attacker learns the required AUTHENTICATE command, and now takes control of the Tor process.

On an NTFS-based Windows installation, only (a) the ‘LOCALSYSTEM’ account (almost equivalent to Unixoid ‘root’), (b) the Tor user, and (c) an administrative user who has previously asked Windows to give him/her/it access to the Tor user's directories can read from or write to Tor files within the Tor user's directories. (If the user is running TBB from a FAT-formatted USB storage device, my understanding is that all users can read and write in the TBB directory.)

Any attacker who can write to the control-port file either runs as the Tor user or can write to the Tor executables. (The only attacker which could run as the user running Tor, but not have write access to Tor itself, is a piece of malware running in the Tor user's account on a computer on which an installable Tor bundle was previously installed.) Given that, this race-condition attack is one of the most difficult attacks available (to implement and to use).

2) If the attacker can only read from the file: The attacker reads the listening port number, then either kills Tor, provokes it to crash, or somehow gets into a situation where the file is still there but Tor is not still listening on that port. Now the attacker binds to that port, and the controller to connect to it. The attacker learns the required AUTHENTICATE command, and takes control of the Tor process when it eventually restarts (assuming password authentication).

Vidalia notices when Tor exits, even if it hasn't established a control port connection yet. Vidalia does not automatically restart Tor. The only remaining potential issue to fix here is ensuring that Vidalia uses a different random password every time it starts Tor.

comment:17 Changed 8 years ago by nickm

Okay, I'm persuaded.

Do people like the code, or does it still need review?

comment:18 Changed 8 years ago by nickm

Added an option to make the file group-readable. How does feature3076 look now?

comment:19 Changed 8 years ago by rransom

In commit f7ba03d65a2757f60ed6fbb16ba17f1c1d87bdc7, you need to escape any spaces in tor_sockaddr_to_str's output. Also, it looks like you tried to squash commit fb23dc232e2efd85c218159164f84fa3caf711d7 into that one and failed.

In the changes file in commit 1a9a29e73efa1771f772e5a411da15d643c56afe, you forgot to finish another

comment:20 Changed 8 years ago by rransom

In control_ports_write_to_file in commit 1a9a29e73efa1771f772e5a411da15d643c56afe, you're joining strings using the empty string as a separator; you want to use "\n" instead.

Also, I don't think you want to log the multi-line string that you're about to write to the file at level notice.

Also, you should free the smartlist when you're done with it.

In tor_cleanup, the ‘Remove our pid file.’ comment is misplaced; you could move and expand it or nuke it.

In commit da3f3a81e276812f49a66416576e85779fbd6580, ControlPortFileGroupReadable deserves a changes/ entry.

Other than that, looks good.

comment:21 Changed 8 years ago by nickm

Agreed except for:

In control_ports_write_to_file in commit 1a9a29e73efa1771f772e5a411da15d643c56afe, you're joining strings using the empty string as a separator; you want to use "\n" instead.

The strings that I'm joining there are already NL-terminated.

comment:22 Changed 8 years ago by nickm

Changes in branch feature3076. Better now?

comment:23 in reply to:  22 Changed 8 years ago by rransom

Replying to nickm:

Changes in branch feature3076. Better now?

Looks good!

comment:24 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merging and closing. Also applying an appropriate change to control-spec.txt to document the new GETINFO values.

comment:25 Changed 7 years ago by nickm

Keywords: tor-client added

comment:26 Changed 7 years ago by nickm

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