OR address is in host order in INTRODUCE2 cell

changed milestone to %Tor: 0.2.4.x-final

added 2016-bug-retrospective component::core tor/tor milestone::Tor: 0.2.4.x-final priority::high resolution::fixed sponsor::R status::closed tor-client tor-hs type::defect labels

-    set_uint32(tmp+1, htonl(extend_info->addr));
+    set_uint32(tmp+1, tor_addr_to_ipv4h(&extend_info->addr));

yeah, not good. Looks like it went into 0.2.1.5-alpha.

I believe the net effect is that the extend request from the hs's circuit to the client's chosen rp will succeed if there's already a tls connection open between them, and will fail otherwise because the extend request will head off to the wrong address.

So that means that some rendezvous attempts by hidden services will fail. Good thing we allow

#define MAX_REND_FAILURES 8

tries (and it was even higher up until #4241 (moved)) -- I guess statistically the odds are pretty good. Still, we could cut down on variance in time-until-success by making the first try actually work.

The clear fix should happen on the client side: it should send the right address rather than the wrong one.

We could also imagine fixing this on the hidden service side -- if it gets an intro2 cell where it recognizes the requested identity key but the addr is different but a permutation of it produces the expected one, it could go ahead and correct it for its extend cell.

I'm inclined to fix only the client side, and let people upgrade if they want things to work. Otherwise we'll drag around the server-side hack for a long time for little real benefit.

I should also point our a privacy problem here: clients on big-endian systems will be sending the correct addr, and clients on little-endian systems will be sending the wrong one. Basically we leak our local host endianness to the hidden service. It doesn't seem like a huge deal but it's worth thinking more about in case I'm wrong.

Trac:
Priority: normal to major
Milestone: Tor: 0.2.6.x-final to Tor: 0.2.5.x-final

diff --git a/src/or/rendservice.c b/src/or/rendservice.c
index 31b612b..7d9978c 100644
--- a/src/or/rendservice.c
+++ b/src/or/rendservice.c
@@ -1292,6 +1292,10 @@ rend_service_introduce(origin_circuit_t *circuit, const u
   if (!rp)
     goto log_error;
 
+  log_notice(LD_REND, "Received intro2 cell, %s %s",
+             fmt_addrport(&rp->addr, rp->port),
+             hex_str(rp->identity_digest, DIGEST_LEN));
+
   /* Check if we'd refuse to talk to this router */
   if (options->StrictNodes &&
       routerset_contains_extendinfo(options->ExcludeNodes, rp)) {

running on moria1 confirms the bug -- the addresses it's printing are in reverse order from the ones the relays advertise.

Replying to arma:

The clear fix should happen on the client side: it should send the right address rather than the wrong one.

See my bug13151-client branch.

Trac:
Status: new to needs_review

Replying to arma:

I believe the net effect is that the extend request from the hs's circuit to the client's chosen rp will succeed if there's already a tls connection open between them, and will fail otherwise because the extend request will head off to the wrong address.

So that means that some rendezvous attempts by hidden services will fail. Good thing we allow {{{ #define MAX_REND_FAILURES 8 }}} tries (and it was even higher up until #4241 (moved)) -- I guess statistically the odds are pretty good. Still, we could cut down on variance in time-until-success by making the first try actually work.

I don't see how the above analysis could be correct, because if it was I'd be seeing a lot more hidden service failures. I regularly start a new tor instance for a short-lived hidden service (using txtorcon) and then connect to it immediately. Sometimes it does take a couple tries to connect, but not usually. Having just started, and being not used for anything else, the HS tor should not be connected to very many nodes... so if a client has only 8 chances to pick one of those nodes as the RP it seems like rendezvouses should be failing much more often than not.

Also, by "tls connection" did you actually mean circuit? My understanding is that tor only has tls connections to the immediate next hop in the circuit, which (if I'm understanding arma's comment correctly, which I must not be) would mean the HS client would need to be picking the HS's guard as its RP for the rendezvous to work. Obviously that can't be right!

lgtm. I think we should backport this to 0.2.4.x.

Cypherpunks: this has nothing to do with how long the hidden service has been online, only with how long the relays you use to connect to the hidden service have been online.

Replying to Sebastian:

lgtm. I think we should backport this to 0.2.4.x.

Cypherpunks: this has nothing to do with how long the hidden service has been online, only with how long the relays you use to connect to the hidden service have been online.

thanks, I think I get it now: the HS transmits both the bad address and the fingerprint in an EXTEND cell down another circuit to the relay at the end and asks to connect to the RP, which it is likely already connected to if it has been up a while.

Replying to cypherpunks:

thanks, I think I get it now: the HS transmits both the bad address and the fingerprint in an EXTEND cell down another circuit to the relay at the end and asks to connect to the RP, which it is likely already connected to if it has been up a while.

Right!

Replying to arma:

We could also imagine fixing this on the hidden service side -- if it gets an intro2 cell where it recognizes the requested identity key but the addr is different but a permutation of it produces the expected one, it could go ahead and correct it for its extend cell.

This fix is now in my bug13151-server-verbose branch.

I'm inclined to fix only the client side, and let people upgrade if they want things to work. Otherwise we'll drag around the server-side hack for a long time for little real benefit.

I still think this is probably true. But maybe it factors into our "do we backport any of this to 0.2.4.x" decisions.

One last additional fix to consider, in addition to the client-side and the hidden-service-side fixes, would be a relay-side fix: We could have relays notice if they've gotten an EXTEND cell for a relay whoise ID they recognize from the consensus with the host IP in the wrong order, and if so, send it to the right IP.

My curernt stance on these patch approaches is "yes, and backport to 0.2.4" on the client-side fix, "probably no" on the hidden-service side, and "maaaaaybe?" on the relay side.

Do you think we should do the relay and hs-dir side fix? I feel just the client-side fix is warranted

What would the hs-dir side be? I don't see any mention of directories above.

I think that the relay side is a maybe; and would like to know what arma thinks.

blergh, I didn't write what I meant. I meant arma's bug13151-server-verbose patch.

No, bug13151-server-verbose is hs-side. I meant doing the same thing, but doing it when we get an EXTEND cell, not when we get an INTRODUCE2 cell.

I've cherry-picked "bug13151-client" to maint-0.2.4 and merged it forward.

I have an untested relay-side patch as "bug13151-relay-side-024" in my public repository.

Currently reviewable branches are my "bug13151-relay-side-024" and arma's "bug13151-server-verbose".

I think we should merge the client-side fix to 0.2.4 and forward (sounds like you already did), and then we should close the bug as fixed. After all, hidden services work right now, so hacking redundant fixes isn't really needed.

Replying to nickm:

I've cherry-picked "bug13151-client" to maint-0.2.4 and merged it forward.

I have an untested relay-side patch as "bug13151-relay-side-024" in my public repository.

Currently reviewable branches are my "bug13151-relay-side-024" and arma's "bug13151-server-verbose".

Hello. bug13151-relay-side-024 looks good to me.

arma's bug13151-server-verbose looks a bit hackish. I don't really like the inline code golf ((i&0xff)<<24)+((i&0xff00)<<8)+((i&0xff0000)>>8)+((i>>24)&0xff) . It also adds a log_notice() to the code.

I also noticed that both of your branches implement very similar functionality (reverse received addr and compare with relay addr, if they match reverse received addr ). I wonder if it can get funcionified (and even unittested).

Replying to arma:

I think we should merge the client-side fix to 0.2.4 and forward (sounds like you already did), and then we should close the bug as fixed. After all, hidden services work right now, so hacking redundant fixes isn't really needed.

Okay. Does anybody think we should merge my relay-side or (some version of) arma's server-verbose branch? If not, let's close this.

OR address is in host order in INTRODUCE2 cell

Child items ...

Activity