Allow DA address to be specified as FQDN #59
Conversation
Currently, src/or/config.c:parse_dir_authority_line() hard-codes the weak assumption that the address component of a DirAuthority entry is a numeric dotted-quad IP address, and generates an error on most FQDN hostnames. The assumption is weak because if the FQDN begins with a digit (e.g., "2byfour.example.net"), parsing succeeds, and the FQDN is successfully resolved later on by addr_port_lookup(). This patch explicitly permits any valid FQDN to be used for a DA's address by reserving the last two items in the smartlist for the "address:port" and "fingerprint" fields, respectively.
Instead of immediately resolving "address" to an IP before publishing it as part of the "dir-address" field of the new certificate, make tor-gencert publish it as-is and leave FQDN-to-IP resolution up to the recipient(s). This patch also modifies the corresponding parsing routine in src/or/routerparse.c:authority_cert_parse_from_string() to resolve the address field at run time from either dotted-quad or FQDN format.
DNS queries provide a vector for a malicious directory authority to de-anonymise clients. So we might only want to allow domain name resolution in test networks, or networks which are using non-default authorities.
When using default authorities, we should probably ban the use of DNS names.
Please update the tor man page to describe the new requirements for hostnames on DirAuthority lines. (Please don't call them FQDNs, unless that's what they must be.)
Please answer the following questions as part of that documentation:
Does the hostname need to be a FQDN, or can it be a single-component hostname or partial domain name?
(yes)
Can the hostname be an IPv6 address?
(no)
What does Tor do when given an IPv6 address as a hostname?
(please check and tell us how it fails on both authority and client)
Does the hostname need to resolve to an IPv4 address?
(yes)
Can it be dual-stack?
(probably),
What if it only resolves to an IPv6 address?
(Please check and tell us how it fails on both authority and client)
What if it does not resolve?
(Please check and tell us how it fails on both authority and client)
| @@ -6304,10 +6304,8 @@ parse_dir_authority_line(const char *line, dirinfo_type_t required_type, | |||
| smartlist_del_keeporder(items, 0); | |||
| } | |||
|
|
|||
| while (smartlist_len(items)) { | |||
| while (smartlist_len(items) > 2) { | |||
We can't reserve the last two items for the address and fingerprint, because the authority format has always supported space-separated fingerprints. A space separated fingerprint can occupy 1 to 40 items, but typically occupies 10.
Instead, please parse the flags, and then parse all remaining items as the address, followed by a space-separated fingerprint.
I got confused by the man page, which says:
DirAuthority [nickname] [flags] address:port fingerprint
We probably should add [fingerprint...] at the end to indicate there could be more than one...
I see now there's a "smartlist_join_strings()" call after processing the "addr:port" field, so you're right of course. Problem is, right now we decide it's time to process "addr:port" if the very first character in that string is a digit, which partially allows parsing hostname:port entries instead of throwing an error in all cases where "addr" is not an actual IP address.
We could decide we've finished parsing flags and reached "addr:port" if the current smartlist item contains a ":" character (unless it's possible for a flag to contain that character, either now or in the future, in which case we're back to not having a good way to know we've reached that field).
Alternatively, we could decide to ban non-IP "address" fields outright, because of the potential security vulnerability introduced by adding DNS to the mix (and I'll figure out a way to cope with that :) ). Right now we're only sort-of, kind-of doing that, which inspired me to try for full support, without thinking of the larger implications.
Please let me know what you think, and I'll either respond to your full review or submit a new patch fixing the man page and throwing an error if addr is not an IP (or backing off, if you're already working on the parsing code as part of some other effort).
Thanks much,
--G
We probably should add [fingerprint...] at the end to indicate there could be more than one...
[fingerprint…] means "zero or more fingerprints".
There must be exactly one fingerprint, but it can be space-separated. We could document that Tor allows spaces in the fingerprint in the man page.
We could decide we've finished parsing flags and reached "addr:port" if the current smartlist item contains a ":" character (unless it's possible for a flag to contain that character, either now or in the future, in which case we're back to not having a good way to know we've reached that field).
The ipv6 flag contains a colon. It looks like "ipv6=[2001:feed::1]:12345"
Let's check for a field with a colon, and without an equals sign.
Just stopping when we have exhausted all known flags makes it hard to add new flags.
Alternatively, we could decide to ban non-IP "address" fields outright, because of the potential security vulnerability introduced by adding DNS to the mix
As I said in the review and the trac ticket, I think we should ban them in the public network, but allow the, in test networks and when using non-default authorities.
| @@ -73,8 +73,7 @@ OPTIONS | |||
|
|
|||
| **-a** __address__:__port__:: | |||
| If provided, advertise the address:port combination as this authority's | |||
| preferred directory port in its certificate. If the address is a hostname, | |||
| the hostname is resolved to an IP before it's published. | |||
| preferred directory port in its certificate. | |||
Please explicitly mention that the hostname can be an IPv4 address or a hostname.
| char *address = NULL; | ||
| tor_assert(tok->n_args); | ||
| /* XXX++ use some tor_addr parse function below instead. -RD */ | ||
| if (tor_addr_port_split(LOG_WARN, tok->args[0], &address, | ||
| &cert->dir_port) < 0 || | ||
| tor_inet_aton(address, &in) == 0) { | ||
| tor_lookup_hostname(address, &cert->addr) != 0) { |
DNS queries provide a vector for a malicious directory authority to de-anonymise clients.
So we might only want to allow domain name resolution in test networks.
|
On Fri, Apr 27, 2018 at 12:42:40AM -0700, teor wrote:
@teor2345 requested changes on this pull request.
DNS queries provide a vector for a malicious directory authority to
de-anonymise clients. So we might only want to allow domain name resolution in
test networks, or networks which are using non-default authorities.
When using default authorities, we should probably ban the use of DNS names.
If we agree (and we do) that using DNS resolution in production would
introduce a potential vulnerability, then I'm becoming convinced it's
probably not a good idea to allow it in test mode either -- why should
we support a behavior in testing that's not actually going to be
allowed when doing it "for real" ? :)
As such, I'd like to withdraw my original patch/request/RFE :)
That being said, we now have a weird situation where it is possible to
resolve hostnames IFF they begin with a digit, even though they are
NOT an IP address (e.g. "2byfour.example.net"). I'm talking about
here:
diff --git a/src/or/config.c b/src/or/config.c
index 94a58f348..e43b58356 100644
--- a/src/or/config.c
+++ b/src/or/config.c
@@ -6414,6 +6414,7 @@ parse_dir_authority_line(const char *line, dirinfo_type_t required_type,
}
addrport = smartlist_get(items, 0);
smartlist_del_keeporder(items, 0);
+ /* XXX address here should be an IP, NOT a hostname starting with a digit! */
if (addr_port_lookup(LOG_WARN, addrport, &address, NULL, &dir_port)<0) {
log_warn(LD_CONFIG, "Error parsing DirAuthority address '%s'", addrport);
goto err;
I'm not familiar enough with the code base to have a good feel for
what should be done. Perhaps addr_port_lookup() should accept a Boolean
flag telling it whether it's OK to resolve hostnames or IP addresses
only, and when called from here we'd set that to False ?
Elsewhere in the code (e.g., src/or/routerparse.c, function
authority_cert_parse_from_string(), cca. line 2376) a combination of
tor_addr_port_split() and tor_inet_aton() is used to the same effect,
so there appear to be many possible ways to code this, and as I mentioned,
I'm not familiar enough with the code to tell which way is the *right* way...
Please feel free to close the trac ticket with something like
"notabug" as well :)
Thanks much,
--G
|
It would be very helpful, particularly in sandbox situations, to specify
the Directory Authority by FQDN hostname instead of by IP address. This
would allow us to defer picking an actual IP address until the simulation
is started, and even to use some "in-game" DNS facility to figure out
the actual address after the simulation is launched.
Right now, specifying a FQDN for the "DirAuthority" config file entry
even partially works already: if the FQDN happens to start with a
digit, it is correctly resolved internally using available DNS resolver
infrastructure :)
The first attached patch makes that work in all cases (even when the
FQDN hostname does not begin with a digit).
The second attached patch allows FQDNs to be inserted into DA certs
created using tor-gencert, and correspondingly resolved when a client
parses the downloaded DA certificate.
I realize there is ongoing work to refactor parsing the DA config entry
(TRAC ticket #17224), so please consider this patch set either independently
on its own merits or as part of that larger effort. In the first case,
I'd be happy to redo and resubmit the patches based on review/feedback.
Thanks,
--Gabriel
The text was updated successfully, but these errors were encountered: