Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#14219 closed defect (fixed)

Visiting a down hidden service that was recently up results in many hsdesc fetches

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs, 025-backport
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorR

Description

Run a hidden service, so it correctly establishes intro points and uploads its descriptor correctly. Then shut off the hidden service.

Then try to visit it, and watch your HS_DESC control events. You get something like:

1421275707.240294 [torctl-log] localhost:9051 650 HS_DESC REQUESTED yjiqkj6p7drpiwyq NO_AUTH $0011BD2485AD45D984EC4159C88FC066E5E3300E~CalyxInstitute14 776oil3sjfhbxkj6eewqu3mtzrsnesm2
1421275708.081418 [torctl-log] localhost:9051 650 HS_DESC RECEIVED yjiqkj6p7drpiwyq NO_AUTH $0011BD2485AD45D984EC4159C88FC066E5E3300E~CalyxInstitute14
1421275710.275964 [torctl-log] localhost:9051 650 HS_DESC REQUESTED yjiqkj6p7drpiwyq NO_AUTH $1D6A27AED313662E35F550B212335D4797DCCDF6~CriptoLabTORRelay3 dvcsya66ae4vvzekabjojyrgkdcttnfh
1421275711.550731 [torctl-log] localhost:9051 650 HS_DESC RECEIVED yjiqkj6p7drpiwyq NO_AUTH $1D6A27AED313662E35F550B212335D4797DCCDF6~CriptoLabTORRelay3
1421275713.156023 [torctl-log] localhost:9051 650 HS_DESC REQUESTED yjiqkj6p7drpiwyq NO_AUTH $002121252467538C9DA1BC2378997E64629814F6~metroholografix 776oil3sjfhbxkj6eewqu3mtzrsnesm2
1421275713.767901 [torctl-log] localhost:9051 650 HS_DESC RECEIVED yjiqkj6p7drpiwyq NO_AUTH $002121252467538C9DA1BC2378997E64629814F6~metroholografix
1421275716.254822 [torctl-log] localhost:9051 650 HS_DESC REQUESTED yjiqkj6p7drpiwyq NO_AUTH $000F18AC2CDAE4C710BA0898DC9E21E72E0117D8~TorNinurtaName 776oil3sjfhbxkj6eewqu3mtzrsnesm2
1421275717.405024 [torctl-log] localhost:9051 650 HS_DESC RECEIVED yjiqkj6p7drpiwyq NO_AUTH $000F18AC2CDAE4C710BA0898DC9E21E72E0117D8~TorNinurtaName

In this case I fetched the hsdesc four times for the service. According to the original design, I was supposed to fetch it twice: that is, I fetch it once, try all the intro points, once they've all failed, I fetch it once more just to see if it changed, and if it didn't change, I should stop and fail right then.

So: is there a bug where we continue rather than failing? Or is the descriptor that I receive different in each case? What's going on?

Child Tickets

Change History (12)

comment:1 Changed 5 years ago by arma

Found it!

The issue is that there are actually two copies of the hsdesc that are published -- one for each replica. So when I fetch the descriptor the first time, I get, say, the one that starts with

rendezvous-service-descriptor blwd4dzs2mnncf4wtc3bpv6pljwx5awb

Then when I try all those intro points and give up, I get another one, and I get, say,

rendezvous-service-descriptor b2szzssgq53trigyvau654eqr4k6w756

Then my third fetch got me

rendezvous-service-descriptor blwd4dzs2mnncf4wtc3bpv6pljwx5awb

Then my fourth fetch was

rendezvous-service-descriptor blwd4dzs2mnncf4wtc3bpv6pljwx5awb

and at that point it failed the hs request.

So that's why it's probabilistic -- you keep fetching until you flip heads (without replacement) twice in a row!

comment:2 Changed 5 years ago by arma

For context, the critical code here is this, in rend_cache_store_v2_desc_as_client():

  /* Do we already have this descriptor? */
  if (e && !strcmp(desc, e->desc)) {
    log_info(LD_REND,"We already have this service descriptor %s.",
             safe_str_client(service_id));
    goto okay;
  }

comment:3 Changed 5 years ago by arma

This fixes it for me:

diff --git a/src/or/rendcommon.c b/src/or/rendcommon.c
index 88d9aab..82202b9 100644
--- a/src/or/rendcommon.c
+++ b/src/or/rendcommon.c
@@ -1251,19 +1251,12 @@ rend_cache_store_v2_desc_as_client(const char *desc,
   /* Do we already have a newer descriptor? */
   tor_snprintf(key, sizeof(key), "2%s", service_id);
   e = (rend_cache_entry_t*) strmap_get_lc(rend_cache, key);
-  if (e && e->parsed->timestamp > parsed->timestamp) {
-    log_info(LD_REND, "We already have a newer service descriptor for "
+  if (e && e->parsed->timestamp >= parsed->timestamp) {
+    log_info(LD_REND, "We already have a new enough service descriptor for "
                       "service ID %s with the same desc ID and version.",
              safe_str_client(service_id));
     goto okay;
   }
-  /* Do we already have this descriptor? */
-  if (e && !strcmp(desc, e->desc)) {
-    log_info(LD_REND,"We already have this service descriptor %s.",
-             safe_str_client(service_id));
-    e->received = time(NULL);
-    goto okay;
-  }
   if (!e) {
     e = tor_malloc_zero(sizeof(rend_cache_entry_t));
     strmap_set_lc(rend_cache, key, e);

comment:4 Changed 4 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.6.x-final
Status: newneeds_review

comment:5 Changed 4 years ago by nickm

Keywords: 025-backport added

comment:6 Changed 4 years ago by nickm

lgtm, if it fixes it for you. Marked for possible 025 backport; turning it into a branch and writing a changes file.

comment:7 Changed 4 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.5.x-final

BTW, what was that "Do we already have this descriptor" check supposed to prevent, if anything?

(Merged into master, marking for possible 0.2.5 backport as branch "bug14219_025" in my public repo)

comment:8 in reply to:  7 Changed 4 years ago by arma

Replying to nickm:

BTW, what was that "Do we already have this descriptor" check supposed to prevent, if anything?

Oh ho! Careful here, if you haven't been following the (somewhat twisty) design.

That "do we have this descriptor" check was part of the following design:

When we receive a new stream request for a hidden service:
A) If we don't already have a copy of the descriptor, fetch one, and parse it so we have a local copy of the struct.
B) Try all the intro points, removing each intro point from our local copy of the struct each time we get a definitive NACK from that intro point.
C) If we run out of intro points (i.e. our local copy of the struct says the hs has no intro points), fetch the descriptor again, in case it had changed since our original one.
D) If the new one is indeed different, parse it and replace our local copy of the struct. Else, don't replace.
E) If it is still the case that there are no intro points in the local copy of our struct, fail. Else goto step B.

So the choice in 'D' of whether to replace our local copy of the struct is critical to whether we remember that those intro points had given us NACKs. We used to ask "is the desc exactly the same", which is very similar to "would replacing the local copy of the struct just give us back those failed intro points". Now we ask "was it published on the same second", which we pretend is just as similar. It probably is in practice.

It ain't pretty and you can't dance to it, etc etc.

marking for possible 0.2.5 backport as branch "bug14219_025" in my public repo

Now I'll let you reconsider whether backporting is a good idea. :)

comment:9 Changed 4 years ago by arma

(Actually, to clarify, the bug in this ticket is that step D's "is it the same" check was faulty. We wanted to know if it was "the same descriptor", but there are actually two versions of the descriptor every time it's published (one for each replica), and they're "the same" for our purposes here but the bytes are different. So if we wanted to be thorough, we would do some more complicated check like "is it the same bytes except for the rendezvous-service-descriptor line". My patch approximates that with the much simpler "is it from the same second" check.)

comment:10 Changed 4 years ago by arma

This bug just results in some load on the network, and some delay for the client when visiting a hidden service that will ultimately fail. I think there's no need for a backport.

comment:11 Changed 4 years ago by nickm

Milestone: Tor: 0.2.5.x-finalTor: 0.2.6.x-final
Resolution: fixed
Status: needs_reviewclosed

Hm, okay. Marking for no backport.

comment:12 Changed 4 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR
Note: See TracTickets for help on using tickets.