Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20860 closed defect (fixed)

Regression breaks 'SETCONF HiddenServiceDir'

Reported by: atagar Owned by:
Priority: Very High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version: Tor: 0.3.0.0-alpha-dev
Severity: Major Keywords: tor-hs
Cc: teor Actual Points: 0.4
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

Hi Nick, tor changes pushed today cause tor to crash when calling SETCONF to create a hidden service. Regression caught courtesy of our jenkins instance running stem integ tests. Here's how to repro...

% mkdir /tmp/hs_dir
% chmod 700 /tmp/hs_dir

Then through the tor control port simply call...

SETCONF HiddenServiceDir="/tmp/hs_dir"

This crashes tor right away, though I don't have much more visibility into what is going awry.

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by asn

Keywords: tor-hs regression added
Milestone: Tor: 0.2.9.x-final

comment:2 Changed 3 years ago by nickm

Cc: teor added

Teor?

comment:3 Changed 3 years ago by nickm

Dec 02 14:22:39.000 [warn] Hidden service ("/tmp/hs_dir") with no ports configured.
Dec 02 14:22:39.000 [warn] options_act(): Bug: Previously validated hidden services line could not be added! (on Tor 0.3.0.0-alpha-dev e6facbfe7abc50de)
Dec 02 14:22:39.000 [err] set_options(): Bug: Acting on config options left us in a broken state. Dying. (on Tor 0.3.0.0-alpha-dev e6facbfe7abc50de)

This appears to crash master, but not to crash 0.2.9. So this might not be because of Teor's 0.2.9 patch yesterday.

Asn, are you sure that this belongs in the 0.2.9 milestone? 0.2.9 isn't crashing for me when I do this.

comment:4 Changed 3 years ago by teor

Keywords: regression removed
Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final
Points: 0.1
Version: Tor: 0.3.0.0-alpha-dev

This doesn't fail for me on 0.2.9 either.
Looks like a result of #20559, which added stricter checks.
We obviously need to handle failures at a higher level, rather than letting them get all the way to options_act().

comment:5 Changed 3 years ago by teor

Actual Points: 0.4
Status: newneeds_review

(The directory creation appears unnecessary, the crash occurs regardless for me.)

Please see my branch bug20860-v3 on https://github.com/teor2345/tor.git

The root cause of this bug is that some checks were only getting performed when we acted on the options, and not during validation. This is problematic, because if tor passes validation but fails when acting on options, it asserts and dies.

This patch resolves this issue by performing more of the checks during validation.

Now the results are:

SETCONF HiddenServiceDir="/tmp/hs_dir"
513 Unacceptable option value: Failed to configure rendezvous options. See logs for details.

With the log message:

Dec 03 09:14:21.000 [warn] Hidden service ("/tmp/hs_dir") with no ports configured.
Dec 03 09:14:21.000 [warn] Controller gave us config lines that didn't validate: Failed to configure rendezvous options. See logs for details.

(Previously, this would appear to succeed:

250 OK

but actually fail, and log failure messages to the log.)

When a valid service is added, the output is:

SETCONF HiddenServiceDir="/tmp/hs_dir" HiddenServicePort=80
250 OK

When a duplicate hidden service is added, the output is:

SETCONF HiddenServiceDir="/tmp/hs_dir" HiddenServicePort=80 HiddenServiceDir="/tmp/hs_dir" HiddenServicePort=80
513 Unacceptable option value: Failed to configure rendezvous options. See logs for details.
Dec 03 09:16:53.000 [warn] Another hidden service is already configured for directory "/tmp/hs_dir".
Dec 03 09:16:53.000 [warn] Controller gave us config lines that didn't validate: Failed to configure rendezvous options. See logs for details.

There is still a bug in check_private_dir where tor will crash if any directory configured in the options can't be created, but that's been present since at least 0.2.7, and is due to missing checks on the parent directory:

    /* XXXX In the case where check==CPD_CHECK, we should look at the
     * parent directory a little harder. */

So, for example:

SETCONF HiddenServiceDir="/" HiddenServicePort=80

will likely cause a crash.

comment:6 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

This looks plausible to me. Merging.

comment:7 Changed 3 years ago by atagar

Thanks! Stem tests updated to account for this. Works great.

Note: See TracTickets for help on using tickets.