Opened 6 years ago

Closed 2 years ago

#8976 closed defect (fixed)

rend_service_introduce() doesn't notice if the rendezvous point is on 127.0.0.1

Reported by: arma Owned by: teor
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.3.21-rc
Severity: Normal Keywords: tor-hs
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor: SponsorR-must

Description

An anonymous person on irc just pointed out to me that rend_service_introduce() has no checks for whether the introduce2 cell specifies a private address.

I've only looked at it cursorily, but I don't think it's the end of the world. The relay that is asked to extend to this private address will probably have ExtendAllowPrivateAddresses set to 0, and will refuse.

Perhaps it will cause the hidden service to keep trying to reach the rendezvous point (wasting lots of circuits), but that could be induced by telling it a public address that doesn't work too.

Anything else scary? Worth fixing anyway I guess.

Child Tickets

Change History (32)

comment:1 Changed 5 years ago by nickm

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

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

comment:3 Changed 4 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.2.8.x-final
Parent ID: #17674
Severity: Normal

comment:4 Changed 4 years ago by teor

Sponsor: SponsorR

This is fixed by the patch in #17674:

ExtendAllowPrivateAddresses only prevents Tor clients and hidden services from sending extend cells with private addresses.

Relays (and now RSOS) will still connect to a private address if they receive an extend cell or a rendezvous request from a badly behaved client. They might not send an extend request, but they do still connect.

comment:5 Changed 4 years ago by nickm

Status: newneeds_review

comment:6 in reply to:  4 Changed 4 years ago by arma

Replying to teor:

ExtendAllowPrivateAddresses only prevents Tor clients and hidden services from sending extend cells with private addresses.

Relays (and now RSOS) will still connect to a private address if they receive an extend cell or a rendezvous request from a badly behaved client. They might not send an extend request, but they do still connect.

Really? Have you tested this? The code seems pretty clear that it shouldn't happen.

comment:7 Changed 4 years ago by arma

Oh, I get it. The TLS connection happens, including probably the whole tls handshake and link handshake, and then we opt not to send the create cell.

Exciting.

comment:8 Changed 4 years ago by dgoulet

I've done an experiment for that and you can see my summary here: https://trac.torproject.org/projects/tor/ticket/17674#comment:6

This is not entirely fixed by the parent ticket. We still have an issue of wasted resources if we don't catch it when we parse the intro cell.

We should stop that once we have the INTRODUCE2 payload and just refuse it which would save us some resources instead of going through the whole process of extending at the second middle node and failing.

Here is a branch with a fix: bug8976_01_028. It has been tested both with chutney and a live HS.

I've added a function to check if we are allow to extend to a given address. We are starting to use this code pattern at multiple place now so better have a call that validates it. If we like this, we should use it also in #17674.

Last edited 4 years ago by dgoulet (previous) (diff)

comment:9 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

Code Review:

Looks good, but we probably also want to disallow:

  • tor_addr_is_multicast
    • it should be in the header in the latest master, but was a private function a few commits back

(I'd like to also block tor_addr_is_local and the local interface addresses for RSOS, but that's out of scope for this ticket. I've created #17788 for that.)

comment:10 Changed 4 years ago by teor

Parent ID: #17674

#17674 is merged and soon to be closed, leaving this ticket open to catch this issue earlier for rendezvous points.

comment:11 Changed 4 years ago by nickm

Owner: set to dgoulet
Status: needs_revisionassigned

setting dgoulet as the owner of this needs_revision ticket.

comment:12 Changed 4 years ago by nickm

Status: assignedneeds_revision

comment:13 Changed 4 years ago by teor

Owner: changed from dgoulet to teor
Parent ID: #17788
Status: needs_revisionassigned

comment:14 Changed 4 years ago by dgoulet

Status: assignedneeds_revision

comment:15 Changed 4 years ago by teor

Status: needs_revisionneeds_review

Revised as part of #17178.

comment:16 Changed 4 years ago by teor

Parent ID: #17788

I need to cherry-pick f519c356a17afec8dd732ec2e46315c5576283d0 from feature-17178-rsos to dgoulet's bug8976_01_028 so that this security fix can make it into 0.2.8.

We can do the rest of #17788 in 0.2.9.

comment:17 Changed 4 years ago by teor

(In #17788, we can also teach hidden services to reject private addresses. But they won't reject any local machine IP addresses, as those addresses unique to each hidden service.)

comment:18 Changed 4 years ago by teor

Please see my branch bug8976_01_028 on https://github.com/teor2345/tor.git
It blocks both local and multicast addresses for rendezvous.

comment:19 Changed 4 years ago by teor

Version: Tor: 0.2.3.21-rc

(Bugfix on b7c172c9e in tor-0.2.3.21.)

comment:20 Changed 4 years ago by nickm

Milestone: Tor: 0.2.8.x-finalTor: 0.2.7.x-final

looks ok; merged. Does this want an 0.2.7 backport? I could go either way. Please close if not.

comment:21 Changed 3 years ago by dgoulet

Sponsor: SponsorRSponsorR-must

comment:22 Changed 3 years ago by teor

Keywords: 027-backport added

This is an issue that's being triggered by rendezvous addresses sent by Tor clients on the public network. We're not sure if it's an attack or not. So let's backport it to 0.2.7.

comment:23 Changed 3 years ago by andrea

Hmmm - seems hard to imagine what conceivable attack could use such a rendezvous address, since if it did go as far as trying to build a circuit to one, it would be from some relay picked by the HS Tor and not under attacker control, and not from the HS Tor's location. Is there a differential behavior in that case depending on whether the address is reachable, though?

I was leaning toward don't-backport on this one since there didn't seem to be any plausible exploitability; do you really think there might be something going on, teor?

comment:24 in reply to:  23 Changed 3 years ago by teor

Replying to andrea:

Hmmm - seems hard to imagine what conceivable attack could use such a rendezvous address, since if it did go as far as trying to build a circuit to one, it would be from some relay picked by the HS Tor and not under attacker control, and not from the HS Tor's location. Is there a differential behavior in that case depending on whether the address is reachable, though?

Whatever the address, the HS will build a 3 relay path to it.
Then, if it's an internal address, the HS refuses to send an extend cell.
If it's publicly routable, the HS sends an extend cell and connects as normal.

(After this patch, if it's an internal address, the HS refuses to build a path.)

I was leaning toward don't-backport on this one since there didn't seem to be any plausible exploitability; do you really think there might be something going on, teor?

I can't imagine how this behaviour is exploitable, but it does allow an attacker to make the HS build lots of circuits through its guard, which are then terminated in a predictable manner by the HS.

It could simply be a bug in some tor clients.

I could go either way with a backport, I suggested one because I'd rather be safe than sorry.

comment:25 Changed 3 years ago by andrea

Eh, backporting always does carry a small but non-zero risk of new bugs in the old branch, though - it's trading off two different versions of 'safe' rather than a question of 'better safe than sorry'. I think my preferred standard is something more like "plausibly exploitable, or fixes a crash/assert/memory leak level bug"

comment:26 in reply to:  25 Changed 3 years ago by teor

Keywords: 027-backport removed
Resolution: fixed
Status: needs_reviewclosed

Replying to andrea:

Eh, backporting always does carry a small but non-zero risk of new bugs in the old branch, though - it's trading off two different versions of 'safe' rather than a question of 'better safe than sorry'. I think my preferred standard is something more like "plausibly exploitable, or fixes a crash/assert/memory leak level bug"

Fair enough - you have more experience with this than I do.

By that standard, I can't see a plausible way to exploit this - the rendezvous protocol already allows client-specified rendezvous points. It's a slight waste of resources, but that's not important enough.

It's also worth noting that this has just been merged, so it's not received much testing in the alpha series. So the risk of introducing an unintentional bug is higher.

Closing as "don't backport".

comment:27 Changed 3 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.4.x-final
Resolution: fixed
Status: closedreopened

This came up again during backport triage for consideration for an 0.2.4 backport. I've made a "bug8976_024" branch that cherry-picks the regular commits.

comment:28 Changed 3 years ago by nickm

Status: reopenedmerge_ready

comment:29 Changed 3 years ago by arma

That's quite a bit of code!

Do we actually think this needs a backport? It looks like it just saves a bit of wasted energy (building circuits) by an onion service in one edge case, while leaving other (unfixable) edge cases in place.

comment:30 Changed 3 years ago by arma

(Also your google doc marks this one as "don't backport"? Unless I'm reading it wrong?)

comment:31 Changed 3 years ago by nickm

that's right; I'm leaning to "no backport".

comment:32 Changed 2 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.7.x-final
Resolution: fixed
Status: merge_readyclosed

Calling this "no backport" indeed because of the size.

Note: See TracTickets for help on using tickets.