Opened 7 years ago

Closed 6 years ago

#10801 closed defect (fixed)

parse_bridge_line() does tor_addr_port_lookup(). Perhaps it should parse, not lookup.

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.5.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client, 024-backport, 025-triaged
Cc: asn, kpdyer, yawning Actual Points:
Parent ID: Points:
Reviewer: Sponsor:


The tor_addr_port_lookup() function uses the DNS resolver if it receives a hostname. We don't want to support hostnames in bridge addresses, do we? Nothing in our spec says we do, does it?

Alternatively, if we *do* want to support hostnames, we should drop the test in test_config.c that does:


Since being on a DNS-hijacking cafe wifi seems to make the test fail, when a.b.c.d _does_ resolve to something.

Child Tickets

Change History (19)

comment:1 Changed 7 years ago by asn

Cc: kpdyer added

I'm also not sure whether this is a feature or a bug.

For example, I know that Kevin Dyer is using DNS names in his FTE bundles so that he can dynamically support multiple IP addresses, or seamlessly change between IP addresses. Bridge domain names might provide a bit of censorship circumvention qualities against adversaries who can only do IP block. Of course, if the adversary can block DNS queries, it doesn't make any difference.

Hm, not sure what to do. I will think about it some more.

comment:2 Changed 7 years ago by dcf

I often use


when I'm testing things; I would have to memorize my bridge's IP address, but that's no big deal.

Resolving the names at the time of parsing is indeed strange. I would be less surprised if the names were resolved at the time of the SOCKS connection, but that's fairly minor. If you assume that the name has to be resolved one way or another, it doesn't matter much when you resolve it, but some transports may not want the name to be resolved (or may want to resolve it in their own way).

For me, it would be nice if Tor would pass unresolved host names to pluggable transports, and allow the pluggable transport to resolve it (or not). One use case is the mnemonic one above. Another is the new meek transport, which doesn't use a bridge IP address, but rather a URL:

Bridge meek url=

The bridge address is bogus and is ignored. The host name in the URL is resolved by the transport. Imagine instead:

Bridge meek

You can in fact write it this way now, but it won't work because Tor will pre-resolve the name, and the transport will receive only an IP address and will not know what to put in the Host header of its HTTP requests. I admit this case is not so compelling, because I wouldn't actually write it that way. A separate url argument allows you to include a path, which the hostname:port syntax doesn't. But for what it's worth, Tor passing host names to transports is what I assumed would happen when I started coding things, and I was surprised that it was resolving the names.

It might be nice from a UX point of view if there was a way to indicate that the address field is going to be ignored, like with meek and flash proxy. A special invalid domain name could work for that (but is maybe not the best way).

comment:3 Changed 7 years ago by yawning

Cc: yawning@… added

comment:4 Changed 6 years ago by yawning

Cc: yawning added; yawning@… removed

For the record the obfsproxy SOCKSv5 code merged as part of #9221 will happly handle requests with DOMAINNAME ATYP, so when this behaviour is changed, we should decide if this is a good thing to do or not.

comment:5 Changed 6 years ago by nickm

IMO the right thing to do is to make bridge addresses be IP-only for now (in 0.2.5), and to later (0.2.6 or later) support accepting hostnames and passing them to the pluggable transports. I don't think resolving hostnames at configure time makes sense.

Anybody think that's a bad idea?

comment:6 Changed 6 years ago by kpdyer

That's ok with me. However, I would like this feature to appear in 0.2.6 and don't want it to drop off the radar.

comment:7 Changed 6 years ago by nickm

Keywords: 025-triaged added

comment:8 in reply to:  6 Changed 6 years ago by nickm

Replying to kpdyer:

That's ok with me. However, I would like this feature to appear in 0.2.6 and don't want it to drop off the radar.

I've added #11338 so we won't forget.

comment:9 Changed 6 years ago by nickm

(someone will still need to bell the cat, though)

comment:10 Changed 6 years ago by nickm

Status: newneeds_review

Simple fix in branch bug10801_024

comment:11 Changed 6 years ago by asn

Looks good to me.

Maybe you want to add the following trivial unit test too:

+  /* Make sure that the default port has lower priority than the real
+     one */
+  r= tor_addr_port_parse(LOG_DEBUG,
+                         "",
+                         &addr, &port, 200);
+  test_assert(r == 0);
+  tt_int_op(port,==,666);

It makes sure that the default port has lower priority than the port in the addr:port string.
I think that's what the code wants to do, but the behavior is not documented.

comment:12 Changed 6 years ago by nickm

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

Added a couple more fixes found by unit tests, and merged to 0.2.5. Marking for backport possibility.

comment:13 Changed 6 years ago by nickm

Recommendation: strongly consider backport if proves stable. Launching DNS requests based on bad bridge lines seems like a poor failure mode indeed.

comment:14 Changed 6 years ago by nickm

Keywords: nickm-backport-02422 added

Adding a tag for tickets I think we should backport into Omitting ones where I said "unsure"

comment:15 Changed 6 years ago by arma

I always thought this was a feature, not a bug. Your local network sees you using a bridge after all, and if you want to demand a certain fingerprint for it you still can, so what's one extra dns resolve give or take.

So I'd argue against backport, especially since apparently we're planning to put the feature back in in 0.2.6.

comment:16 Changed 6 years ago by nickm

The DNS resolve potentially travels much much farther than your local network.

(We're not planning to put it back in 0.2.6; we're planning for it to work differently in 0.2.6.)

comment:17 Changed 6 years ago by nickm

Current status: doesn't look like this gets an backport.

comment:18 Changed 6 years ago by nickm

Keywords: nickm-backport-02422 removed

comment:19 Changed 6 years ago by arma

Milestone: Tor: 0.2.4.x-finalTor: 0.2.5.x-final
Resolution: fixed
Status: needs_reviewclosed

I'm closing, since it's a behavior change in 0.2.4.x that might surprise some users during a minor version update. Onward and upward!

Note: See TracTickets for help on using tickets.