Opened 3 years ago

Closed 3 years ago

Last modified 17 months ago

#20638 closed defect (fixed)

Non-anonymous single-hop HS enabled tor doesn't detect already existing anonymous, HS at start-up

Reported by: ahf Owned by: teor
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.3-alpha
Severity: Normal Keywords: tor-hs, sos
Cc: asn Actual Points: 1.1
Parent ID: Points: 0.5
Reviewer: asn Sponsor:

Description

While trying to configure a toy non-anonymous single-hop onion service I ran into something that I believe is a bug.

The torrc that I reused from an earlier experiment already contained a HS configuration entry created in anonymous mode. When I enabled HiddenServiceSingleHopMode and HiddenServiceNonAnonymousMode and started up tor everything started up correctly and I was able to reach the configured HS service correctly. But when I sent the tor process a SIGHUP tor exited with the following error:

Nov 11 17:40:06.000 [notice] Read configuration file "/Users/ahf/torrc".
Nov 11 17:40:06.000 [notice] HiddenServiceSingleHopMode is enabled; disabling UseEntryGuards.
Nov 11 17:40:06.000 [warn] We are configured with HiddenServiceNonAnonymousMode 1, but one or more hidden service keys were created in an anonymous mode. This is not allowed.
Nov 11 17:40:06.000 [err] Reading config failed--see warnings above. For usage, try -h.
Nov 11 17:40:06.000 [warn] Restart failed (config error?). Exiting.

Steps to reproduce:

1) Create a anonymous HS in the torrc.
2) Start tor to make it create the keys for the HS.
3) Stop tor.
4) Enable HiddenServiceSingleHopMode and HiddenServiceNonAnonymousMode
5) Start tor (which will start without problems).
6) Send tor a SIGHUP which will cause it to exit with an error listed above.

I would expect tor to exit when starting up and not when it's re-reading its configuration file.

I've been able to reproduce this with the 0.2.9.5-alpha Debian package and with Git HEAD (0980787f91cfc420) on the master branch.

Child Tickets

Change History (16)

comment:1 Changed 3 years ago by ahf

It seems like at the initial start of tor that the:

if (!rend_service_list) { /* No global HS list. Nothing to see here. */
  return 0;
}

path in in rend_service_list_verify_single_onion_poison() is taken, which will make the start up of tor proceed.

comment:2 Changed 3 years ago by dgoulet

Keywords: tor-hs sos added
Milestone: Tor: 0.2.9.x-final
Version: Tor: 0.3.0.0-alpha-devTor: 0.2.9.3-alpha

If that check is really the problem, it has been released in tor-0.2.9.3-alpha so changing version here.

comment:3 in reply to:  1 ; Changed 3 years ago by arma

Cc: asn teor added

Replying to ahf:

It seems like at the initial start of tor that the:

if (!rend_service_list) { /* No global HS list. Nothing to see here. */
  return 0;
}

path in in rend_service_list_verify_single_onion_poison() is taken, which will make the start up of tor proceed.

Looks plausible!

It looks like this code went in during commit b560f852, as part of ticket #17178. So I cc asn and teor since they're listed on that commit. :)

rend_service_list_verify_single_onion_poison() is called from inside options_validate_single_onion() which is called from inside options_validate(), which is the function used to examine the new proposed 'options' set before acting on any of them. So it is not right for a function inside options_validate() to try to look at the rend_service_list.

Better would be to either move that rend_service_list_verify_single_onion_poison() check to options_act() after it's called rend_config_services(), or to change rend_config_services() so it does the checks you want when validate_only is true, i.e. when it's being called from options_validate. I'd be weakly inclined towards the latter approach, because this is exactly the sort of thing that counts as "checking to see if you're going to like the new options, before committing to them". Specifically, see the rend_service_check_private_dir() calls in rend_config_services() -- maybe that's a good place for doing this further examination of the directory?

comment:4 in reply to:  3 ; Changed 3 years ago by arma

Replying to arma:

Specifically, see the rend_service_check_private_dir() calls in rend_config_services()

I opened #20692 for the fact that we have two identical code stanzas there, just waiting for somebody to fix one and not notice the other. Careful!

comment:5 in reply to:  4 Changed 3 years ago by teor

Replying to arma:

Replying to arma:

Specifically, see the rend_service_check_private_dir() calls in rend_config_services()

I opened #20692 for the fact that we have two identical code stanzas there, just waiting for somebody to fix one and not notice the other. Careful!

This is fixed in #20484, which is under review for inclusion in 0.3.0.

comment:6 Changed 3 years ago by teor

(Also, the code stanzas were previously not identical, which caused #20529.)

comment:7 Changed 3 years ago by teor

Owner: set to teor
Status: newassigned

comment:8 Changed 3 years ago by teor

Actual Points: 1.0
Points: 0.5
Status: assignedneeds_review

My branch bug20638_029 on github fixes this issue.

Passes:

Non-Anonymous:

src/or/tor HiddenServiceDir /tmp/tor.$$.nonanon HiddenServicePort 80 DataDirectory /tmp/tor.$$ HiddenServiceNonAnonymousMode 1 HiddenServiceSingleHopMode 1 SocksPort 0 PidFile /tmp/tor.pid ControlPort 12345

Anonymous:

src/or/tor HiddenServiceDir /tmp/tor.$$.anon HiddenServicePort 80 DataDirectory /tmp/tor.$$ SocksPort 0 PidFile /tmp/tor.pid ControlPort 12345
  • They also re-read their existing config ok when HUP'd.

Using stem's tor-prompt works fine, with the expected failure when Flags=NonAnonymous does not match the anonymity setting:

./tor-prompt -i 12345
ADD_ONION NEW:BEST Port=1234 [Flags=NonAnonymous]
DEL_ONION <service-id>

comment:9 Changed 3 years ago by asn

Status: needs_reviewneeds_revision

Nice work teor. Patch seems to work for me. The poisoning detection now works without a need for SIGHUP.

Some nitpicking around the patch:

  • I feel that the else { block in rend_service_check_dir_and_add() is a bit convoluted. For example, I think the BUG(!s_list) block can be turned into an assert, to assist with readability.
  • On the same note, in rendservice.c I now see three blocks of:
      /* If no special service list is provided, then just use the global one. */
      /* ended up with more of this */
      if (!service_list) {
        if (BUG(!rend_service_list)) {
          return -1;
        }
        s_list = rend_service_list;
      } else {
        s_list = service_list;
      }
    
    This is one more than we started with (also see comment:4). I wonder, if we can abstract that logic into a get_hs_service_list() function that returns the service list if found, otherwise it returns NULL.
  • Now some comment nitpicking. Double comment here:
        if (service->directory != NULL) { /* Skip dupe for ephemeral services. */
          /* Skip dupe for ephemeral services. */
    

Also, what does this comment mean? I think it's one of those with a version string that will rot in the codebase:

/* Ignore service failures until 030 */
  • Finally, I think it might be worthwhile to add a small paragraph to the commit msg of 59d525d29 to actually explain how the patch fixes the bug.

I'm turning this into needs_revision, please feel free to fix whichever of the comments above you find reasonable.

comment:10 Changed 3 years ago by asn

(BTW, I just reviewed #20559 as well, and it seems like the branch there addresses almost all of the issues I pointed above, so that's great. I think only my review comment wrt the commit msg, and the double comment are still relevant from above.)

comment:11 in reply to:  10 Changed 3 years ago by teor

Actual Points: 1.01.1
Reviewer: asn
Status: needs_revisionneeds_review

Replying to asn:

(BTW, I just reviewed #20559 as well, and it seems like the branch there addresses almost all of the issues I pointed above, so that's great.

Sorry about that, I was trying to minimise code changes in a late alpha.

I think only my review comment wrt the commit msg, and the double comment are still relevant from above.)

See my branch bug20638_029_v2 for the fixup and commit message change.

comment:12 Changed 3 years ago by nickm

The code looks okay here. I am still worried by the size of the change, but I don't think anything terrible will happen if I squash and merge into 0.2.9

comment:13 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I've squashed and merged this to maint-0.2.9 and forwards. Thanks!

comment:14 Changed 3 years ago by nickm

(Please have a look, though, and make sure my merge commit of 5efbd41daa8741 looks okay to you)

comment:15 Changed 3 years ago by teor

I found #20853 when checking this merge, but that's a separate issue that has nothing to do with this ticket.

So the merge looks fine to me. Thanks!

comment:16 Changed 17 months ago by teor

Cc: teor removed

Remove useless CC

Note: See TracTickets for help on using tickets.