Opened 4 years ago

Closed 4 years ago

Last modified 18 months ago

#10777 closed defect (fixed)

Remotely triggerable circuit destruction by path bias code

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

Description

TBB 3.5 - lot of Unable to connect errors
TBB user reports:

Additional info:
 - I am on 3.5.1 now and have the same issues
 - it seems thing work fine if I use the default torrc file

What I change is to add about 20 MapAddress lines to block ad servers.

I use the same list on 2.3.25 and there it causes no problems.

Child Tickets

Change History (29)

comment:1 Changed 4 years ago by cypherpunks

Summary: Tor 0.2.4.x breaks something if many MapAddress definedTor 0.2.4.x breaks something if some MapAddress defined

comment:2 Changed 4 years ago by nickm

Keywords: tor-client added; client removed
Milestone: Tor: 0.2.4.x-final

Are there instructions on how to reproduce this?

comment:3 Changed 4 years ago by nickm

Status: newneeds_information

comment:4 Changed 4 years ago by cypherpunks

Bug reporter sent examples with Mapaddress. I'll wait for confirmation before posting full answer.

comment:5 Changed 4 years ago by cypherpunks

Priority: normalmajor
Summary: Tor 0.2.4.x breaks something if some MapAddress definedRemotely triggerable circuit destruction by path biase code

comment:6 Changed 4 years ago by cypherpunks

Summary: Remotely triggerable circuit destruction by path biase codeRemotely triggerable circuit destruction by path bias code

comment:7 Changed 4 years ago by cypherpunks

Client sends begin cell with something-trigger.

Exit relay process it and does:
connection_exit_connect:

  log_debug(LD_EXIT,"about to try connecting");
  switch (connection_connect(conn, conn->address, addr, port, &socket_error)) {
    case -1: {
      int reason = errno_to_stream_end_reason(socket_error);
      connection_edge_end(edge_conn, reason);

errno_to_stream_end_reason:

    E_CASE(EBADF):
    E_CASE(EFAULT):
    E_CASE(EINVAL):
    S_CASE(EISCONN):
    S_CASE(ENOTSOCK):
    S_CASE(EPROTONOSUPPORT):
    S_CASE(EAFNOSUPPORT):
    E_CASE(EACCES):
    S_CASE(ENOTCONN):
    S_CASE(ENETUNREACH):
      return END_STREAM_REASON_INTERNAL;

Exit sent relay_end cell.

Client process this cell by connection_ap_process_end_not_open.

  if (rh->length > 0) {
    if (reason == END_STREAM_REASON_TORPROTOCOL ||
        reason == END_STREAM_REASON_INTERNAL ||
        reason == END_STREAM_REASON_DESTROY) {
      /* All three of these reasons could mean a failed tag
       * hit the exit and it complained. Do not probe.
       * Fail the circuit. */
      circ->path_state = PATH_STATE_USE_FAILED;
      log_warn(LD_APP,"Got '%d' end reason.", reason);
      return -END_CIRC_REASON_TORPROTOCOL;

Client destroys circuit, detaches all streams and close them.

First, it's bug for exit side to count some of those reasons as internal. (EHOSTUNREACH counted as END_STREAM_REASON_NOROUTE while ENETUNREACH is internal for some reason)

Second, it's bug in general to use stream end reason for path bias detection purpose. Without warns (except non informative "connection_edge_process_relay_cell (at origin) failed."), without chance to reattach not yet connected streams to new circuit, with destroying all already attached streams. With reasons that chose by exit under external pressing.

That was series of minor bugs, now it's solid remotely triggerable circuit destruction.

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

comment:8 Changed 4 years ago by nickm

Cc: mikeperry added
Status: needs_informationneeds_review

To be clear, it's circuit destruction that's triggerable by the exit node, right? But the exit node can already trigger circuit destruction by sending a DESTROY cell. The real problematic case is if the user can be tricked into sending something that causes an ENETUNREACH response from the exit node.

In any case, we should ENETUNREACH to give NOROUTE. There's a patch for that as "bug10777_noroute_024"

If a third party *can* trigger this, we need to remove the case END_STREAM_REASON_INTERNAL case from connection_ap_process_end_notopen, treating it as neither a path-bias success nor a path-bias failure. There's a patch for that as "bug10777_nointernal_024."

Mike, I am leaning towards merging both. Please let me know if this makes path bias useless.

Also, there's maybe a third bug: If the user triggered this by using MapAddress to map advertising networks to some netblock we should have recognized as private., that should probably have taken effect and caused the stream to get blocked connection to a private address *before* the RELAY_BEGIN cell is ever sent. (Was it a private network block, or something else?)

comment:9 Changed 4 years ago by cypherpunks

Path bias detection code that used stream end reasons is bug. Every stream end reason can be triggered by third party pressure.

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

comment:10 Changed 4 years ago by cypherpunks

Was it a private network block, or something else?

It was broadcast address.
You can't to block all broadcast addresses because client can't to know all of them for exit relay and x.x.x.255 not always means broadcast address.

And you no need broadcast address only to trigger this bug. If exit relay process ICMP codes you can to answer with need code for controllable address. Then it's not only about ENETUNREACH.

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

comment:11 in reply to:  9 Changed 4 years ago by nickm

Replying to cypherpunks:

Path bias detection code that used stream end reasons is bug. Every stream end reason can be triggered by third party pressure.

Including TORPROTOCOL and DESTROY? How?

comment:12 Changed 4 years ago by cypherpunks

END_STREAM_REASON_DESTROY can't happens actually in wire, relay can't to send it. Every place it assigns end_reason to END_STREAM_REASON_DESTROY circuit already destroyed and CELL_DESTROY sent.
It's never working checks of END_STREAM_REASON_DESTROY by client.

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

comment:13 Changed 4 years ago by cypherpunks

END_STREAM_REASON_TORPROTOCOL is non triggerable by 3rd party, it seems. At least I didn't found way to trigger relay to send it, (still looking in code though). Not sure it's usable for path bias detection code however. But that probably another bug.

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

comment:14 Changed 4 years ago by cypherpunks

Are there instructions on how to reproduce this?

Additional information from the bug reporter:

The problem also happens with unmodified torrc. It seems this way it
happens less often.

It is mostly entries like this, to block ad servers:

MapAddress ad-emea.doubleclick.net 255.255.255.255
MapAddress oglasi.slo-tech.com 255.255.255.255
MapAddress ads.poraba.com 255.255.255.255
MapAddress googleads.g.doubleclick.net 255.255.255.255
MapAddress pagead2.googlesyndication.com 255.255.255.255
MapAddress d2.zedo.com 255.255.255.255
MapAddress cdn2.adexprt.com 255.255.255.255
MapAddress d7.zedo.com 255.255.255.255
MapAddress cdn1.clkads.com 255.255.255.255
MapAddress cdn.adnxs.com 255.255.255.255
MapAddress tpc.googlesyndication.com 255.255.255.255
MapAddress ssl.google-analytics.com 255.255.255.255

comment:15 Changed 4 years ago by cypherpunks

Why EACCES is internal reason?

comment:16 Changed 4 years ago by nickm

Why EACCES is internal reason.

You're right, it shouldn't be. Neither should EPERM:

       EACCES, EPERM
              The user tried to connect to a broadcast address without  having
              the  socket  broadcast  flag  enabled  or the connection request
              failed because of a local firewall rule.

I pushed an updated bug10777_netunreach_024. Any other cases?

The branches to review are still "bug10777_netunreach_024" and "bug10777_nointernal_024".

comment:17 Changed 4 years ago by mikeperry

bug10777_netunreach_024 seems ok. However, I am not a fan of bug10777_nointernal_024..
This branch would prevent us from detecting that entire class of route manipulation (tagging the initial relay cells after a circuit was established).

Specifically, in circuit_receive_relay_cell(), we use END_STREAM_REASON_INTERNAL to mean that the relay cell was not recognized, which is exactly what we'd see for a simple XOR tag that was meant to be undone by a cooperating exit node, but hit an honest exit node instead.

comment:18 in reply to:  17 Changed 4 years ago by mikeperry

Replying to mikeperry:

bug10777_netunreach_024 seems ok. However, I am not a fan of bug10777_nointernal_024..
This branch would prevent us from detecting that entire class of route manipulation (tagging the initial relay cells after a circuit was established).

Specifically, in circuit_receive_relay_cell(), we use END_STREAM_REASON_INTERNAL to mean that the relay cell was not recognized, which is exactly what we'd see for a simple XOR tag that was meant to be undone by a cooperating exit node, but hit an honest exit node instead.

Actually, no I'm wrong. We only return INTERNAL if the decryption itself fails, not for unrecognized. Still digging for other cases...

comment:19 Changed 4 years ago by mikeperry

I don't see any first order ways to trigger END_STREAM_REASON_INTERNAL, assuming that the recognized digest is strong enough. However, if you can get past that, in begin_cell_parse() and a couple other places, you can tag the relay command field and trigger those. That might not be a concern though?

comment:20 Changed 4 years ago by nickm

Mike: Okay. I'll go through and try to find any way that a tagging attempt could cause END_STREAM_REASON_INTERNAL in an obvious way, and if I don't, I'll plan to merge.

cypherpunks: do you see any more error codes that should change?

comment:21 Changed 4 years ago by cypherpunks

We only return INTERNAL if the decryption itself fails, not for unrecognized. Still digging for other cases...

All working end stream reasons can be sent only after success relay cell decryption. Exit or AP can't to send relay_end if digest or 'recognized' field was wrong. Any STREAM END reason, including END_STREAM_REASON_INTERNAL and END_STREAM_REASON_TORPROTOCOL. Exit sends end stream (it's not circuit!) only if plain text was invalid for some reasons.

If you believe tagging attack can be undetectable by honest exit node during digest checking, why do you think checking end stream reason have any sense?

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

comment:22 Changed 4 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

I'll go through and try to find any way that a tagging attempt could cause END_STREAM_REASON_INTERNAL in an obvious way, and if I don't, I'll plan to merge.

Didn't find any, so ...

merged those branches into 0.2.4 and master. Thanks!

comment:23 Changed 4 years ago by cypherpunks

why do you think

Any answer, please? Does it need new ticket?

comment:24 in reply to:  23 Changed 4 years ago by nickm

Replying to cypherpunks:

why do you think

Any answer, please? Does it need new ticket?

Mike, please open a new ticket if you think there needs to be one.

comment:25 Changed 4 years ago by cypherpunks

Any chance to get answer?

comment:26 in reply to:  21 Changed 4 years ago by mikeperry

Replying to cypherpunks:

We only return INTERNAL if the decryption itself fails, not for unrecognized. Still digging for other cases...

All working end stream reasons can be sent only after success relay cell decryption. Exit or AP can't to send relay_end if digest or 'recognized' field was wrong. Any STREAM END reason, including END_STREAM_REASON_INTERNAL and END_STREAM_REASON_TORPROTOCOL. Exit sends end stream (it's not circuit!) only if plain text was invalid for some reasons.

If you believe tagging attack can be undetectable by honest exit node during digest checking, why do you think checking end stream reason have any sense?

I don't think that. I was asking if Nick might think that. I think we're both going on the assumption that the recognized digest is strong enough to trust for this purpose, and so it is fine to remove the INTERNAL response. I did not check that there are no other ways to generate the other reasons though. It seems like you might be able get TORPROTOCOL if you messed with a relay cell such that it didn't cause the circuit to fail.

Causing the stream to fail such that it is retried on a new circuit (without failing the old one) is still a source for path bias, though. Even if Tor doesn't retry that stream, there is a high likelihood that the client will.

comment:27 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:28 Changed 18 months ago by nickm

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

comment:29 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.