Opened 5 years ago

Closed 5 years ago

#12948 closed defect (fixed)

TBB Linux 4.0-Alpha-1 HashedControlPassword not working

Reported by: vis15 Owned by: isis
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Keywords: TBB, Linux, HashedControlPassword, Alpha, regression
Cc: isis, nickm, mcs, brade Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

On TBB 4.0-Alpha Linux 64-bit when trying to connect to the control port with the password of "secret" I get the following error: "515 Authentication failed: Password did not match HashedControlPassword *or* authentication cookie."

In start-tor-browser script file, line 225 "unset TOR_CONTROL_PASSWD" this leaves the control password blank. Every time I start TBB the HashedControlPassword is different.

When setting TOR_CONTROL_PASSWD in the terminal, TBB fails to start. Error message is: "[warn] Bad password or authentication cookie on controller."

When setting TOR_CONTROL_PASSWD in start-tor-browser script file the same error message is reported "[warn] Bad password or authentication cookie on controller."

When manually setting HashedControlPassword in torrc, tor ignored it and sometimes changed it.

All errors happened on a clean/brand new Linux Mint 64-bit Virtual Machine.

Child Tickets

Change History (16)

comment:1 Changed 5 years ago by arma

Bug reporter (on irc) says that 3.6.4 is working. He suggests that #10178 might be related.

comment:2 in reply to:  1 Changed 5 years ago by isis

Cc: isis added
Owner: changed from tbb-team to isis
Status: newaccepted

Replying to arma:

Bug reporter (on irc) says that 3.6.4 is working. He suggests that #10178 might be related.


<strikeout>Bug #10178 is related, it's what causes this. That was my patch.</strikeout>

These are not the droids you're looking for.

Last edited 5 years ago by isis (previous) (diff)

comment:3 Changed 5 years ago by vis15

Last edited 5 years ago by vis15 (previous) (diff)

comment:4 Changed 5 years ago by mikeperry

Cc: nickm added
Component: Tor BrowserTor

Ok, I *think* this might be due to the new command line parser in 0.2.5.1. It appears that the HashedControlPassword command line parameter is overriding the torrc HashedControlPassword entry that we're adding manually. Based on the manpage, providing one HashedControlPassword on the command line and one in the torrc should cause both to apply, which was the case in 0.2.4.x.

nickm - Am I right? Is there any way we can have the command line HashedControlPassword not override the torrc value, but be treated as an additional password, like it was in 0.2.4.x?

Here's the changelog entry that made me suspect Tor as the culprit here:

  o Minor bugfixes (command line):
    - Use a single command-line parser for parsing torrc options on the
      command line and for finding special command-line options to avoid
      inconsistent behavior for torrc option arguments that have the same
      names as command-line options. Fixes bugs 4647 and 9578; bugfix on
      0.0.9pre5.

comment:5 in reply to:  4 Changed 5 years ago by isis

Replying to mikeperry:

Ok, I *think* this might be due to the new command line parser in 0.2.5.1.


I likewise spent some time debugging it, and believe that the above is the most likely culprit. I'll stay the owner for now, and I'm still willing to help. However if there's a more appropriate owner, feel free to reassign.

@vis15: Thanks for reporting this!

comment:6 Changed 5 years ago by nickm

nickm - Am I right? Is there any way we can have the command line HashedControlPassword not override the torrc value, but be treated as an additional password, like it was in 0.2.4.x?

I believe this part of the manpage is what you need:

By default, an option on the command line overrides an option found in the
configuration file, and an option in a configuration file overrides one in
the defaults file.

This rule is simple for options that take a single value, but it can become
complicated for options that are allowed to occur more than once: if you
specify four SOCKSPorts in your configuration file, and one more SOCKSPort on
the command line, the option on the command line will replace __all__ of the
SOCKSPorts in the configuration file.  If this isn't what you want, prefix
the option name with a plus sign, and it will be appended to the previous set
of options instead.

Alternatively, you might want to remove every instance of an option in the
configuration file, and not replace it at all: you might want to say on the
command line that you want no SOCKSPorts at all.  To do that, prefix the
option name with a forward slash.

So if you say "+HashedControlPassword foo" on the command line, it will append to the old list of HashedControlPasswords rather than replacing it.

comment:7 Changed 5 years ago by arma

The plot thickens!

HashedControlPassword is special because of this entry in option_abbrevs_[]:

  { "HashedControlPassword", "__HashedControlSessionPassword", 1, 0},

So when you say HashedControlPassword on the command-line, tor quietly rewrites it for you so it's like you said __HashedControlSessionPassword. And because the line starts with __, saveconf knows not to write it to the torrc file for you.

At least, that's how it used to behave. It looks like 0.2.5.x might be different now.

Last edited 5 years ago by arma (previous) (diff)

comment:8 Changed 5 years ago by arma

(We put the __HashedControlSessionPassword hack in specifically for Vidalia, when it was in just this situation: see bug #586.

The goal is that you can specify a long-term hashedcontrolpassword in your torrc yourself, and also Vidalia or TorLauncher or whatever your controller is can generate one on the fly for that session, without worrying about clobbering the one you set. But it looks like we've lost this behavior in 0.2.5.x.)

comment:9 in reply to:  8 ; Changed 5 years ago by mikeperry

Replying to arma:

(We put the __HashedControlSessionPassword hack in specifically for Vidalia, when it was in just this situation: see bug #586.

The goal is that you can specify a long-term hashedcontrolpassword in your torrc yourself, and also Vidalia or TorLauncher or whatever your controller is can generate one on the fly for that session, without worrying about clobbering the one you set. But it looks like we've lost this behavior in 0.2.5.x.)

Ok. We don't want Tor Launcher's password saved in the torrc. But we also want it to be used in addition to any HashedControlPasswords in the torrc.

Is +_HashedControlSessionPassword what we want then?

comment:10 in reply to:  9 Changed 5 years ago by arma

Replying to mikeperry:

Ok. We don't want Tor Launcher's password saved in the torrc. But we also want it to be used in addition to any HashedControlPasswords in the torrc.

That's exactly the behavior I thought Tor had.

Is +_HashedControlSessionPassword what we want then?

I'm currently thinking this is a Tor regression in 0.2.5.x, and Tor Launcher shouldn't have to workaround it (assuming we can identify and fix it).

comment:11 Changed 5 years ago by arma

Commit d98dfb374679 is when we lost the call to config_expand_abbrev(&options_format, s, 1, 1)

comment:12 Changed 5 years ago by arma

Confirmed: this patch fixes it for me:

index 6bb6209..921503b 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -1932,7 +1932,8 @@ config_parse_commandline(int argc, char **argv, int ignore
     }
 
     param = tor_malloc_zero(sizeof(config_line_t));
-    param->key = is_cmdline ? tor_strdup(argv[i]) : tor_strdup(s);
+    param->key = is_cmdline ? tor_strdup(argv[i]) :
+                   tor_strdup(config_expand_abbrev(&options_format, s, 1, 1));
     param->value = arg;
     param->command = command;
     param->next = NULL;

comment:13 Changed 5 years ago by arma

Keywords: regression added
Milestone: Tor: unspecifiedTor: 0.2.5.x-final
Status: acceptedneeds_review

My bug12948 branch has that patch plus a changelog file.

comment:14 Changed 5 years ago by arma

It also occurs to me that since this case is really only for these two cases:

  { "l", "Log", 1, 0},
[...]
  { "HashedControlPassword", "__HashedControlSessionPassword", 1, 0},

and I don't think anybody even remembers the "-l means log" case, we might be better served stripping the commandline_only element of config_abbrev_t, and removing the command_line option to config_expand_abbrev(), and just adding an explicit special-case where this patch went, that says "if it's HashedControlPassword, then rewrite it to this other thing, and here's why".

I guess there's value to having the general-case infrastructure if we later find a second reason to use it, but in the meantime it sure seems overengineered.

comment:15 Changed 5 years ago by mcs

Cc: mcs brade added

comment:16 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks like that branch was against release-0.2.5, not maint-0.2.5? Cherry-picking it to maint-0.2.5 and merging forward.

Note: See TracTickets for help on using tickets.