#22101 closed defect (wontfix)

Can't have relative DataDirectory with CookieAuthentication enabled

Reported by: pastly Owned by: Jigsaw52
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-18, review-group-19
Cc: danielpinto52@…, mcs Actual Points:
Parent ID: #22731 Points:
Reviewer: Sponsor:

Description

DataDirectory ./datadir-3hop
Log notice file ./datadir-3hop/notice.log
RunAsDaemon 1
ControlPort 9980
CookieAuthentication 1

leads to

Apr 29 12:29:03.000 [notice] Tor 0.3.0.6 (git-47d2e4f06ec26a79) opening new log file.
Apr 29 12:29:02.994 [warn] OpenSSL version from headers does not match the version we're running with. If you get weird crashes, that might be why. (Compiled with 1000105f: OpenSSL 1.0.1e 11 Feb 2013; running with 1000105f: OpenSSL 1.0.1e-fips 11 Feb 2013).
Apr 29 12:29:03.013 [notice] Tor 0.3.0.6 (git-47d2e4f06ec26a79) running on Linux with Libevent 2.0.21-stable, OpenSSL 1.0.1e-fips and Zlib 1.2.7.
Apr 29 12:29:03.013 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Apr 29 12:29:03.014 [notice] Read configuration file "/home/mtraudt/src/qtime-with-load-testing/torrc-3hop".
Apr 29 12:29:03.015 [warn] Path for DataDirectory (./datadir-3hop) is relative and will resolve to /home/mtraudt/src/qtime-with-load-testing/./datadir-3hop. Is this what you wanted?
Apr 29 12:29:03.016 [notice] Opening Socks listener on 127.0.0.1:9050
Apr 29 12:29:03.016 [notice] Opening Control listener on 127.0.0.1:9980
Apr 29 12:29:03.000 [warn] Couldn't open "./datadir-3hop/control_auth_cookie.tmp" (./datadir-3hop/control_auth_cookie) for writing: No such file or directory
Apr 29 12:29:03.000 [warn] Error writing auth cookie to "./datadir-3hop/control_auth_cookie".
Apr 29 12:29:03.000 [warn] Error creating control cookie authentication file.
Apr 29 12:29:03.000 [err] set_options(): Bug: Acting on config options left us in a broken state. Dying. (on Tor 0.3.0.6 47d2e4f06ec26a79)

Setting DataDirectory to the absolute path in the torrc allows Tor to start

Child Tickets

TicketStatusOwnerSummaryComponent
#20456closedRelative paths don't work for PidFile and control_auth_cookieCore Tor/Tor

Change History (31)

comment:1 Changed 17 months ago by dgoulet

Milestone: Tor: 0.3.1.x-final

comment:2 Changed 17 months ago by Jigsaw52

I've looked into this for a bit and found the cause of the issue:

In the function options_act (in config.c), finish_daemon is called and finish_daemon changes the current directory to the DataDirectory.

Later on options_act, init_control_cookie_authentication is called, which will try to create the control_auth_cookie file inside the DataDirectory.

Because DataDirectory is a relative path, it will try to create the control_auth_cookie file inside CURRENT_DIR/DataDirectory.

The problem is that, because CURRENT_DIR was changed to DataDirectory by finish_daemon, it will try to create the control_auth_cookie file inside DataDirectory/DataDirectory which does not exist, resulting in the error we see on the logs.

There may more bugs similar to this, as any call to get_datadir_fname after CURRENT_DIR is changed will result in the wrong path when a relative path is used for the DataDirectory.

comment:3 Changed 17 months ago by teor

Version: Tor: 0.3.0.6

Thanks for looking into this, Jigsaw52!

I think we should make get_datadir_fname() return an absolute path. We need to do this when we change CURRENT_DIR, but maybe we should always do it for consistency.

(Removing the version, because this bug has been in tor for a long time.)

comment:4 in reply to:  2 ; Changed 17 months ago by pastly

Replying to Jigsaw52:

There may more bugs similar to this, as any call to get_datadir_fname after CURRENT_DIR is changed will result in the wrong path when a relative path is used for the DataDirectory.

Replying to teor

I think we should make get_datadir_fname() return an absolute path. We need to do this when we change CURRENT_DIR, but maybe we should always do it for consistency.

(IMHO and without looking into it any more than the bug reports I've already made) it is starting to sound like this is also causing #22102. Perhaps there's a whole sea of buggy assumptions that could be squashed in one fell swoop.

comment:5 in reply to:  4 Changed 17 months ago by teor

Replying to pastly:

Replying to Jigsaw52:

There may more bugs similar to this, as any call to get_datadir_fname after CURRENT_DIR is changed will result in the wrong path when a relative path is used for the DataDirectory.

Replying to teor

I think we should make get_datadir_fname() return an absolute path. We need to do this when we change CURRENT_DIR, but maybe we should always do it for consistency.

(IMHO and without looking into it any more than the bug reports I've already made) it is starting to sound like this is also causing #22102. Perhaps there's a whole sea of buggy assumptions that could be squashed in one fell swoop.

There's at least one more for the PidFile, too.

comment:6 Changed 17 months ago by Jigsaw52

Status: newneeds_review

I wrote a patch to make get_datadir_fname() always return an absolute path.
I save the current directory in the tor_init function and use it to create the absolute path.

Here is my branch: https://github.com/Jigsaw52/tor/tree/relative_path_fix_22101

Also, I believe this is also causing #20456.

Last edited 17 months ago by Jigsaw52 (previous) (diff)

comment:7 Changed 17 months ago by Jigsaw52

Cc: danielpinto52@… added

comment:8 Changed 17 months ago by nickm

Status: needs_reviewneeds_revision

What about the third case of the options_get_datadir_fname2_suffix(), where we just copy the entire DataDirectory into the fname output?

On the whole, let me suggest an alternative fix to this problem: There are over 30 places in the codebase where we refer to options->DataDirectory in one way or another. What if they were all replaced with a function like get_data_directory(), and options->DataDirectory were only used inside functions that cared what the user actually typed? That one function could be the place that makes DataDirectory an absolute path.

comment:9 Changed 17 months ago by Jigsaw52

Owner: set to Jigsaw52
Status: needs_revisionassigned

I followed your suggestion and updated the code get_data_directory.
I used the same branch: https://github.com/Jigsaw52/tor/tree/relative_path_fix_22101
Also, #22102 is not caused by this.

comment:10 Changed 17 months ago by Jigsaw52

Status: assignedneeds_review

comment:11 Changed 17 months ago by Jigsaw52

I think it might be a good idea to make get_initial_directory() a global utility function and check all calls to make_path_absolute to see if it actually makes sense to use the current directory instead of the initial directory.

comment:12 Changed 17 months ago by Jigsaw52

I've merged and refactored my code for this fix and the fix for #22102.

The main changes are:

  • I created the get_data_directory() function suggested above and replaced instances of DataDirectory with it where possible.
  • I've changed make_path_absolute() to use the tor starting directory instead of the current directory to make an absolute path and moved it to util.c.
  • I've changed expand_filename() to return an absolute path.

The branch is here: https://github.com/Jigsaw52/tor/tree/relative_paths_fixes

This fixes #22101, #22102 and #20456.

comment:13 Changed 17 months ago by arma

The issue is that RunAsDaemon is set as well, right? So the problem is (a) relative paths plus (b) setting runasdaemon?

In the past, our answer was "you cannot have relative paths and also set runasdaemon."

Trying to do both has certainly confused people in the past about what behavior they ought to get. (Some behavior will now be relative to the datadirectory, and some of it won't? That cannot end well, right?)

comment:14 Changed 17 months ago by Jigsaw52

Yes, the problem only happens when RunAsDaemon is set.

What happens is that, when RunAsDaemon is set, the relative paths are relative to the DataDirectory but when RunAsDaemon is not set, they are relative to the tor starting directory.

I tried to fix the issue by making the relative paths always relative to the tor starting directory.

comment:15 Changed 17 months ago by nickm

Huh, I'm not sure whether or not that's behavior people would have expected. What do you think, Roger? Should everything be relative to the datadirectory, the cwd, or what?

comment:16 Changed 16 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

0.3.1 feature freeze today: these don't seem like they will be all sorted out in time. Let's try to make progress in 0.3.2.

comment:17 Changed 15 months ago by nickm

Keywords: review-group-18 added

comment:18 Changed 15 months ago by isis

Reviewer: isis

comment:19 in reply to:  15 Changed 15 months ago by arma

Replying to nickm:

Huh, I'm not sure whether or not that's behavior people would have expected. What do you think, Roger? Should everything be relative to the datadirectory, the cwd, or what?

I'm also not certain what behavior we should expect people to expect.

I would be ok with "if you want to use relative paths, you'd best specify them from wherever you are starting your Tor".

The trouble is, we already have users, and they have already done whatever they've done. So if we change the behavior on them, we're going to be breaking things for those users who rely on the current behavior. And I'm sure somewhere out there is a person who thinks that relative paths, relative to the datadir, makes perfect sense.

Things aren't *so* bad since this only applies when you set runasdaemon, and I think it's mostly packages that set runasdaemon. But even there, is there somebody who wrote HiddenServiceDir hidserv in their /etc/tor/torrc on debian? Probably?

I wonder if there are any reasonable migation paths, like warning people about things for a while first.

comment:20 Changed 15 months ago by arma

Taking a step back:

Couldn't open "./datadir-3hop/control_auth_cookie.tmp" (./datadir-3hop/control_auth_cookie) for writing: No such file or directory

Is Tor applying its "make the relative path absolute" algorithm twice here? That would be a bug, and one worth fixing.

comment:21 Changed 15 months ago by nickm

Okay; these are going to have to wait, IIUC, until somebody can figure out

  • What the best behavior is, and
  • How we can change it without breaking everybody who relies on the current behavior.

comment:22 Changed 15 months ago by nickm

Keywords: review-group-19 added

comment:23 in reply to:  20 Changed 15 months ago by arma

Replying to arma:

Is Tor applying its "make the relative path absolute" algorithm twice here? That would be a bug, and one worth fixing.

In particular, check out how init_cookie_authentication() in config.c just takes an fname argument, and that comes straight out of get_controller_cookie_file_name() in control.c. And when CookieAuthFile isn't set, it uses get_datadir_fname("control_auth_cookie") instead. The get_datadir_fname mess in config.h is deeply confusing to me, but, is it prepending the datadir onto the control_auth_cookie name, after it has done a chdir() to the datadir?

I'm having trouble getting strace -f to cooperate with me, but I think in pastly's case Tor is trying to write to ./datadir-3hop/datadir-3hop/control_auth_cookie.tmp which is why it fails. And that's a bug in how we handle the default control_auth_cookie filename. Which should be fixable separate from the bigger-picture stuff above.

comment:24 Changed 15 months ago by isis

Reviewer: isis

comment:25 Changed 15 months ago by arma

Though. If you change pastly's torrc to no longer include CookieAuthentication 1, Tor starts up, but its log file has a bunch of other complaints, e.g.

Jun 26 17:23:50.000 [warn] Couldn't open "./datadir-3hop/unverified-microdesc-consensus.tmp" (./datadir-3hop/unverified-microdesc-consensus) for writing: No such file or directory
Jun 26 17:23:50.000 [warn] Couldn't open "./datadir-3hop/cached-certs.tmp" (./datadir-3hop/cached-certs) for writing: No such file or directory
Jun 26 17:23:50.000 [warn] Error writing certificates to disk.
Jun 26 17:23:50.000 [warn] Failed to unlink ./datadir-3hop/unverified-microdesc-consensus: No such file or directory

It looks like get_datadir_fname() in general can't handle having a relative path for a datadir.

comment:26 Changed 15 months ago by arma

But! I have a fine 'state' file sitting in ./datadir-3hop/

And that appears to be written, in or_state_save(), using get_datadir_fname("state").

So now I'm just confused.

I think we might want to do a full refactoring here. (Easy for me to say.)

comment:27 in reply to:  26 Changed 15 months ago by arma

Replying to arma:

But! I have a fine 'state' file sitting in ./datadir-3hop/

And that appears to be written, in or_state_save(), using get_datadir_fname("state").

So now I'm just confused.

I think we might want to do a full refactoring here. (Easy for me to say.)

Ah ha!

Jun 26 17:26:56.000 [warn] Unable to write state to file "./datadir-3hop/state"; will try again later

I guess Tor wrote a state file before it did the chdir, and then after the chdir it tried to write the other state file location and failed.

Ok, my theory remains intact!

comment:28 Changed 15 months ago by mcs

Cc: mcs added

comment:29 in reply to:  25 Changed 15 months ago by arma

Replying to arma:

It looks like get_datadir_fname() in general can't handle having a relative path for a datadir.

Nick suggested that I open a new ticket if this turns into a different bug, so I did: #22731.

I believe if we do that ticket as described, we can close #22101 and #20456.

comment:30 Changed 15 months ago by nickm

Parent ID: #22731
Status: needs_reviewneeds_revision

Marking this as needs_revision and child of #22731. Once #22731 is done, we can close this ticket.

comment:31 Changed 13 months ago by nickm

Resolution: wontfix
Status: needs_revisionclosed

wontfixing in favor of parent.

Note: See TracTickets for help on using tickets.