Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#5603 closed defect (fixed)

get_configured_bridge_by_addr_port_digest is not robust

Reported by: asn Owned by:
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

In bridge_add_from_config() we call get_configured_bridge_by_addr_port_digest() to check if a carbon-copy version of the bridge already exists (so that we don't add duplicate bridges after SIGHUPs).

I think there are two shortcomings in get_configured_bridge_by_addr_port_digest():

a) It's not pluggable transports aware. If the bridge's transport changes, tor won't notice it.
b) If a bridge has a fingerprint and it remains the same, but its address/port changes, tor won't notice it.

  SMARTLIST_FOREACH_BEGIN(bridge_list, bridge_info_t *, bridge)
    {
      if (tor_digest_is_zero(bridge->identity) &&
          !tor_addr_compare(&bridge->addr, addr, CMP_EXACT) &&
          bridge->port == port)
        return bridge;
      if (digest && tor_memeq(bridge->identity, digest, DIGEST_LEN))
        return bridge;
    }

Child Tickets

Change History (9)

comment:1 Changed 7 years ago by arma

Priority: normalmajor

As we start making pluggable transports more commonly used, it looks like this bug is going to start biting our users.

comment:2 Changed 7 years ago by asn

Summary: tor doesn't notice some Bridge line edits after SIGHUPget_configured_bridge_by_addr_port_digest is not robust

I've been kinda hesitant in fixing this, because get_configured_bridge_by_addr_port_digest() is a popular function, and I'm afraid that I might break stuff. For example, it's called when adding bridges from the config, when we learn a new identity digest for a bridge, when we add a bridge to the routerlist, etc..

get_configured_bridge_by_addr_port_digest() doesn't have a well-defined interface or behavior either. For example, some interesting things about the function:

a) It seems that all its arguments are optional. If an addrport is not given, but a digest is given and it matches the digest of a configured bridge, the configured bridge is returned. Similarly, if a digest is not given, but the addrport can match a configured bridge, the configured bridge is returned.

b) If all arguments are provided (addr, port and digest), if the given digest matches a configured bridge's digest, the configured bridge is returned, even if the addrport doesn't match. The same is not true, if the digests don't match but the addrports match.

c) If an addrport and digest are given, and a configured bridge doesn't have a stored digest but its addrport matches the provided addrport, the configured bridge is returned.

I'm not sure which of the above properties are bugs and which are design choices (for example, learned_router_identity() seems to use c)), but it's hard to fix this ticket without a well defined interface/behavior.

To minimize the breakage, maybe we can teach get_configured_bridge_by_addr_port_digest() about pluggable transports in 0.2.3, and then refactor it in 0.2.4. Note that this would not solve bugs like b) If a bridge has a fingerprint and it remains the same, but its address/port changes, tor won't notice it. in bridge_add_from_config().

Teaching the function about PTs would involve adding a const char *transport argument to the function. transport should be optional: if it's not given, we still return bridges that match the other given arguments. If transport is given, we don't return bridges that match the other arguments but have a different transport.

Refactoring the function would involve specifying exactly what the function expects as arguments, and how it treats its arguments. We should also see if a single function can handle all our use cases, and whether we need to split it into multiple functions with different logic.

comment:3 Changed 7 years ago by nickm

I think that kind of change might not necessary; the bridge_add_from_config() code could just change instead. That is, you could change the logic from:

   if ((b = get_configured_bridge_by_addr_port_digest(...)) {
      b->marked_for_removal = 0;
      return;
   }

to:

   if ((b = get_configured_bridge_by_addr_port_digest(...)) &&
       addr matches && port matches && digest matches && transport matches..) {
      b->marked_for_removal = 0;
      return;
   }

comment:4 in reply to:  3 Changed 7 years ago by asn

Replying to nickm:

I think that kind of change might not necessary; the bridge_add_from_config() code could just change instead. That is, you could change the logic from:

   if ((b = get_configured_bridge_by_addr_port_digest(...)) {
      b->marked_for_removal = 0;
      return;
   }

to:

   if ((b = get_configured_bridge_by_addr_port_digest(...)) &&
       addr matches && port matches && digest matches && transport matches..) {
      b->marked_for_removal = 0;
      return;
   }

Hm, true. This sounds more sane.

A complication that might occur is that we can end up having bridges with the same addr/port but with different digests/transports. This does not make much sense, and does not go well with the way that find_transport_by_bridge_addrport() works.

Maybe we should start returning an int in bridge_add_from_config(), signal such conflicts by return -1, and then killing tor while it reads the config? I'm not sure how we should treat such conflicts otherwise (ignore the new bridge and warn the user?).

comment:5 Changed 7 years ago by nickm

I think "only add the first of any set of conflicting bridges, and warn the user" is reasonable behavior.

comment:6 Changed 7 years ago by asn

Status: newneeds_review

Please see branch bug5603 in https://git.gitorious.org/mytor/mytor.git.

I ended up adding the last of any set of conflicting bridges and warning the user, it made the logic easier.

comment:7 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay; I tweaked it a little, added a changes file, and merged. Please have a look at 64167e1772b62a0 and 7cad2e73481bdf when you can?

comment:8 Changed 7 years ago by nickm

Keywords: tor-client added

comment:9 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.