Opened 8 years ago

Closed 7 years ago

#7011 closed defect (fixed)

Stored IPv6 TransportProxy bindaddr lacks brackets

Reported by: dcf Owned by: asn
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: ln5, asn, aagbsn Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

A managed proxy can announce that it is listening on an IPv6 address:

VERSION 1
SMETHOD dummy [::]:10000
SMETHODS DONE

tor caches the address in ~/.tor/state without brackets around the IPv6 address:

Oct 02 03:18:30.000 [info] handle_proxy_line(): Got a line from managed proxy '/home/david/branches/tor/dummy.sh': (VERSION 1)
Oct 02 03:18:30.000 [info] handle_proxy_line(): Got a line from managed proxy '/home/david/branches/tor/dummy.sh': (SMETHOD dummy [::]:10000)
Oct 02 03:18:30.000 [info] parse_smethod_line(): Server transport dummy at [::]:10000.
Oct 02 03:18:30.000 [info] handle_proxy_line(): Got a line from managed proxy '/home/david/branches/tor/dummy.sh': (SMETHODS DONE)
Oct 02 03:18:30.000 [info] handle_methods_done(): Server managed proxy '/home/david/branches/tor/dummy.sh' configuration completed!
Oct 02 03:18:30.000 [info] save_transport_to_state(): It's the first time we see this transport. Let's save its address:port
Oct 02 03:18:30.000 [notice] Registered server transport 'dummy' at ':::10000'
TransportProxy dummy :::10000

The next time running the transport, tor writes to the environment a TOR_PT_SERVER_BINDADDR without brackets as well.

TOR_PT_ORPORT=127.0.0.1:9001
TOR_PT_EXTENDED_SERVER_PORT=
TOR_PT_STATE_LOCATION=/home/david/.tor/pt_state/
TOR_PT_MANAGED_TRANSPORT_VER=1
TOR_PT_SERVER_TRANSPORTS=dummy
TOR_PT_SERVER_BINDADDR=dummy-:::10000

Perhaps the IPv6 address should have brackets? The attached script can be used to reproduce this:

tor SocksPort 0 ORPort 9001 Log "info stderr" ServerTransportPlugin "dummy exec $(pwd)/dummy.sh"

Child Tickets

Attachments (4)

dummy.sh (317 bytes) - added by dcf 8 years ago.
fmt_addrport.patch (41.6 KB) - added by dcf 7 years ago.
fmt_addrport-bis.patch (42.1 KB) - added by dcf 7 years ago.
fmt_addrport-bisbis.patch (45.8 KB) - added by dcf 7 years ago.

Download all attachments as: .zip

Change History (22)

Changed 8 years ago by dcf

Attachment: dummy.sh added

comment:1 Changed 8 years ago by rransom

Component: Pluggable transportTor
Keywords: tor-bridge added

comment:2 Changed 8 years ago by asn

Priority: minornormal

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.

comment:3 in reply to:  2 Changed 8 years ago by dcf

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.

comment:4 Changed 8 years ago by arma

Milestone: Tor: 0.2.4.x-final

Changed 7 years ago by dcf

Attachment: fmt_addrport.patch added

comment:5 Changed 7 years ago by dcf

Status: newneeds_review

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.

comment:6 Changed 7 years ago by nickm

Cc: asn added

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 #7014 does, though, so it will conflict with the fix for that.

asn, have you had a chance to review this?

comment:7 Changed 7 years ago by asn

Status: needs_reviewneeds_revision

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?

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

comment:9 in reply to:  8 ; Changed 7 years ago by asn

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 #7014?

Changed 7 years ago by dcf

Attachment: fmt_addrport-bis.patch added

comment:10 in reply to:  9 Changed 7 years ago by dcf

Status: needs_revisionneeds_review

Replying to asn:

Then I guess the only thing that remains is fixing that check-spaces complaint and rebasing to the new #7014?

Here is a new patch rebased against the new #7014 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.

comment:11 Changed 7 years ago by asn

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?

comment:12 Changed 7 years ago by asn

Cc: aagbsn added

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?

comment:13 Changed 7 years ago by aagbsn

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

comment:14 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

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

The solution above looks ok.

comment:15 in reply to:  13 ; Changed 7 years ago by 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?

comment:16 in reply to:  15 Changed 7 years ago by aagbsn

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.

Changed 7 years ago by dcf

Attachment: fmt_addrport-bisbis.patch added

comment:17 Changed 7 years ago by dcf

Status: needs_revisionneeds_review

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.

comment:18 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

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

Note: See TracTickets for help on using tickets.