Opened 4 months ago

Closed 6 weeks ago

#22731 closed defect (implemented)

Relative DataDirectory + RunAsDaemon = Tor can't read or write most of its datadirectory files

Reported by: arma Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-23
Cc: mcs Actual Points: .1
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

Bug #22101 shows a case where if you start your Tor with a relative (non absolute) DataDirectory, and RunAsDaemon 1, and CookieAuthentication 1 but no CookieAuthFile, then Tor will attempt to save your control_auth_cookie file to $datadir/$datadir/control_auth_cookie. (Yes, you read that correctly, $datadir/$datadir/.) The attempt will fail because Tor doesn't mkdir the $datadir/$datadir directory.

If you start with a relative DataDirectory, and RunAsDaemon, but no CookieAuthentication, then your Tor will start, but it will attempt to (and fail to) write each of your datadirectory files -- state, cached-microdescriptors, etc, because it is trying to write them to $datadir/$datadir. I think that might be what teor saw in #20456, but he doesn't give us an actual torrc so I can't be sure.

The bug is because we call get_datadir_fname(), which creates a filename that prepends $datadir, after we do the chdir($datadir) operation that comes with RunAsDaemon.

Child Tickets

TicketStatusOwnerSummaryComponent
#22101closedJigsaw52Can't have relative DataDirectory with CookieAuthentication enabledCore Tor/Tor
#22102closedJigsaw52Can't HUP with a relative path in Log lineCore Tor/Tor

Change History (13)

comment:1 Changed 4 months ago by arma

I think the fix is that we should refuse to start when DataDirectory is relative and RunAsDaemon is set.

It basically doesn't work at this point, so I don't think we'd be ruining anybody's day.

And it would simplify a bunch of the logic that has confused us in related bugs (#22101, #20456).

comment:2 Changed 4 months ago by mcs

Cc: mcs added

comment:3 Changed 4 months ago by mcs

Some info about Tor Launcher:

  • It always uses absolute paths for the config directives that it generates in code, e.g., for DataDirectory.
  • Relative paths are used in a torrc-defaults file for some static configuration, e.g. ClientTransportPlugin obfs2,obfs3,obfs4,scramblesuit exec PluggableTransports/obfs4proxy
  • Tor Launcher does not enable RunAsDaemon.

comment:4 in reply to:  1 Changed 4 months ago by catalyst

Replying to arma:

I think the fix is that we should refuse to start when DataDirectory is relative and RunAsDaemon is set.

It basically doesn't work at this point, so I don't think we'd be ruining anybody's day.

And it would simplify a bunch of the logic that has confused us in related bugs (#22101, #20456).

I agree. This is probably the simplest way to solve this set of problems.

comment:5 Changed 4 months ago by nickm

(Once this is done, we can probably close #22101 and #22102, unless they turn out to be more complicated than we had thought.)

comment:6 Changed 6 weeks ago by nickm

Actual Points: .1
Owner: set to nickm
Status: newaccepted

See branch bug22731 in my public repository for a simple implementation here. It turns out that the warn_about_relative_paths() function already did most of what we want.

comment:7 Changed 6 weeks ago by nickm

Status: acceptedneeds_review

comment:8 Changed 6 weeks ago by nickm

Keywords: review-group-23 added

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

comment:9 in reply to:  6 Changed 6 weeks ago by dgoulet

Replying to nickm:

See branch bug22731 in my public repository for a simple implementation here. It turns out that the warn_about_relative_paths() function already did most of what we want.

I see no such branch in nickm/bug22731 :S ...

comment:10 Changed 6 weeks ago by dgoulet

Status: needs_reviewneeds_information

comment:11 Changed 6 weeks ago by nickm

Status: needs_informationneeds_review

Whoops. Try ticket22731 q?

comment:12 Changed 6 weeks ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

Much better!

lgtm;

comment:13 Changed 6 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merging!

Note: See TracTickets for help on using tickets.