From bea4c76fa0e1a93c629d6f45e043c233872e3be0 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Dec 2015 13:27:58 +0100 Subject: resolved: extend list of pseudo RR types Also, explain the situation with a longer comment. --- src/resolve/dns-type.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/src/resolve/dns-type.c b/src/resolve/dns-type.c index a626ecf01a..393fee0356 100644 --- a/src/resolve/dns-type.c +++ b/src/resolve/dns-type.c @@ -44,7 +44,22 @@ int dns_type_from_string(const char *s) { return sc->id; } -/* XXX: find an authoritative list of all pseudo types? */ -bool dns_type_is_pseudo(uint16_t n) { - return IN_SET(n, DNS_TYPE_ANY, DNS_TYPE_AXFR, DNS_TYPE_IXFR, DNS_TYPE_OPT); +bool dns_type_is_pseudo(uint16_t type) { + + /* Checks whether the specified type is a "pseudo-type". What + * a "pseudo-type" precisely is, is defined only very weakly, + * but apparently entails all RR types that are not actually + * stored as RRs on the server and should hence also not be + * cached. We use this list primarily to validate NSEC type + * bitfields. */ + + return IN_SET(type, + 0, /* A Pseudo RR type, according to RFC 2931 */ + DNS_TYPE_ANY, + DNS_TYPE_AXFR, + DNS_TYPE_IXFR, + DNS_TYPE_OPT, + DNS_TYPE_TSIG, + DNS_TYPE_TKEY + ); } -- cgit v1.2.3-54-g00ecf From 6728a58d10c019d6ebcf2949d0cb598afa5a7c6f Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Dec 2015 13:28:33 +0100 Subject: resolved: no need to check for NULL explicitly before invoking dns_packet_unref() --- src/resolve/resolved-dns-packet.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index e90500ce70..2117b70979 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -171,8 +171,7 @@ DnsPacket *dns_packet_unref(DnsPacket *p) { assert(p->n_ref > 0); - if (p->more) - dns_packet_unref(p->more); + dns_packet_unref(p->more); if (p->n_ref == 1) dns_packet_free(p); -- cgit v1.2.3-54-g00ecf From c33be4a6f229ed26407f19fbc463decb3d9b4cbc Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Dec 2015 13:46:05 +0100 Subject: resolved: refuse to cache ANY kind of pseudo-RR-type --- src/resolve/dns-type.c | 2 +- src/resolve/resolved-dns-cache.c | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/resolve/dns-type.c b/src/resolve/dns-type.c index 393fee0356..8ce8a566f1 100644 --- a/src/resolve/dns-type.c +++ b/src/resolve/dns-type.c @@ -51,7 +51,7 @@ bool dns_type_is_pseudo(uint16_t type) { * but apparently entails all RR types that are not actually * stored as RRs on the server and should hence also not be * cached. We use this list primarily to validate NSEC type - * bitfields. */ + * bitfields, and to verify what to cache. */ return IN_SET(type, 0, /* A Pseudo RR type, according to RFC 2931 */ diff --git a/src/resolve/resolved-dns-cache.c b/src/resolve/resolved-dns-cache.c index 9ab44400bd..676fa08ffb 100644 --- a/src/resolve/resolved-dns-cache.c +++ b/src/resolve/resolved-dns-cache.c @@ -302,7 +302,7 @@ static int dns_cache_put_positive( if (rr->key->class == DNS_CLASS_ANY) return 0; - if (rr->key->type == DNS_TYPE_ANY) + if (dns_type_is_pseudo(rr->key->type)) return 0; /* Entry exists already? Update TTL and timestamp */ @@ -370,9 +370,9 @@ static int dns_cache_put_negative( if (key->class == DNS_CLASS_ANY) return 0; - if (key->type == DNS_TYPE_ANY) - /* This is particularly important to filter out as we use this as a - * pseudo-type for NXDOMAIN entries */ + if (dns_type_is_pseudo(key->type)) + /* ANY is particularly important to filter out as we + * use this as a pseudo-type for NXDOMAIN entries */ return 0; if (soa_ttl <= 0) { if (log_get_max_level() >= LOG_DEBUG) { -- cgit v1.2.3-54-g00ecf From e6b57b378709af68d1828e26aec684f88bd04172 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Dec 2015 13:46:53 +0100 Subject: resolved: refuse OPT RRs in incoming packets that are not in the additional section We later rely that the DnsAnswer object contains all RRs from the original packet, at least when it comes to the answer and authorization sections, hence we better make sure we don#t silently end up removing an OPT RR from these two sections. --- src/resolve/resolved-dns-packet.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 2117b70979..7c5be538b8 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -1993,8 +1993,18 @@ int dns_packet_extract(DnsPacket *p) { goto finish; if (rr->key->type == DNS_TYPE_OPT) { - if (p->opt) - return -EBADMSG; + + /* The OPT RR is only valid in the Additional section */ + if (i < DNS_PACKET_ANCOUNT(p) + DNS_PACKET_NSCOUNT(p)) { + r = -EBADMSG; + goto finish; + } + + /* Two OPT RRs? */ + if (p->opt) { + r = -EBADMSG; + goto finish; + } p->opt = dns_resource_record_ref(rr); } else { -- cgit v1.2.3-54-g00ecf From c463eb783e5ad999d400180c69b912c54fa07ee1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Thu, 10 Dec 2015 15:01:04 +0100 Subject: resolved: generalize DNS RR type validity checks Check the validity of RR types as we parse or receive data from IPC clients, and use the same code for all of them. --- src/resolve/dns-type.c | 22 ++++++++++++++++++++++ src/resolve/dns-type.h | 5 ++++- src/resolve/resolved-bus.c | 3 +++ src/resolve/resolved-dns-packet.c | 9 ++++++--- src/resolve/resolved-dns-transaction.c | 4 ++-- 5 files changed, 37 insertions(+), 6 deletions(-) diff --git a/src/resolve/dns-type.c b/src/resolve/dns-type.c index 8ce8a566f1..8281da3b7c 100644 --- a/src/resolve/dns-type.c +++ b/src/resolve/dns-type.c @@ -63,3 +63,25 @@ bool dns_type_is_pseudo(uint16_t type) { DNS_TYPE_TKEY ); } + +bool dns_type_is_valid_query(uint16_t type) { + + /* The types valid as questions in packets */ + + return !IN_SET(type, + 0, + DNS_TYPE_OPT, + DNS_TYPE_TSIG, + DNS_TYPE_TKEY); +} + +bool dns_type_is_valid_rr(uint16_t type) { + + /* The types valid as RR in packets (but not necessarily + * stored on servers). */ + + return !IN_SET(type, + DNS_TYPE_ANY, + DNS_TYPE_AXFR, + DNS_TYPE_IXFR); +} diff --git a/src/resolve/dns-type.h b/src/resolve/dns-type.h index 2868025ad7..038a0d0e54 100644 --- a/src/resolve/dns-type.h +++ b/src/resolve/dns-type.h @@ -25,7 +25,10 @@ const char *dns_type_to_string(int type); int dns_type_from_string(const char *s); -bool dns_type_is_pseudo(uint16_t n); + +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); /* DNS record types, taken from * http://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml. diff --git a/src/resolve/resolved-bus.c b/src/resolve/resolved-bus.c index 1427638233..c8c0d3d9b8 100644 --- a/src/resolve/resolved-bus.c +++ b/src/resolve/resolved-bus.c @@ -553,6 +553,9 @@ static int bus_method_resolve_record(sd_bus_message *message, void *userdata, sd if (r == 0) return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid name '%s'", name); + if (!dns_type_is_valid_query(type)) + return sd_bus_error_setf(error, SD_BUS_ERROR_INVALID_ARGS, "Invalid RR type for query %" PRIu16, type); + r = check_ifindex_flags(ifindex, &flags, 0, error); if (r < 0) return r; diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c index 7c5be538b8..4e069ab4cb 100644 --- a/src/resolve/resolved-dns-packet.c +++ b/src/resolve/resolved-dns-packet.c @@ -1525,9 +1525,7 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) { goto fail; if (key->class == DNS_CLASS_ANY || - key->type == DNS_TYPE_ANY || - key->type == DNS_TYPE_AXFR || - key->type == DNS_TYPE_IXFR) { + !dns_type_is_valid_rr(key->type)) { r = -EBADMSG; goto fail; } @@ -1971,6 +1969,11 @@ int dns_packet_extract(DnsPacket *p) { if (r < 0) goto finish; + if (!dns_type_is_valid_query(key->type)) { + r = -EBADMSG; + goto finish; + } + r = dns_question_add(question, key); if (r < 0) goto finish; diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index bcf6d5c810..5cd03bc01d 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -107,11 +107,11 @@ int dns_transaction_new(DnsTransaction **ret, DnsScope *s, DnsResourceKey *key) assert(key); /* Don't allow looking up invalid or pseudo RRs */ - if (IN_SET(key->type, DNS_TYPE_OPT, 0, DNS_TYPE_TSIG, DNS_TYPE_TKEY)) + if (!dns_type_is_valid_query(key->type)) return -EINVAL; /* We only support the IN class */ - if (key->class != DNS_CLASS_IN) + if (key->class != DNS_CLASS_IN && key->class != DNS_CLASS_ANY) return -EOPNOTSUPP; r = hashmap_ensure_allocated(&s->manager->dns_transactions, NULL); -- cgit v1.2.3-54-g00ecf From 79e249313887840e0fc52f69afc0daeed754bff1 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 11 Dec 2015 13:36:25 +0100 Subject: resolved: rework how and when the number of answer RRs to cache is determined Instead of figuring out how many RRs to cache right before we do so, determine this at the time we install the answer RRs, so that we can still alter this as we manipulate the answer during validation. The primary purpose of this is to pave the way so that we can drop unsigned RRsets from the answer and invalidate the number of RRs to cache at the same time. --- src/resolve/resolved-dns-transaction.c | 27 +++++++++++++++------------ src/resolve/resolved-dns-transaction.h | 1 + 2 files changed, 16 insertions(+), 12 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 5cd03bc01d..7e8f4d888b 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -351,6 +351,7 @@ static int dns_transaction_open_tcp(DnsTransaction *t) { t->server = dns_server_ref(server); t->received = dns_packet_unref(t->received); t->answer = dns_answer_unref(t->answer); + t->n_answer_cacheable = 0; t->answer_rcode = 0; t->stream->complete = on_stream_complete; t->stream->transaction = t; @@ -375,8 +376,6 @@ static void dns_transaction_next_dns_server(DnsTransaction *t) { } static void dns_transaction_cache_answer(DnsTransaction *t) { - unsigned n_cache; - assert(t); /* For mDNS we cache whenever we get the packet, rather than @@ -391,20 +390,11 @@ static void dns_transaction_cache_answer(DnsTransaction *t) { if (!DNS_PACKET_SHALL_CACHE(t->received)) return; - /* According to RFC 4795, section 2.9. only the RRs from the - * answer section shall be cached. However, if we know the - * message is authenticated, we might as well cache - * everything. */ - if (t->answer_authenticated) - n_cache = dns_answer_size(t->answer); - else - n_cache = DNS_PACKET_ANCOUNT(t->received); - dns_cache_put(&t->scope->cache, t->key, t->answer_rcode, t->answer, - n_cache, + t->n_answer_cacheable, t->answer_authenticated, 0, t->received->family, @@ -618,6 +608,15 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) { t->answer_rcode = DNS_PACKET_RCODE(p); t->answer_authenticated = t->scope->dnssec_mode == DNSSEC_TRUST && DNS_PACKET_AD(p); + /* According to RFC 4795, section 2.9. only the RRs + * from the answer section shall be cached. However, + * if we know the message is authenticated, we might + * as well cache everything. */ + if (t->answer_authenticated) + t->n_answer_cacheable = (unsigned) -1; /* everything! */ + else + t->n_answer_cacheable = DNS_PACKET_ANCOUNT(t->received); /* only the answer section */ + r = dns_transaction_request_dnssec_keys(t); if (r < 0) { dns_transaction_complete(t, DNS_TRANSACTION_RESOURCES); @@ -770,6 +769,7 @@ static int dns_transaction_prepare(DnsTransaction *t, usec_t ts) { t->start_usec = ts; t->received = dns_packet_unref(t->received); t->answer = dns_answer_unref(t->answer); + t->n_answer_cacheable = 0; t->answer_rcode = 0; t->answer_source = _DNS_TRANSACTION_SOURCE_INVALID; @@ -1444,6 +1444,9 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { t->answer = validated; validated = NULL; + /* Everything that's now in t->answer is known to be good, hence cacheable. */ + t->n_answer_cacheable = (unsigned) -1; /* everything! */ + t->answer_authenticated = true; t->dnssec_result = DNSSEC_VALIDATED; return 1; diff --git a/src/resolve/resolved-dns-transaction.h b/src/resolve/resolved-dns-transaction.h index 2328e7937c..1f35a4dd8f 100644 --- a/src/resolve/resolved-dns-transaction.h +++ b/src/resolve/resolved-dns-transaction.h @@ -74,6 +74,7 @@ struct DnsTransaction { DnsPacket *sent, *received; DnsAnswer *answer; + unsigned n_answer_cacheable; /* Specifies how many RRs of the answer shall be cached, from the beginning */ int answer_rcode; DnsTransactionSource answer_source; bool answer_authenticated; -- cgit v1.2.3-54-g00ecf From 203f1b35d962bab3c67ecf57ce6bd9ec87bf7078 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 11 Dec 2015 13:55:26 +0100 Subject: resolved: rework dnssec validation results This adds a new validation result DNSSEC_UNSUPPORTED_ALGORITHM which is returned when we encounter an unsupported crypto algorithm when trying to validate RRSIG/DNSKEY combinations. Previously we'd return ENOTSUPP in this case, but it's better to consider this a non-error DNSSEC validation result, since our reaction to this case needs to be more like in cases such as expired or missing keys: we need to keep continue validation looking for another RRSIG/DNSKEY combination that works better for us. This also reworks how dnssec_validate_rrsig_search() propagates errors from dnssec_validate_rrsig(). Previously, errors such as unsupported algorithms or expired signatures would not be propagated, but simply be returned as "missing-key". --- src/resolve/resolved-dns-dnssec.c | 66 +++++++++++++++++++++++++++++---------- src/resolve/resolved-dns-dnssec.h | 10 ++++-- 2 files changed, 58 insertions(+), 18 deletions(-) diff --git a/src/resolve/resolved-dns-dnssec.c b/src/resolve/resolved-dns-dnssec.c index df12e86167..792c9d8692 100644 --- a/src/resolve/resolved-dns-dnssec.c +++ b/src/resolve/resolved-dns-dnssec.c @@ -295,8 +295,10 @@ int dnssec_verify_rrset( * using the signature "rrsig" and the key "dnskey". It's * assumed the RRSIG and DNSKEY match. */ - if (!dnssec_algorithm_supported(rrsig->rrsig.algorithm)) - return -EOPNOTSUPP; + if (!dnssec_algorithm_supported(rrsig->rrsig.algorithm)) { + *result = DNSSEC_UNSUPPORTED_ALGORITHM; + return 0; + } if (a->n_rrs > VERIFY_RRS_MAX) return -E2BIG; @@ -508,7 +510,7 @@ int dnssec_verify_rrset_search( usec_t realtime, DnssecResult *result) { - bool found_rrsig = false, found_dnskey = false; + bool found_rrsig = false, found_invalid = false, found_expired_rrsig = false, found_unsupported_algorithm = false; DnsResourceRecord *rrsig; int r; @@ -524,6 +526,7 @@ int dnssec_verify_rrset_search( DNS_ANSWER_FOREACH(rrsig, a) { DnsResourceRecord *dnskey; + /* Is this an RRSIG RR that applies to RRs matching our key? */ r = dnssec_key_match_rrsig(key, rrsig); if (r < 0) return r; @@ -536,14 +539,13 @@ int dnssec_verify_rrset_search( DNS_ANSWER_FOREACH(dnskey, validated_dnskeys) { DnssecResult one_result; + /* Is this a DNSKEY RR that matches they key of our RRSIG? */ r = dnssec_rrsig_match_dnskey(rrsig, dnskey); if (r < 0) return r; if (r == 0) continue; - found_dnskey = true; - /* Take the time here, if it isn't set yet, so * that we do all validations with the same * time. */ @@ -556,22 +558,53 @@ int dnssec_verify_rrset_search( * combination. */ r = dnssec_verify_rrset(a, key, rrsig, dnskey, realtime, &one_result); - if (r < 0 && r != EOPNOTSUPP) + if (r < 0) return r; - if (one_result == DNSSEC_VALIDATED) { + + switch (one_result) { + + case DNSSEC_VALIDATED: + /* Yay, the RR has been validated, + * return immediately. */ *result = DNSSEC_VALIDATED; return 0; - } - /* If the signature is invalid, or done using - an unsupported algorithm, let's try another - key and/or signature. After all they - key_tags and stuff are not unique, and - might be shared by multiple keys. */ + case DNSSEC_INVALID: + /* If the signature is invalid, let's try another + key and/or signature. After all they + key_tags and stuff are not unique, and + might be shared by multiple keys. */ + found_invalid = true; + continue; + + case DNSSEC_UNSUPPORTED_ALGORITHM: + /* If the key algorithm is + unsupported, try another + RRSIG/DNSKEY pair, but remember we + encountered this, so that we can + return a proper error when we + encounter nothing better. */ + found_unsupported_algorithm = true; + continue; + + case DNSSEC_SIGNATURE_EXPIRED: + /* If the signature is expired, try + another one, but remember it, so + that we can return this */ + found_expired_rrsig = true; + continue; + + default: + assert_not_reached("Unexpected DNSSEC validation result"); + } } } - if (found_dnskey) + if (found_expired_rrsig) + *result = DNSSEC_SIGNATURE_EXPIRED; + else if (found_unsupported_algorithm) + *result = DNSSEC_UNSUPPORTED_ALGORITHM; + else if (found_invalid) *result = DNSSEC_INVALID; else if (found_rrsig) *result = DNSSEC_MISSING_KEY; @@ -756,10 +789,11 @@ DEFINE_STRING_TABLE_LOOKUP(dnssec_mode, DnssecMode); static const char* const dnssec_result_table[_DNSSEC_RESULT_MAX] = { [DNSSEC_VALIDATED] = "validated", [DNSSEC_INVALID] = "invalid", - [DNSSEC_UNSIGNED] = "unsigned", + [DNSSEC_SIGNATURE_EXPIRED] = "signature-expired", + [DNSSEC_UNSUPPORTED_ALGORITHM] = "unsupported-algorithm", [DNSSEC_NO_SIGNATURE] = "no-signature", [DNSSEC_MISSING_KEY] = "missing-key", - [DNSSEC_SIGNATURE_EXPIRED] = "signature-expired", + [DNSSEC_UNSIGNED] = "unsigned", [DNSSEC_FAILED_AUXILIARY] = "failed-auxiliary", }; DEFINE_STRING_TABLE_LOOKUP(dnssec_result, DnssecResult); diff --git a/src/resolve/resolved-dns-dnssec.h b/src/resolve/resolved-dns-dnssec.h index f0825ba23f..f33abe3e11 100644 --- a/src/resolve/resolved-dns-dnssec.h +++ b/src/resolve/resolved-dns-dnssec.h @@ -43,12 +43,18 @@ enum DnssecMode { }; enum DnssecResult { + /* These four are returned by dnssec_verify_rrset() */ DNSSEC_VALIDATED, DNSSEC_INVALID, - DNSSEC_UNSIGNED, + DNSSEC_SIGNATURE_EXPIRED, + DNSSEC_UNSUPPORTED_ALGORITHM, + + /* These two are added by dnssec_verify_rrset_search() */ DNSSEC_NO_SIGNATURE, DNSSEC_MISSING_KEY, - DNSSEC_SIGNATURE_EXPIRED, + + /* These two are added by the DnsTransaction logic */ + DNSSEC_UNSIGNED, DNSSEC_FAILED_AUXILIARY, _DNSSEC_RESULT_MAX, _DNSSEC_RESULT_INVALID = -1 -- cgit v1.2.3-54-g00ecf From 56352fe92d0bf4310699b93427ab4fb1d44e5312 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 11 Dec 2015 14:00:08 +0100 Subject: resolved: refactor DNSSEC answer validation This changes answer validation to be more accepting to unordered RRs in responses. The agorithm we now implement goes something like this: 1. populate validated keys list for this transaction from DS RRs 2. as long as the following changes the unvalidated answer list: 2a. try to validate the first RRset we find in unvalidated answer list 2b. if that worked: add to validated answer; if DNSKEY also add to validated keys list; remove from unvalidated answer. 2c. continue at 2a, with the next RRset, or restart from the beginning when we hit the end 3. as long as the following changes the unvalidated answer list: 3a. try to validate the first RRset again. This will necessarily fail, but we learn the precise error 3b. If this was a "primary" response to the question, fail the entire transaction. "Primary" in this context means that it is directly a response to the query, or a CNAME/DNAME for it. 3c. Otherwise, remove the RRset from the unvalidated answer list. Note that we the too loops in 2 + 3 are actually coded as a single one, but the dnskeys_finalized bool indicates which loop we are currently processing. Note that loop 2 does not drop any invalidated RRsets yet, that's something only loop 3 does. This is because loop 2 might still encounter additional DNSKEYS which might validate more stuff, and if we'd already have dropped those RRsets we couldn't validate those anymore. The first loop is hence a "constructive" loop, the second loop a "destructive" one: the first one validates whatever is possible, the second one then deletes whatever still isn't. --- src/resolve/resolved-dns-transaction.c | 138 +++++++++++++++++++-------------- 1 file changed, 80 insertions(+), 58 deletions(-) diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index 7e8f4d888b..8b3e59a1ea 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -1280,10 +1280,59 @@ void dns_transaction_notify(DnsTransaction *t, DnsTransaction *source) { dns_transaction_process_dnssec(t); } +static int dns_transaction_is_primary_response(DnsTransaction *t, DnsResourceRecord *rr) { + int r; + + assert(t); + assert(rr); + + /* Check if the specified RR is the "primary" response, + * i.e. either matches the question precisely or is a + * CNAME/DNAME for it */ + + r = dns_resource_key_match_rr(t->key, rr, NULL); + if (r != 0) + return r; + + r = dns_resource_key_match_cname_or_dname(t->key, rr->key, NULL); + if (r != 0) + return r; + + return 0; +} + +static int dns_transaction_validate_dnskey_by_ds(DnsTransaction *t) { + DnsResourceRecord *rr; + int ifindex, r; + + assert(t); + + /* Add all DNSKEY RRs from the answer that are validated by DS + * RRs from the list of validated keys to the lis of validated + * keys. */ + + DNS_ANSWER_FOREACH_IFINDEX(rr, ifindex, t->answer) { + + r = dnssec_verify_dnskey_search(rr, t->validated_keys); + if (r < 0) + return r; + if (r == 0) + continue; + + /* If so, the DNSKEY is validated too. */ + r = dns_answer_add_extend(&t->validated_keys, rr, ifindex); + if (r < 0) + return r; + } + + return 0; +} + int dns_transaction_validate_dnssec(DnsTransaction *t) { _cleanup_(dns_answer_unrefp) DnsAnswer *validated = NULL; + bool dnskeys_finalized = false; DnsResourceRecord *rr; - int ifindex, r; + int r; assert(t); @@ -1312,22 +1361,12 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { } /* First see if there are DNSKEYs we already known a validated DS for. */ - DNS_ANSWER_FOREACH_IFINDEX(rr, ifindex, t->answer) { - - r = dnssec_verify_dnskey_search(rr, t->validated_keys); - if (r < 0) - return r; - if (r == 0) - continue; - - /* If so, the DNSKEY is validated too. */ - r = dns_answer_add_extend(&t->validated_keys, rr, ifindex); - if (r < 0) - return r; - } + r = dns_transaction_validate_dnskey_by_ds(t); + if (r < 0) + return r; for (;;) { - bool changed = false, missing_key_for_transaction = false; + bool changed = false; DNS_ANSWER_FOREACH(rr, t->answer) { DnssecResult result; @@ -1346,9 +1385,7 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { log_debug("Looking at %s: %s", rrs ? strstrip(rrs) : "???", dnssec_result_to_string(result)); } - switch (result) { - - case DNSSEC_VALIDATED: + if (result == DNSSEC_VALIDATED) { /* Add the validated RRset to the new list of validated RRsets */ r = dns_answer_copy_by_key(&validated, t->answer, rr->key); @@ -1372,71 +1409,56 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { if (r < 0) return r; + /* Exit the loop, we dropped something from the answer, start from the beginning */ changed = true; break; - case DNSSEC_INVALID: - case DNSSEC_NO_SIGNATURE: - case DNSSEC_SIGNATURE_EXPIRED: - - /* Is this the RRset that we were looking for? If so, this is fatal for the whole transaction */ - r = dns_resource_key_match_rr(t->key, rr, NULL); - if (r < 0) - return r; - if (r > 0) { - t->dnssec_result = result; - return 0; - } + } else if (dnskeys_finalized) { + /* If we haven't read all DNSKEYs yet + * a negative result of the validation + * is irrelevant, as there might be + * more DNSKEYs coming. */ - /* Is this a CNAME for a record we were looking for? If so, it's also fatal for the whole transaction */ - r = dns_resource_key_match_cname_or_dname(t->key, rr->key, NULL); + r = dns_transaction_is_primary_response(t, rr); if (r < 0) return r; if (r > 0) { + /* This is a primary response + * to our question, and it + * failed validation. That's + * fatal. */ t->dnssec_result = result; return 0; } - /* This is just something auxiliary. Just remove the RRset and continue. */ + /* This is just some auxiliary + * data. Just remove the RRset and + * continue. */ r = dns_answer_remove_by_key(&t->answer, rr->key); if (r < 0) return r; + /* Exit the loop, we dropped something from the answer, start from the beginning */ changed = true; break; - - case DNSSEC_MISSING_KEY: - /* They key is missing? Let's continue - * with the next iteration, maybe - * we'll find it in an DNSKEY RRset - * later on. */ - - r = dns_resource_key_equal(rr->key, t->key); - if (r < 0) - return r; - if (r > 0) - missing_key_for_transaction = true; - - break; - - default: - assert_not_reached("Unexpected DNSSEC result"); } - - if (changed) - break; } if (changed) continue; - /* This didn't work either, there's no point in - * continuing. */ - if (missing_key_for_transaction) { - t->dnssec_result = DNSSEC_MISSING_KEY; - return 0; + if (!dnskeys_finalized) { + /* OK, now we know we have added all DNSKEYs + * we possibly could to our validated + * list. Now run the whole thing once more, + * and strip everything we still cannot + * validate. + */ + dnskeys_finalized = true; + continue; } + /* We're done */ break; } -- cgit v1.2.3-54-g00ecf From 29c1519ed4899d139fa7b2079311cff6c4fb64a8 Mon Sep 17 00:00:00 2001 From: Lennart Poettering Date: Fri, 11 Dec 2015 15:10:56 +0100 Subject: resolved: don't eat up errors dns_resource_key_match_soa() and dns_resource_key_match_cname_or_dname() may return errors as negative return values. Make sure to propagate those. --- src/resolve/resolved-dns-answer.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/resolve/resolved-dns-answer.c b/src/resolve/resolved-dns-answer.c index 55e6ffbad7..6a37345e7e 100644 --- a/src/resolve/resolved-dns-answer.c +++ b/src/resolve/resolved-dns-answer.c @@ -228,9 +228,9 @@ int dns_answer_contains_rr(DnsAnswer *a, DnsResourceRecord *rr) { int dns_answer_find_soa(DnsAnswer *a, const DnsResourceKey *key, DnsResourceRecord **ret) { DnsResourceRecord *rr; + int r; assert(key); - assert(ret); if (!a) return 0; @@ -240,8 +240,12 @@ int dns_answer_find_soa(DnsAnswer *a, const DnsResourceKey *key, DnsResourceReco return 0; DNS_ANSWER_FOREACH(rr, a) { - if (dns_resource_key_match_soa(key, rr->key)) { - *ret = rr; + r = dns_resource_key_match_soa(key, rr->key); + if (r < 0) + return r; + if (r > 0) { + if (ret) + *ret = rr; return 1; } } @@ -251,6 +255,7 @@ int dns_answer_find_soa(DnsAnswer *a, const DnsResourceKey *key, DnsResourceReco int dns_answer_find_cname_or_dname(DnsAnswer *a, const DnsResourceKey *key, DnsResourceRecord **ret) { DnsResourceRecord *rr; + int r; assert(key); @@ -262,7 +267,10 @@ int dns_answer_find_cname_or_dname(DnsAnswer *a, const DnsResourceKey *key, DnsR return 0; DNS_ANSWER_FOREACH(rr, a) { - if (dns_resource_key_match_cname_or_dname(key, rr->key, NULL)) { + r = dns_resource_key_match_cname_or_dname(key, rr->key, NULL); + if (r < 0) + return r; + if (r > 0) { if (ret) *ret = rr; return 1; -- cgit v1.2.3-54-g00ecf