From 40667ebe74cc5c42bb38e94af06d89ee46dbe839 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 16:20:49 +0100 Subject: [PATCH 01/41] resolve-host: propagate error properly --- src/resolve-host/resolve-host.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/resolve-host/resolve-host.c b/src/resolve-host/resolve-host.c index 3e4b52a3a9..3f3563850b 100644 --- a/src/resolve-host/resolve-host.c +++ b/src/resolve-host/resolve-host.c @@ -399,10 +399,8 @@ static int resolve_record(sd_bus *bus, const char *name) { return log_oom(); r = dns_packet_read_rr(p, &rr, NULL, NULL); - if (r < 0) { - log_error("Failed to parse RR."); - return r; - } + if (r < 0) + return log_error_errno(r, "Failed to parse RR."); r = dns_resource_record_to_string(rr, &s); if (r < 0) { From d3c7e9139c50bec5096925a09b9c1341942c72c4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 16:23:48 +0100 Subject: [PATCH 02/41] resolved: split out a new dns_type_may_redirect() call Let's abstract which RRs shall honour CNAMEs, and which ones should not. --- src/resolve/dns-type.c | 19 +++++++++++++++++++ src/resolve/dns-type.h | 1 + src/resolve/resolved-dns-cache.c | 6 +----- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/resolve/dns-type.c b/src/resolve/dns-type.c index cc52ef9abe..0571d65f0b 100644 --- a/src/resolve/dns-type.c +++ b/src/resolve/dns-type.c @@ -95,6 +95,25 @@ bool dns_class_is_valid_rr(uint16_t class) { return class != DNS_CLASS_ANY; } +bool dns_type_may_redirect(uint16_t type) { + /* The following record types should never be redirected using + * CNAME/DNAME RRs. See + * . */ + + if (dns_type_is_pseudo(type)) + return false; + + return !IN_SET(type, + DNS_TYPE_CNAME, + DNS_TYPE_DNAME, + DNS_TYPE_NSEC3, + DNS_TYPE_NSEC, + DNS_TYPE_RRSIG, + DNS_TYPE_NXT, + DNS_TYPE_SIG, + DNS_TYPE_KEY); +} + const char *dns_class_to_string(uint16_t class) { switch (class) { diff --git a/src/resolve/dns-type.h b/src/resolve/dns-type.h index bea0adaa16..c3bb26a5ee 100644 --- a/src/resolve/dns-type.h +++ b/src/resolve/dns-type.h @@ -128,6 +128,7 @@ enum { bool dns_type_is_pseudo(uint16_t type); bool dns_type_is_valid_query(uint16_t type); bool dns_type_is_valid_rr(uint16_t type); +bool dns_type_may_redirect(uint16_t type); bool dns_class_is_pseudo(uint16_t class); bool dns_class_is_valid_rr(uint16_t class); diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c index f50d780ebb..31154fbc77 100644 --- a/src/resolve/resolved-dns-cache.c +++ b/src/resolve/resolved-dns-cache.c @@ -672,11 +672,7 @@ static DnsCacheItem *dns_cache_get_by_key_follow_cname_dname_nsec(DnsCache *c, D if (i && i->type == DNS_CACHE_NXDOMAIN) return i; - /* The following record types should never be redirected. See - * . */ - if (!IN_SET(k->type, DNS_TYPE_CNAME, DNS_TYPE_DNAME, - DNS_TYPE_NSEC3, DNS_TYPE_NSEC, DNS_TYPE_RRSIG, - DNS_TYPE_NXT, DNS_TYPE_SIG, DNS_TYPE_KEY)) { + if (dns_type_may_redirect(k->type)) { /* Check if we have a CNAME record instead */ i = hashmap_get(c->by_key, &DNS_RESOURCE_KEY_CONST(k->class, DNS_TYPE_CNAME, n)); if (i) From a5444ca9fd88bf23cc95ac8d96803590698512ea Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 16:24:58 +0100 Subject: [PATCH 03/41] resolved: when caching NXDOMAIN for an RR, make sure we flush out old ANY entries We use ANY RR keys to store NXDOMAIN information, but we previously didn't flush out old ANY RR items in the cache when adding new entries. Fix that. --- src/resolve/resolved-dns-cache.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c index 31154fbc77..e8541d5e14 100644 --- a/src/resolve/resolved-dns-cache.c +++ b/src/resolve/resolved-dns-cache.c @@ -470,6 +470,14 @@ static int dns_cache_put_negative( i->key = dns_resource_key_new(key->class, DNS_TYPE_ANY, DNS_RESOURCE_KEY_NAME(key)); if (!i->key) return -ENOMEM; + + /* Make sure to remove any previous entry for this + * specific ANY key. (For non-ANY keys the cache data + * is already cleared by the caller.) Note that we + * don't bother removing positive or NODATA cache + * items in this case, because it would either be slow + * or require explicit indexing by name */ + dns_cache_remove_by_key(c, key); } else i->key = dns_resource_key_ref(key); @@ -607,7 +615,6 @@ int dns_cache_put( /* See https://tools.ietf.org/html/rfc2308, which say that a * matching SOA record in the packet is used to to enable * negative caching. */ - r = dns_answer_find_soa(answer, key, &soa, &flags); if (r < 0) goto fail; From 81f7fc5e841472b626698f386ed9445dac13944a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 16:27:13 +0100 Subject: [PATCH 04/41] resolved: when looking for a SOA RR in a reply, pick the right one If there are multiple SOA RRs, and we look for a suitable one covering our request, then make sure to pick the one that is furthest away from the root name, not just the first one we encounter. --- src/resolve/resolved-dns-answer.c | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c index 70577453e8..7e64ca52b7 100644 --- a/src/resolve/resolved-dns-answer.c +++ b/src/resolve/resolved-dns-answer.c @@ -303,8 +303,8 @@ int dns_answer_contains_nsec_or_nsec3(DnsAnswer *a) { } int dns_answer_find_soa(DnsAnswer *a, const DnsResourceKey *key, DnsResourceRecord **ret, DnsAnswerFlags *flags) { - DnsResourceRecord *rr; - DnsAnswerFlags rr_flags; + DnsResourceRecord *rr, *soa = NULL; + DnsAnswerFlags rr_flags, soa_flags = 0; int r; assert(key); @@ -318,15 +318,29 @@ int dns_answer_find_soa(DnsAnswer *a, const DnsResourceKey *key, DnsResourceReco if (r < 0) return r; if (r > 0) { - if (ret) - *ret = rr; - if (flags) - *flags = rr_flags; - return 1; + + if (soa) { + r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(rr->key), DNS_RESOURCE_KEY_NAME(soa->key)); + if (r < 0) + return r; + if (r > 0) + continue; + } + + soa = rr; + soa_flags = rr_flags; } } - return 0; + if (!soa) + return 0; + + if (ret) + *ret = soa; + if (flags) + *flags = soa_flags; + + return 1; } int dns_answer_find_cname_or_dname(DnsAnswer *a, const DnsResourceKey *key, DnsResourceRecord **ret, DnsAnswerFlags *flags) { From 65b200e70d01cb7c513114b602c9b5cf41348f77 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 16:28:35 +0100 Subject: [PATCH 05/41] resolved: when matching SOA RRs, honour RR class --- src/resolve/resolved-dns-rr.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c index 98a3a3351d..ac24169344 100644 --- a/src/resolve/resolved-dns-rr.c +++ b/src/resolve/resolved-dns-rr.c @@ -261,7 +261,7 @@ int dns_resource_key_match_soa(const DnsResourceKey *key, const DnsResourceKey * /* Checks whether 'soa' is a SOA record for the specified key. */ - if (soa->class != DNS_CLASS_IN) + if (soa->class != key->class) return 0; if (soa->type != DNS_TYPE_SOA) From 0936416a1cba187dc76903dae73246f4cbddf312 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 16:29:13 +0100 Subject: [PATCH 06/41] resolved: when matching SOA RRs, don't eat up errors --- src/resolve/resolved-dns-rr.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c index ac24169344..067fb1cd08 100644 --- a/src/resolve/resolved-dns-rr.c +++ b/src/resolve/resolved-dns-rr.c @@ -267,10 +267,7 @@ int dns_resource_key_match_soa(const DnsResourceKey *key, const DnsResourceKey * if (soa->type != DNS_TYPE_SOA) return 0; - if (!dns_name_endswith(DNS_RESOURCE_KEY_NAME(key), DNS_RESOURCE_KEY_NAME(soa))) - return 0; - - return 1; + return dns_name_endswith(DNS_RESOURCE_KEY_NAME(key), DNS_RESOURCE_KEY_NAME(soa)); } static void dns_resource_key_hash_func(const void *i, struct siphash *state) { From 7b50eb2efa122200e39646c19a29abab302f7d24 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 16:31:29 +0100 Subject: [PATCH 07/41] resolved: internalize string buffer of dns_resource_record_to_string() Let's simplify usage and memory management of DnsResourceRecord's dns_resource_record_to_string() call: cache the formatted string as part of the object, and return it on subsequent calls, freeing it when the DnsResourceRecord itself is freed. --- src/resolve-host/resolve-host.c | 8 +-- src/resolve/resolved-dns-answer.c | 8 +-- src/resolve/resolved-dns-cache.c | 11 ++-- src/resolve/resolved-dns-rr.c | 74 ++++++++++++++------------ src/resolve/resolved-dns-rr.h | 3 +- src/resolve/resolved-dns-transaction.c | 7 +-- src/resolve/resolved-dns-zone.c | 21 +++----- src/resolve/test-dnssec.c | 35 ++++-------- 8 files changed, 73 insertions(+), 94 deletions(-) diff --git a/src/resolve-host/resolve-host.c b/src/resolve-host/resolve-host.c index 3f3563850b..4752992cba 100644 --- a/src/resolve-host/resolve-host.c +++ b/src/resolve-host/resolve-host.c @@ -368,7 +368,7 @@ static int resolve_record(sd_bus *bus, const char *name) { while ((r = sd_bus_message_enter_container(reply, 'r', "iqqay")) > 0) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *rr = NULL; _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL; - _cleanup_free_ char *s = NULL; + const char *s; uint16_t c, t; int ifindex; const void *d; @@ -402,10 +402,10 @@ static int resolve_record(sd_bus *bus, const char *name) { if (r < 0) return log_error_errno(r, "Failed to parse RR."); - r = dns_resource_record_to_string(rr, &s); - if (r < 0) { + s = dns_resource_record_to_string(rr); + if (!s) { log_error("Failed to format RR."); - return r; + return -ENOMEM; } ifname[0] = 0; diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c index 7e64ca52b7..4dea4a2d45 100644 --- a/src/resolve/resolved-dns-answer.c +++ b/src/resolve/resolved-dns-answer.c @@ -657,18 +657,18 @@ int dns_answer_reserve_or_clone(DnsAnswer **a, unsigned n_free) { void dns_answer_dump(DnsAnswer *answer, FILE *f) { DnsResourceRecord *rr; DnsAnswerFlags flags; - int ifindex, r; + int ifindex; if (!f) f = stdout; DNS_ANSWER_FOREACH_FULL(rr, ifindex, flags, answer) { - _cleanup_free_ char *t = NULL; + const char *t; fputc('\t', f); - r = dns_resource_record_to_string(rr, &t); - if (r < 0) { + t = dns_resource_record_to_string(rr); + if (!t) { log_oom(); continue; } diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c index e8541d5e14..f9fe2c2e30 100644 --- a/src/resolve/resolved-dns-cache.c +++ b/src/resolve/resolved-dns-cache.c @@ -938,13 +938,13 @@ void dns_cache_dump(DnsCache *cache, FILE *f) { DnsCacheItem *j; LIST_FOREACH(by_key, j, i) { - _cleanup_free_ char *t = NULL; fputc('\t', f); if (j->rr) { - r = dns_resource_record_to_string(j->rr, &t); - if (r < 0) { + const char *t; + t = dns_resource_record_to_string(j->rr); + if (!t) { log_oom(); continue; } @@ -952,13 +952,14 @@ void dns_cache_dump(DnsCache *cache, FILE *f) { fputs(t, f); fputc('\n', f); } else { - r = dns_resource_key_to_string(j->key, &t); + _cleanup_free_ char *z = NULL; + r = dns_resource_key_to_string(j->key, &z); if (r < 0) { log_oom(); continue; } - fputs(t, f); + fputs(z, f); fputs(" -- ", f); fputs(j->type == DNS_CACHE_NODATA ? "NODATA" : "NXDOMAIN", f); fputc('\n', f); diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c index 067fb1cd08..04d442bf03 100644 --- a/src/resolve/resolved-dns-rr.c +++ b/src/resolve/resolved-dns-rr.c @@ -448,6 +448,7 @@ DnsResourceRecord* dns_resource_record_unref(DnsResourceRecord *rr) { dns_resource_key_unref(rr->key); } + free(rr->to_string); free(rr); return NULL; @@ -763,16 +764,19 @@ static char *format_txt(DnsTxtItem *first) { return s; } -int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { +const char *dns_resource_record_to_string(DnsResourceRecord *rr) { _cleanup_free_ char *k = NULL, *t = NULL; char *s; int r; assert(rr); + if (rr->to_string) + return rr->to_string; + r = dns_resource_key_to_string(rr->key, &k); if (r < 0) - return r; + return NULL; switch (rr->unparseable ? _DNS_TYPE_INVALID : rr->key->type) { @@ -784,7 +788,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { rr->srv.port, strna(rr->srv.name)); if (r < 0) - return -ENOMEM; + return NULL; break; case DNS_TYPE_PTR: @@ -793,25 +797,25 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { case DNS_TYPE_DNAME: s = strjoin(k, " ", rr->ptr.name, NULL); if (!s) - return -ENOMEM; + return NULL; break; case DNS_TYPE_HINFO: s = strjoin(k, " ", rr->hinfo.cpu, " ", rr->hinfo.os, NULL); if (!s) - return -ENOMEM; + return NULL; break; case DNS_TYPE_SPF: /* exactly the same as TXT */ case DNS_TYPE_TXT: t = format_txt(rr->txt.items); if (!t) - return -ENOMEM; + return NULL; s = strjoin(k, " ", t, NULL); if (!s) - return -ENOMEM; + return NULL; break; case DNS_TYPE_A: { @@ -819,22 +823,22 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { r = in_addr_to_string(AF_INET, (const union in_addr_union*) &rr->a.in_addr, &x); if (r < 0) - return r; + return NULL; s = strjoin(k, " ", x, NULL); if (!s) - return -ENOMEM; + return NULL; break; } case DNS_TYPE_AAAA: r = in_addr_to_string(AF_INET6, (const union in_addr_union*) &rr->aaaa.in6_addr, &t); if (r < 0) - return r; + return NULL; s = strjoin(k, " ", t, NULL); if (!s) - return -ENOMEM; + return NULL; break; case DNS_TYPE_SOA: @@ -848,7 +852,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { rr->soa.expire, rr->soa.minimum); if (r < 0) - return -ENOMEM; + return NULL; break; case DNS_TYPE_MX: @@ -857,7 +861,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { rr->mx.priority, rr->mx.exchange); if (r < 0) - return -ENOMEM; + return NULL; break; case DNS_TYPE_LOC: @@ -870,17 +874,17 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { rr->loc.horiz_pre, rr->loc.vert_pre); if (!t) - return -ENOMEM; + return NULL; s = strjoin(k, " ", t, NULL); if (!s) - return -ENOMEM; + return NULL; break; case DNS_TYPE_DS: t = hexmem(rr->ds.digest, rr->ds.digest_size); if (!t) - return -ENOMEM; + return NULL; r = asprintf(&s, "%s %u %u %u %s", k, @@ -889,13 +893,13 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { rr->ds.digest_type, t); if (r < 0) - return -ENOMEM; + return NULL; break; case DNS_TYPE_SSHFP: t = hexmem(rr->sshfp.fingerprint, rr->sshfp.fingerprint_size); if (!t) - return -ENOMEM; + return NULL; r = asprintf(&s, "%s %u %u %s", k, @@ -903,7 +907,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { rr->sshfp.fptype, t); if (r < 0) - return -ENOMEM; + return NULL; break; case DNS_TYPE_DNSKEY: { @@ -913,7 +917,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { t = base64mem(rr->dnskey.key, rr->dnskey.key_size); if (!t) - return -ENOMEM; + return NULL; r = asprintf(&s, "%s %u %u %.*s%.*u %s", k, @@ -923,7 +927,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { alg ? 0 : 1, alg ? 0u : (unsigned) rr->dnskey.algorithm, t); if (r < 0) - return -ENOMEM; + return NULL; break; } @@ -936,15 +940,15 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { t = base64mem(rr->rrsig.signature, rr->rrsig.signature_size); if (!t) - return -ENOMEM; + return NULL; r = format_timestamp_dns(expiration, sizeof(expiration), rr->rrsig.expiration); if (r < 0) - return r; + return NULL; r = format_timestamp_dns(inception, sizeof(inception), rr->rrsig.inception); if (r < 0) - return r; + return NULL; /* TYPE?? follows * http://tools.ietf.org/html/rfc3597#section-5 */ @@ -963,21 +967,21 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { rr->rrsig.signer, t); if (r < 0) - return -ENOMEM; + return NULL; break; } case DNS_TYPE_NSEC: t = format_types(rr->nsec.types); if (!t) - return -ENOMEM; + return NULL; r = asprintf(&s, "%s %s %s", k, rr->nsec.next_domain_name, t); if (r < 0) - return -ENOMEM; + return NULL; break; case DNS_TYPE_NSEC3: { @@ -986,16 +990,16 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { if (rr->nsec3.salt_size > 0) { salt = hexmem(rr->nsec3.salt, rr->nsec3.salt_size); if (!salt) - return -ENOMEM; + return NULL; } hash = base32hexmem(rr->nsec3.next_hashed_name, rr->nsec3.next_hashed_name_size, false); if (!hash) - return -ENOMEM; + return NULL; t = format_types(rr->nsec3.types); if (!t) - return -ENOMEM; + return NULL; r = asprintf(&s, "%s %"PRIu8" %"PRIu8" %"PRIu16" %s %s %s", k, @@ -1006,7 +1010,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { hash, t); if (r < 0) - return -ENOMEM; + return NULL; break; } @@ -1014,16 +1018,16 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) { default: t = hexmem(rr->generic.data, rr->generic.size); if (!t) - return -ENOMEM; + return NULL; r = asprintf(&s, "%s \\# %zu %s", k, rr->generic.size, t); if (r < 0) - return -ENOMEM; + return NULL; break; } - *ret = s; - return 0; + rr->to_string = s; + return s; } int dns_resource_record_to_wire_format(DnsResourceRecord *rr, bool canonical) { diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h index a35f01ce10..f2997883a8 100644 --- a/src/resolve/resolved-dns-rr.h +++ b/src/resolve/resolved-dns-rr.h @@ -95,6 +95,7 @@ struct DnsTxtItem { struct DnsResourceRecord { unsigned n_ref; DnsResourceKey *key; + char *to_string; uint32_t ttl; bool unparseable:1; bool wire_format_canonical:1; @@ -253,7 +254,7 @@ DnsResourceRecord* dns_resource_record_unref(DnsResourceRecord *rr); int dns_resource_record_new_reverse(DnsResourceRecord **ret, int family, const union in_addr_union *address, const char *name); int dns_resource_record_new_address(DnsResourceRecord **ret, int family, const union in_addr_union *address, const char *name); int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecord *b); -int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret); +const char* dns_resource_record_to_string(DnsResourceRecord *rr); DEFINE_TRIVIAL_CLEANUP_FUNC(DnsResourceRecord*, dns_resource_record_unref); int dns_resource_record_to_wire_format(DnsResourceRecord *rr, bool canonical); diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 347b1683f9..d2aec13264 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -1906,12 +1906,7 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { if (r < 0) return r; - if (log_get_max_level() >= LOG_DEBUG) { - _cleanup_free_ char *rrs = NULL; - - (void) dns_resource_record_to_string(rr, &rrs); - log_debug("Looking at %s: %s", rrs ? strstrip(rrs) : "???", dnssec_result_to_string(result)); - } + log_debug("Looking at %s: %s", strna(dns_resource_record_to_string(rr)), dnssec_result_to_string(result)); if (result == DNSSEC_VALIDATED) { diff --git a/src/resolve/resolved-dns-zone.c b/src/resolve/resolved-dns-zone.c index 20c8a4da90..f60b0bddc1 100644 --- a/src/resolve/resolved-dns-zone.c +++ b/src/resolve/resolved-dns-zone.c @@ -471,15 +471,12 @@ return_empty: } void dns_zone_item_conflict(DnsZoneItem *i) { - _cleanup_free_ char *pretty = NULL; - assert(i); if (!IN_SET(i->state, DNS_ZONE_ITEM_PROBING, DNS_ZONE_ITEM_VERIFYING, DNS_ZONE_ITEM_ESTABLISHED)) return; - dns_resource_record_to_string(i->rr, &pretty); - log_info("Detected conflict on %s", strna(pretty)); + log_info("Detected conflict on %s", strna(dns_resource_record_to_string(i->rr))); dns_zone_item_probe_stop(i); @@ -492,8 +489,6 @@ void dns_zone_item_conflict(DnsZoneItem *i) { } void dns_zone_item_notify(DnsZoneItem *i) { - _cleanup_free_ char *pretty = NULL; - assert(i); assert(i->probe_transaction); @@ -530,15 +525,13 @@ void dns_zone_item_notify(DnsZoneItem *i) { log_debug("Got a successful probe reply, but peer has lexicographically lower IP address and thus lost."); } - dns_resource_record_to_string(i->rr, &pretty); - log_debug("Record %s successfully probed.", strna(pretty)); + log_debug("Record %s successfully probed.", strna(dns_resource_record_to_string(i->rr))); dns_zone_item_probe_stop(i); i->state = DNS_ZONE_ITEM_ESTABLISHED; } static int dns_zone_item_verify(DnsZoneItem *i) { - _cleanup_free_ char *pretty = NULL; int r; assert(i); @@ -546,8 +539,7 @@ static int dns_zone_item_verify(DnsZoneItem *i) { if (i->state != DNS_ZONE_ITEM_ESTABLISHED) return 0; - dns_resource_record_to_string(i->rr, &pretty); - log_debug("Verifying RR %s", strna(pretty)); + log_debug("Verifying RR %s", strna(dns_resource_record_to_string(i->rr))); i->state = DNS_ZONE_ITEM_VERIFYING; r = dns_zone_item_probe_start(i); @@ -632,7 +624,6 @@ void dns_zone_verify_all(DnsZone *zone) { void dns_zone_dump(DnsZone *zone, FILE *f) { Iterator iterator; DnsZoneItem *i; - int r; if (!zone) return; @@ -644,10 +635,10 @@ void dns_zone_dump(DnsZone *zone, FILE *f) { DnsZoneItem *j; LIST_FOREACH(by_key, j, i) { - _cleanup_free_ char *t = NULL; + const char *t; - r = dns_resource_record_to_string(j->rr, &t); - if (r < 0) { + t = dns_resource_record_to_string(j->rr); + if (!t) { log_oom(); continue; } diff --git a/src/resolve/test-dnssec.c b/src/resolve/test-dnssec.c index 5b4ee220c4..6104d8b4c0 100644 --- a/src/resolve/test-dnssec.c +++ b/src/resolve/test-dnssec.c @@ -56,7 +56,6 @@ static void test_dnssec_verify_rrset2(void) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *nsec = NULL, *rrsig = NULL, *dnskey = NULL; _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL; - _cleanup_free_ char *x = NULL, *y = NULL, *z = NULL; DnssecResult result; nsec = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_NSEC, "nasa.gov"); @@ -77,8 +76,7 @@ static void test_dnssec_verify_rrset2(void) { assert_se(bitmap_set(nsec->nsec.types, DNS_TYPE_DNSKEY) >= 0); assert_se(bitmap_set(nsec->nsec.types, 65534) >= 0); - assert_se(dns_resource_record_to_string(nsec, &x) >= 0); - log_info("NSEC: %s", x); + log_info("NSEC: %s", strna(dns_resource_record_to_string(nsec))); rrsig = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_RRSIG, "NaSa.GOV."); assert_se(rrsig); @@ -96,8 +94,7 @@ static void test_dnssec_verify_rrset2(void) { rrsig->rrsig.signature = memdup(signature_blob, rrsig->rrsig.signature_size); assert_se(rrsig->rrsig.signature); - assert_se(dns_resource_record_to_string(rrsig, &y) >= 0); - log_info("RRSIG: %s", y); + log_info("RRSIG: %s", strna(dns_resource_record_to_string(rrsig))); dnskey = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_DNSKEY, "nASA.gOV"); assert_se(dnskey); @@ -109,8 +106,7 @@ static void test_dnssec_verify_rrset2(void) { dnskey->dnskey.key = memdup(dnskey_blob, sizeof(dnskey_blob)); assert_se(dnskey->dnskey.key); - assert_se(dns_resource_record_to_string(dnskey, &z) >= 0); - log_info("DNSKEY: %s", z); + log_info("DNSKEY: %s", strna(dns_resource_record_to_string(dnskey))); log_info("DNSKEY keytag: %u", dnssec_keytag(dnskey)); assert_se(dnssec_key_match_rrsig(nsec->key, rrsig) > 0); @@ -152,7 +148,6 @@ static void test_dnssec_verify_rrset(void) { _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *a = NULL, *rrsig = NULL, *dnskey = NULL; _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL; - _cleanup_free_ char *x = NULL, *y = NULL, *z = NULL; DnssecResult result; a = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_A, "nAsA.gov"); @@ -160,8 +155,7 @@ static void test_dnssec_verify_rrset(void) { a->a.in_addr.s_addr = inet_addr("52.0.14.116"); - assert_se(dns_resource_record_to_string(a, &x) >= 0); - log_info("A: %s", x); + log_info("A: %s", strna(dns_resource_record_to_string(a))); rrsig = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_RRSIG, "NaSa.GOV."); assert_se(rrsig); @@ -179,8 +173,7 @@ static void test_dnssec_verify_rrset(void) { rrsig->rrsig.signature = memdup(signature_blob, rrsig->rrsig.signature_size); assert_se(rrsig->rrsig.signature); - assert_se(dns_resource_record_to_string(rrsig, &y) >= 0); - log_info("RRSIG: %s", y); + log_info("RRSIG: %s", strna(dns_resource_record_to_string(rrsig))); dnskey = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_DNSKEY, "nASA.gOV"); assert_se(dnskey); @@ -192,8 +185,7 @@ static void test_dnssec_verify_rrset(void) { dnskey->dnskey.key = memdup(dnskey_blob, sizeof(dnskey_blob)); assert_se(dnskey->dnskey.key); - assert_se(dns_resource_record_to_string(dnskey, &z) >= 0); - log_info("DNSKEY: %s", z); + log_info("DNSKEY: %s", strna(dns_resource_record_to_string(dnskey))); log_info("DNSKEY keytag: %u", dnssec_keytag(dnskey)); assert_se(dnssec_key_match_rrsig(a->key, rrsig) > 0); @@ -239,7 +231,6 @@ static void test_dnssec_verify_dns_key(void) { }; _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *dnskey = NULL, *ds1 = NULL, *ds2 = NULL; - _cleanup_free_ char *a = NULL, *b = NULL, *c = NULL; /* The two DS RRs in effect for nasa.gov on 2015-12-01. */ ds1 = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_DS, "nasa.gov"); @@ -252,8 +243,7 @@ static void test_dnssec_verify_dns_key(void) { ds1->ds.digest = memdup(ds1_fprint, ds1->ds.digest_size); assert_se(ds1->ds.digest); - assert_se(dns_resource_record_to_string(ds1, &a) >= 0); - log_info("DS1: %s", a); + log_info("DS1: %s", strna(dns_resource_record_to_string(ds1))); ds2 = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_DS, "NASA.GOV"); assert_se(ds2); @@ -265,8 +255,7 @@ static void test_dnssec_verify_dns_key(void) { ds2->ds.digest = memdup(ds2_fprint, ds2->ds.digest_size); assert_se(ds2->ds.digest); - assert_se(dns_resource_record_to_string(ds2, &b) >= 0); - log_info("DS2: %s", b); + log_info("DS2: %s", strna(dns_resource_record_to_string(ds2))); dnskey = dns_resource_record_new_full(DNS_CLASS_IN, DNS_TYPE_DNSKEY, "nasa.GOV"); assert_se(dnskey); @@ -278,8 +267,7 @@ static void test_dnssec_verify_dns_key(void) { dnskey->dnskey.key = memdup(dnskey_blob, sizeof(dnskey_blob)); assert_se(dnskey->dnskey.key); - assert_se(dns_resource_record_to_string(dnskey, &c) >= 0); - log_info("DNSKEY: %s", c); + log_info("DNSKEY: %s", strna(dns_resource_record_to_string(dnskey))); log_info("DNSKEY keytag: %u", dnssec_keytag(dnskey)); assert_se(dnssec_verify_dnskey(dnskey, ds1) > 0); @@ -310,8 +298,8 @@ static void test_dnssec_nsec3_hash(void) { static const uint8_t salt[] = { 0xB0, 0x1D, 0xFA, 0xCE }; static const uint8_t next_hashed_name[] = { 0x84, 0x10, 0x26, 0x53, 0xc9, 0xfa, 0x4d, 0x85, 0x6c, 0x97, 0x82, 0xe2, 0x8f, 0xdf, 0x2d, 0x5e, 0x87, 0x69, 0xc4, 0x52 }; _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *rr = NULL; - _cleanup_free_ char *a = NULL, *b = NULL; uint8_t h[DNSSEC_HASH_SIZE_MAX]; + _cleanup_free_ char *b = NULL; int k; /* The NSEC3 RR for eurid.eu on 2015-12-14. */ @@ -328,8 +316,7 @@ static void test_dnssec_nsec3_hash(void) { assert_se(rr->nsec3.next_hashed_name); rr->nsec3.next_hashed_name_size = sizeof(next_hashed_name); - assert_se(dns_resource_record_to_string(rr, &a) >= 0); - log_info("NSEC3: %s", a); + log_info("NSEC3: %s", strna(dns_resource_record_to_string(rr))); k = dnssec_nsec3_hash(rr, "eurid.eu", &h); assert_se(k >= 0); From 0b8086379f0a062582c20471d6cd2646a6d22e93 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 19:53:15 +0100 Subject: [PATCH 08/41] util-lib: make sure more bitmap calls can deal with NULL objects fine --- src/basic/bitmap.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/basic/bitmap.c b/src/basic/bitmap.c index 95f59e400a..c8b2427b1c 100644 --- a/src/basic/bitmap.c +++ b/src/basic/bitmap.c @@ -140,7 +140,8 @@ bool bitmap_isset(Bitmap *b, unsigned n) { bool bitmap_isclear(Bitmap *b) { unsigned i; - assert(b); + if (!b) + return true; for (i = 0; i < b->n_bitmaps; i++) if (b->bitmaps[i] != 0) @@ -150,7 +151,9 @@ bool bitmap_isclear(Bitmap *b) { } void bitmap_clear(Bitmap *b) { - assert(b); + + if (!b) + return; b->bitmaps = mfree(b->bitmaps); b->n_bitmaps = 0; From 7d7fa31c62242e11494ace491a8c98fb070d4e8a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 19:53:41 +0100 Subject: [PATCH 09/41] bitmap: don't do bitwise XOR on booleans It's weird doing bitwise operations on booleans. Let's use the boolean XOR (i.e. "!=") instead of the bitweise XOR (i.e. "^") on them. --- src/basic/bitmap.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/basic/bitmap.c b/src/basic/bitmap.c index c8b2427b1c..50078822a7 100644 --- a/src/basic/bitmap.c +++ b/src/basic/bitmap.c @@ -200,7 +200,10 @@ bool bitmap_equal(Bitmap *a, Bitmap *b) { Bitmap *c; unsigned i; - if (!a ^ !b) + if (a == b) + return true; + + if (!a != !b) return false; if (!a) From 10b45040024da140fc2ea5761f905e745c11c982 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 19:54:54 +0100 Subject: [PATCH 10/41] resolved: don't choke on NSEC/NSEC3 RRs with no bitmap This might happen in some cases (empty non-terminals...) and we should not choke on it. --- src/resolve/resolved-dns-packet.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 14faf9e4ab..7908d0bcf5 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -643,7 +643,6 @@ static int dns_packet_append_types(DnsPacket *p, Bitmap *types, size_t *start) { int r; assert(p); - assert(types); saved_size = p->size; From d38d5ca65b3f8fd19348a7919cf1f1f07c955393 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 19:56:05 +0100 Subject: [PATCH 11/41] resolved: never use data from failed transactions Otherwise if we have an A lookup that failed DNSSEC validation, but an AAAA lookup that succeeded, we might end up using the A data, but we really should not. --- src/resolve/resolved-dns-query.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/resolve/resolved-dns-query.c b/src/resolve/resolved-dns-query.c index 18d2d01bf2..610b914e74 100644 --- a/src/resolve/resolved-dns-query.c +++ b/src/resolve/resolved-dns-query.c @@ -1039,8 +1039,7 @@ static void dns_query_accept(DnsQuery *q, DnsQueryCandidate *c) { if (state == DNS_TRANSACTION_SUCCESS) continue; - dns_answer_unref(q->answer); - q->answer = dns_answer_ref(t->answer); + q->answer = dns_answer_unref(q->answer); q->answer_rcode = t->answer_rcode; q->answer_dnssec_result = t->answer_dnssec_result; From e7ff0e0b391341bdc4d9c08dff1c477e1df6a682 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 19:57:34 +0100 Subject: [PATCH 12/41] resolved: properly implement RRSIG validation of wildcarded RRsets Note that this is still not complete, one additional step is still missing: when we verified that a wildcard RRset is properly signed, we still need to do an NSEC/NSEC3 proof that no more specific RRset exists. --- src/resolve/resolved-dns-dnssec.c | 23 ++++++++++++- src/shared/dns-domain.c | 56 +++++++++++++++++++++++++++++++ src/shared/dns-domain.h | 6 ++++ src/test/test-dns-domain.c | 42 +++++++++++++++++++++++ 4 files changed, 126 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index 814cb1c0f9..9ddad38fa6 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -384,10 +384,17 @@ int dnssec_verify_rrset( gcry_md_write(md, wire_format_name, r); for (k = 0; k < n; k++) { + const char *suffix; size_t l; rr = list[k]; - r = dns_name_to_wire_format(DNS_RESOURCE_KEY_NAME(rr->key), wire_format_name, sizeof(wire_format_name), true); + r = dns_name_suffix(DNS_RESOURCE_KEY_NAME(rr->key), rrsig->rrsig.labels, &suffix); + if (r < 0) + goto finish; + if (r > 0) /* This is a wildcard! */ + gcry_md_write(md, (uint8_t[]) { 1, '*'}, 2); + + r = dns_name_to_wire_format(suffix, wire_format_name, sizeof(wire_format_name), true); if (r < 0) goto finish; gcry_md_write(md, wire_format_name, r); @@ -497,6 +504,8 @@ int dnssec_rrsig_match_dnskey(DnsResourceRecord *rrsig, DnsResourceRecord *dnske } int dnssec_key_match_rrsig(const DnsResourceKey *key, DnsResourceRecord *rrsig) { + int r; + assert(key); assert(rrsig); @@ -509,6 +518,18 @@ int dnssec_key_match_rrsig(const DnsResourceKey *key, DnsResourceRecord *rrsig) if (rrsig->rrsig.type_covered != key->type) return 0; + /* Make sure signer is a parent of the RRset */ + r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(rrsig->key), rrsig->rrsig.signer); + if (r <= 0) + return r; + + /* Make sure the owner name has at least as many labels as the "label" fields indicates. */ + r = dns_name_count_labels(DNS_RESOURCE_KEY_NAME(rrsig->key)); + if (r < 0) + return r; + if (r < rrsig->rrsig.labels) + return 0; + return dns_name_equal(DNS_RESOURCE_KEY_NAME(rrsig->key), DNS_RESOURCE_KEY_NAME(key)); } diff --git a/src/shared/dns-domain.c b/src/shared/dns-domain.c index c46f7d21b7..f3dbf60395 100644 --- a/src/shared/dns-domain.c +++ b/src/shared/dns-domain.c @@ -1159,3 +1159,59 @@ finish: return 0; } + +int dns_name_suffix(const char *name, unsigned n_labels, const char **ret) { + const char* labels[DNS_N_LABELS_MAX+1]; + unsigned n = 0; + const char *p; + int r; + + assert(name); + assert(ret); + + p = name; + for (;;) { + if (n > DNS_N_LABELS_MAX) + return -EINVAL; + + labels[n] = p; + + r = dns_name_parent(&p); + if (r < 0) + return r; + if (r == 0) + break; + + n++; + } + + if (n < n_labels) + return -EINVAL; + + *ret = labels[n - n_labels]; + return (int) (n - n_labels); +} + +int dns_name_count_labels(const char *name) { + unsigned n = 0; + const char *p; + int r; + + assert(name); + + p = name; + for (;;) { + r = dns_name_parent(&p); + if (r < 0) + return r; + if (r == 0) + break; + + if (n >= DNS_N_LABELS_MAX) + return -EINVAL; + + n++; + } + + return (int) n; +} diff --git a/src/shared/dns-domain.h b/src/shared/dns-domain.h index 02b51832b6..7b509729fb 100644 --- a/src/shared/dns-domain.h +++ b/src/shared/dns-domain.h @@ -42,6 +42,9 @@ /* Maximum length of a full hostname, on the wire, including the final NUL byte */ #define DNS_WIRE_FOMAT_HOSTNAME_MAX 255 +/* Maximum number of labels per valid hostname */ +#define DNS_N_LABELS_MAX 127 + int dns_label_unescape(const char **name, char *dest, size_t sz); int dns_label_unescape_suffix(const char *name, const char **label_end, char *dest, size_t sz); int dns_label_escape(const char *p, size_t l, char *dest, size_t sz); @@ -96,3 +99,6 @@ bool dns_service_name_is_valid(const char *name); int dns_service_join(const char *name, const char *type, const char *domain, char **ret); int dns_service_split(const char *joined, char **name, char **type, char **domain); + +int dns_name_suffix(const char *name, unsigned n_labels, const char **ret); +int dns_name_count_labels(const char *name); diff --git a/src/test/test-dns-domain.c b/src/test/test-dns-domain.c index de003e251c..3a15ea5c06 100644 --- a/src/test/test-dns-domain.c +++ b/src/test/test-dns-domain.c @@ -475,6 +475,46 @@ static void test_dns_name_change_suffix(void) { test_dns_name_change_suffix_one("a", "b", "c", 0, NULL); } +static void test_dns_name_suffix_one(const char *name, unsigned n_labels, const char *result, int ret) { + const char *p = NULL; + + assert_se(ret == dns_name_suffix(name, n_labels, &p)); + assert_se(streq_ptr(p, result)); +} + +static void test_dns_name_suffix(void) { + test_dns_name_suffix_one("foo.bar", 2, "foo.bar", 0); + test_dns_name_suffix_one("foo.bar", 1, "bar", 1); + test_dns_name_suffix_one("foo.bar", 0, "", 2); + test_dns_name_suffix_one("foo.bar", 3, NULL, -EINVAL); + test_dns_name_suffix_one("foo.bar", 4, NULL, -EINVAL); + + test_dns_name_suffix_one("bar", 1, "bar", 0); + test_dns_name_suffix_one("bar", 0, "", 1); + test_dns_name_suffix_one("bar", 2, NULL, -EINVAL); + test_dns_name_suffix_one("bar", 3, NULL, -EINVAL); + + test_dns_name_suffix_one("", 0, "", 0); + test_dns_name_suffix_one("", 1, NULL, -EINVAL); + test_dns_name_suffix_one("", 2, NULL, -EINVAL); +} + +static void test_dns_name_count_labels_one(const char *name, int n) { + assert_se(dns_name_count_labels(name) == n); +} + +static void test_dns_name_count_labels(void) { + test_dns_name_count_labels_one("foo.bar.quux.", 3); + test_dns_name_count_labels_one("foo.bar.quux", 3); + test_dns_name_count_labels_one("foo.bar.", 2); + test_dns_name_count_labels_one("foo.bar", 2); + test_dns_name_count_labels_one("foo.", 1); + test_dns_name_count_labels_one("foo", 1); + test_dns_name_count_labels_one("", 0); + test_dns_name_count_labels_one(".", 0); + test_dns_name_count_labels_one("..", -EINVAL); +} + int main(int argc, char *argv[]) { test_dns_label_unescape(); @@ -495,6 +535,8 @@ int main(int argc, char *argv[]) { test_dns_service_join(); test_dns_service_split(); test_dns_name_change_suffix(); + test_dns_name_suffix(); + test_dns_name_count_labels(); return 0; } From 13b78323bad1e41e0474b833da2a0b72aab56f09 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 20:00:34 +0100 Subject: [PATCH 13/41] resolved: when doing NSEC3 proof, first find right NSEC3 suffix When doing an NSEC3 proof, before detrmining whether a name is the closest encloser we first need to figure out the longest common suffix we have with any NSEC3 RR in the reply. --- src/resolve/resolved-dns-dnssec.c | 47 +++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index 9ddad38fa6..dbda71b03d 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -912,7 +912,7 @@ finish: static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *result) { _cleanup_free_ char *next_closer_domain = NULL, *l = NULL; uint8_t hashed[DNSSEC_HASH_SIZE_MAX]; - const char *p, *pp = NULL; + const char *suffix, *p, *pp = NULL; DnsResourceRecord *rr; DnsAnswerFlags flags; int hashed_size, r; @@ -920,7 +920,42 @@ static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecR assert(key); assert(result); - /* First step, look for the closest encloser NSEC3 RR in 'answer' that matches 'key' */ + /* First step, look for the longest common suffix we find with any NSEC3 RR in the response. */ + suffix = DNS_RESOURCE_KEY_NAME(key); + for (;;) { + DNS_ANSWER_FOREACH_FLAGS(rr, flags, answer) { + _cleanup_free_ char *hashed_domain = NULL, *label = NULL; + + if ((flags & DNS_ANSWER_AUTHENTICATED) == 0) + continue; + + if (rr->key->type != DNS_TYPE_NSEC3) + continue; + + /* RFC 5155, Section 8.2 says we MUST ignore NSEC3 RRs with flags != 0 or 1 */ + if (!IN_SET(rr->nsec3.flags, 0, 1)) + continue; + + r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(rr->key), suffix); + if (r < 0) + return r; + if (r > 0) + goto found_suffix; + } + + /* Strip one label from the front */ + r = dns_name_parent(&suffix); + if (r < 0) + return r; + if (r == 0) + break; + } + + *result = DNSSEC_NSEC_NO_RR; + return 0; + +found_suffix: + /* Second step, find the closest encloser NSEC3 RR in 'answer' that matches 'key' */ p = DNS_RESOURCE_KEY_NAME(key); for (;;) { DNS_ANSWER_FOREACH_FLAGS(rr, flags, answer) { @@ -936,7 +971,7 @@ static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecR if (!IN_SET(rr->nsec3.flags, 0, 1)) continue; - r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(rr->key), p); + r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(rr->key), suffix); if (r < 0) return r; if (r == 0) @@ -956,7 +991,7 @@ static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecR if (!label) return -ENOMEM; - hashed_domain = strjoin(label, ".", p, NULL); + hashed_domain = strjoin(label, ".", suffix, NULL); if (!hashed_domain) return -ENOMEM; @@ -964,7 +999,7 @@ static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecR if (r < 0) return r; if (r > 0) - goto found; + goto found_closest_encloser; } /* We didn't find the closest encloser with this name, @@ -984,7 +1019,7 @@ static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecR *result = DNSSEC_NSEC_NO_RR; return 0; -found: +found_closest_encloser: /* We found a closest encloser in 'p'; next closer is 'pp' */ /* Ensure this is not a DNAME domain, see RFC5155, section 8.3. */ From db5b0e92b3c23e6f360bd0f44a655b35921a6c98 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 21:06:29 +0100 Subject: [PATCH 14/41] resolved: tighten search for NSEC3 RRs a bit Be stricter when searching suitable NSEC3 RRs for proof: generalize the check we use to find suitable NSEC3 RRs, in nsec3_is_good(), and add additional checks, such as checking whether all NSEC3 RRs use the same parameters, have the same suffix and so on. --- src/resolve/resolved-dns-dnssec.c | 138 ++++++++++++++----------- src/resolve/resolved-dns-transaction.c | 2 +- src/shared/dns-domain.c | 18 ++++ src/shared/dns-domain.h | 2 + src/test/test-dns-domain.c | 32 ++++++ 5 files changed, 133 insertions(+), 59 deletions(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index dbda71b03d..edd1efc739 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -909,11 +909,63 @@ finish: return r; } +static int nsec3_is_good(DnsResourceRecord *rr, DnsAnswerFlags flags, DnsResourceRecord *nsec3) { + const char *a, *b; + int r; + + assert(rr); + + if ((flags & DNS_ANSWER_AUTHENTICATED) == 0) + return 0; + + if (rr->key->type != DNS_TYPE_NSEC3) + return 0; + + /* RFC 5155, Section 8.2 says we MUST ignore NSEC3 RRs with flags != 0 or 1 */ + if (!IN_SET(rr->nsec3.flags, 0, 1)) + return 0; + + if (!nsec3) + return 1; + + /* If a second NSEC3 RR is specified, also check if they are from the same zone. */ + + if (nsec3 == rr) /* Shortcut */ + return 1; + + if (rr->key->class != nsec3->key->class) + return 0; + if (rr->nsec3.algorithm != nsec3->nsec3.algorithm) + return 0; + if (rr->nsec3.iterations != nsec3->nsec3.iterations) + return 0; + if (rr->nsec3.salt_size != nsec3->nsec3.salt_size) + return 0; + if (memcmp(rr->nsec3.salt, nsec3->nsec3.salt, rr->nsec3.salt_size) != 0) + return 0; + + a = DNS_RESOURCE_KEY_NAME(rr->key); + r = dns_name_parent(&a); /* strip off hash */ + if (r < 0) + return r; + if (r == 0) + return 0; + + b = DNS_RESOURCE_KEY_NAME(nsec3->key); + r = dns_name_parent(&b); /* strip off hash */ + if (r < 0) + return r; + if (r == 0) + return 0; + + return dns_name_equal(a, b); +} + static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *result) { _cleanup_free_ char *next_closer_domain = NULL, *l = NULL; uint8_t hashed[DNSSEC_HASH_SIZE_MAX]; const char *suffix, *p, *pp = NULL; - DnsResourceRecord *rr; + DnsResourceRecord *rr, *suffix_rr; DnsAnswerFlags flags; int hashed_size, r; @@ -923,20 +975,16 @@ static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecR /* First step, look for the longest common suffix we find with any NSEC3 RR in the response. */ suffix = DNS_RESOURCE_KEY_NAME(key); for (;;) { - DNS_ANSWER_FOREACH_FLAGS(rr, flags, answer) { + DNS_ANSWER_FOREACH_FLAGS(suffix_rr, flags, answer) { _cleanup_free_ char *hashed_domain = NULL, *label = NULL; - if ((flags & DNS_ANSWER_AUTHENTICATED) == 0) + r = nsec3_is_good(suffix_rr, flags, NULL); + if (r < 0) + return r; + if (r == 0) continue; - if (rr->key->type != DNS_TYPE_NSEC3) - continue; - - /* RFC 5155, Section 8.2 says we MUST ignore NSEC3 RRs with flags != 0 or 1 */ - if (!IN_SET(rr->nsec3.flags, 0, 1)) - continue; - - r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(rr->key), suffix); + r = dns_name_equal_skip(DNS_RESOURCE_KEY_NAME(suffix_rr->key), 1, suffix); if (r < 0) return r; if (r > 0) @@ -958,42 +1006,34 @@ found_suffix: /* Second step, find the closest encloser NSEC3 RR in 'answer' that matches 'key' */ p = DNS_RESOURCE_KEY_NAME(key); for (;;) { + _cleanup_free_ char *hashed_domain = NULL, *label = NULL; + + hashed_size = dnssec_nsec3_hash(suffix_rr, p, hashed); + if (hashed_size == -EOPNOTSUPP) { + *result = DNSSEC_NSEC_UNSUPPORTED_ALGORITHM; + return 0; + } + if (hashed_size < 0) + return hashed_size; + + label = base32hexmem(hashed, hashed_size, false); + if (!label) + return -ENOMEM; + + hashed_domain = strjoin(label, ".", suffix, NULL); + if (!hashed_domain) + return -ENOMEM; + DNS_ANSWER_FOREACH_FLAGS(rr, flags, answer) { - _cleanup_free_ char *hashed_domain = NULL, *label = NULL; - if ((flags & DNS_ANSWER_AUTHENTICATED) == 0) - continue; - - if (rr->key->type != DNS_TYPE_NSEC3) - continue; - - /* RFC 5155, Section 8.2 says we MUST ignore NSEC3 RRs with flags != 0 or 1 */ - if (!IN_SET(rr->nsec3.flags, 0, 1)) - continue; - - r = dns_name_endswith(DNS_RESOURCE_KEY_NAME(rr->key), suffix); + r = nsec3_is_good(rr, flags, suffix_rr); if (r < 0) return r; if (r == 0) continue; - hashed_size = dnssec_nsec3_hash(rr, p, hashed); - if (hashed_size == -EOPNOTSUPP) { - *result = DNSSEC_NSEC_UNSUPPORTED_ALGORITHM; - return 0; - } - if (hashed_size < 0) - return hashed_size; if (rr->nsec3.next_hashed_name_size != (size_t) hashed_size) - return -EBADMSG; - - label = base32hexmem(hashed, hashed_size, false); - if (!label) - return -ENOMEM; - - hashed_domain = strjoin(label, ".", suffix, NULL); - if (!hashed_domain) - return -ENOMEM; + continue; r = dns_name_equal(DNS_RESOURCE_KEY_NAME(rr->key), hashed_domain); if (r < 0) @@ -1056,26 +1096,8 @@ found_closest_encloser: DNS_ANSWER_FOREACH_FLAGS(rr, flags, answer) { _cleanup_free_ char *label = NULL, *next_hashed_domain = NULL; - const char *nsec3_parent; - if ((flags & DNS_ANSWER_AUTHENTICATED) == 0) - continue; - - if (rr->key->type != DNS_TYPE_NSEC3) - continue; - - /* RFC 5155, Section 8.2 says we MUST ignore NSEC3 RRs with flags != 0 or 1 */ - if (!IN_SET(rr->nsec3.flags, 0, 1)) - continue; - - nsec3_parent = DNS_RESOURCE_KEY_NAME(rr->key); - r = dns_name_parent(&nsec3_parent); - if (r < 0) - return r; - if (r == 0) - continue; - - r = dns_name_equal(p, nsec3_parent); + r = nsec3_is_good(rr, flags, suffix_rr); if (r < 0) return r; if (r == 0) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index d2aec13264..1d02d7e942 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -1516,7 +1516,7 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) { if (!soa) return -ENOMEM; - log_debug("Requesting SOA to validate transaction %" PRIu16 " (%s, unsigned non-SOA/NS RRset).", t->id, DNS_RESOURCE_KEY_NAME(rr->key)); + log_debug("Requesting SOA to validate transaction %" PRIu16 " (%s, unsigned non-SOA/NS RRset <%s>).", t->id, DNS_RESOURCE_KEY_NAME(rr->key), dns_resource_record_to_string(rr)); r = dns_transaction_request_dnssec_rr(t, soa); if (r < 0) return r; diff --git a/src/shared/dns-domain.c b/src/shared/dns-domain.c index f3dbf60395..f44e80c9db 100644 --- a/src/shared/dns-domain.c +++ b/src/shared/dns-domain.c @@ -1215,3 +1215,21 @@ int dns_name_count_labels(const char *name) { return (int) n; } + +int dns_name_equal_skip(const char *a, unsigned n_labels, const char *b) { + int r; + + assert(a); + assert(b); + + while (n_labels > 0) { + + r = dns_name_parent(&a); + if (r <= 0) + return r; + + n_labels --; + } + + return dns_name_equal(a, b); +} diff --git a/src/shared/dns-domain.h b/src/shared/dns-domain.h index 7b509729fb..dd8ae3ac98 100644 --- a/src/shared/dns-domain.h +++ b/src/shared/dns-domain.h @@ -102,3 +102,5 @@ int dns_service_split(const char *joined, char **name, char **type, char **domai int dns_name_suffix(const char *name, unsigned n_labels, const char **ret); int dns_name_count_labels(const char *name); + +int dns_name_equal_skip(const char *a, unsigned n_labels, const char *b); diff --git a/src/test/test-dns-domain.c b/src/test/test-dns-domain.c index 3a15ea5c06..78c5631ff1 100644 --- a/src/test/test-dns-domain.c +++ b/src/test/test-dns-domain.c @@ -515,6 +515,37 @@ static void test_dns_name_count_labels(void) { test_dns_name_count_labels_one("..", -EINVAL); } +static void test_dns_name_equal_skip_one(const char *a, unsigned n_labels, const char *b, int ret) { + assert_se(dns_name_equal_skip(a, n_labels, b) == ret); +} + +static void test_dns_name_equal_skip(void) { + test_dns_name_equal_skip_one("foo", 0, "bar", 0); + test_dns_name_equal_skip_one("foo", 0, "foo", 1); + test_dns_name_equal_skip_one("foo", 1, "foo", 0); + test_dns_name_equal_skip_one("foo", 2, "foo", 0); + + test_dns_name_equal_skip_one("foo.bar", 0, "foo.bar", 1); + test_dns_name_equal_skip_one("foo.bar", 1, "foo.bar", 0); + test_dns_name_equal_skip_one("foo.bar", 2, "foo.bar", 0); + test_dns_name_equal_skip_one("foo.bar", 3, "foo.bar", 0); + + test_dns_name_equal_skip_one("foo.bar", 0, "bar", 0); + test_dns_name_equal_skip_one("foo.bar", 1, "bar", 1); + test_dns_name_equal_skip_one("foo.bar", 2, "bar", 0); + test_dns_name_equal_skip_one("foo.bar", 3, "bar", 0); + + test_dns_name_equal_skip_one("foo.bar", 0, "", 0); + test_dns_name_equal_skip_one("foo.bar", 1, "", 0); + test_dns_name_equal_skip_one("foo.bar", 2, "", 1); + test_dns_name_equal_skip_one("foo.bar", 3, "", 0); + + test_dns_name_equal_skip_one("", 0, "", 1); + test_dns_name_equal_skip_one("", 1, "", 0); + test_dns_name_equal_skip_one("", 1, "foo", 0); + test_dns_name_equal_skip_one("", 2, "foo", 0); +} + int main(int argc, char *argv[]) { test_dns_label_unescape(); @@ -537,6 +568,7 @@ int main(int argc, char *argv[]) { test_dns_name_change_suffix(); test_dns_name_suffix(); test_dns_name_count_labels(); + test_dns_name_equal_skip(); return 0; } From 3ecc3df8ffa2d40c814fd6c1c4d554994d60b7af Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Mon, 21 Dec 2015 22:07:41 +0100 Subject: [PATCH 15/41] update DNSSEC TODO --- src/resolve/resolved-dns-dnssec.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index edd1efc739..482ee4a0b3 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -36,7 +36,7 @@ * TODO: * * - Make trust anchor store read additional DS+DNSKEY data from disk - * - wildcard zones compatibility + * - wildcard zones compatibility (NSEC/NSEC3 wildcard check is missing) * - multi-label zone compatibility * - cname/dname compatibility * - per-interface DNSSEC setting From 6b2f709364b3bb4277de3d6fa2e5b45ba3c12424 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Dec 2015 18:19:48 +0100 Subject: [PATCH 16/41] resolved: make use of dns_type_may_redirect() where possible --- src/resolve/resolved-dns-answer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c index 4dea4a2d45..399b518644 100644 --- a/src/resolve/resolved-dns-answer.c +++ b/src/resolve/resolved-dns-answer.c @@ -351,7 +351,7 @@ int dns_answer_find_cname_or_dname(DnsAnswer *a, const DnsResourceKey *key, DnsR assert(key); /* For a {C,D}NAME record we can never find a matching {C,D}NAME record */ - if (key->type == DNS_TYPE_CNAME || key->type == DNS_TYPE_DNAME) + if (!dns_type_may_redirect(key->type)) return 0; DNS_ANSWER_FOREACH_FLAGS(rr, rr_flags, a) { From d1c4ee32480cb997b673ca8396ca95c70be610f7 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Dec 2015 18:20:09 +0100 Subject: [PATCH 17/41] resolved: be stricter when searching for a DS RR for a DNSKEY RR --- src/resolve/resolved-dns-dnssec.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index 482ee4a0b3..f37f1d91be 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -831,6 +831,15 @@ int dnssec_verify_dnskey_search(DnsResourceRecord *dnskey, DnsAnswer *validated_ if (ds->key->type != DNS_TYPE_DS) continue; + if (ds->key->class != dnskey->key->class) + continue; + + r = dns_name_equal(DNS_RESOURCE_KEY_NAME(dnskey->key), DNS_RESOURCE_KEY_NAME(ds->key)); + if (r < 0) + return r; + if (r == 0) + continue; + r = dnssec_verify_dnskey(dnskey, ds); if (r < 0) return r; From 94aa70712929f2eafb654a07d29808156522543c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Dec 2015 18:21:25 +0100 Subject: [PATCH 18/41] resolved: don't insist on finding DNSKEYs for RRsets of zones with DNSSEC off --- src/resolve/resolved-dns-transaction.c | 87 ++++++++++++++++++++++++++ 1 file changed, 87 insertions(+) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 1d02d7e942..9ac9de01ad 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -1855,6 +1855,71 @@ static int dns_transaction_requires_nsec(DnsTransaction *t) { return true; } +static int dns_transaction_dnskey_authenticated(DnsTransaction *t, DnsResourceRecord *rr) { + DnsResourceRecord *rrsig; + bool found = false; + int r; + + /* Checks whether any of the DNSKEYs used for the RRSIGs for + * the specified RRset is authenticated (i.e. has a matching + * DS RR). */ + + DNS_ANSWER_FOREACH(rrsig, t->answer) { + DnsTransaction *dt; + Iterator i; + + r = dnssec_key_match_rrsig(rr->key, rrsig); + if (r < 0) + return r; + if (r == 0) + continue; + + SET_FOREACH(dt, t->dnssec_transactions, i) { + + if (dt->key->class != rr->key->class) + continue; + + if (dt->key->type == DNS_TYPE_DNSKEY) { + + r = dns_name_equal(DNS_RESOURCE_KEY_NAME(dt->key), rrsig->rrsig.signer); + if (r < 0) + return r; + if (r == 0) + continue; + + /* OK, we found an auxiliary DNSKEY + * lookup. If that lookup is + * authenticated, report this. */ + + if (dt->answer_authenticated) + return true; + + found = true; + + } else if (dt->key->type == DNS_TYPE_DS) { + + r = dns_name_equal(DNS_RESOURCE_KEY_NAME(dt->key), rrsig->rrsig.signer); + if (r < 0) + return r; + if (r == 0) + continue; + + /* OK, we found an auxiliary DS + * lookup. If that lookup is + * authenticated and non-zero, we + * won! */ + + if (!dt->answer_authenticated) + return false; + + return dns_answer_match_key(dt->answer, dt->key, NULL); + } + } + } + + return found ? false : -ENXIO; +} + int dns_transaction_validate_dnssec(DnsTransaction *t) { _cleanup_(dns_answer_unrefp) DnsAnswer *validated = NULL; bool dnskeys_finalized = false; @@ -1936,6 +2001,7 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { break; } else if (dnskeys_finalized) { + /* If we haven't read all DNSKEYs yet * a negative result of the validation * is irrelevant, as there might be @@ -1957,6 +2023,27 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { } } + if (IN_SET(result, + DNSSEC_MISSING_KEY, + DNSSEC_SIGNATURE_EXPIRED, + DNSSEC_UNSUPPORTED_ALGORITHM)) { + + r = dns_transaction_dnskey_authenticated(t, rr); + if (r < 0 && r != -ENXIO) + return r; + if (r == 0) { + /* The DNSKEY transaction was not authenticated, this means there's + * no DS for this, which means it's OK if no keys are found for this signature. */ + + r = dns_answer_move_by_key(&validated, &t->answer, rr->key, 0); + if (r < 0) + return r; + + changed = true; + break; + } + } + r = dns_transaction_is_primary_response(t, rr); if (r < 0) return r; From ed29bfdce6ef8b1c6e14bb4e77e19e7048f35dd4 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Tue, 22 Dec 2015 18:22:19 +0100 Subject: [PATCH 19/41] resolved: if we accepted unauthenticated NSEC/NSEC3 RRs, use them for proofs But keep track that the proof is not authenticated. --- src/resolve/resolved-dns-dnssec.c | 23 +++++++++++++---------- src/resolve/resolved-dns-dnssec.h | 2 +- src/resolve/resolved-dns-transaction.c | 7 ++++--- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index f37f1d91be..14fa58c544 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -924,9 +924,6 @@ static int nsec3_is_good(DnsResourceRecord *rr, DnsAnswerFlags flags, DnsResourc assert(rr); - if ((flags & DNS_ANSWER_AUTHENTICATED) == 0) - return 0; - if (rr->key->type != DNS_TYPE_NSEC3) return 0; @@ -970,16 +967,18 @@ static int nsec3_is_good(DnsResourceRecord *rr, DnsAnswerFlags flags, DnsResourc return dns_name_equal(a, b); } -static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *result) { +static int dnssec_test_nsec3(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *result, bool *authenticated) { _cleanup_free_ char *next_closer_domain = NULL, *l = NULL; uint8_t hashed[DNSSEC_HASH_SIZE_MAX]; const char *suffix, *p, *pp = NULL; DnsResourceRecord *rr, *suffix_rr; DnsAnswerFlags flags; int hashed_size, r; + bool a; assert(key); assert(result); + assert(authenticated); /* First step, look for the longest common suffix we find with any NSEC3 RR in the response. */ suffix = DNS_RESOURCE_KEY_NAME(key); @@ -1047,8 +1046,10 @@ found_suffix: r = dns_name_equal(DNS_RESOURCE_KEY_NAME(rr->key), hashed_domain); if (r < 0) return r; - if (r > 0) + if (r > 0) { + a = flags & DNS_ANSWER_AUTHENTICATED; goto found_closest_encloser; + } } /* We didn't find the closest encloser with this name, @@ -1086,6 +1087,7 @@ found_closest_encloser: if (!pp) { /* No next closer NSEC3 RR. That means there's a direct NSEC3 RR for our key. */ *result = bitmap_isset(rr->nsec3.types, key->type) ? DNSSEC_NSEC_FOUND : DNSSEC_NSEC_NODATA; + *authenticated = a; return 0; } @@ -1129,6 +1131,7 @@ found_closest_encloser: else *result = DNSSEC_NSEC_NXDOMAIN; + *authenticated = a && (flags & DNS_ANSWER_AUTHENTICATED); return 1; } } @@ -1137,7 +1140,7 @@ found_closest_encloser: return 0; } -int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *result) { +int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *result, bool *authenticated) { DnsResourceRecord *rr; bool have_nsec3 = false; DnsAnswerFlags flags; @@ -1145,6 +1148,7 @@ int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *r assert(key); assert(result); + assert(authenticated); /* Look for any NSEC/NSEC3 RRs that say something about the specified key. */ @@ -1153,9 +1157,6 @@ int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *r if (rr->key->class != key->class) continue; - if ((flags & DNS_ANSWER_AUTHENTICATED) == 0) - continue; - switch (rr->key->type) { case DNS_TYPE_NSEC: @@ -1165,6 +1166,7 @@ int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *r return r; if (r > 0) { *result = bitmap_isset(rr->nsec.types, key->type) ? DNSSEC_NSEC_FOUND : DNSSEC_NSEC_NODATA; + *authenticated = flags & DNS_ANSWER_AUTHENTICATED; return 0; } @@ -1173,6 +1175,7 @@ int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *r return r; if (r > 0) { *result = DNSSEC_NSEC_NXDOMAIN; + *authenticated = flags & DNS_ANSWER_AUTHENTICATED; return 0; } break; @@ -1185,7 +1188,7 @@ int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *r /* OK, this was not sufficient. Let's see if NSEC3 can help. */ if (have_nsec3) - return dnssec_test_nsec3(answer, key, result); + return dnssec_test_nsec3(answer, key, result, authenticated); /* No approproate NSEC RR found, report this. */ *result = DNSSEC_NSEC_NO_RR; diff --git a/src/resolve/resolved-dns-dnssec.h b/src/resolve/resolved-dns-dnssec.h index d17d5142f5..9ad20c8c69 100644 --- a/src/resolve/resolved-dns-dnssec.h +++ b/src/resolve/resolved-dns-dnssec.h @@ -89,7 +89,7 @@ typedef enum DnssecNsecResult { DNSSEC_NSEC_OPTOUT, } DnssecNsecResult; -int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *result); +int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *result, bool *authenticated); const char* dnssec_mode_to_string(DnssecMode m) _const_; DnssecMode dnssec_mode_from_string(const char *s) _pure_; diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 9ac9de01ad..0f48f9bf81 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -2112,9 +2112,10 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { } else if (r == 0) { DnssecNsecResult nr; + bool authenticated = false; /* Bummer! Let's check NSEC/NSEC3 */ - r = dnssec_test_nsec(t->answer, t->key, &nr); + r = dnssec_test_nsec(t->answer, t->key, &nr, &authenticated); if (r < 0) return r; @@ -2125,7 +2126,7 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { log_debug("Proved NXDOMAIN via NSEC/NSEC3 for transaction %u (%s)", t->id, dns_transaction_key_string(t)); t->answer_dnssec_result = DNSSEC_VALIDATED; t->answer_rcode = DNS_RCODE_NXDOMAIN; - t->answer_authenticated = true; + t->answer_authenticated = authenticated; break; case DNSSEC_NSEC_NODATA: @@ -2133,7 +2134,7 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { log_debug("Proved NODATA via NSEC/NSEC3 for transaction %u (%s)", t->id, dns_transaction_key_string(t)); t->answer_dnssec_result = DNSSEC_VALIDATED; t->answer_rcode = DNS_RCODE_SUCCESS; - t->answer_authenticated = true; + t->answer_authenticated = authenticated; break; case DNSSEC_NSEC_OPTOUT: From a150ff5e4e2481eb28d6ed6e0d3e176623e25f5a Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Wed, 23 Dec 2015 19:06:36 +0100 Subject: [PATCH 20/41] resolved: gather statistics about resolved names This collects statistical data about transactions, dnssec verifications and the cache, and exposes it over the bus. The systemd-resolve-host tool learns new options to query these statistics and reset them. --- src/resolve-host/resolve-host.c | 251 +++++++++++++++++++++---- src/resolve/resolved-bus.c | 85 +++++++++ src/resolve/resolved-dns-cache.c | 27 ++- src/resolve/resolved-dns-cache.h | 4 + src/resolve/resolved-dns-transaction.c | 17 ++ src/resolve/resolved-manager.h | 3 + 6 files changed, 346 insertions(+), 41 deletions(-) diff --git a/src/resolve-host/resolve-host.c b/src/resolve-host/resolve-host.c index 4752992cba..67c93c2618 100644 --- a/src/resolve-host/resolve-host.c +++ b/src/resolve-host/resolve-host.c @@ -33,6 +33,7 @@ #include "parse-util.h" #include "resolved-def.h" #include "resolved-dns-packet.h" +#include "terminal-util.h" #define DNS_CALL_TIMEOUT_USEC (45*USEC_PER_SEC) @@ -42,7 +43,14 @@ static uint16_t arg_type = 0; static uint16_t arg_class = 0; static bool arg_legend = true; static uint64_t arg_flags = 0; -static bool arg_resolve_service = false; + +static enum { + MODE_RESOLVE_HOST, + MODE_RESOLVE_RECORD, + MODE_RESOLVE_SERVICE, + MODE_STATISTICS, + MODE_RESET_STATISTICS, +} arg_mode = MODE_RESOLVE_HOST; static void print_source(uint64_t flags, usec_t rtt) { char rtt_str[FORMAT_TIMESTAMP_MAX]; @@ -637,6 +645,121 @@ static int resolve_service(sd_bus *bus, const char *name, const char *type, cons return 0; } +static int show_statistics(sd_bus *bus) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + _cleanup_(sd_bus_message_unrefp) sd_bus_message *reply = NULL; + uint64_t n_current_transactions, n_total_transactions, + cache_size, n_cache_hit, n_cache_miss, + n_dnssec_secure, n_dnssec_insecure, n_dnssec_bogus, n_dnssec_indeterminate; + int r; + + assert(bus); + + r = sd_bus_get_property(bus, + "org.freedesktop.resolve1", + "/org/freedesktop/resolve1", + "org.freedesktop.resolve1.Manager", + "TransactionStatistics", + &error, + &reply, + "(tt)"); + if (r < 0) + return log_error_errno(r, "Failed to get transaction statistics: %s", bus_error_message(&error, r)); + + r = sd_bus_message_read(reply, "(tt)", + &n_current_transactions, + &n_total_transactions); + if (r < 0) + return bus_log_parse_error(r); + + printf("%sTransactions%s\n" + "Current Transactions: %" PRIu64 "\n" + " Total Transactions: %" PRIu64 "\n", + ansi_highlight(), + ansi_normal(), + n_current_transactions, + n_total_transactions); + + reply = sd_bus_message_unref(reply); + + r = sd_bus_get_property(bus, + "org.freedesktop.resolve1", + "/org/freedesktop/resolve1", + "org.freedesktop.resolve1.Manager", + "CacheStatistics", + &error, + &reply, + "(ttt)"); + + r = sd_bus_message_read(reply, "(ttt)", + &cache_size, + &n_cache_hit, + &n_cache_miss); + if (r < 0) + return bus_log_parse_error(r); + + printf("\n%sCache%s\n" + " Current Cache Size: %" PRIu64 "\n" + " Cache Hits: %" PRIu64 "\n" + " Cache Misses: %" PRIu64 "\n", + ansi_highlight(), + ansi_normal(), + cache_size, + n_cache_hit, + n_cache_miss); + + reply = sd_bus_message_unref(reply); + + r = sd_bus_get_property(bus, + "org.freedesktop.resolve1", + "/org/freedesktop/resolve1", + "org.freedesktop.resolve1.Manager", + "DNSSECStatistics", + &error, + &reply, + "(tttt)"); + + r = sd_bus_message_read(reply, "(tttt)", + &n_dnssec_secure, + &n_dnssec_insecure, + &n_dnssec_bogus, + &n_dnssec_indeterminate); + if (r < 0) + return bus_log_parse_error(r); + + printf("\n%sDNSSEC%s\n" + " Secure RRsets: %" PRIu64 "\n" + " Insecure RRsets: %" PRIu64 "\n" + " Bogus RRsets: %" PRIu64 "\n" + "Indeterminate RRsets: %" PRIu64 "\n", + ansi_highlight(), + ansi_normal(), + n_dnssec_secure, + n_dnssec_insecure, + n_dnssec_bogus, + n_dnssec_indeterminate); + + return 0; +} + +static int reset_statistics(sd_bus *bus) { + _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL; + int r; + + r = sd_bus_call_method(bus, + "org.freedesktop.resolve1", + "/org/freedesktop/resolve1", + "org.freedesktop.resolve1.Manager", + "ResetStatistics", + &error, + NULL, + NULL); + if (r < 0) + return log_error_errno(r, "Failed to reset statistics: %s", bus_error_message(&error, r)); + + return 0; +} + static void help_dns_types(void) { int i; const char *t; @@ -681,6 +804,8 @@ static void help(void) { " --cname=BOOL Do [not] follow CNAME redirects\n" " --search=BOOL Do [not] use search domains\n" " --legend=BOOL Do [not] print column headers\n" + " --statistics Show resolver statistics\n" + " --reset-statistics Reset resolver statistics\n" , program_invocation_short_name, program_invocation_short_name); } @@ -693,20 +818,24 @@ static int parse_argv(int argc, char *argv[]) { ARG_SERVICE_ADDRESS, ARG_SERVICE_TXT, ARG_SEARCH, + ARG_STATISTICS, + ARG_RESET_STATISTICS, }; static const struct option options[] = { - { "help", no_argument, NULL, 'h' }, - { "version", no_argument, NULL, ARG_VERSION }, - { "type", required_argument, NULL, 't' }, - { "class", required_argument, NULL, 'c' }, - { "legend", required_argument, NULL, ARG_LEGEND }, - { "protocol", required_argument, NULL, 'p' }, - { "cname", required_argument, NULL, ARG_CNAME }, - { "service", no_argument, NULL, ARG_SERVICE }, - { "service-address", required_argument, NULL, ARG_SERVICE_ADDRESS }, - { "service-txt", required_argument, NULL, ARG_SERVICE_TXT }, - { "search", required_argument, NULL, ARG_SEARCH }, + { "help", no_argument, NULL, 'h' }, + { "version", no_argument, NULL, ARG_VERSION }, + { "type", required_argument, NULL, 't' }, + { "class", required_argument, NULL, 'c' }, + { "legend", required_argument, NULL, ARG_LEGEND }, + { "protocol", required_argument, NULL, 'p' }, + { "cname", required_argument, NULL, ARG_CNAME }, + { "service", no_argument, NULL, ARG_SERVICE }, + { "service-address", required_argument, NULL, ARG_SERVICE_ADDRESS }, + { "service-txt", required_argument, NULL, ARG_SERVICE_TXT }, + { "search", required_argument, NULL, ARG_SEARCH }, + { "statistics", no_argument, NULL, ARG_STATISTICS, }, + { "reset-statistics", no_argument, NULL, ARG_RESET_STATISTICS }, {} }; @@ -763,6 +892,7 @@ static int parse_argv(int argc, char *argv[]) { arg_type = (uint16_t) r; assert((int) arg_type == r); + arg_mode = MODE_RESOLVE_RECORD; break; case 'c': @@ -806,7 +936,7 @@ static int parse_argv(int argc, char *argv[]) { break; case ARG_SERVICE: - arg_resolve_service = true; + arg_mode = MODE_RESOLVE_SERVICE; break; case ARG_CNAME: @@ -849,6 +979,14 @@ static int parse_argv(int argc, char *argv[]) { arg_flags &= ~SD_RESOLVED_NO_SEARCH; break; + case ARG_STATISTICS: + arg_mode = MODE_STATISTICS; + break; + + case ARG_RESET_STATISTICS: + arg_mode = MODE_RESET_STATISTICS; + break; + case '?': return -EINVAL; @@ -861,7 +999,7 @@ static int parse_argv(int argc, char *argv[]) { return -EINVAL; } - if (arg_type != 0 && arg_resolve_service) { + if (arg_type != 0 && arg_mode != MODE_RESOLVE_RECORD) { log_error("--service and --type= may not be combined."); return -EINVAL; } @@ -883,20 +1021,57 @@ int main(int argc, char **argv) { if (r <= 0) goto finish; - if (optind >= argc) { - log_error("No arguments passed"); - r = -EINVAL; - goto finish; - } - r = sd_bus_open_system(&bus); if (r < 0) { log_error_errno(r, "sd_bus_open_system: %m"); goto finish; } - if (arg_resolve_service) { + switch (arg_mode) { + case MODE_RESOLVE_HOST: + if (optind >= argc) { + log_error("No arguments passed"); + r = -EINVAL; + goto finish; + } + + while (argv[optind]) { + int family, ifindex, k; + union in_addr_union a; + + k = parse_address(argv[optind], &family, &a, &ifindex); + if (k >= 0) + k = resolve_address(bus, family, &a, ifindex); + else + k = resolve_host(bus, argv[optind]); + + if (r == 0) + r = k; + + optind++; + } + break; + + case MODE_RESOLVE_RECORD: + if (optind >= argc) { + log_error("No arguments passed"); + r = -EINVAL; + goto finish; + } + + while (argv[optind]) { + int k; + + k = resolve_record(bus, argv[optind]); + if (r == 0) + r = k; + + optind++; + } + break; + + case MODE_RESOLVE_SERVICE: if (argc < optind + 1) { log_error("Domain specification required."); r = -EINVAL; @@ -914,27 +1089,27 @@ int main(int argc, char **argv) { goto finish; } - goto finish; - } + break; - while (argv[optind]) { - int family, ifindex, k; - union in_addr_union a; - - if (arg_type != 0) - k = resolve_record(bus, argv[optind]); - else { - k = parse_address(argv[optind], &family, &a, &ifindex); - if (k >= 0) - k = resolve_address(bus, family, &a, ifindex); - else - k = resolve_host(bus, argv[optind]); + case MODE_STATISTICS: + if (argc > optind) { + log_error("Too many arguments."); + r = -EINVAL; + goto finish; } - if (r == 0) - r = k; + r = show_statistics(bus); + break; - optind++; + case MODE_RESET_STATISTICS: + if (argc > optind) { + log_error("Too many arguments."); + r = -EINVAL; + goto finish; + } + + r = reset_statistics(bus); + break; } finish: diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index af08a0555d..273bfbe949 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -1214,16 +1214,101 @@ static int bus_property_get_search_domains( return sd_bus_message_close_container(reply); } +static int bus_property_get_transaction_statistics( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *reply, + void *userdata, + sd_bus_error *error) { + + Manager *m = userdata; + + assert(reply); + assert(m); + + return sd_bus_message_append(reply, "(tt)", + (uint64_t) hashmap_size(m->dns_transactions), + (uint64_t) m->n_transactions_total); +} + +static int bus_property_get_cache_statistics( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *reply, + void *userdata, + sd_bus_error *error) { + + uint64_t size = 0, hit = 0, miss = 0; + Manager *m = userdata; + DnsScope *s; + + assert(reply); + assert(m); + + LIST_FOREACH(scopes, s, m->dns_scopes) { + size += dns_cache_size(&s->cache); + hit += s->cache.n_hit; + miss += s->cache.n_miss; + } + + return sd_bus_message_append(reply, "(ttt)", size, hit, miss); +} + +static int bus_property_get_dnssec_statistics( + sd_bus *bus, + const char *path, + const char *interface, + const char *property, + sd_bus_message *reply, + void *userdata, + sd_bus_error *error) { + + Manager *m = userdata; + + assert(reply); + assert(m); + + return sd_bus_message_append(reply, "(tttt)", + (uint64_t) m->n_dnssec_secure, + (uint64_t) m->n_dnssec_insecure, + (uint64_t) m->n_dnssec_bogus, + (uint64_t) m->n_dnssec_indeterminate); +} + +static int bus_method_reset_statistics(sd_bus_message *message, void *userdata, sd_bus_error *error) { + Manager *m = userdata; + DnsScope *s; + + assert(message); + assert(m); + + LIST_FOREACH(scopes, s, m->dns_scopes) + s->cache.n_hit = s->cache.n_miss = 0; + + m->n_transactions_total = 0; + m->n_dnssec_secure = m->n_dnssec_insecure = m->n_dnssec_bogus = m->n_dnssec_indeterminate = 0; + + return sd_bus_reply_method_return(message, NULL); +} + static const sd_bus_vtable resolve_vtable[] = { SD_BUS_VTABLE_START(0), SD_BUS_PROPERTY("LLMNRHostname", "s", NULL, offsetof(Manager, llmnr_hostname), 0), SD_BUS_PROPERTY("DNSServers", "a(iiay)", bus_property_get_dns_servers, 0, 0), SD_BUS_PROPERTY("SearchDomains", "a(is)", bus_property_get_search_domains, 0, 0), + SD_BUS_PROPERTY("TransactionStatistics", "(tt)", bus_property_get_transaction_statistics, 0, 0), + SD_BUS_PROPERTY("CacheStatistics", "(ttt)", bus_property_get_cache_statistics, 0, 0), + SD_BUS_PROPERTY("DNSSECStatistics", "(tttt)", bus_property_get_dnssec_statistics, 0, 0), SD_BUS_METHOD("ResolveHostname", "isit", "a(iiay)st", bus_method_resolve_hostname, SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_METHOD("ResolveAddress", "iiayt", "a(is)t", bus_method_resolve_address, SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_METHOD("ResolveRecord", "isqqt", "a(iqqay)t", bus_method_resolve_record, SD_BUS_VTABLE_UNPRIVILEGED), SD_BUS_METHOD("ResolveService", "isssit", "a(qqqsa(iiay)s)aayssst", bus_method_resolve_service, SD_BUS_VTABLE_UNPRIVILEGED), + SD_BUS_METHOD("ResetStatistics", NULL, NULL, bus_method_reset_statistics, 0), SD_BUS_VTABLE_END, }; diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c index f9fe2c2e30..49d5090d36 100644 --- a/src/resolve/resolved-dns-cache.c +++ b/src/resolve/resolved-dns-cache.c @@ -740,6 +740,8 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r log_debug("Ignoring cache for ANY lookup: %s", key_str); } + c->n_miss++; + *ret = NULL; *rcode = DNS_RCODE_SUCCESS; return 0; @@ -757,6 +759,8 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r log_debug("Cache miss for %s", key_str); } + c->n_miss++; + *ret = NULL; *rcode = DNS_RCODE_SUCCESS; return 0; @@ -794,9 +798,15 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r *rcode = DNS_RCODE_SUCCESS; *authenticated = nsec->authenticated; - return !bitmap_isset(nsec->rr->nsec.types, key->type) && - !bitmap_isset(nsec->rr->nsec.types, DNS_TYPE_CNAME) && - !bitmap_isset(nsec->rr->nsec.types, DNS_TYPE_DNAME); + if (!bitmap_isset(nsec->rr->nsec.types, key->type) && + !bitmap_isset(nsec->rr->nsec.types, DNS_TYPE_CNAME) && + !bitmap_isset(nsec->rr->nsec.types, DNS_TYPE_DNAME)) { + c->n_hit++; + return 1; + } + + c->n_miss++; + return 0; } if (log_get_max_level() >= LOG_DEBUG) { @@ -811,6 +821,8 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r } if (n <= 0) { + c->n_hit++; + *ret = NULL; *rcode = nxdomain ? DNS_RCODE_NXDOMAIN : DNS_RCODE_SUCCESS; *authenticated = have_authenticated && !have_non_authenticated; @@ -830,6 +842,8 @@ int dns_cache_lookup(DnsCache *c, DnsResourceKey *key, int *rcode, DnsAnswer **r return r; } + c->n_hit++; + *ret = answer; *rcode = DNS_RCODE_SUCCESS; *authenticated = have_authenticated && !have_non_authenticated; @@ -974,3 +988,10 @@ bool dns_cache_is_empty(DnsCache *cache) { return hashmap_isempty(cache->by_key); } + +unsigned dns_cache_size(DnsCache *cache) { + if (!cache) + return 0; + + return hashmap_size(cache->by_key); +} diff --git a/src/resolve/resolved-dns-cache.h b/src/resolve/resolved-dns-cache.h index 856c975299..9c85ca4c58 100644 --- a/src/resolve/resolved-dns-cache.h +++ b/src/resolve/resolved-dns-cache.h @@ -29,6 +29,8 @@ typedef struct DnsCache { Hashmap *by_key; Prioq *by_expiry; + unsigned n_hit; + unsigned n_miss; } DnsCache; #include "resolved-dns-answer.h" @@ -47,4 +49,6 @@ int dns_cache_check_conflicts(DnsCache *cache, DnsResourceRecord *rr, int owner_ void dns_cache_dump(DnsCache *cache, FILE *f); bool dns_cache_is_empty(DnsCache *cache); +unsigned dns_cache_size(DnsCache *cache); + int dns_cache_export_shared_to_packet(DnsCache *cache, DnsPacket *p); diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 0f48f9bf81..af6f28ea0f 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -153,6 +153,8 @@ int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsResourceKey *key) LIST_PREPEND(transactions_by_scope, s->transactions, t); t->scope = s; + s->manager->n_transactions_total ++; + if (ret) *ret = t; @@ -1996,6 +1998,8 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { if (r < 0) return r; + t->scope->manager->n_dnssec_secure++; + /* Exit the loop, we dropped something from the answer, start from the beginning */ changed = true; break; @@ -2018,6 +2022,8 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { if (r < 0) return r; + t->scope->manager->n_dnssec_insecure++; + changed = true; break; } @@ -2039,11 +2045,22 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { if (r < 0) return r; + t->scope->manager->n_dnssec_insecure++; + changed = true; break; } } + if (IN_SET(result, + DNSSEC_INVALID, + DNSSEC_SIGNATURE_EXPIRED, + DNSSEC_NO_SIGNATURE, + DNSSEC_UNSUPPORTED_ALGORITHM)) + t->scope->manager->n_dnssec_bogus++; + else + t->scope->manager->n_dnssec_indeterminate++; + r = dns_transaction_is_primary_response(t, rr); if (r < 0) return r; diff --git a/src/resolve/resolved-manager.h b/src/resolve/resolved-manager.h index b52273403a..dd85d3ba47 100644 --- a/src/resolve/resolved-manager.h +++ b/src/resolve/resolved-manager.h @@ -128,6 +128,9 @@ struct Manager { sd_bus_slot *prepare_for_sleep_slot; sd_event_source *sigusr1_event_source; + + unsigned n_transactions_total; + unsigned n_dnssec_secure, n_dnssec_insecure, n_dnssec_bogus, n_dnssec_indeterminate; }; /* Manager */ From f61dfddbff4c826bfcbca7b413674770546fa527 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 24 Dec 2015 00:24:10 +0100 Subject: [PATCH 21/41] resolved: log each dnssec failure, in a recognizable way --- src/resolve/resolved-dns-transaction.c | 8 ++++++++ src/systemd/sd-messages.h | 2 ++ 2 files changed, 10 insertions(+) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index af6f28ea0f..f138d55ad8 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -226,6 +226,14 @@ void dns_transaction_complete(DnsTransaction *t, DnsTransactionState state) { assert(t); assert(!DNS_TRANSACTION_IS_LIVE(state)); + if (state == DNS_TRANSACTION_DNSSEC_FAILED) + log_struct(LOG_NOTICE, + LOG_MESSAGE("DNSSEC validation failed for question %s: %s", dns_transaction_key_string(t), dnssec_result_to_string(t->answer_dnssec_result)), + "DNS_TRANSACTION=%" PRIu16, t->id, + "DNS_QUESTION=%s", dns_transaction_key_string(t), + "DNSSEC_RESULT=%s", dnssec_result_to_string(t->answer_dnssec_result), + NULL); + /* Note that this call might invalidate the query. Callers * should hence not attempt to access the query or transaction * after calling this function. */ diff --git a/src/systemd/sd-messages.h b/src/systemd/sd-messages.h index 072832a916..bc658f62b0 100644 --- a/src/systemd/sd-messages.h +++ b/src/systemd/sd-messages.h @@ -86,6 +86,8 @@ _SD_BEGIN_DECLARATIONS; #define SD_MESSAGE_BOOTCHART SD_ID128_MAKE(9f,26,aa,56,2c,f4,40,c2,b1,6c,77,3d,04,79,b5,18) +#define SD_MESSAGE_DNSSEC_FAILURE SD_ID128_MAKE(16,75,d7,f1,72,17,40,98,b1,10,8b,f8,c7,dc,8f,5d) + _SD_END_DECLARATIONS; #endif From b63fca62453969f816c687ac4cf438d5cefa4552 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 24 Dec 2015 14:08:22 +0100 Subject: [PATCH 22/41] resolved: deal with unsigned DS/NSEC/NSEC3 properly Previously, we'd insist on an RRSIG for all DS/NSEC/NSEC3 RRs. With this change we don't do that anymore, but also allow unsigned DS/NSEC/NSEC3 if we can prove that the zone they are located in is unsigned. --- src/resolve/resolved-dns-transaction.c | 49 ++++++++++++-------------- 1 file changed, 23 insertions(+), 26 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index f138d55ad8..323e45b36f 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -1325,7 +1325,7 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) { * - For RRSIG we get the matching DNSKEY * - For DNSKEY we get the matching DS * - For unsigned SOA/NS we get the matching DS - * - For unsigned CNAME/DNAME we get the parent SOA RR + * - For unsigned CNAME/DNAME/DS we get the parent SOA RR * - For other unsigned RRs we get the matching SOA RR * - For SOA/NS/DS queries with no matching response RRs, and no NSEC/NSEC3, the parent's SOA RR * - For other queries with no matching response RRs, and no NSEC/NSEC3, the SOA RR @@ -1414,15 +1414,6 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) { break; } - case DNS_TYPE_DS: - case DNS_TYPE_NSEC: - case DNS_TYPE_NSEC3: - /* Don't acquire anything for - * DS/NSEC/NSEC3. We require they come with an - * RRSIG without us asking for anything, and - * that's sufficient. */ - break; - case DNS_TYPE_SOA: case DNS_TYPE_NS: { _cleanup_(dns_resource_key_unrefp) DnsResourceKey *ds = NULL; @@ -1458,6 +1449,7 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) { break; } + case DNS_TYPE_DS: case DNS_TYPE_CNAME: case DNS_TYPE_DNAME: { _cleanup_(dns_resource_key_unrefp) DnsResourceKey *soa = NULL; @@ -1468,7 +1460,10 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) { * unsigned CNAME/DNAME RRs, maybe that's the * apex. But do all that only if this is * actually a response to our original - * question. */ + * question. + * + * Similar for DS RRs, which are signed when + * the parent SOA is signed. */ r = dns_transaction_is_primary_response(t, rr); if (r < 0) @@ -1493,7 +1488,7 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) { if (!soa) return -ENOMEM; - log_debug("Requesting parent SOA to validate transaction %" PRIu16 " (%s, unsigned CNAME/DNAME RRset).", t->id, DNS_RESOURCE_KEY_NAME(rr->key)); + log_debug("Requesting parent SOA to validate transaction %" PRIu16 " (%s, unsigned CNAME/DNAME/DS RRset).", t->id, DNS_RESOURCE_KEY_NAME(rr->key)); r = dns_transaction_request_dnssec_rr(t, soa); if (r < 0) return r; @@ -1504,10 +1499,11 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) { default: { _cleanup_(dns_resource_key_unrefp) DnsResourceKey *soa = NULL; - /* For other unsigned RRsets, look for proof - * the zone is unsigned, by requesting the SOA - * RR of the zone. However, do so only if they - * are directly relevant to our original + /* For other unsigned RRsets (including + * NSEC/NSEC3!), look for proof the zone is + * unsigned, by requesting the SOA RR of the + * zone. However, do so only if they are + * directly relevant to our original * question. */ r = dns_transaction_is_primary_response(t, rr); @@ -1689,13 +1685,6 @@ static int dns_transaction_requires_rrsig(DnsTransaction *t, DnsResourceRecord * switch (rr->key->type) { - case DNS_TYPE_DNSKEY: - case DNS_TYPE_DS: - case DNS_TYPE_NSEC: - case DNS_TYPE_NSEC3: - /* We never consider DNSKEY, DS, NSEC, NSEC3 RRs if they aren't signed. */ - return true; - case DNS_TYPE_RRSIG: /* RRSIGs are the signatures themselves, they need no signing. */ return false; @@ -1705,7 +1694,7 @@ static int dns_transaction_requires_rrsig(DnsTransaction *t, DnsResourceRecord * DnsTransaction *dt; Iterator i; - /* For SOA or NS RRs we look for a matching DS transaction, or a SOA transaction of the parent */ + /* For SOA or NS RRs we look for a matching DS transaction */ SET_FOREACH(dt, t->dnssec_transactions, i) { @@ -1736,13 +1725,18 @@ static int dns_transaction_requires_rrsig(DnsTransaction *t, DnsResourceRecord * return true; } + case DNS_TYPE_DS: case DNS_TYPE_CNAME: case DNS_TYPE_DNAME: { const char *parent = NULL; DnsTransaction *dt; Iterator i; - /* CNAME/DNAME RRs cannot be located at a zone apex, hence look directly for the parent SOA. */ + /* + * CNAME/DNAME RRs cannot be located at a zone apex, hence look directly for the parent SOA. + * + * DS RRs are signed if the parent is signed, hence also look at the parent SOA + */ SET_FOREACH(dt, t->dnssec_transactions, i) { @@ -1757,6 +1751,9 @@ static int dns_transaction_requires_rrsig(DnsTransaction *t, DnsResourceRecord * if (r < 0) return r; if (r == 0) { + if (rr->key->type == DNS_TYPE_DS) + return true; + /* A CNAME/DNAME without a parent? That's sooo weird. */ log_debug("Transaction %" PRIu16 " claims CNAME/DNAME at root. Refusing.", t->id); return -EBADMSG; @@ -1779,7 +1776,7 @@ static int dns_transaction_requires_rrsig(DnsTransaction *t, DnsResourceRecord * DnsTransaction *dt; Iterator i; - /* Any other kind of RR. Let's see if our SOA lookup was authenticated */ + /* Any other kind of RR (including DNSKEY/NSEC/NSEC3). Let's see if our SOA lookup was authenticated */ SET_FOREACH(dt, t->dnssec_transactions, i) { From ac720200b7e5b80cc4985087e38f3452e5b3b080 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 25 Dec 2015 12:54:27 +0100 Subject: [PATCH 23/41] resolved: generate an explicit transaction error when we cannot reach server via TCP Previously, if we couldn't reach a server via UDP we'd generate an MAX_ATTEMPTS transaction result, but if we couldn't reach it via TCP we'd generate a RESOURCES transaction result. While it is OK to generate two different errors I think, "RESOURCES" is certainly a misnomer. Introduce a new transaction result "CONNECTION_FAILURE" instead. --- src/libsystemd/sd-bus/bus-common-errors.h | 1 + src/resolve/resolved-bus.c | 3 +++ src/resolve/resolved-dns-transaction.c | 6 ++++++ src/resolve/resolved-dns-transaction.h | 1 + 4 files changed, 11 insertions(+) diff --git a/src/libsystemd/sd-bus/bus-common-errors.h b/src/libsystemd/sd-bus/bus-common-errors.h index f2092795f4..d7d210b0c4 100644 --- a/src/libsystemd/sd-bus/bus-common-errors.h +++ b/src/libsystemd/sd-bus/bus-common-errors.h @@ -72,6 +72,7 @@ #define BUS_ERROR_NO_RESOURCES "org.freedesktop.resolve1.NoResources" #define BUS_ERROR_CNAME_LOOP "org.freedesktop.resolve1.CNameLoop" #define BUS_ERROR_ABORTED "org.freedesktop.resolve1.Aborted" +#define BUS_ERROR_CONNECTION_FAILURE "org.freedesktop.resolve1.ConnectionFailure" #define _BUS_ERROR_DNS "org.freedesktop.resolve1.DnsError." #define BUS_ERROR_NO_SUCH_TRANSFER "org.freedesktop.import1.NoSuchTransfer" diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 273bfbe949..5c7893d01c 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -57,6 +57,9 @@ static int reply_query_state(DnsQuery *q) { case DNS_TRANSACTION_RESOURCES: return sd_bus_reply_method_errorf(q->request, BUS_ERROR_NO_RESOURCES, "Not enough resources"); + case DNS_TRANSACTION_CONNECTION_FAILURE: + return sd_bus_reply_method_errorf(q->request, BUS_ERROR_CONNECTION_FAILURE, "DNS server connection failure"); + case DNS_TRANSACTION_ABORTED: return sd_bus_reply_method_errorf(q->request, BUS_ERROR_ABORTED, "Query aborted"); diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 323e45b36f..2875330812 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -308,6 +308,11 @@ static int on_stream_complete(DnsStream *s, int error) { t->stream = dns_stream_free(t->stream); + if (IN_SET(error, ENOTCONN, ECONNRESET, ECONNREFUSED, ECONNABORTED, EPIPE)) { + dns_transaction_complete(t, DNS_TRANSACTION_CONNECTION_FAILURE); + return 0; + } + if (error != 0) { dns_transaction_complete(t, DNS_TRANSACTION_RESOURCES); return 0; @@ -2221,6 +2226,7 @@ static const char* const dns_transaction_state_table[_DNS_TRANSACTION_STATE_MAX] [DNS_TRANSACTION_ATTEMPTS_MAX_REACHED] = "attempts-max-reached", [DNS_TRANSACTION_INVALID_REPLY] = "invalid-reply", [DNS_TRANSACTION_RESOURCES] = "resources", + [DNS_TRANSACTION_CONNECTION_FAILURE] = "connection-failure", [DNS_TRANSACTION_ABORTED] = "aborted", [DNS_TRANSACTION_DNSSEC_FAILED] = "dnssec-failed", }; diff --git a/src/resolve/resolved-dns-transaction.h b/src/resolve/resolved-dns-transaction.h index fea25aab09..eebb6ec631 100644 --- a/src/resolve/resolved-dns-transaction.h +++ b/src/resolve/resolved-dns-transaction.h @@ -36,6 +36,7 @@ enum DnsTransactionState { DNS_TRANSACTION_ATTEMPTS_MAX_REACHED, DNS_TRANSACTION_INVALID_REPLY, DNS_TRANSACTION_RESOURCES, + DNS_TRANSACTION_CONNECTION_FAILURE, DNS_TRANSACTION_ABORTED, DNS_TRANSACTION_DNSSEC_FAILED, _DNS_TRANSACTION_STATE_MAX, From fbd5e3526cbf5dd956d6cb8996f257e63183eb81 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 25 Dec 2015 12:58:07 +0100 Subject: [PATCH 24/41] resolved: don't set TCP_NODELAY twice for TCP sockets We previously set it once in the scope code and once in the stream code. Remove it from the latter, as all other socket options are set in the former. --- src/resolve/resolved-dns-stream.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/resolve/resolved-dns-stream.c b/src/resolve/resolved-dns-stream.c index 1c501182fb..180f8e0877 100644 --- a/src/resolve/resolved-dns-stream.c +++ b/src/resolve/resolved-dns-stream.c @@ -347,7 +347,6 @@ DnsStream *dns_stream_free(DnsStream *s) { DEFINE_TRIVIAL_CLEANUP_FUNC(DnsStream*, dns_stream_free); int dns_stream_new(Manager *m, DnsStream **ret, DnsProtocol protocol, int fd) { - static const int one = 1; _cleanup_(dns_stream_freep) DnsStream *s = NULL; int r; @@ -364,10 +363,6 @@ int dns_stream_new(Manager *m, DnsStream **ret, DnsProtocol protocol, int fd) { s->fd = -1; s->protocol = protocol; - r = setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &one, sizeof(one)); - if (r < 0) - return -errno; - r = sd_event_add_io(m->event, &s->io_event_source, fd, EPOLLIN, on_stream_io, s); if (r < 0) return r; From 0e4fb6b2dedf590741220b806c92e9e68857b457 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 25 Dec 2015 15:01:37 +0100 Subject: [PATCH 25/41] resolved: no need to store return value of dns_server_possible_features() The call already updates possible_features, it's pointless doing this in the caller a second time. --- src/resolve/resolved-dns-scope.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c index b284cb8b27..a4df9b010a 100644 --- a/src/resolve/resolved-dns-scope.c +++ b/src/resolve/resolved-dns-scope.c @@ -320,7 +320,8 @@ static int dns_scope_socket(DnsScope *s, int type, int family, const union in_ad if (!srv) return -ESRCH; - srv->possible_features = dns_server_possible_features(srv); + /* Determine current feature level to use */ + (void) dns_server_possible_features(srv); if (type == SOCK_DGRAM && srv->possible_features < DNS_SERVER_FEATURE_LEVEL_UDP) return -EAGAIN; From b652d4a2099d1c167584dcc1d179d47c58dc38a2 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 25 Dec 2015 15:05:46 +0100 Subject: [PATCH 26/41] resolved: add an automatic downgrade to non-DNSSEC mode This adds a mode that makes resolved automatically downgrade from DNSSEC support to classic non-DNSSEC resolving if the configured DNS server is not capable of DNSSEC. Enabling this mode increases compatibility with crappy network equipment, but of course opens up the system to downgrading attacks. The new mode can be enabled by setting DNSSEC=downgrade-ok in resolved.conf. DNSSEC=yes otoh remains a "strict" mode, where DNS resolving rather fails then allow downgrading. Downgrading is done: - when the server does not support EDNS0+DO - or when the server supports it but does not augment returned RRs with RRSIGs. The latter is detected when requesting DS or SOA RRs for the root domain (which is necessary to do proofs for unsigned data) --- src/resolve/resolved-dns-dnssec.c | 2 + src/resolve/resolved-dns-dnssec.h | 11 +++- src/resolve/resolved-dns-server.c | 20 +++++-- src/resolve/resolved-dns-server.h | 12 ++++- src/resolve/resolved-dns-transaction.c | 75 +++++++++++++++++++++++--- 5 files changed, 106 insertions(+), 14 deletions(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index 14fa58c544..a856f0717e 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -1197,6 +1197,7 @@ int dnssec_test_nsec(DnsAnswer *answer, DnsResourceKey *key, DnssecNsecResult *r static const char* const dnssec_mode_table[_DNSSEC_MODE_MAX] = { [DNSSEC_NO] = "no", + [DNSSEC_DOWNGRADE_OK] = "downgrade-ok", [DNSSEC_YES] = "yes", }; DEFINE_STRING_TABLE_LOOKUP(dnssec_mode, DnssecMode); @@ -1211,5 +1212,6 @@ static const char* const dnssec_result_table[_DNSSEC_RESULT_MAX] = { [DNSSEC_UNSIGNED] = "unsigned", [DNSSEC_FAILED_AUXILIARY] = "failed-auxiliary", [DNSSEC_NSEC_MISMATCH] = "nsec-mismatch", + [DNSSEC_INCOMPATIBLE_SERVER] = "incompatible-server", }; DEFINE_STRING_TABLE_LOOKUP(dnssec_result, DnssecResult); diff --git a/src/resolve/resolved-dns-dnssec.h b/src/resolve/resolved-dns-dnssec.h index 9ad20c8c69..d7aecbce13 100644 --- a/src/resolve/resolved-dns-dnssec.h +++ b/src/resolve/resolved-dns-dnssec.h @@ -32,7 +32,14 @@ enum DnssecMode { /* No DNSSEC validation is done */ DNSSEC_NO, - /* Validate locally, if the server knows DO, but if not, don't. Don't trust the AD bit */ + /* Validate locally, if the server knows DO, but if not, + * don't. Don't trust the AD bit. If the server doesn't do + * DNSSEC properly, downgrade to non-DNSSEC operation. Of + * course, we then are vulnerable to a downgrade attack, but + * that's life and what is configured. */ + DNSSEC_DOWNGRADE_OK, + + /* Insist on DNSSEC server support, and rather fail than downgrading. */ DNSSEC_YES, _DNSSEC_MODE_MAX, @@ -54,6 +61,8 @@ enum DnssecResult { DNSSEC_UNSIGNED, DNSSEC_FAILED_AUXILIARY, DNSSEC_NSEC_MISMATCH, + DNSSEC_INCOMPATIBLE_SERVER, + _DNSSEC_RESULT_MAX, _DNSSEC_RESULT_INVALID = -1 }; diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c index d565f99c09..b0db5bbb16 100644 --- a/src/resolve/resolved-dns-server.c +++ b/src/resolve/resolved-dns-server.c @@ -228,9 +228,11 @@ void dns_server_packet_received(DnsServer *s, DnsServerFeatureLevel features, us assert(s); if (features == DNS_SERVER_FEATURE_LEVEL_LARGE) { - /* even if we successfully receive a reply to a request announcing - support for large packets, that does not mean we can necessarily - receive large packets. */ + /* Even if we successfully receive a reply to a + request announcing support for large packets, that + does not mean we can necessarily receive large + packets. */ + if (s->verified_features < DNS_SERVER_FEATURE_LEVEL_LARGE - 1) { s->verified_features = DNS_SERVER_FEATURE_LEVEL_LARGE - 1; assert_se(sd_event_now(s->manager->event, clock_boottime_or_monotonic(), &s->verified_usec) >= 0); @@ -278,6 +280,17 @@ void dns_server_packet_failed(DnsServer *s, DnsServerFeatureLevel features) { s->n_failed_attempts = (unsigned) -1; } +void dns_server_packet_rrsig_missing(DnsServer *s) { + _cleanup_free_ char *ip = NULL; + assert(s); + assert(s->manager); + + in_addr_to_string(s->family, &s->address, &ip); + log_warning("DNS server %s does not augment replies with RRSIG records, DNSSEC not available.", strna(ip)); + + s->rrsig_missing = true; +} + static bool dns_server_grace_period_expired(DnsServer *s) { usec_t ts; @@ -307,6 +320,7 @@ DnsServerFeatureLevel dns_server_possible_features(DnsServer *s) { s->possible_features = DNS_SERVER_FEATURE_LEVEL_BEST; s->n_failed_attempts = 0; s->verified_usec = 0; + s->rrsig_missing = false; in_addr_to_string(s->family, &s->address, &ip); log_info("Grace period over, resuming full feature set for DNS server %s", strna(ip)); diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h index b07fc3af3d..3011904bfd 100644 --- a/src/resolve/resolved-dns-server.h +++ b/src/resolve/resolved-dns-server.h @@ -61,8 +61,6 @@ struct DnsServer { int family; union in_addr_union address; - bool marked:1; - usec_t resend_timeout; usec_t max_rtt; @@ -73,6 +71,15 @@ struct DnsServer { usec_t verified_usec; usec_t features_grace_period_usec; + /* Indicates whether responses are augmented with RRSIG by + * server or not. Note that this is orthogonal to the feature + * level stuff, as it's only information describing responses, + * and has no effect on how the questions are asked. */ + bool rrsig_missing:1; + + /* Used when GC'ing old DNS servers when configuration changes. */ + bool marked:1; + /* If linked is set, then this server appears in the servers linked list */ bool linked:1; LIST_FIELDS(DnsServer, servers); @@ -95,6 +102,7 @@ void dns_server_move_back_and_unmark(DnsServer *s); void dns_server_packet_received(DnsServer *s, DnsServerFeatureLevel features, usec_t rtt, size_t size); void dns_server_packet_lost(DnsServer *s, DnsServerFeatureLevel features, usec_t usec); void dns_server_packet_failed(DnsServer *s, DnsServerFeatureLevel features); +void dns_server_packet_rrsig_missing(DnsServer *s); DnsServer *dns_server_find(DnsServer *first, int family, const union in_addr_union *in_addr); diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 2875330812..5933e0e462 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -478,10 +478,19 @@ static void dns_transaction_process_dnssec(DnsTransaction *t) { return; } + if (t->answer_dnssec_result == DNSSEC_INCOMPATIBLE_SERVER && + t->scope->dnssec_mode == DNSSEC_YES) { + /* We are not in automatic downgrade mode, and the + * server is bad, refuse operation. */ + dns_transaction_complete(t, DNS_TRANSACTION_DNSSEC_FAILED); + return; + } + if (!IN_SET(t->answer_dnssec_result, - _DNSSEC_RESULT_INVALID, /* No DNSSEC validation enabled */ - DNSSEC_VALIDATED, /* Answer is signed and validated successfully */ - DNSSEC_UNSIGNED)) { /* Answer is right-fully unsigned */ + _DNSSEC_RESULT_INVALID, /* No DNSSEC validation enabled */ + DNSSEC_VALIDATED, /* Answer is signed and validated successfully */ + DNSSEC_UNSIGNED, /* Answer is right-fully unsigned */ + DNSSEC_INCOMPATIBLE_SERVER)) { /* Server does not do DNSSEC (Yay, we are downgrade attack vulnerable!) */ dns_transaction_complete(t, DNS_TRANSACTION_DNSSEC_FAILED); return; } @@ -1001,7 +1010,7 @@ static int dns_transaction_make_packet(DnsTransaction *t) { if (t->sent) return 0; - r = dns_packet_new_query(&p, t->scope->protocol, 0, t->scope->dnssec_mode == DNSSEC_YES); + r = dns_packet_new_query(&p, t->scope->protocol, 0, t->scope->dnssec_mode != DNSSEC_NO); if (r < 0) return r; @@ -1336,9 +1345,14 @@ int dns_transaction_request_dnssec_keys(DnsTransaction *t) { * - For other queries with no matching response RRs, and no NSEC/NSEC3, the SOA RR */ - if (t->scope->dnssec_mode != DNSSEC_YES) + if (t->scope->dnssec_mode == DNSSEC_NO) return 0; + if (t->current_features < DNS_SERVER_FEATURE_LEVEL_DO) + return 0; /* Server doesn't do DNSSEC, there's no point in requesting any RRs then. */ + if (t->server && t->server->rrsig_missing) + return 0; /* Server handles DNSSEC requests, but isn't augmenting responses with RRSIGs. No point in trying DNSSEC then. */ + DNS_ANSWER_FOREACH(rr, t->answer) { if (dns_type_is_pseudo(rr->key->type)) @@ -1682,7 +1696,7 @@ static int dns_transaction_requires_rrsig(DnsTransaction *t, DnsResourceRecord * /* Checks if the RR we are looking for must be signed with an * RRSIG. This is used for positive responses. */ - if (t->scope->dnssec_mode != DNSSEC_YES) + if (t->scope->dnssec_mode == DNSSEC_NO) return false; if (dns_type_is_pseudo(rr->key->type)) @@ -1819,7 +1833,7 @@ static int dns_transaction_requires_nsec(DnsTransaction *t) { /* Checks if we need to insist on NSEC/NSEC3 RRs for proving * this negative reply */ - if (t->scope->dnssec_mode != DNSSEC_YES) + if (t->scope->dnssec_mode == DNSSEC_NO) return false; if (dns_type_is_pseudo(t->key->type)) @@ -1932,6 +1946,17 @@ static int dns_transaction_dnskey_authenticated(DnsTransaction *t, DnsResourceRe return found ? false : -ENXIO; } +static int dns_transaction_known_signed(DnsTransaction *t, DnsResourceRecord *rr) { + assert(t); + assert(rr); + + /* We know that the root domain is signed, hence if it appears + * not to be signed, there's a problem with the DNS server */ + + return rr->key->class == DNS_CLASS_IN && + dns_name_is_root(DNS_RESOURCE_KEY_NAME(rr->key)); +} + int dns_transaction_validate_dnssec(DnsTransaction *t) { _cleanup_(dns_answer_unrefp) DnsAnswer *validated = NULL; bool dnskeys_finalized = false; @@ -1945,7 +1970,7 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { * t->validated_keys, let's see which RRs we can now * authenticate with that. */ - if (t->scope->dnssec_mode != DNSSEC_YES) + if (t->scope->dnssec_mode == DNSSEC_NO) return 0; /* Already validated */ @@ -1963,6 +1988,13 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { if (t->answer_source != DNS_TRANSACTION_NETWORK) return 0; + if (t->current_features < DNS_SERVER_FEATURE_LEVEL_DO || + (t->server && t->server->rrsig_missing)) { + /* The server does not support DNSSEC, or doesn't augment responses with RRSIGs. */ + t->answer_dnssec_result = DNSSEC_INCOMPATIBLE_SERVER; + return 0; + } + log_debug("Validating response from transaction %" PRIu16 " (%s).", t->id, dns_transaction_key_string(t)); /* First see if there are DNSKEYs we already known a validated DS for. */ @@ -2037,6 +2069,33 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { changed = true; break; } + + r = dns_transaction_known_signed(t, rr); + if (r < 0) + return r; + if (r > 0) { + /* This is an RR we know has to be signed. If it isn't this means + * the server is not attaching RRSIGs, hence complain. */ + + dns_server_packet_rrsig_missing(t->server); + + if (t->scope->dnssec_mode == DNSSEC_DOWNGRADE_OK) { + + /* Downgrading is OK? If so, just consider the information unsigned */ + + r = dns_answer_move_by_key(&validated, &t->answer, rr->key, 0); + if (r < 0) + return r; + + t->scope->manager->n_dnssec_insecure++; + changed = true; + break; + } + + /* Otherwise, fail */ + t->answer_dnssec_result = DNSSEC_INCOMPATIBLE_SERVER; + return 0; + } } if (IN_SET(result, From 49cce12d4a07a77c6321b743e538c648d33c037c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 25 Dec 2015 15:57:49 +0100 Subject: [PATCH 27/41] resolved: name TCP and UDP socket calls uniformly Previously the calls for emitting DNS UDP packets were just called dns_{transacion|scope}_emit(), but the one to establish a DNS TCP connection was called dns_transaction_open_tcp(). Clean this up, and rename them dns_{transaction|scope}_emit_udp() and dns_transaction_open_tcp(). --- src/resolve/resolved-dns-scope.c | 8 ++++---- src/resolve/resolved-dns-scope.h | 6 +++--- src/resolve/resolved-dns-transaction.c | 25 ++++++++++++++----------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c index a4df9b010a..b29f15d2c0 100644 --- a/src/resolve/resolved-dns-scope.c +++ b/src/resolve/resolved-dns-scope.c @@ -279,7 +279,7 @@ static int dns_scope_emit_one(DnsScope *s, int fd, DnsServer *server, DnsPacket return 1; } -int dns_scope_emit(DnsScope *s, int fd, DnsServer *server, DnsPacket *p) { +int dns_scope_emit_udp(DnsScope *s, int fd, DnsServer *server, DnsPacket *p) { int r; assert(s); @@ -405,11 +405,11 @@ static int dns_scope_socket(DnsScope *s, int type, int family, const union in_ad return ret; } -int dns_scope_udp_dns_socket(DnsScope *s, DnsServer **server) { +int dns_scope_socket_udp(DnsScope *s, DnsServer **server) { return dns_scope_socket(s, SOCK_DGRAM, AF_UNSPEC, NULL, 53, server); } -int dns_scope_tcp_socket(DnsScope *s, int family, const union in_addr_union *address, uint16_t port, DnsServer **server) { +int dns_scope_socket_tcp(DnsScope *s, int family, const union in_addr_union *address, uint16_t port, DnsServer **server) { return dns_scope_socket(s, SOCK_STREAM, family, address, port, server); } @@ -869,7 +869,7 @@ static int on_conflict_dispatch(sd_event_source *es, usec_t usec, void *userdata return 0; } - r = dns_scope_emit(scope, -1, NULL, p); + r = dns_scope_emit_udp(scope, -1, NULL, p); if (r < 0) log_debug_errno(r, "Failed to send conflict packet: %m"); } diff --git a/src/resolve/resolved-dns-scope.h b/src/resolve/resolved-dns-scope.h index 2fc2e07deb..177e0e32fa 100644 --- a/src/resolve/resolved-dns-scope.h +++ b/src/resolve/resolved-dns-scope.h @@ -82,9 +82,9 @@ DnsScope* dns_scope_free(DnsScope *s); void dns_scope_packet_received(DnsScope *s, usec_t rtt); void dns_scope_packet_lost(DnsScope *s, usec_t usec); -int dns_scope_emit(DnsScope *s, int fd, DnsServer *server, DnsPacket *p); -int dns_scope_tcp_socket(DnsScope *s, int family, const union in_addr_union *address, uint16_t port, DnsServer **server); -int dns_scope_udp_dns_socket(DnsScope *s, DnsServer **server); +int dns_scope_emit_udp(DnsScope *s, int fd, DnsServer *server, DnsPacket *p); +int dns_scope_socket_tcp(DnsScope *s, int family, const union in_addr_union *address, uint16_t port, DnsServer **server); +int dns_scope_socket_udp(DnsScope *s, DnsServer **server); DnsScopeMatch dns_scope_good_domain(DnsScope *s, int ifindex, uint64_t flags, const char *domain); int dns_scope_good_key(DnsScope *s, DnsResourceKey *key); diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 5933e0e462..70c170daee 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -349,13 +349,13 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { switch (t->scope->protocol) { case DNS_PROTOCOL_DNS: - fd = dns_scope_tcp_socket(t->scope, AF_UNSPEC, NULL, 53, &server); + fd = dns_scope_socket_tcp(t->scope, AF_UNSPEC, NULL, 53, &server); break; case DNS_PROTOCOL_LLMNR: /* When we already received a reply to this (but it was truncated), send to its sender address */ if (t->received) - fd = dns_scope_tcp_socket(t->scope, t->received->family, &t->received->sender, t->received->sender_port, NULL); + fd = dns_scope_socket_tcp(t->scope, t->received->family, &t->received->sender, t->received->sender_port, NULL); else { union in_addr_union address; int family = AF_UNSPEC; @@ -372,7 +372,7 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { if (family != t->scope->family) return -ESRCH; - fd = dns_scope_tcp_socket(t->scope, family, &address, LLMNR_PORT, NULL); + fd = dns_scope_socket_tcp(t->scope, family, &address, LLMNR_PORT, NULL); } break; @@ -717,7 +717,7 @@ static int on_dns_packet(sd_event_source *s, int fd, uint32_t revents, void *use return 0; } -static int dns_transaction_emit(DnsTransaction *t) { +static int dns_transaction_emit_udp(DnsTransaction *t) { int r; assert(t); @@ -726,7 +726,7 @@ static int dns_transaction_emit(DnsTransaction *t) { DnsServer *server = NULL; _cleanup_close_ int fd = -1; - fd = dns_scope_udp_dns_socket(t->scope, &server); + fd = dns_scope_socket_udp(t->scope, &server); if (fd < 0) return fd; @@ -739,7 +739,7 @@ static int dns_transaction_emit(DnsTransaction *t) { t->server = dns_server_ref(server); } - r = dns_scope_emit(t->scope, t->dns_udp_fd, t->server, t->sent); + r = dns_scope_emit_udp(t->scope, t->dns_udp_fd, t->server, t->sent); if (r < 0) return r; @@ -759,17 +759,17 @@ static int on_transaction_timeout(sd_event_source *s, usec_t usec, void *userdat if (!t->initial_jitter_scheduled || t->initial_jitter_elapsed) { /* Timeout reached? Increase the timeout for the server used */ switch (t->scope->protocol) { + case DNS_PROTOCOL_DNS: assert(t->server); - dns_server_packet_lost(t->server, t->current_features, usec - t->start_usec); - break; + case DNS_PROTOCOL_LLMNR: case DNS_PROTOCOL_MDNS: dns_scope_packet_lost(t->scope, usec - t->start_usec); - break; + default: assert_not_reached("Invalid DNS protocol."); } @@ -795,15 +795,18 @@ static usec_t transaction_get_resend_timeout(DnsTransaction *t) { assert(t->scope); switch (t->scope->protocol) { + case DNS_PROTOCOL_DNS: assert(t->server); - return t->server->resend_timeout; + case DNS_PROTOCOL_MDNS: assert(t->n_attempts > 0); return (1 << (t->n_attempts - 1)) * USEC_PER_SEC; + case DNS_PROTOCOL_LLMNR: return t->scope->resend_timeout; + default: assert_not_reached("Invalid DNS protocol."); } @@ -1117,7 +1120,7 @@ int dns_transaction_go(DnsTransaction *t) { } else { /* Try via UDP, and if that fails due to large size or lack of * support try via TCP */ - r = dns_transaction_emit(t); + r = dns_transaction_emit_udp(t); if (r == -EMSGSIZE || r == -EAGAIN) r = dns_transaction_open_tcp(t); } From 08f904fddc163ccf9524218cd900a89716ee4a63 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 12:36:24 +0100 Subject: [PATCH 28/41] resolved: don't unnecessarily allocate memory in dns_packet_append_name() When compression support is off, there's no point in duplicating the name string. Hence, don't do it. --- src/resolve/resolved-dns-packet.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 7908d0bcf5..c425b89034 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -499,7 +499,7 @@ int dns_packet_append_name( saved_size = p->size; while (*name) { - _cleanup_free_ char *s = NULL; + const char *z = name; char label[DNS_LABEL_MAX]; size_t n = 0; int k; @@ -518,12 +518,6 @@ int dns_packet_append_name( } } - s = strdup(name); - if (!s) { - r = -ENOMEM; - goto fail; - } - r = dns_label_unescape(&name, label, sizeof(label)); if (r < 0) goto fail; @@ -544,6 +538,14 @@ int dns_packet_append_name( goto fail; if (allow_compression) { + _cleanup_free_ char *s = NULL; + + s = strdup(z); + if (!s) { + r = -ENOMEM; + goto fail; + } + r = hashmap_ensure_allocated(&p->names, &dns_name_hash_ops); if (r < 0) goto fail; From 735323d9d3cddcd08247e541ccfdd8285c87c701 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 12:43:03 +0100 Subject: [PATCH 29/41] shared: fix error propagation in dns_name_compare_func() --- src/shared/dns-domain.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/shared/dns-domain.c b/src/shared/dns-domain.c index f44e80c9db..159944858d 100644 --- a/src/shared/dns-domain.c +++ b/src/shared/dns-domain.c @@ -533,7 +533,7 @@ int dns_name_compare_func(const void *a, const void *b) { if (k > 0) r = k; if (w > 0) - r = w; + q = w; la[r] = lb[q] = 0; r = strcasecmp(la, lb); From 5651285934ade033c0662afbbee940855f6bd2fb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 12:43:28 +0100 Subject: [PATCH 30/41] shared: fix handling of suffix "." in dns_name_compare_func() All our other domain name handling functions make no destinction between domain names that end in a dot plus a NUL, or those just ending in a NUL. Make sure dns_name_compare_func() and dns_label_unescape_suffix() do the same. --- src/shared/dns-domain.c | 18 +++++++++++------- src/test/test-dns-domain.c | 18 ++++++++++++++++-- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/shared/dns-domain.c b/src/shared/dns-domain.c index 159944858d..0273b9e3c9 100644 --- a/src/shared/dns-domain.c +++ b/src/shared/dns-domain.c @@ -154,20 +154,24 @@ int dns_label_unescape_suffix(const char *name, const char **label_terminal, cha return 0; } - assert(**label_terminal == '.' || **label_terminal == 0); + terminal = *label_terminal; + assert(*terminal == '.' || *terminal == 0); - /* skip current terminal character */ - terminal = *label_terminal - 1; + /* Skip current terminal character (and accept domain names ending it ".") */ + if (*terminal == 0) + terminal--; + if (terminal >= name && *terminal == '.') + terminal--; - /* point name to the last label, and terminal to the preceding terminal symbol (or make it a NULL pointer) */ + /* Point name to the last label, and terminal to the preceding terminal symbol (or make it a NULL pointer) */ for (;;) { if (terminal < name) { - /* reached the first label, so indicate that there are no more */ + /* Reached the first label, so indicate that there are no more */ terminal = NULL; break; } - /* find the start of the last label */ + /* Find the start of the last label */ if (*terminal == '.') { const char *y; unsigned slashes = 0; @@ -176,7 +180,7 @@ int dns_label_unescape_suffix(const char *name, const char **label_terminal, cha slashes ++; if (slashes % 2 == 0) { - /* the '.' was not escaped */ + /* The '.' was not escaped */ name = terminal + 1; break; } else { diff --git a/src/test/test-dns-domain.c b/src/test/test-dns-domain.c index 78c5631ff1..1b0cb153f7 100644 --- a/src/test/test-dns-domain.c +++ b/src/test/test-dns-domain.c @@ -140,9 +140,9 @@ static void test_dns_label_unescape_suffix(void) { test_dns_label_unescape_suffix_one("hallo\\", "hallo", "hallo", 20, -EINVAL, -EINVAL); test_dns_label_unescape_suffix_one("hallo\\032 ", "hallo ", "", 20, 7, 0); test_dns_label_unescape_suffix_one(".", "", "", 20, 0, 0); - test_dns_label_unescape_suffix_one("..", "", "", 20, 0, 0); + test_dns_label_unescape_suffix_one("..", "", "", 20, 0, -EINVAL); test_dns_label_unescape_suffix_one(".foobar", "foobar", "", 20, 6, -EINVAL); - test_dns_label_unescape_suffix_one("foobar.", "", "foobar", 20, 0, 6); + test_dns_label_unescape_suffix_one("foobar.", "foobar", "", 20, 6, 0); test_dns_label_unescape_suffix_one("foo\\\\bar", "foo\\bar", "", 20, 7, 0); test_dns_label_unescape_suffix_one("foo.bar", "bar", "foo", 20, 3, 3); test_dns_label_unescape_suffix_one("foo..bar", "bar", "", 20, 3, -EINVAL); @@ -546,6 +546,19 @@ static void test_dns_name_equal_skip(void) { test_dns_name_equal_skip_one("", 2, "foo", 0); } +static void test_dns_name_compare_func(void) { + assert_se(dns_name_compare_func("", "") == 0); + assert_se(dns_name_compare_func("", ".") == 0); + assert_se(dns_name_compare_func(".", "") == 0); + assert_se(dns_name_compare_func("foo", "foo.") == 0); + assert_se(dns_name_compare_func("foo.", "foo") == 0); + assert_se(dns_name_compare_func("foo", "foo") == 0); + assert_se(dns_name_compare_func("foo.", "foo.") == 0); + assert_se(dns_name_compare_func("heise.de", "HEISE.DE.") == 0); + + assert_se(dns_name_compare_func("de.", "heise.de") != 0); +} + int main(int argc, char *argv[]) { test_dns_label_unescape(); @@ -569,6 +582,7 @@ int main(int argc, char *argv[]) { test_dns_name_suffix(); test_dns_name_count_labels(); test_dns_name_equal_skip(); + test_dns_name_compare_func(); return 0; } From c61d2b441a93c56847afe26fb7a2735cdb32ec46 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 12:53:08 +0100 Subject: [PATCH 31/41] resolved: introduce dns_transaction_reset_answer() Let's unify how we reset the answer data we collected, after all pretty much every time we do it incompletely so far, let's fix it. --- src/resolve/resolved-dns-transaction.c | 27 ++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 70c170daee..94b540df0d 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -29,6 +29,17 @@ #include "resolved-llmnr.h" #include "string-table.h" +static void dns_transaction_reset_answer(DnsTransaction *t) { + assert(t); + + t->received = dns_packet_unref(t->received); + t->answer = dns_answer_unref(t->answer); + t->answer_rcode = 0; + t->answer_dnssec_result = _DNSSEC_RESULT_INVALID; + t->answer_source = _DNS_TRANSACTION_SOURCE_INVALID; + t->answer_authenticated = false; +} + DnsTransaction* dns_transaction_free(DnsTransaction *t) { DnsQueryCandidate *c; DnsZoneItem *i; @@ -40,9 +51,7 @@ DnsTransaction* dns_transaction_free(DnsTransaction *t) { sd_event_source_unref(t->timeout_event_source); dns_packet_unref(t->sent); - dns_packet_unref(t->received); - - dns_answer_unref(t->answer); + dns_transaction_reset_answer(t); sd_event_source_unref(t->dns_udp_event_source); safe_close(t->dns_udp_fd); @@ -398,9 +407,9 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { dns_server_unref(t->server); t->server = dns_server_ref(server); - t->received = dns_packet_unref(t->received); - t->answer = dns_answer_unref(t->answer); - t->answer_rcode = 0; + + dns_transaction_reset_answer(t); + t->stream->complete = on_stream_complete; t->stream->transaction = t; @@ -836,10 +845,8 @@ static int dns_transaction_prepare(DnsTransaction *t, usec_t ts) { t->n_attempts++; t->start_usec = ts; - t->received = dns_packet_unref(t->received); - t->answer = dns_answer_unref(t->answer); - t->answer_rcode = 0; - t->answer_source = _DNS_TRANSACTION_SOURCE_INVALID; + + dns_transaction_reset_answer(t); /* Check the trust anchor. Do so only on classic DNS, since DNSSEC does not apply otherwise. */ if (t->scope->protocol == DNS_PROTOCOL_DNS) { From 5a7e41a370e39f68707d3c2ee9cc60d8c0bd33da Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 12:58:01 +0100 Subject: [PATCH 32/41] resolved: ignore additional DNS responses we get while validating No need to choke on them. --- src/resolve/resolved-dns-transaction.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 94b540df0d..1ea8fa44f7 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -518,10 +518,12 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) { assert(t); assert(p); - assert(t->state == DNS_TRANSACTION_PENDING); assert(t->scope); assert(t->scope->manager); + if (t->state != DNS_TRANSACTION_PENDING) + return; + /* Note that this call might invalidate the query. Callers * should hence not attempt to access the query or transaction * after calling this function. */ From 598f44bd2c3b6143480358035643b98fcca353ed Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 14:15:51 +0100 Subject: [PATCH 33/41] resolved: make sure we GC stream transactions properly Make sure to GC a transaction after dealing with a reply, even if the transaction is not complete yet. --- src/resolve/resolved-dns-transaction.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 1ea8fa44f7..b0f40303d9 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -342,6 +342,8 @@ static int on_stream_complete(DnsStream *s, int error) { /* If the response wasn't useful, then complete the transition now */ if (t->state == DNS_TRANSACTION_PENDING) dns_transaction_complete(t, DNS_TRANSACTION_INVALID_REPLY); + else + dns_transaction_gc(t); return 0; } From cbe4216dd1b76f26460c553aefeeebf29bce221c Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 14:18:11 +0100 Subject: [PATCH 34/41] resolved: remember explicitly whether we already tried a stream connection On LLMNR we never want to retry stream connections (since local TCP connections should work, and we don't want to unnecessarily delay operation), explicitly remember whether we already tried one, instead of deriving this from a still stored stream object. This way, we can free the stream early, without forgetting that we tried it. --- src/resolve/resolved-dns-transaction.c | 7 +++---- src/resolve/resolved-dns-transaction.h | 2 ++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index b0f40303d9..2eb1675c5b 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -421,6 +421,8 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { if (t->scope->link) t->stream->ifindex = t->scope->link->ifindex; + t->tried_stream = true; + return 0; } @@ -826,13 +828,10 @@ static usec_t transaction_get_resend_timeout(DnsTransaction *t) { } static int dns_transaction_prepare(DnsTransaction *t, usec_t ts) { - bool had_stream; int r; assert(t); - had_stream = !!t->stream; - dns_transaction_stop(t); if (t->n_attempts >= TRANSACTION_ATTEMPTS_MAX(t->scope->protocol)) { @@ -840,7 +839,7 @@ static int dns_transaction_prepare(DnsTransaction *t, usec_t ts) { return 0; } - if (t->scope->protocol == DNS_PROTOCOL_LLMNR && had_stream) { + if (t->scope->protocol == DNS_PROTOCOL_LLMNR && t->tried_stream) { /* If we already tried via a stream, then we don't * retry on LLMNR. See RFC 4795, Section 2.7. */ dns_transaction_complete(t, DNS_TRANSACTION_ATTEMPTS_MAX_REACHED); diff --git a/src/resolve/resolved-dns-transaction.h b/src/resolve/resolved-dns-transaction.h index eebb6ec631..a351a5043b 100644 --- a/src/resolve/resolved-dns-transaction.h +++ b/src/resolve/resolved-dns-transaction.h @@ -69,6 +69,8 @@ struct DnsTransaction { uint16_t id; + bool tried_stream:1; + bool initial_jitter_scheduled:1; bool initial_jitter_elapsed:1; From 2a6658ef553db3ee62528adbeccfb9f40cbe7173 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 14:37:07 +0100 Subject: [PATCH 35/41] resolved: never proceed processing truncated packets Make sure we don't end up processing packets that are truncated. Instead, actually let the TCP connection do its thing. --- src/resolve/resolved-dns-transaction.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 2eb1675c5b..5eb569d49d 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -664,9 +664,9 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) { dns_transaction_complete(t, DNS_TRANSACTION_RESOURCES); return; } - - return; } + + return; } /* Parse message, if it isn't parsed yet. */ From 2c6bf498b6ce50ecc770080fbe31bdde13995e57 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 14:38:37 +0100 Subject: [PATCH 36/41] resolved: improve some log messages a bit Indicate thar we ignore invalid messages --- src/resolve/resolved-dns-transaction.c | 2 +- src/resolve/resolved-llmnr.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 5eb569d49d..e772004106 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -727,7 +727,7 @@ static int on_dns_packet(sd_event_source *s, int fd, uint32_t revents, void *use DNS_PACKET_ID(p) == t->id) dns_transaction_process_reply(t, p); else - log_debug("Invalid DNS packet, ignoring."); + log_debug("Invalid DNS UDP packet, ignoring."); return 0; } diff --git a/src/resolve/resolved-llmnr.c b/src/resolve/resolved-llmnr.c index ed754c3899..182c6e45ea 100644 --- a/src/resolve/resolved-llmnr.c +++ b/src/resolve/resolved-llmnr.c @@ -117,7 +117,7 @@ static int on_llmnr_packet(sd_event_source *s, int fd, uint32_t revents, void *u dns_scope_process_query(scope, NULL, p); } else - log_debug("Invalid LLMNR UDP packet."); + log_debug("Invalid LLMNR UDP packet, ignoring."); return 0; } From 919c2ae05c829de6a2a478341c7133ccb5f594f1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 14:39:49 +0100 Subject: [PATCH 37/41] resolved: make sure we reset the DNSSEC result when we accept a response packet --- src/resolve/resolved-dns-transaction.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index e772004106..ba1f8cc7af 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -693,6 +693,7 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) { dns_answer_unref(t->answer); t->answer = dns_answer_ref(p->answer); t->answer_rcode = DNS_PACKET_RCODE(p); + t->answer_dnssec_result = _DNSSEC_RESULT_INVALID; t->answer_authenticated = false; r = dns_transaction_request_dnssec_keys(t); From f32f0e57ca117455fb24ca72238c4958cd800b28 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 14:53:17 +0100 Subject: [PATCH 38/41] resolved: add dns_transaction_close_connection() This new call unifies how we shut down all connection resources, such as UDP sockets, event sources, and TCP stream objects. This patch just adds the basic hook-up, this function will be used more in later commits. --- src/resolve/resolved-dns-transaction.c | 14 ++++++++++---- src/resolve/resolved-dns-transaction.h | 7 ++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index ba1f8cc7af..cf7dae0e69 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -40,6 +40,14 @@ static void dns_transaction_reset_answer(DnsTransaction *t) { t->answer_authenticated = false; } +static void dns_transaction_close_connection(DnsTransaction *t) { + assert(t); + + t->stream = dns_stream_free(t->stream); + t->dns_udp_event_source = sd_event_source_unref(t->dns_udp_event_source); + t->dns_udp_fd = safe_close(t->dns_udp_fd); +} + DnsTransaction* dns_transaction_free(DnsTransaction *t) { DnsQueryCandidate *c; DnsZoneItem *i; @@ -49,15 +57,12 @@ DnsTransaction* dns_transaction_free(DnsTransaction *t) { return NULL; sd_event_source_unref(t->timeout_event_source); + dns_transaction_close_connection(t); dns_packet_unref(t->sent); dns_transaction_reset_answer(t); - sd_event_source_unref(t->dns_udp_event_source); - safe_close(t->dns_udp_fd); - dns_server_unref(t->server); - dns_stream_free(t->stream); if (t->scope) { hashmap_remove_value(t->scope->transactions_by_key, t->key, t); @@ -259,6 +264,7 @@ void dns_transaction_complete(DnsTransaction *t, DnsTransactionState state) { t->state = state; + dns_transaction_close_connection(t); dns_transaction_stop(t); /* Notify all queries that are interested, but make sure the diff --git a/src/resolve/resolved-dns-transaction.h b/src/resolve/resolved-dns-transaction.h index a351a5043b..faf3ce6fb9 100644 --- a/src/resolve/resolved-dns-transaction.h +++ b/src/resolve/resolved-dns-transaction.h @@ -100,18 +100,19 @@ struct DnsTransaction { sd_event_source *timeout_event_source; unsigned n_attempts; + /* UDP connection logic, if we need it */ int dns_udp_fd; sd_event_source *dns_udp_event_source; + /* TCP connection logic, if we need it */ + DnsStream *stream; + /* The active server */ DnsServer *server; /* The features of the DNS server at time of transaction start */ DnsServerFeatureLevel current_features; - /* TCP connection logic, if we need it */ - DnsStream *stream; - /* Query candidates this transaction is referenced by and that * shall be notified about this specific transaction * completing. */ From 97cc656cf40e104c0305bde71ec6e835650ac3cb Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 18:48:37 +0100 Subject: [PATCH 39/41] resolved: reuse dns_transaction_stop() when destructing transaction objects --- src/resolve/resolved-dns-transaction.c | 27 +++++++++++++------------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index cf7dae0e69..e708559137 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -48,6 +48,16 @@ static void dns_transaction_close_connection(DnsTransaction *t) { t->dns_udp_fd = safe_close(t->dns_udp_fd); } +static void dns_transaction_stop(DnsTransaction *t) { + assert(t); + + t->timeout_event_source = sd_event_source_unref(t->timeout_event_source); + t->stream = dns_stream_free(t->stream); + + /* Note that we do not drop the UDP socket here, as we want to + * reuse it to repeat the interaction. */ +} + DnsTransaction* dns_transaction_free(DnsTransaction *t) { DnsQueryCandidate *c; DnsZoneItem *i; @@ -56,8 +66,8 @@ DnsTransaction* dns_transaction_free(DnsTransaction *t) { if (!t) return NULL; - sd_event_source_unref(t->timeout_event_source); dns_transaction_close_connection(t); + dns_transaction_stop(t); dns_packet_unref(t->sent); dns_transaction_reset_answer(t); @@ -72,8 +82,6 @@ DnsTransaction* dns_transaction_free(DnsTransaction *t) { hashmap_remove(t->scope->manager->dns_transactions, UINT_TO_PTR(t->id)); } - dns_resource_key_unref(t->key); - while ((c = set_steal_first(t->notify_query_candidates))) set_remove(c->transactions, t); set_free(t->notify_query_candidates); @@ -93,8 +101,9 @@ DnsTransaction* dns_transaction_free(DnsTransaction *t) { set_free(t->dnssec_transactions); dns_answer_unref(t->validated_keys); - + dns_resource_key_unref(t->key); free(t->key_string); + free(t); return NULL; } @@ -177,16 +186,6 @@ int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsResourceKey *key) return 0; } -static void dns_transaction_stop(DnsTransaction *t) { - assert(t); - - t->timeout_event_source = sd_event_source_unref(t->timeout_event_source); - t->stream = dns_stream_free(t->stream); - - /* Note that we do not drop the UDP socket here, as we want to - * reuse it to repeat the interaction. */ -} - static void dns_transaction_tentative(DnsTransaction *t, DnsPacket *p) { _cleanup_free_ char *pretty = NULL; DnsZoneItem *z; From 519ef04651b07a547f010d6462603669d7fde4e5 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sat, 26 Dec 2015 18:49:32 +0100 Subject: [PATCH 40/41] resolved: rework OPT RR generation logic This moves management of the OPT RR out of the scope management and into the server and packet management. There are now explicit calls for appending and truncating the OPT RR from a packet (dns_packet_append_opt() and dns_packet_truncate_opt()) as well as a call to do the right thing depending on a DnsServer's feature level (dns_server_adjust_opt()). This also unifies the code to pick a server between the TCP and UDP code paths, and makes sure the feature level used for the transaction is selected at the time the server is picked, and not changed until the next time we pick a server. The server selction code is now unified in dns_transaction_pick_server(). This all fixes problems when changing between UDP and TCP communication for the same server, and makes sure the UDP and TCP codepaths are more alike. It also makes sure we never keep the UDP port open when switchung to TCP, so that we don't have to handle incoming datagrams on the latter we don't expect. As the new code picks the DNS server at the time we make a connection, we don't need to invalidate the DNS server anymore when changing to the next one, thus dns_transaction_next_dns_server() has been removed. --- src/resolve/resolved-dns-packet.c | 35 +++++++- src/resolve/resolved-dns-packet.h | 4 +- src/resolve/resolved-dns-scope.c | 93 ++++++++------------ src/resolve/resolved-dns-scope.h | 6 +- src/resolve/resolved-dns-server.c | 28 ++++++ src/resolve/resolved-dns-server.h | 2 + src/resolve/resolved-dns-transaction.c | 115 ++++++++++++++++--------- 7 files changed, 178 insertions(+), 105 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index c425b89034..5f79701296 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -58,6 +58,7 @@ int dns_packet_new(DnsPacket **ret, DnsProtocol protocol, size_t mtu) { p->size = p->rindex = DNS_PACKET_HEADER_SIZE; p->allocated = a; p->protocol = protocol; + p->opt_start = p->opt_size = (size_t) -1; p->n_ref = 1; *ret = p; @@ -681,7 +682,7 @@ fail: } /* Append the OPT pseudo-RR described in RFC6891 */ -int dns_packet_append_opt_rr(DnsPacket *p, uint16_t max_udp_size, bool edns0_do, size_t *start) { +int dns_packet_append_opt(DnsPacket *p, uint16_t max_udp_size, bool edns0_do, size_t *start) { size_t saved_size; int r; @@ -689,6 +690,11 @@ int dns_packet_append_opt_rr(DnsPacket *p, uint16_t max_udp_size, bool edns0_do, /* we must never advertise supported packet size smaller than the legacy max */ assert(max_udp_size >= DNS_PACKET_UNICAST_SIZE_MAX); + if (p->opt_start != (size_t) -1) + return -EBUSY; + + assert(p->opt_size == (size_t) -1); + saved_size = p->size; /* empty name */ @@ -721,6 +727,11 @@ int dns_packet_append_opt_rr(DnsPacket *p, uint16_t max_udp_size, bool edns0_do, if (r < 0) goto fail; + DNS_PACKET_HEADER(p)->arcount = htobe16(DNS_PACKET_ARCOUNT(p) + 1); + + p->opt_start = saved_size; + p->opt_size = p->size - saved_size; + if (start) *start = saved_size; @@ -731,6 +742,27 @@ fail: return r; } +int dns_packet_truncate_opt(DnsPacket *p) { + assert(p); + + if (p->opt_start == (size_t) -1) { + assert(p->opt_size == (size_t) -1); + return 0; + } + + assert(p->opt_size != (size_t) -1); + assert(DNS_PACKET_ARCOUNT(p) > 0); + + if (p->opt_start + p->opt_size != p->size) + return -EBUSY; + + dns_packet_truncate(p, p->opt_start); + DNS_PACKET_HEADER(p)->arcount = htobe16(DNS_PACKET_ARCOUNT(p) - 1); + p->opt_start = p->opt_size = (size_t) -1; + + return 1; +} + int dns_packet_append_rr(DnsPacket *p, const DnsResourceRecord *rr, size_t *start, size_t *rdata_start) { size_t saved_size, rdlength_offset, end, rdlength, rds; int r; @@ -1046,7 +1078,6 @@ fail: return r; } - int dns_packet_read(DnsPacket *p, size_t sz, const void **ret, size_t *start) { assert(p); diff --git a/src/resolve/resolved-dns-packet.h b/src/resolve/resolved-dns-packet.h index 36da86dee5..a09ace5b75 100644 --- a/src/resolve/resolved-dns-packet.h +++ b/src/resolve/resolved-dns-packet.h @@ -76,6 +76,7 @@ struct DnsPacket { size_t size, allocated, rindex; void *_data; /* don't access directly, use DNS_PACKET_DATA()! */ Hashmap *names; /* For name compression */ + size_t opt_start, opt_size; /* Parsed data */ DnsQuestion *question; @@ -173,9 +174,10 @@ int dns_packet_append_label(DnsPacket *p, const char *s, size_t l, bool canonica int dns_packet_append_name(DnsPacket *p, const char *name, bool allow_compression, bool canonical_candidate, size_t *start); int dns_packet_append_key(DnsPacket *p, const DnsResourceKey *key, size_t *start); int dns_packet_append_rr(DnsPacket *p, const DnsResourceRecord *rr, size_t *start, size_t *rdata_start); -int dns_packet_append_opt_rr(DnsPacket *p, uint16_t max_udp_size, bool edns0_do, size_t *start); +int dns_packet_append_opt(DnsPacket *p, uint16_t max_udp_size, bool edns0_do, size_t *start); void dns_packet_truncate(DnsPacket *p, size_t sz); +int dns_packet_truncate_opt(DnsPacket *p); int dns_packet_read(DnsPacket *p, size_t sz, const void **ret, size_t *start); int dns_packet_read_blob(DnsPacket *p, void *d, size_t sz, size_t *start); diff --git a/src/resolve/resolved-dns-scope.c b/src/resolve/resolved-dns-scope.c index b29f15d2c0..13be2a3792 100644 --- a/src/resolve/resolved-dns-scope.c +++ b/src/resolve/resolved-dns-scope.c @@ -162,17 +162,15 @@ void dns_scope_packet_lost(DnsScope *s, usec_t usec) { s->resend_timeout = MIN(s->resend_timeout * 2, MULTICAST_RESEND_TIMEOUT_MAX_USEC); } -static int dns_scope_emit_one(DnsScope *s, int fd, DnsServer *server, DnsPacket *p) { +static int dns_scope_emit_one(DnsScope *s, int fd, DnsPacket *p) { union in_addr_union addr; int ifindex = 0, r; int family; uint32_t mtu; - size_t saved_size = 0; assert(s); assert(p); assert(p->protocol == s->protocol); - assert((s->protocol == DNS_PROTOCOL_DNS) != (fd < 0)); if (s->link) { mtu = s->link->mtu; @@ -181,30 +179,13 @@ static int dns_scope_emit_one(DnsScope *s, int fd, DnsServer *server, DnsPacket mtu = manager_find_mtu(s->manager); switch (s->protocol) { + case DNS_PROTOCOL_DNS: - assert(server); + assert(fd >= 0); if (DNS_PACKET_QDCOUNT(p) > 1) return -EOPNOTSUPP; - if (server->possible_features >= DNS_SERVER_FEATURE_LEVEL_EDNS0) { - bool edns_do; - size_t packet_size; - - edns_do = server->possible_features >= DNS_SERVER_FEATURE_LEVEL_DO; - - if (server->possible_features >= DNS_SERVER_FEATURE_LEVEL_LARGE) - packet_size = DNS_PACKET_UNICAST_SIZE_LARGE_MAX; - else - packet_size = server->received_udp_packet_max; - - r = dns_packet_append_opt_rr(p, packet_size, edns_do, &saved_size); - if (r < 0) - return r; - - DNS_PACKET_HEADER(p)->arcount = htobe16(be16toh(DNS_PACKET_HEADER(p)->arcount) + 1); - } - if (p->size > DNS_PACKET_UNICAST_SIZE_MAX) return -EMSGSIZE; @@ -215,15 +196,11 @@ static int dns_scope_emit_one(DnsScope *s, int fd, DnsServer *server, DnsPacket if (r < 0) return r; - if (saved_size > 0) { - dns_packet_truncate(p, saved_size); - - DNS_PACKET_HEADER(p)->arcount = htobe16(be16toh(DNS_PACKET_HEADER(p)->arcount) - 1); - } - break; case DNS_PROTOCOL_LLMNR: + assert(fd < 0); + if (DNS_PACKET_QDCOUNT(p) > 1) return -EOPNOTSUPP; @@ -250,6 +227,8 @@ static int dns_scope_emit_one(DnsScope *s, int fd, DnsServer *server, DnsPacket break; case DNS_PROTOCOL_MDNS: + assert(fd < 0); + if (!ratelimit_test(&s->ratelimit)) return -EBUSY; @@ -279,13 +258,13 @@ static int dns_scope_emit_one(DnsScope *s, int fd, DnsServer *server, DnsPacket return 1; } -int dns_scope_emit_udp(DnsScope *s, int fd, DnsServer *server, DnsPacket *p) { +int dns_scope_emit_udp(DnsScope *s, int fd, DnsPacket *p) { int r; assert(s); assert(p); assert(p->protocol == s->protocol); - assert((s->protocol == DNS_PROTOCOL_DNS) != (fd < 0)); + assert((s->protocol == DNS_PROTOCOL_DNS) == (fd >= 0)); do { /* If there are multiple linked packets, set the TC bit in all but the last of them */ @@ -294,18 +273,24 @@ int dns_scope_emit_udp(DnsScope *s, int fd, DnsServer *server, DnsPacket *p) { dns_packet_set_flags(p, true, true); } - r = dns_scope_emit_one(s, fd, server, p); + r = dns_scope_emit_one(s, fd, p); if (r < 0) return r; p = p->more; - } while(p); + } while (p); return 0; } -static int dns_scope_socket(DnsScope *s, int type, int family, const union in_addr_union *address, uint16_t port, DnsServer **server) { - DnsServer *srv = NULL; +static int dns_scope_socket( + DnsScope *s, + int type, + int family, + const union in_addr_union *address, + DnsServer *server, + uint16_t port) { + _cleanup_close_ int fd = -1; union sockaddr_union sa = {}; socklen_t salen; @@ -313,32 +298,27 @@ static int dns_scope_socket(DnsScope *s, int type, int family, const union in_ad int ret, r; assert(s); - assert((family == AF_UNSPEC) == !address); - if (family == AF_UNSPEC) { - srv = dns_scope_get_dns_server(s); - if (!srv) - return -ESRCH; + if (server) { + assert(family == AF_UNSPEC); + assert(!address); - /* Determine current feature level to use */ - (void) dns_server_possible_features(srv); - - if (type == SOCK_DGRAM && srv->possible_features < DNS_SERVER_FEATURE_LEVEL_UDP) - return -EAGAIN; - - sa.sa.sa_family = srv->family; - if (srv->family == AF_INET) { + sa.sa.sa_family = server->family; + if (server->family == AF_INET) { sa.in.sin_port = htobe16(port); - sa.in.sin_addr = srv->address.in; + sa.in.sin_addr = server->address.in; salen = sizeof(sa.in); - } else if (srv->family == AF_INET6) { + } else if (server->family == AF_INET6) { sa.in6.sin6_port = htobe16(port); - sa.in6.sin6_addr = srv->address.in6; + sa.in6.sin6_addr = server->address.in6; sa.in6.sin6_scope_id = s->link ? s->link->ifindex : 0; salen = sizeof(sa.in6); } else return -EAFNOSUPPORT; } else { + assert(family != AF_UNSPEC); + assert(address); + sa.sa.sa_family = family; if (family == AF_INET) { @@ -396,21 +376,18 @@ static int dns_scope_socket(DnsScope *s, int type, int family, const union in_ad if (r < 0 && errno != EINPROGRESS) return -errno; - if (server) - *server = srv; - ret = fd; fd = -1; return ret; } -int dns_scope_socket_udp(DnsScope *s, DnsServer **server) { - return dns_scope_socket(s, SOCK_DGRAM, AF_UNSPEC, NULL, 53, server); +int dns_scope_socket_udp(DnsScope *s, DnsServer *server, uint16_t port) { + return dns_scope_socket(s, SOCK_DGRAM, AF_UNSPEC, NULL, server, port); } -int dns_scope_socket_tcp(DnsScope *s, int family, const union in_addr_union *address, uint16_t port, DnsServer **server) { - return dns_scope_socket(s, SOCK_STREAM, family, address, port, server); +int dns_scope_socket_tcp(DnsScope *s, int family, const union in_addr_union *address, DnsServer *server, uint16_t port) { + return dns_scope_socket(s, SOCK_STREAM, family, address, server, port); } DnsScopeMatch dns_scope_good_domain(DnsScope *s, int ifindex, uint64_t flags, const char *domain) { @@ -869,7 +846,7 @@ static int on_conflict_dispatch(sd_event_source *es, usec_t usec, void *userdata return 0; } - r = dns_scope_emit_udp(scope, -1, NULL, p); + r = dns_scope_emit_udp(scope, -1, p); if (r < 0) log_debug_errno(r, "Failed to send conflict packet: %m"); } diff --git a/src/resolve/resolved-dns-scope.h b/src/resolve/resolved-dns-scope.h index 177e0e32fa..a0676bd30e 100644 --- a/src/resolve/resolved-dns-scope.h +++ b/src/resolve/resolved-dns-scope.h @@ -82,9 +82,9 @@ DnsScope* dns_scope_free(DnsScope *s); void dns_scope_packet_received(DnsScope *s, usec_t rtt); void dns_scope_packet_lost(DnsScope *s, usec_t usec); -int dns_scope_emit_udp(DnsScope *s, int fd, DnsServer *server, DnsPacket *p); -int dns_scope_socket_tcp(DnsScope *s, int family, const union in_addr_union *address, uint16_t port, DnsServer **server); -int dns_scope_socket_udp(DnsScope *s, DnsServer **server); +int dns_scope_emit_udp(DnsScope *s, int fd, DnsPacket *p); +int dns_scope_socket_tcp(DnsScope *s, int family, const union in_addr_union *address, DnsServer *server, uint16_t port); +int dns_scope_socket_udp(DnsScope *s, DnsServer *server, uint16_t port); DnsScopeMatch dns_scope_good_domain(DnsScope *s, int ifindex, uint64_t flags, const char *domain); int dns_scope_good_key(DnsScope *s, DnsResourceKey *key); diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c index b0db5bbb16..e6e4feeedd 100644 --- a/src/resolve/resolved-dns-server.c +++ b/src/resolve/resolved-dns-server.c @@ -342,6 +342,34 @@ DnsServerFeatureLevel dns_server_possible_features(DnsServer *s) { return s->possible_features; } +int dns_server_adjust_opt(DnsServer *server, DnsPacket *packet, DnsServerFeatureLevel level) { + size_t packet_size; + bool edns_do; + int r; + + assert(server); + assert(packet); + assert(packet->protocol == DNS_PROTOCOL_DNS); + + /* Fix the OPT field in the packet to match our current feature level. */ + + r = dns_packet_truncate_opt(packet); + if (r < 0) + return r; + + if (level < DNS_SERVER_FEATURE_LEVEL_EDNS0) + return 0; + + edns_do = level >= DNS_SERVER_FEATURE_LEVEL_DO; + + if (level >= DNS_SERVER_FEATURE_LEVEL_LARGE) + packet_size = DNS_PACKET_UNICAST_SIZE_LARGE_MAX; + else + packet_size = server->received_udp_packet_max; + + return dns_packet_append_opt(packet, packet_size, edns_do, NULL); +} + static void dns_server_hash_func(const void *p, struct siphash *state) { const DnsServer *s = p; diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h index 3011904bfd..638f29548f 100644 --- a/src/resolve/resolved-dns-server.h +++ b/src/resolve/resolved-dns-server.h @@ -104,6 +104,8 @@ void dns_server_packet_lost(DnsServer *s, DnsServerFeatureLevel features, usec_t void dns_server_packet_failed(DnsServer *s, DnsServerFeatureLevel features); void dns_server_packet_rrsig_missing(DnsServer *s); +int dns_server_adjust_opt(DnsServer *server, DnsPacket *packet, DnsServerFeatureLevel level); + DnsServer *dns_server_find(DnsServer *first, int family, const union in_addr_union *in_addr); void dns_server_unlink_all(DnsServer *first); diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index e708559137..97aceae381 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -308,6 +308,27 @@ void dns_transaction_complete(DnsTransaction *t, DnsTransactionState state) { dns_transaction_gc(t); } +static int dns_transaction_pick_server(DnsTransaction *t) { + DnsServer *server; + + assert(t); + assert(t->scope->protocol == DNS_PROTOCOL_DNS); + + server = dns_scope_get_dns_server(t->scope); + if (!server) + return -ESRCH; + + t->current_features = dns_server_possible_features(server); + + if (server == t->server) + return 0; + + dns_server_unref(t->server); + t->server = dns_server_ref(server); + + return 1; +} + static int on_stream_complete(DnsStream *s, int error) { _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL; DnsTransaction *t; @@ -344,7 +365,9 @@ static int on_stream_complete(DnsStream *s, int error) { dns_transaction_process_reply(t, p); t->block_gc--; - /* If the response wasn't useful, then complete the transition now */ + /* If the response wasn't useful, then complete the transition + * now. After all, we are the worst feature set now with TCP + * sockets, and there's really no point in retrying. */ if (t->state == DNS_TRANSACTION_PENDING) dns_transaction_complete(t, DNS_TRANSACTION_INVALID_REPLY); else @@ -354,24 +377,31 @@ static int on_stream_complete(DnsStream *s, int error) { } static int dns_transaction_open_tcp(DnsTransaction *t) { - DnsServer *server = NULL; _cleanup_close_ int fd = -1; int r; assert(t); - if (t->stream) - return 0; + dns_transaction_close_connection(t); switch (t->scope->protocol) { + case DNS_PROTOCOL_DNS: - fd = dns_scope_socket_tcp(t->scope, AF_UNSPEC, NULL, 53, &server); + r = dns_transaction_pick_server(t); + if (r < 0) + return r; + + r = dns_server_adjust_opt(t->server, t->sent, t->current_features); + if (r < 0) + return r; + + fd = dns_scope_socket_tcp(t->scope, AF_UNSPEC, NULL, t->server, 53); break; case DNS_PROTOCOL_LLMNR: /* When we already received a reply to this (but it was truncated), send to its sender address */ if (t->received) - fd = dns_scope_socket_tcp(t->scope, t->received->family, &t->received->sender, t->received->sender_port, NULL); + fd = dns_scope_socket_tcp(t->scope, t->received->family, &t->received->sender, NULL, t->received->sender_port); else { union in_addr_union address; int family = AF_UNSPEC; @@ -388,7 +418,7 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { if (family != t->scope->family) return -ESRCH; - fd = dns_scope_socket_tcp(t->scope, family, &address, LLMNR_PORT, NULL); + fd = dns_scope_socket_tcp(t->scope, family, &address, NULL, LLMNR_PORT); } break; @@ -403,7 +433,6 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { r = dns_stream_new(t->scope->manager, &t->stream, t->scope->protocol, fd); if (r < 0) return r; - fd = -1; r = dns_stream_write_packet(t->stream, t->sent); @@ -412,11 +441,6 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { return r; } - dns_server_unref(t->server); - t->server = dns_server_ref(server); - - dns_transaction_reset_answer(t); - t->stream->complete = on_stream_complete; t->stream->transaction = t; @@ -426,21 +450,13 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { if (t->scope->link) t->stream->ifindex = t->scope->link->ifindex; + dns_transaction_reset_answer(t); + t->tried_stream = true; return 0; } -static void dns_transaction_next_dns_server(DnsTransaction *t) { - assert(t); - - t->server = dns_server_unref(t->server); - t->dns_udp_event_source = sd_event_source_unref(t->dns_udp_event_source); - t->dns_udp_fd = safe_close(t->dns_udp_fd); - - dns_scope_next_dns_server(t->scope); -} - static void dns_transaction_cache_answer(DnsTransaction *t) { assert(t); @@ -662,7 +678,7 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) { } /* On DNS, couldn't send? Try immediately again, with a new server */ - dns_transaction_next_dns_server(t); + dns_scope_next_dns_server(t->scope); r = dns_transaction_go(t); if (r < 0) { @@ -743,29 +759,44 @@ static int dns_transaction_emit_udp(DnsTransaction *t) { assert(t); - if (t->scope->protocol == DNS_PROTOCOL_DNS && !t->server) { - DnsServer *server = NULL; - _cleanup_close_ int fd = -1; + if (t->scope->protocol == DNS_PROTOCOL_DNS) { - fd = dns_scope_socket_udp(t->scope, &server); - if (fd < 0) - return fd; - - r = sd_event_add_io(t->scope->manager->event, &t->dns_udp_event_source, fd, EPOLLIN, on_dns_packet, t); + r = dns_transaction_pick_server(t); if (r < 0) return r; - t->dns_udp_fd = fd; - fd = -1; - t->server = dns_server_ref(server); - } + if (t->current_features < DNS_SERVER_FEATURE_LEVEL_UDP) + return -EAGAIN; - r = dns_scope_emit_udp(t->scope, t->dns_udp_fd, t->server, t->sent); + if (r > 0 || t->dns_udp_fd < 0) { /* Server changed, or no connection yet. */ + int fd; + + dns_transaction_close_connection(t); + + fd = dns_scope_socket_udp(t->scope, t->server, 53); + if (fd < 0) + return fd; + + r = sd_event_add_io(t->scope->manager->event, &t->dns_udp_event_source, fd, EPOLLIN, on_dns_packet, t); + if (r < 0) { + safe_close(fd); + return r; + } + + t->dns_udp_fd = fd; + } + + r = dns_server_adjust_opt(t->server, t->sent, t->current_features); + if (r < 0) + return r; + } else + dns_transaction_close_connection(t); + + r = dns_scope_emit_udp(t->scope, t->dns_udp_fd, t->sent); if (r < 0) return r; - if (t->server) - t->current_features = t->server->possible_features; + dns_transaction_reset_answer(t); return 0; } @@ -802,7 +833,7 @@ static int on_transaction_timeout(sd_event_source *s, usec_t usec, void *userdat log_debug("Timeout reached on transaction %" PRIu16 ".", t->id); /* ...and try again with a new server */ - dns_transaction_next_dns_server(t); + dns_scope_next_dns_server(t->scope); r = dns_transaction_go(t); if (r < 0) @@ -1084,10 +1115,12 @@ int dns_transaction_go(DnsTransaction *t) { random_bytes(&jitter, sizeof(jitter)); switch (t->scope->protocol) { + case DNS_PROTOCOL_LLMNR: jitter %= LLMNR_JITTER_INTERVAL_USEC; accuracy = LLMNR_JITTER_INTERVAL_USEC; break; + case DNS_PROTOCOL_MDNS: jitter %= MDNS_JITTER_RANGE_USEC; jitter += MDNS_JITTER_MIN_USEC; @@ -1152,7 +1185,7 @@ int dns_transaction_go(DnsTransaction *t) { } /* Couldn't send? Try immediately again, with a new server */ - dns_transaction_next_dns_server(t); + dns_scope_next_dns_server(t->scope); return dns_transaction_go(t); } From f4461e5641d53f27d6e76e0607bdaa9c0c58c1f6 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Sun, 27 Dec 2015 01:35:00 +0100 Subject: [PATCH 41/41] resolved: rename "features" variables to "feature_level" The name "features" suggests an orthogonal bitmap or suchlike, but the variables really encode only a linear set of feature levels. The type used is already called DnsServerFeatureLevel, hence fix up the variables accordingly, too. --- src/resolve/resolved-dns-server.c | 44 +++++++++++++------------- src/resolve/resolved-dns-server.h | 14 ++++---- src/resolve/resolved-dns-transaction.c | 2 +- 3 files changed, 30 insertions(+), 30 deletions(-) diff --git a/src/resolve/resolved-dns-server.c b/src/resolve/resolved-dns-server.c index e6e4feeedd..a57385d47d 100644 --- a/src/resolve/resolved-dns-server.c +++ b/src/resolve/resolved-dns-server.c @@ -68,8 +68,8 @@ int dns_server_new( s->n_ref = 1; s->manager = m; - s->verified_features = _DNS_SERVER_FEATURE_LEVEL_INVALID; - s->possible_features = DNS_SERVER_FEATURE_LEVEL_BEST; + s->verified_feature_level = _DNS_SERVER_FEATURE_LEVEL_INVALID; + s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_BEST; s->features_grace_period_usec = DNS_SERVER_FEATURE_GRACE_PERIOD_MIN_USEC; s->received_udp_packet_max = DNS_PACKET_UNICAST_SIZE_MAX; s->type = type; @@ -224,25 +224,25 @@ void dns_server_move_back_and_unmark(DnsServer *s) { } } -void dns_server_packet_received(DnsServer *s, DnsServerFeatureLevel features, usec_t rtt, size_t size) { +void dns_server_packet_received(DnsServer *s, DnsServerFeatureLevel level, usec_t rtt, size_t size) { assert(s); - if (features == DNS_SERVER_FEATURE_LEVEL_LARGE) { + if (level == DNS_SERVER_FEATURE_LEVEL_LARGE) { /* Even if we successfully receive a reply to a request announcing support for large packets, that does not mean we can necessarily receive large packets. */ - if (s->verified_features < DNS_SERVER_FEATURE_LEVEL_LARGE - 1) { - s->verified_features = DNS_SERVER_FEATURE_LEVEL_LARGE - 1; + if (s->verified_feature_level < DNS_SERVER_FEATURE_LEVEL_LARGE - 1) { + s->verified_feature_level = DNS_SERVER_FEATURE_LEVEL_LARGE - 1; assert_se(sd_event_now(s->manager->event, clock_boottime_or_monotonic(), &s->verified_usec) >= 0); } - } else if (s->verified_features < features) { - s->verified_features = features; + } else if (s->verified_feature_level < level) { + s->verified_feature_level = level; assert_se(sd_event_now(s->manager->event, clock_boottime_or_monotonic(), &s->verified_usec) >= 0); } - if (s->possible_features == features) + if (s->possible_feature_level == level) s->n_failed_attempts = 0; /* Remember the size of the largest UDP packet we received from a server, @@ -257,11 +257,11 @@ void dns_server_packet_received(DnsServer *s, DnsServerFeatureLevel features, us } } -void dns_server_packet_lost(DnsServer *s, DnsServerFeatureLevel features, usec_t usec) { +void dns_server_packet_lost(DnsServer *s, DnsServerFeatureLevel level, usec_t usec) { assert(s); assert(s->manager); - if (s->possible_features == features) + if (s->possible_feature_level == level) s->n_failed_attempts ++; if (s->resend_timeout > usec) @@ -270,11 +270,11 @@ void dns_server_packet_lost(DnsServer *s, DnsServerFeatureLevel features, usec_t s->resend_timeout = MIN(s->resend_timeout * 2, DNS_TIMEOUT_MAX_USEC); } -void dns_server_packet_failed(DnsServer *s, DnsServerFeatureLevel features) { +void dns_server_packet_failed(DnsServer *s, DnsServerFeatureLevel level) { assert(s); assert(s->manager); - if (s->possible_features != features) + if (s->possible_feature_level != level) return; s->n_failed_attempts = (unsigned) -1; @@ -310,36 +310,36 @@ static bool dns_server_grace_period_expired(DnsServer *s) { return true; } -DnsServerFeatureLevel dns_server_possible_features(DnsServer *s) { +DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s) { assert(s); - if (s->possible_features != DNS_SERVER_FEATURE_LEVEL_BEST && + if (s->possible_feature_level != DNS_SERVER_FEATURE_LEVEL_BEST && dns_server_grace_period_expired(s)) { _cleanup_free_ char *ip = NULL; - s->possible_features = DNS_SERVER_FEATURE_LEVEL_BEST; + s->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_BEST; s->n_failed_attempts = 0; s->verified_usec = 0; s->rrsig_missing = false; in_addr_to_string(s->family, &s->address, &ip); log_info("Grace period over, resuming full feature set for DNS server %s", strna(ip)); - } else if (s->possible_features <= s->verified_features) - s->possible_features = s->verified_features; + } else if (s->possible_feature_level <= s->verified_feature_level) + s->possible_feature_level = s->verified_feature_level; else if (s->n_failed_attempts >= DNS_SERVER_FEATURE_RETRY_ATTEMPTS && - s->possible_features > DNS_SERVER_FEATURE_LEVEL_WORST) { + s->possible_feature_level > DNS_SERVER_FEATURE_LEVEL_WORST) { _cleanup_free_ char *ip = NULL; - s->possible_features --; + s->possible_feature_level --; s->n_failed_attempts = 0; s->verified_usec = 0; in_addr_to_string(s->family, &s->address, &ip); log_warning("Using degraded feature set (%s) for DNS server %s", - dns_server_feature_level_to_string(s->possible_features), strna(ip)); + dns_server_feature_level_to_string(s->possible_feature_level), strna(ip)); } - return s->possible_features; + return s->possible_feature_level; } int dns_server_adjust_opt(DnsServer *server, DnsPacket *packet, DnsServerFeatureLevel level) { diff --git a/src/resolve/resolved-dns-server.h b/src/resolve/resolved-dns-server.h index 638f29548f..da21e6571a 100644 --- a/src/resolve/resolved-dns-server.h +++ b/src/resolve/resolved-dns-server.h @@ -64,8 +64,8 @@ struct DnsServer { usec_t resend_timeout; usec_t max_rtt; - DnsServerFeatureLevel verified_features; - DnsServerFeatureLevel possible_features; + DnsServerFeatureLevel verified_feature_level; + DnsServerFeatureLevel possible_feature_level; size_t received_udp_packet_max; unsigned n_failed_attempts; usec_t verified_usec; @@ -99,11 +99,13 @@ DnsServer* dns_server_unref(DnsServer *s); void dns_server_unlink(DnsServer *s); void dns_server_move_back_and_unmark(DnsServer *s); -void dns_server_packet_received(DnsServer *s, DnsServerFeatureLevel features, usec_t rtt, size_t size); -void dns_server_packet_lost(DnsServer *s, DnsServerFeatureLevel features, usec_t usec); -void dns_server_packet_failed(DnsServer *s, DnsServerFeatureLevel features); +void dns_server_packet_received(DnsServer *s, DnsServerFeatureLevel level, usec_t rtt, size_t size); +void dns_server_packet_lost(DnsServer *s, DnsServerFeatureLevel level, usec_t usec); +void dns_server_packet_failed(DnsServer *s, DnsServerFeatureLevel level); void dns_server_packet_rrsig_missing(DnsServer *s); +DnsServerFeatureLevel dns_server_possible_feature_level(DnsServer *s); + int dns_server_adjust_opt(DnsServer *server, DnsPacket *packet, DnsServerFeatureLevel level); DnsServer *dns_server_find(DnsServer *first, int family, const union in_addr_union *in_addr); @@ -120,6 +122,4 @@ void manager_next_dns_server(Manager *m); DEFINE_TRIVIAL_CLEANUP_FUNC(DnsServer*, dns_server_unref); -DnsServerFeatureLevel dns_server_possible_features(DnsServer *s); - extern const struct hash_ops dns_server_hash_ops; diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 97aceae381..3ca4a5ab74 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -318,7 +318,7 @@ static int dns_transaction_pick_server(DnsTransaction *t) { if (!server) return -ESRCH; - t->current_features = dns_server_possible_features(server); + t->current_features = dns_server_possible_feature_level(server); if (server == t->server) return 0;