Tor Browser alpha is broken on Linux (and probably OS X) if directory is nested too deep

While testing the upcoming 6.5a3 builds I got greeted with

Sep 20 16:28:18.534 [warn] Unix socket path '"/home/thomas/Arbeit/Tor/tor-browser-bundle/gitian/tor-browser_en-US/Browser/TorBrowser/Data/Tor/control.socket"' is too long to fit.
Sep 20 16:28:18.534 [warn] Failed to parse/validate config: Failed to bind one of the listener ports.

And Tor Browser failed to start. Moving both the 6.5a3 and the 6.5a3-hardened bundle into my home dir solved the problem

added TorBrowserTeam201610R component::applications/tor browser owner::tbb-team parent::14270 priority::high resolution::fixed severity::major status::closed type::defect labels

Kathy and I were apparently lucky; our installation paths are fairly short.

The longest Unix domain socket path that can be used on Linux is 108 bytes. See: https://linux.die.net/man/7/unix

On OSX, it is even shorter at 104 bytes: https://developer.apple.com/legacy/library/documentation/Darwin/Reference/ManPages/man4/unix.4.html

Should we be placing the sockets in /tmp instead of in the tor data directory? I think we would need to create a subdirectory with sufficiently restrictive permissions (tor does some permission checks).

Please use $XDG_RUNTIME_DIR if it's set: https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.7.html "The directory MUST be owned by the user, and he MUST be the only one having read and write access to it. Its Unix access mode MUST be 0700." It tends to be something short like "/run/user/1000".

Replying to cypherpunks:

Please use $XDG_RUNTIME_DIR if it's set:

Thank you for that suggestion. Here is a proposal for the control and SOCKS port Unix domain socket paths:

1. The socket basenames will always be control.socket and socks.socket.
2. If $XDG_RUNTIME_DIR is set, create a unique subdirectory within that directory and place the sockets there (this will allow more than one copy of Tor Browser to be used at the same time). Tor Launcher will use Mozilla's nsIFile.createUnique() function to create the subdirectory and it will be deleted during browser exit.
3. If the length of the path <tor-data-dir>/control.socket is shorter than 100 bytes, use <tor-data-dir>/control.socket and <tor-data-dir>/socks.socket (compatible with TB 6.5a3's behavior).
4. Otherwise, create a unique directory within /tmp (similar to the $XDG_RUNTIME_DIR scenario).

That looks fine, and I like that you'll be able to run multiple copies.

Maybe you want the length check on $XDG_RUNTIME_DIR too, because the failure is non-obvious and the specification doesn't give any explicit length limit (the statement "on Unix-like operating systems AF_UNIX sockets...must be supported" implies a maximum length of 106 if connectat/bindat don't exist).

Trac:
Keywords: N/A deleted, TorBrowserTeam201609 added

Trac:
Cc: mcs, brade to mcs, brade, arthuredelstein

Moving tickets to October.

Trac:
Keywords: TorBrowserTeam201609 deleted, TorBrowserTeam201610 added

Here is a patch for review: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20185-01&id=ee4b3ad5d22fdc3abf582afd2aca936a7cb1b778

You will need to apply the latest patch from #20111 (moved) before applying this one.

Trac:
Keywords: TorBrowserTeam201610 deleted, TorBrowserTeam201610R added
Status: new to needs_review

I think I don't understand why _createUniquIPCDir() would return null if the destination would be too long (at least after reading the .idl file I am still wondering). Could you elaborate?

The code breaks for me in _createUniqueIPCDir() adding some logging gives me:

TorLauncher NOTE: Failed to create unique dir: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.createUnique]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: re[/torlauncher/modules/tl-util.jsm](/torlauncher/modules/tl-util.jsm) :: TLUtilInternal._createUniqueIPCDir :: line 823"  data: no] with aBasePath /run/user/1000

• there are again some places where you mix blocks with and without braces in if-else statements.

Trac:
Keywords: TorBrowserTeam201610R deleted, TorBrowserTeam201610 added
Status: needs_review to needs_revision

Oh, I should have added that XDG_RUNTIME_DIR points to it.

Replying to gk:

I think I don't understand why _createUniquIPCDir() would return null if the destination would be too long (at least after reading the .idl file I am still wondering). Could you elaborate?

Our patch does not rely on _createUniquIPCDir() to do any length checking; that is done by _isIPCPathLengthOK(). Are you concerned about the fact that we do not check the path length in the /tmp/Tor case? Our assumption is that such a path will not be too long (and we have no additional options).

Replying to gk:

The code breaks for me in _createUniqueIPCDir() adding some logging gives me: {{{ TorLauncher NOTE: Failed to create unique dir: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.createUnique]" nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)" location: "JS frame :: re/torlauncher/modules/tl-util.jsm :: TLUtilInternal._createUniqueIPCDir :: line 823" data: no] with aBasePath /run/user/1000 }}}

Hmm. We will try to reproduce what you are seeing. We probably should put a try/catch inside _createUniqueIPCDir() in any case.

• there are again some places where you mix blocks with and without braces in if-else statements.

Sorry about that. You may have to correct us a few more times (old habits tend to persist).

Replying to mcs:

Replying to gk:

I think I don't understand why _createUniquIPCDir() would return null if the destination would be too long (at least after reading the .idl file I am still wondering). Could you elaborate?

Our patch does not rely on _createUniquIPCDir() to do any length checking; that is done by _isIPCPathLengthOK(). Are you concerned about the fact that we do not check the path length in the /tmp/Tor case? Our assumption is that such a path will not be too long (and we have no additional options).

I am just confused about the comment above this function because I somehow fail to see how null gets returned if the destination would be too long.

Replying to gk:

I am just confused about the comment above this function because I somehow fail to see how null gets returned if the destination would be too long.

Sorry; I was not looking at the comment. The part about returning null is wrong (leftover from an earlier iteration of the code).

See #20337 (moved) for an alternative Linux-specific way to solve the issue (for a non-sandboxed case).

We forgot to append "/Tor" in the XDG_RUNTIME_DIR case. Here is a revised patch: https://gitweb.torproject.org/user/brade/tor-launcher.git/commit/?h=bug20185-02&id=4dd8f6130f931616cf014e0ded444c30e04c8bad

We also improved some error checking, added missing braces, and fixed the confusing comment. Please review.

Trac:
Keywords: TorBrowserTeam201610 deleted, TorBrowserTeam201610R added
Status: needs_revision to needs_review

Thanks! Looks good to me now and merged to master and maint-0.2.10 (commit 4dd8f6130f931616cf014e0ded444c30e04c8bad and c916965ec2fe0b29c30414bb3c0ce3bf6ca207c5).

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

mentioned in issue #20327 (moved)

mentioned in issue #20691 (moved)

mentioned in issue #20761 (moved)

moved to tpo/applications/tor-browser#20185 (closed)