Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#19733 closed defect (fixed)

GETINFO response parser doesn't handle AF_UNIX entries.

Reported by: yawning Owned by: tbb-team
Priority: Very Low Milestone:
Component: Applications/Tor Browser Version:
Severity: Minor Keywords: tbb-sandboxing, tbb-torbutton, TorBrowserTeam201609R
Cc: brade, mcs Actual Points:
Parent ID: #14270 Points:
Reviewer: Sponsor:

Description (last modified by yawning)

torbutton issues a GETINFO command to validate the configured socks port, but the response parser doesn't handle AF_UNIX entries.

[07-21 19:17:44] Torbutton WARN: unexpected tor response: net/listeners/socks="127.0.0.1:9050" "unix:/var/run/tor/socks"

This currently doesn't matter because the number of people using an AF_UNIX capable Tor Browser is ~1, but it is something to note for the future sandboxing efforts.

Child Tickets

Change History (14)

comment:1 Changed 3 years ago by yawning

Description: modified (diff)
Summary: PROTOCOLINFO response parser doesn't handle AF_UNIX entries.GETINFO response parser doesn't handle AF_UNIX entries.

Whoops, the command that's puking is GETINFO, not PROTOCOLINFO.

comment:2 Changed 3 years ago by gk

Parent ID: #14270

comment:3 Changed 3 years ago by gk

Component: Applications/TorbuttonApplications/Tor Browser
Keywords: tbb-torbutton added
Owner: set to tbb-team

comment:4 Changed 3 years ago by mcs

Cc: brade mcs added

comment:5 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201608R added
Status: newneeds_review

Kathy and I decided to begin our UNIX domain socket journey by fixing this bug. Here is a patch:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19733-01&id=db21620ac199bd4bc25939f14c6caaaa44093b89

comment:6 Changed 3 years ago by mcs

Status: needs_reviewneeds_revision

Mozilla changed their implementation to use file: URLs instead of simple paths (see https://bugzilla.mozilla.org/show_bug.cgi?id=1211567#c22). We will revise this patch accordingly.

comment:8 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609R added; TorBrowserTeam201608R removed

Moving review tickets to September

comment:9 in reply to:  7 ; Changed 3 years ago by arthuredelstein

Replying to mcs:

Here is a new patch:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19733-02&id=f1932e73ee5199969f49ce595fb30ec14e76cb52

Minor things:

+ if (socksAddr && (socksAddr.substr(0, 5) == "file:")) {

Can this be sockAddr.startsWith("file:")?

+ foundSocksListener = (socketPath == path);
+          foundSocksListener = ((socksAddr == torSocksAddr) &&
+                                (socksPort == torSocksPort));

I would suggest using === (triple equals) for safety.

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

Replying to arthuredelstein:

Replying to mcs:

Here is a new patch:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19733-02&id=f1932e73ee5199969f49ce595fb30ec14e76cb52

Minor things:

+ if (socksAddr && (socksAddr.substr(0, 5) == "file:")) {

Can this be sockAddr.startsWith("file:")?

Done.

+ foundSocksListener = (socketPath == path);
+          foundSocksListener = ((socksAddr == torSocksAddr) &&
+                                (socksPort == torSocksPort));

I would suggest using === (triple equals) for safety.

OK. We fixed another place too where == was used instead of ===. While testing, we discovered that file: URLs that begin with file:/// were not handled correctly (and they should begin that way). So we rewrote the code that converts the network.proxy.socks value to a file path. Please review:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19733-03&id=a07ceb4e7974a1369a1d6484cbfce0a7ec9e503f

comment:11 Changed 3 years ago by gk

Keywords: TorBrowserTeam201609 added; TorBrowserTeam201609R removed
Status: needs_reviewneeds_revision

Looks good with one nit: I guess removing the very first line in torbutton.js is just an oversight? I think we should keep that one. :)

comment:12 in reply to:  11 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201609R added; TorBrowserTeam201609 removed
Status: needs_revisionneeds_review

Replying to gk:

Looks good with one nit: I guess removing the very first line in torbutton.js is just an oversight? I think we should keep that one. :)

Yes indeed. Here is a revised patch:
https://gitweb.torproject.org/user/brade/torbutton.git/commit/?h=bug19733-04&id=5ea022aadf6416a1f046eac47a3ec28ea2a5dd7d

comment:13 Changed 3 years ago by gk

Resolution: fixed
Status: needs_reviewclosed

Thanks, applied with commit 5ea022aadf6416a1f046eac47a3ec28ea2a5dd7d.

comment:14 Changed 3 years ago by bugzilla

Keywords: tbb-sandboxing added; tbb-sandbox removed
Note: See TracTickets for help on using tickets.