Stored IPv6 TransportProxy bindaddr lacks brackets

changed milestone to %Tor: 0.2.4.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac milestone::Tor: 0.2.4.x-final in Legacy / Trac owner::asn in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac status::closed in Legacy / Trac tor-bridge in Legacy / Trac type::defect in Legacy / Trac labels

Trac:
dummy.sh

Trac:
Keywords: N/A deleted, tor-bridge added
Component: Pluggable transport to Tor

I started looking at src/or/transports.c to find occurences of fmt_addr() that should be changed to fmt_and_decorate_addr() and I noticed that all the fmt_addr()s should be changed (https://gitweb.torproject.org/tor.git/blob/HEAD:/src/or/transports.c#l277 https://gitweb.torproject.org/tor.git/blob/HEAD:/src/or/transports.c#l284 https://gitweb.torproject.org/tor.git/blob/HEAD:/src/or/transports.c#l333 https://gitweb.torproject.org/tor.git/blob/HEAD:/src/or/transports.c#l645 etc.).

I started thinking that maybe it would make sense to change the behavior of fmt_addr() so that it does decorate IPv6 addresses by default, but then jeroen in IRC reminded me that bracket decoration is only necessary when ':' is used a separator, and maybe we shouldn't decorate all the addresses.

I think I will proceed with looking at all the pluggable-transport-related code and turning fmt_addr() to fmt_and_decorate_addr() when needed.

Trac:
Priority: minor to normal

Replying to asn:

I started thinking that maybe it would make sense to change the behavior of fmt_addr() so that it does decorate IPv6 addresses by default, but then jeroen in IRC reminded me that bracket decoration is only necessary when ':' is used a separator, and maybe we shouldn't decorate all the addresses.

Maybe what you want is a new fmt_addr_port or similar that takes both an address and a port number. This would additionally allow factoring out some "%s:%u" and "%s:%d" formats; such specifiers can be taken as hints that decoration is needed.

Trac:
Milestone: N/A to Tor: 0.2.4.x-final

Trac:
fmt_addrport.patch

Here is a patch set doing these things. It is meant to be applied on top of https://gitorious.org/mytor/mytor/commit/acfebfdc3907cb1838b7b7dcf6913f22ebce4c72 from https://trac.torproject.org/projects/tor/ticket/7014#comment:3.

[PATCH 1/5] decorates all addresses in log messages. This should be uncontroversial as it doesn't affect any network or on-disk format. [PATCH 2/5] decorates addresses in HTTP CONNECT proxy requests. [PATCH 3/5] decorates the TransportProxy statefile entry, the subject of this ticket. [PATCH 4/5] adds a new fmt_addrport function to factor out the common "%s:%d" formatting. [PATCH 5/5] uses fmt_addrport where appropriate.

There was only one fmt_addr I wasn't sure how to deal with, in pt_get_extra_info_descriptor_string in transports.c:

      const char *addr_str = fmt_addr(&t->addr);
      ...
      smartlist_add_asprintf(string_chunks,
                             "transport %s %s:%u",
                             t->name, addr_str, t->port);

I don't know for sure whether the brackets are wanted here or not. I am guessing they are.

Trac:
Status: new to needs_review

This looks okay to me for inclusion in 0.2.4. It doesn't need to go in 0.2.3, does it? It's much too big and broad for that.

It's got the same memory leak problem as legacy/trac#7014 (moved) does, though, so it will conflict with the fix for that.

asn, have you had a chance to review this?

Trac:
Cc: ln5 to ln5, asn

Maybe review of this patchset would be easier if [PATCH 4/5] did not invalidate the previous patches? I think it would make more sense if the patchset introduced fmt_addrport() right from the beginning and then simply used it.

Also, this patchset makes make check-spaces fail because of wide lines.

David, do you have the time and will to address the above comments or shall I?

Trac:
Status: needs_review to needs_revision

I actually like the approach of switching to fmt_addrport(). the fmt_addrport() change is a refactoring, and refactoring commits should not IMO change behavior.

Replying to nickm:

I actually like the approach of switching to fmt_addrport(). the fmt_addrport() change is a refactoring, and refactoring commits should not IMO change behavior.

Oh. Looking at it this way, it's indeed better.

Then I guess the only thing that remains is fixing that check-spaces complaint and rebasing to the new legacy/trac#7014 (moved)?

Trac:
fmt_addrport-bis.patch

Replying to asn:

Then I guess the only thing that remains is fixing that check-spaces complaint and rebasing to the new legacy/trac#7014 (moved)?

Here is a new patch rebased against the new legacy/trac#7014 (moved) and without any new check-spaces errors.

It still remains to decide what to do with the fmt_addr in pt_get_extra_info_descriptor_string.

Trac:
Status: needs_revision to needs_review

pt_get_extra_info_descriptor_string needs some attention indeed. Mainly because of the fmt_addr32() that is called.

How about changing:

      /* If the transport proxy returned "0.0.0.0" as its address, and
       * we know our external IP address, use it. Otherwise, use the
       * returned address. */
      const char *addr_str = fmt_addr(&t->addr);
      uint32_t external_ip_address = 0;
      if (tor_addr_is_null(&t->addr) &&
          router_pick_published_address(get_options(),
                                        &external_ip_address) >= 0) {
        /* returned addr was 0.0.0.0 and we found our external IP
           address: use it. */
        addr_str = fmt_addr32(external_ip_address);
      }

      smartlist_add_asprintf(string_chunks,
                             "transport %s %s:%u",
                             t->name, addr_str, t->port);

to

      /* If the transport proxy returned "0.0.0.0" as its address, and
       * we know our external IP address, use it. Otherwise, use the
       * returned address. */
      const char *addrport = NULL;
      uint32_t external_ip_address = 0;
      if (tor_addr_is_null(&t->addr) &&
          router_pick_published_address(get_options(),
                                        &external_ip_address) >= 0) {
        tor_addr_t addr;
        tor_addr_from_ipv4h(&addr, external_ip_address);
        addrport = fmt_addrport(&addr, t->port);
      } else {
        addrport = fmt_addrport(&t->addr, t->port);
      }

      smartlist_add_asprintf(string_chunks,
                             "transport %s %s",
                             t->name, addrport);

BTW, router_pick_published_address() seems to only set 32-bit IP addresses, so using tor_addr_from_ipv4h() should be OK. Why does router_pick_published_address() only work for 32-bit IP addresses?

Aaron, what will happen if bridgedb starts getting IPv6 addresses in the "transport" extra-info lines? So for example instead of "transport bla 7.7.7.7:4444" it will get "transport bla [1234:4444::34]:5555". Is BridgeDB prepared for this occasion?

Trac:
Cc: ln5, asn to ln5, asn, aagbsn

BridgeDB parses IPv6 transport lines. See: https://tor.extc.org/?transport=obfs2&ipv6=True :-)

So IIRC I'm waiting on a final revised version of this. Is that right?

The solution above looks ok.

Trac:
Status: needs_review to needs_revision

Replying to aagbsn:

BridgeDB parses IPv6 transport lines. See: https://tor.extc.org/?transport=obfs2&ipv6=True :-)

So what is it doing, is it assuming that the last colon in an address:port string is always a port separator, even when address is IPv6 without brackets? The code in comment:5 would, for example, format the address 1234::5678 and port 9999 as

transport foo 1234::5678:9999

and not

transport foo [1234::5678]:9999

So in other words, is there code somewhere that already expects IPv6 addresses in transport lines not to have brackets, and are we going to break it if we add brackets?

Replying to dcf:

Replying to aagbsn:

BridgeDB parses IPv6 transport lines. See: https://tor.extc.org/?transport=obfs2&ipv6=True :-)

So what is it doing, is it assuming that the last colon in an address:port string is always a port separator, even when address is IPv6 without brackets? The code in comment:5 would, for example, format the address 1234::5678 and port 9999 as {{{ transport foo 1234::5678:9999 }}} and not {{{ transport foo [1234::5678]:9999 }}}

So in other words, is there code somewhere that already expects IPv6 addresses in transport lines not to have brackets, and are we going to break it if we add brackets?

No, I should have clarified. BridgeDB expects IPv6 addresses to be bracketed, just like the "or-address" and "a" lines are. If you -don't- add brackets, BridgeDB will not parse the IP and skip the line.

Trac:
fmt_addrport-bisbis.patch

Here is a revised patch, including asn's change from comment:11, and even a changes file.

To be clear, this patch fixes three related bugs:

Lack of brackets in HTTP CONNECT proxy requests.
Lack of brackets in TransportProxy statefile entries.
Lack of brackets in extra-info transport lines.

Then, further, it adds the fmt_addrport function and uses it in most places.

Trac:
Status: needs_revision to needs_review

Great; I merged it into master with7ea904cb. You may want to check the merge to make sure I didn't resolve the conflict incorrectly; it seemed pretty trivial though.

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

closed

mentioned in issue legacy/trac#12138 (moved)

moved from legacy/trac#7011 (moved)

added Bug label and removed 1 deleted label

added Bridge label and removed 1 deleted label

Stored IPv6 TransportProxy bindaddr lacks brackets

Child items 0

Activity