Opened 3 years ago

Closed 3 years ago

#20185 closed defect (fixed)

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

Reported by: gk Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Major Keywords: TorBrowserTeam201610R
Cc: mcs, brade, arthuredelstein Actual Points:
Parent ID: #14270 Points:
Reviewer: Sponsor:

Description

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

Child Tickets

Change History (18)

comment:1 Changed 3 years ago by mcs

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).

comment:2 Changed 3 years ago by cypherpunks

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".

comment:3 in reply to:  2 Changed 3 years ago by mcs

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).

comment:4 Changed 3 years ago by cypherpunks

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).

comment:5 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609 added

comment:6 Changed 3 years ago by arthuredelstein

Cc: arthuredelstein added

comment:7 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201609 removed

Moving tickets to October.

comment:8 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201610 removed
Status: newneeds_review

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 before applying this one.

comment:9 Changed 3 years ago by 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?

comment:10 Changed 3 years ago by gk

Keywords: TorBrowserTeam201610 added; TorBrowserTeam201610R removed
Status: needs_reviewneeds_revision

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 :: resource://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.

comment:11 Changed 3 years ago by gk

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

comment:12 in reply to:  9 ; Changed 3 years ago by 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).

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

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 :: resource://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).

comment:14 in reply to:  12 ; Changed 3 years ago by gk

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.

comment:15 in reply to:  14 Changed 3 years ago by mcs

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).

comment:16 Changed 3 years ago by yawning

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

comment:17 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201610R added; TorBrowserTeam201610 removed
Status: needs_revisionneeds_review

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.

comment:18 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

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

Note: See TracTickets for help on using tickets.