Opened 4 years ago

Closed 3 years ago

#18753 closed defect (implemented)

Unix socket paths cannot contain spaces

Reported by: special Owned by: nickm
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: nickm-deferred-20160905, tbb-needs, review-group-10
Cc: gk, brade, mcs, h9F7Gn Actual Points: 1
Parent ID: Points: 1
Reviewer: dgoulet Sponsor:

Description

Tor does not handle quotes when parsing *Port configuration lines, and uses only the first space-separated part of the value as the port. It's not possible to have a unix socket in a path containing spaces.

A second related bug: Tor does not error when garbage flags are included at the end of a *Port configuration line.

Child Tickets

Change History (22)

comment:1 Changed 4 years ago by special

This might also apply to HiddenServicePort and the control port ADD_ONION command.

comment:2 Changed 3 years ago by nickm

Points: 1

comment:3 Changed 3 years ago by yawning

Owner: set to yawning
Status: newassigned

I'll take this. The functionality here is likely needed for the future Tor Browser sandboxing efforts. We should also re-examine our control port stuff that deals with paths to ensure sanity.

comment:4 Changed 3 years ago by nickm

Keywords: nickm-deferred-20160905 added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

Hi, Yawning! I'm deferring these tickets assigned to you from 0.2.9 to 0.2.???, since you're going to be out for September. But if you wind up wanting to do any of them for 0.2.9 anyway, please feel free to move them back.

(This is my ticket-deferring afternoon)

comment:5 Changed 3 years ago by gk

Cc: gk added

comment:6 Changed 3 years ago by mcs

Cc: brade mcs added

comment:7 Changed 3 years ago by gk

Keywords: tbb-needs added

comment:8 Changed 3 years ago by gk

Status: assignedneeds_information

If we could this bug back on track for 0.2.9 that would be neat, although I guess we want to backport a fix for the tor we ship in Tor Browser anyway.

mcs/brade: Do you have a preferred syntax for this fix?

comment:9 Changed 3 years ago by bugzilla

You are going to push a socket into the interface intended for a port and try to make it work.
(OT: What about #3501?)

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

Replying to gk:

mcs/brade: Do you have a preferred syntax for this fix?

We do not have a syntax in mind; anything that works well and fits with tor's configuration philosophy is good as far as we are concerned. The syntax suggested by nickm in #tor-dev would be fine:

SocksPort unix:"/foo/bar/baz quux/socket" IPv6Traffic PreferIPv6Automap

comment:11 Changed 3 years ago by nickm

Branch spaces_in_unix_addrs in my public repository may DTRT here. Needs review and acceptance testing.

comment:12 Changed 3 years ago by nickm

Actual Points: 1
Owner: changed from yawning to nickm
Status: needs_informationaccepted

comment:13 Changed 3 years ago by nickm

Status: acceptedneeds_review

comment:14 Changed 3 years ago by mcs

This change should work for Tor Browser. We need to modify Tor Launcher to use the new quoted syntax for the ControlPort and SocksPort options, and we need to modify Torbutton to correctly parse responses to GETINFO net/listeners/socks. But Kathy and I made enough of a start on the Tor Launcher and Torbutton changes to convince ourselves that this tor patch works.

comment:15 Changed 3 years ago by gk

Cc: h9F7Gn added

Resolved #20327 as duplicate.

comment:16 Changed 3 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.9.x-final

comment:17 Changed 3 years ago by nickm

Keywords: review-group-10 added

comment:18 Changed 3 years ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_information

This lgtm and works well for SocksPort, HS Port and ControlPort but doesn't work with ControlSocket which takes a path (without unix: prefix though). Do we care about that one not having the possibility to put spaces or quoted string?

Here is an example of it not working:

ControlSocket "/tmp/a control socket"

Apart from that, lgtm;

comment:19 Changed 3 years ago by nickm

I think we now generally prefer the newer FooPort unix:.... syntax to the older FooSocket ... syntax.

comment:20 Changed 3 years ago by dgoulet

Status: needs_informationmerge_ready

Should we then move ControlSocket to the deprecated option list? :)

comment:21 Changed 3 years ago by nickm

I'd take a patch. :)

comment:22 Changed 3 years ago by nickm

Resolution: implemented
Status: merge_readyclosed

(Merged!)

Note: See TracTickets for help on using tickets.