Opened 3 years ago

Closed 3 years ago

Last modified 18 months ago

#13151 closed defect (fixed)

OR address is in host order in INTRODUCE2 cell

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-hs, tor-client, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorR

Description

It seems that the IP address of the RP is in host order in INTRODUCE2 cells, when it should be in network order. Here is the bad code https://gitweb.torproject.org/tor.git/blob/f8f0cb0443c0709454c9223f25266ec1b0c464b8:/src/or/rendclient.c#l274 :

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

and here is the code that tries to parse it https://gitweb.torproject.org/tor.git/blob/f8f0cb0443c0709454c9223f25266ec1b0c464b8:/src/or/rendservice.c#l1785 :

tor_addr_from_ipv4n(&extend_info->addr, get_uint32(buf + 1));

Roger found that the bug was introduced in 960a0f0a.

That said, rendezvous seems to work IRL so this bug is not so severe. Rendezvous is completed properly, because the HS probably uses the identity digest to map to a node.

I encountered that bug while fiddling with #12844 and trying to use a relay I just made as my RP. It didn't work.

Child Tickets

Change History (32)

comment:1 Changed 3 years ago by arma

-    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.

comment:2 Changed 3 years ago by arma

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

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) -- 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.

comment:3 Changed 3 years ago by arma

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.

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

Status: newneeds_review

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.

comment:5 in reply to:  2 Changed 3 years ago by cypherpunks

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) -- 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!

comment:6 Changed 3 years ago by 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.

comment:7 in reply to:  6 ; Changed 3 years ago by cypherpunks

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.

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

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!

comment:9 in reply to:  2 Changed 3 years ago by arma

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.

comment:10 Changed 3 years ago by nickm

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.

comment:11 Changed 3 years ago by Sebastian

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

comment:12 Changed 3 years ago by nickm

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.

comment:13 Changed 3 years ago by Sebastian

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

comment:14 Changed 3 years ago by nickm

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.

comment:15 Changed 3 years ago by 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".

comment:16 Changed 3 years ago by 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.

comment:17 in reply to:  15 Changed 3 years ago by asn

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).

comment:18 in reply to:  16 ; Changed 3 years ago by nickm

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.

comment:19 in reply to:  18 Changed 3 years ago by asn

Replying to nickm:

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.

I'm also not sure if we should merge the relay-side or the HS-side. I don't like adding hotfix code to unrelated functions. However, I can see why that would be a good idea.

Maybe a better action would be to ask the TBB team to include the client-side patch in TBB as a guest patch? This will force a big percentage of the clients to upgrade, and stop causing unnecessary EXTEND requests on the network.

comment:20 in reply to:  18 ; Changed 3 years ago by cypherpunks

Replying to nickm:

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.

What about merging the client-side fix, and also arma's HS-side fix with the log message changed to info?

comment:21 in reply to:  20 Changed 3 years ago by nickm

Replying to cypherpunks:

What about merging the client-side fix, and also arma's HS-side fix with the log message changed to info?

It makes me a little nervous to provide a way to probe whether a hidden service knows about a particular node. (It also makes me a little nervous to provide more ways to probe whether a hidden service has upgraded, though I recognize that sometimes it can't be avoided.)

comment:22 Changed 3 years ago by cypherpunks

This bug is the root cause of relays trying and failing to connect to the backwards IP addresses of other relays, where there may or may not be something listening. It should be fixed fully.

Also, some hidden service operators would want to apply the server-side fix regardless, leaving them in a much smaller crowd than if everyone had it.

comment:23 Changed 3 years ago by Sebastian

I still don't think we should do it (merge the hs and relay side fixes):
It adds complexity, the failure mode is not so bad (hidden services mostly worked, and the situation improves quickly as the majority of users upgrade), and the "I get to learn if you're on bigendian" changes to "I get to learn whether you're on littleendian and haven't upgraded".

comment:24 Changed 3 years ago by cypherpunks

I don't see that as much of a big deal. For example, Tor Browser bug fixes already give fine-grained version information by whether or not particular bugs are fixed, and many HS are sending precise HTTP Server: headers, etc. The fix looks to be correctly implemented and reasonably simple, although maybe there should be a comment explaining the line where the IP address is byteswapped?

comment:25 Changed 3 years ago by arma

Keywords: SponsorR added

comment:26 in reply to:  22 Changed 3 years ago by arma

Replying to cypherpunks:

It should be fixed fully.

I'll point out that this is a case where the anonymity of ms cypherpunks might be hurting her. We don't know really know what the motivations are (though I'm sure she means well), and this really is a question of taste.

I'd like to put out an 0.2.5.8-rc in the next couple of days, with the client-side fix that's already merged. With TBB 4's upcoming release it will use the Tor 0.2.5.x branch, and then I think we'll be all set.

comment:27 Changed 3 years ago by arma

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

We went a step farther, and fixed it in 0.2.4.x also.

Closing as done. Thanks all!

comment:28 Changed 3 years ago by Sebastian

Resolution: fixed
Status: needs_reviewclosed

now actually closed

comment:29 Changed 2 years ago by dgoulet

Keywords: SponsorR removed
Sponsor: SponsorR

comment:30 Changed 18 months ago by nickm

Keywords: 2016-bug-retrospective added

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

comment:31 Changed 18 months ago by nickm

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

comment:32 Changed 18 months ago by nickm

Mark bugs for 2016 bug retrospective based on hand-examination of changelogs for 0.2.5 onwards.

Note: See TracTickets for help on using tickets.