diff --git a/src/resolve/meson.build b/src/resolve/meson.build index e82e28ae061..bd6c5b60eaf 100644 --- a/src/resolve/meson.build +++ b/src/resolve/meson.build @@ -168,6 +168,10 @@ executables += [ resolve_test_template + { 'sources' : files('test-dns-query.c'), }, + resolve_test_template + { + 'sources' : files('test-dns-transaction.c'), + 'conditions' : ['HAVE_OPENSSL'], + }, resolve_test_template + { 'sources' : files('test-resolved-stream.c'), }, diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c index da35f9cdd64..7acad458341 100644 --- a/src/resolve/resolved-dns-transaction.c +++ b/src/resolve/resolved-dns-transaction.c @@ -849,6 +849,14 @@ static void dns_transaction_cache_answer(DnsTransaction *t) { if (FLAGS_SET(t->query_flags, SD_RESOLVED_NO_VALIDATE) && t->scope->dnssec_mode != DNSSEC_NO) return; + /* Same if the reply could not be validated merely because the query went out without DO (e.g. a + * bypass query without DO, or one sent while the server was degraded): later lookups may send DO + * and validate again, and nothing guarantees that the cache gets flushed before they do. */ + if (t->server && + t->answer_dnssec_result == DNSSEC_INCOMPATIBLE_SERVER && + dns_server_dnssec_supported(t->server)) + return; + /* Packet from localhost? */ if (!t->scope->manager->cache_from_localhost && in_addr_is_localhost(t->received->family, &t->received->sender) != 0) @@ -1022,7 +1030,8 @@ static void dns_transaction_process_dnssec(DnsTransaction *t) { return; } - if (t->answer_dnssec_result == DNSSEC_INCOMPATIBLE_SERVER) + /* Don't blame the server if the query merely went out below DO, it may support DNSSEC just fine. */ + if (t->answer_dnssec_result == DNSSEC_INCOMPATIBLE_SERVER && !dns_server_dnssec_supported(t->server)) dns_server_warn_downgrade(t->server); dns_transaction_cache_answer(t); @@ -1597,6 +1606,17 @@ static int on_transaction_timeout(sd_event_source *s, usec_t usec, void *userdat case DNS_PROTOCOL_DNS: assert(t->server); dns_server_packet_lost(t->server, t->stream ? IPPROTO_TCP : IPPROTO_UDP, t->current_feature_level); + + /* If the server's feature level changed meanwhile (packet loss degraded it, or the + * grace period ended), the retry goes out at a different level, possibly with or + * without DO. Use a new connection and ID for it, like + * dns_transaction_maybe_restart() does, so that a late reply to an earlier attempt + * cannot pass for a reply to the retry. Close the connection first, while t->sent + * still refers to the packet that may be queued on it. */ + if (t->current_feature_level != dns_server_possible_feature_level(t->server)) { + dns_transaction_close_connection(t, /* use_graveyard= */ true); + dns_transaction_shuffle_id(t); + } break; case DNS_PROTOCOL_LLMNR: @@ -2448,10 +2468,25 @@ static bool dns_transaction_dnssec_supported(DnsTransaction *t) { if (!t->server) return true; - /* Note that we do not check the feature level actually used for the transaction but instead the feature level - * the server is known to support currently, as the transaction feature level might be lower than what the - * server actually supports, since we might have downgraded this transaction's feature level because we got a - * SERVFAIL earlier and wanted to check whether downgrading fixes it. */ + /* Same if the answer came from the cache on a retry: server and feature level are then stale. */ + if (t->answer_source != DNS_TRANSACTION_NETWORK) + return true; + + /* A query sent below DO cannot get RRSIGs back, whatever the server is capable of, e.g. after + * packet loss degraded the server or a SERVFAIL clamped this transaction. Validating it can only + * fail, so treat it like a reply from a server without DNSSEC. Except in strict mode, where + * unsigned data is only an error if it needed signing (negative trust anchors, insecure + * delegations), so leave that to validation, as dns_server_dnssec_supported() does. + * + * A bypass query goes out as the client built it, hence it's the client's DO bit that counts there, + * not the feature level. Note that we cannot look at t->sent for this: dns_packet_do() only works on + * extracted packets, and we never extract the packets we send. (If the client's packet has + * trailing garbage, extraction drops its OPT RR, so we see no DO here even if the copy we send + * carries it.) */ + if (t->scope->dnssec_mode != DNSSEC_YES && + !(t->bypass ? dns_packet_do(t->bypass) : + DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(t->current_feature_level))) + return false; return dns_server_dnssec_supported(t->server); } @@ -3639,9 +3674,11 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) { return 0; if (!dns_transaction_dnssec_supported_full(t)) { - /* The server does not support DNSSEC, or doesn't augment responses with RRSIGs. */ + /* The server does not support DNSSEC, or doesn't augment responses with RRSIGs, or this + * query or an auxiliary one went out without DO. */ t->answer_dnssec_result = DNSSEC_INCOMPATIBLE_SERVER; - log_debug("Not validating response for %" PRIu16 ", used server feature level does not support DNSSEC.", t->id); + log_debug("Not validating response for %" PRIu16 ", the server does not support DNSSEC, or the " + "query or an auxiliary one was sent without DO.", t->id); return 0; } diff --git a/src/resolve/test-dns-transaction.c b/src/resolve/test-dns-transaction.c new file mode 100644 index 00000000000..a15869e6912 --- /dev/null +++ b/src/resolve/test-dns-transaction.c @@ -0,0 +1,379 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ + +#include "sd-event.h" + +#include "dns-answer.h" +#include "dns-packet.h" +#include "dns-question.h" +#include "dns-rr.h" +#include "resolved-dns-cache.h" +#include "resolved-dns-scope.h" +#include "resolved-dns-server.h" +#include "resolved-dns-transaction.h" +#include "resolved-manager.h" +#include "set.h" +#include "tests.h" + +typedef struct TransactionEnv { + Manager manager; + DnsScope *scope; + DnsServer *server; +} TransactionEnv; + +static void transaction_env_teardown(TransactionEnv *env) { + assert(env); + + dns_scope_free(env->scope); + if (env->server) + dns_server_unlink(env->server); + hashmap_free(env->manager.dns_transactions); + sd_event_unref(env->manager.event); +} + +static void transaction_env_setup(TransactionEnv *env, DnssecMode dnssec_mode) { + union in_addr_union server_addr = { .in.s_addr = htobe32(0xc0000201) }; /* 192.0.2.1 */ + + assert(env); + + env->manager = (Manager) { + .dnssec_mode = dnssec_mode, + .enable_cache = DNS_CACHE_MODE_YES, + .cache_max[DNS_PROTOCOL_DNS] = DEFAULT_CACHE_MAX, + }; + + ASSERT_OK(sd_event_new(&env->manager.event)); + + ASSERT_OK(dns_scope_new(&env->manager, &env->scope, DNS_SCOPE_GLOBAL, /* link= */ NULL, + /* delegate= */ NULL, DNS_PROTOCOL_DNS, AF_INET)); + ASSERT_NOT_NULL(env->scope); + ASSERT_EQ(env->scope->dnssec_mode, dnssec_mode); + + ASSERT_OK(dns_server_new(&env->manager, &env->server, DNS_SERVER_SYSTEM, /* link= */ NULL, + /* delegate= */ NULL, AF_INET, &server_addr, 53, /* ifindex= */ 0, + /* server_name= */ NULL, RESOLVE_CONFIG_SOURCE_DBUS)); + ASSERT_NOT_NULL(env->server); +} + +/* Builds a client query for the specified key as the stub would hand it to a bypass transaction, i.e. + * already extracted, with an OPT RR that carries the specified DO bit. */ +static void make_bypass_packet(DnsResourceKey *key, bool edns0_do, DnsPacket **ret) { + _cleanup_(dns_packet_unrefp) DnsPacket *p = NULL; + + assert(key); + assert(ret); + + ASSERT_OK(dns_packet_new_query(&p, DNS_PROTOCOL_DNS, 0, /* dnssec_checking_disabled= */ false)); + ASSERT_OK(dns_packet_append_key(p, key, 0, NULL)); + DNS_PACKET_HEADER(p)->qdcount = htobe16(1); + ASSERT_OK(dns_packet_append_opt(p, DNS_PACKET_UNICAST_SIZE_MAX, edns0_do, + /* include_rfc6975= */ false, /* nsid= */ NULL, + DNS_RCODE_SUCCESS, NULL)); + ASSERT_OK(dns_packet_extract(p)); + ASSERT_EQ(dns_packet_do(p), edns0_do); + + *ret = TAKE_PTR(p); +} + +/* Sets up a transaction as if its query had been sent at the specified feature level, and builds a matching + * reply with a single unsigned A RR, plus an OPT RR (with DO, if applicable) as a server at that level would + * send it. If bypass_do is non-negative, this is a bypass transaction for a client query with that DO bit: + * such a query goes out as the client built it, so the DO bit of query and reply is the client's then, and + * the feature level only says what the server is at. */ +static void transaction_setup_sent( + TransactionEnv *env, + DnsServerFeatureLevel level, + int bypass_do, + DnsTransaction **ret_transaction, + DnsPacket **ret_reply) { + + _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL; + _cleanup_(dns_resource_record_unrefp) DnsResourceRecord *rr = NULL; + _cleanup_(dns_question_unrefp) DnsQuestion *question = NULL; + _cleanup_(dns_answer_unrefp) DnsAnswer *answer = NULL; + _cleanup_(dns_packet_unrefp) DnsPacket *reply = NULL, *bypass = NULL; + DnsTransaction *t = NULL; + bool sent_do; + + assert(env); + assert(ret_transaction); + assert(ret_reply); + + sent_do = bypass_do >= 0 ? bypass_do > 0 : DNS_SERVER_FEATURE_LEVEL_IS_DNSSEC(level); + + key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, "www.example.com"); + ASSERT_NOT_NULL(key); + + /* Auxiliary DNSSEC transactions fail at once with network-down, as the test manager has no links. + * They inherit NO_NETWORK, so they wouldn't use the network either way. */ + if (bypass_do >= 0) { + make_bypass_packet(key, sent_do, &bypass); + ASSERT_OK(dns_transaction_new(&t, env->scope, /* key= */ NULL, bypass, + SD_RESOLVED_NO_NETWORK)); + } else + ASSERT_OK(dns_transaction_new(&t, env->scope, key, /* bypass= */ NULL, + SD_RESOLVED_NO_NETWORK)); + ASSERT_NOT_NULL(t); + + /* Keep the transaction around for inspection once it completed */ + t->block_gc++; + + /* What dns_transaction_pick_server() and dns_transaction_prepare() would have done. */ + t->server = dns_server_ref(env->server); + t->n_picked_servers = 1; + t->current_feature_level = level; + t->state = DNS_TRANSACTION_PENDING; + t->n_attempts = 1; + + if (bypass_do >= 0) + /* What dns_transaction_make_packet() does for a bypass transaction */ + ASSERT_OK(dns_packet_dup(&t->sent, t->bypass)); + else { + ASSERT_OK(dns_packet_new_query(&t->sent, DNS_PROTOCOL_DNS, 0, + /* dnssec_checking_disabled= */ true)); + ASSERT_OK(dns_packet_append_key(t->sent, key, 0, NULL)); + DNS_PACKET_HEADER(t->sent)->qdcount = htobe16(1); + + /* What dns_transaction_emit_udp() then does for a regular transaction */ + ASSERT_OK(dns_server_adjust_opt(env->server, t->sent, level)); + } + DNS_PACKET_HEADER(t->sent)->id = t->id; + + rr = dns_resource_record_new(key); + ASSERT_NOT_NULL(rr); + rr->a.in_addr.s_addr = htobe32(0xc0000202); /* 192.0.2.2 */ + rr->ttl = 3600; + + question = dns_question_new(1); + ASSERT_NOT_NULL(question); + ASSERT_OK(dns_question_add(question, key, 0)); + + answer = dns_answer_new(1); + ASSERT_NOT_NULL(answer); + ASSERT_OK(dns_answer_add(answer, rr, /* ifindex= */ 0, 0, /* rrsig= */ NULL)); + + ASSERT_OK(dns_scope_make_reply_packet(env->scope, t->id, DNS_RCODE_SUCCESS, question, answer, + /* soa= */ NULL, /* tentative= */ false, &reply)); + if (bypass_do >= 0 || DNS_SERVER_FEATURE_LEVEL_IS_EDNS0(level)) + ASSERT_OK(dns_packet_append_opt(reply, DNS_PACKET_UNICAST_SIZE_MAX, sent_do, + /* include_rfc6975= */ false, /* nsid= */ NULL, + DNS_RCODE_SUCCESS, NULL)); + reply->ipproto = IPPROTO_UDP; + reply->family = env->server->family; + reply->sender = env->server->address; + + *ret_transaction = t; + *ret_reply = TAKE_PTR(reply); +} + +/* ================================================================ + * dns_transaction_process_reply() + * ================================================================ */ + +/* Feeds an unsigned reply to a transaction sent at the specified feature level, to a server that is at that + * level. Unless server_incapable is set, nothing marks the server as DNSSEC-incapable, which below DO is the + * state packet loss leaves behind. See transaction_setup_sent() for bypass_do. */ +static DnsTransaction* process_unsigned_reply( + TransactionEnv *env, + DnssecMode dnssec_mode, + DnsServerFeatureLevel level, + int bypass_do, + bool server_incapable) { + + _cleanup_(dns_packet_unrefp) DnsPacket *reply = NULL; + DnsTransaction *t; + + assert(env); + + transaction_env_setup(env, dnssec_mode); + + env->server->verified_feature_level = level; + env->server->possible_feature_level = level; + env->server->packet_rrsig_missing = server_incapable; + ASSERT_EQ(dns_server_dnssec_supported(env->server), !server_incapable); + + transaction_setup_sent(env, level, bypass_do, &t, &reply); + + dns_transaction_process_reply(t, reply, /* encrypted= */ false); + + return t; +} + +TEST(dns_transaction_process_reply_no_do_allow_downgrade) { + _cleanup_(transaction_env_teardown) TransactionEnv env = {}; + DnsTransaction *t; + + t = process_unsigned_reply(&env, DNSSEC_ALLOW_DOWNGRADE, DNS_SERVER_FEATURE_LEVEL_TCP, + /* bypass_do= */ -1, /* server_incapable= */ false); + + /* Accepted as unsigned, without auxiliary lookups, and without blaming the server */ + ASSERT_EQ(t->state, DNS_TRANSACTION_SUCCESS); + ASSERT_EQ(t->answer_dnssec_result, DNSSEC_INCOMPATIBLE_SERVER); + ASSERT_FALSE(FLAGS_SET(t->answer_query_flags, SD_RESOLVED_AUTHENTICATED)); + ASSERT_TRUE(set_isempty(t->dnssec_transactions)); + ASSERT_EQ(dns_answer_size(t->answer), 1u); + ASSERT_FALSE(env.server->warned_downgrade); + + /* But not cached, as later lookups may go out with DO again before the cache is flushed */ + ASSERT_EQ(dns_cache_size(&env.scope->cache), 0u); + + t->block_gc--; + ASSERT_NULL(dns_transaction_gc(t)); +} + +TEST(dns_transaction_process_reply_no_do_strict) { + _cleanup_(transaction_env_teardown) TransactionEnv env = {}; + DnsTransaction *t; + + t = process_unsigned_reply(&env, DNSSEC_YES, DNS_SERVER_FEATURE_LEVEL_TCP, + /* bypass_do= */ -1, /* server_incapable= */ false); + + /* Still validated, so it fails on the DS lookup this needs, not on the server */ + ASSERT_EQ(t->state, DNS_TRANSACTION_DNSSEC_FAILED); + ASSERT_EQ(t->answer_dnssec_result, DNSSEC_FAILED_AUXILIARY); + ASSERT_FALSE(FLAGS_SET(t->answer_query_flags, SD_RESOLVED_AUTHENTICATED)); + + t->block_gc--; + ASSERT_NULL(dns_transaction_gc(t)); +} + +TEST(dns_transaction_process_reply_do_allow_downgrade) { + _cleanup_(transaction_env_teardown) TransactionEnv env = {}; + DnsTransaction *t; + + t = process_unsigned_reply(&env, DNSSEC_ALLOW_DOWNGRADE, DNS_SERVER_FEATURE_LEVEL_DO, + /* bypass_do= */ -1, /* server_incapable= */ false); + + /* Sent with DO, so the reply is validated as usual: the DS lookup is requested, and fails here */ + ASSERT_EQ(t->state, DNS_TRANSACTION_DNSSEC_FAILED); + ASSERT_EQ(t->answer_dnssec_result, DNSSEC_FAILED_AUXILIARY); + ASSERT_FALSE(set_isempty(t->dnssec_transactions)); + ASSERT_TRUE(dns_server_dnssec_supported(env.server)); + + t->block_gc--; + ASSERT_NULL(dns_transaction_gc(t)); +} + +TEST(dns_transaction_process_reply_no_do_incapable_server) { + _cleanup_(transaction_env_teardown) TransactionEnv env = {}; + DnsTransaction *t; + + t = process_unsigned_reply(&env, DNSSEC_ALLOW_DOWNGRADE, DNS_SERVER_FEATURE_LEVEL_TCP, + /* bypass_do= */ -1, /* server_incapable= */ true); + + /* Same outcome, but this time the server is to blame, and the downgrade notice is due */ + ASSERT_EQ(t->state, DNS_TRANSACTION_SUCCESS); + ASSERT_EQ(t->answer_dnssec_result, DNSSEC_INCOMPATIBLE_SERVER); + ASSERT_TRUE(env.server->warned_downgrade); + + /* Cached too, later lookups cannot be validated either */ + ASSERT_GT(dns_cache_size(&env.scope->cache), 0u); + + t->block_gc--; + ASSERT_NULL(dns_transaction_gc(t)); +} + +TEST(dns_transaction_process_reply_no_do_server_back_at_do) { + _cleanup_(transaction_env_teardown) TransactionEnv env = {}; + _cleanup_(dns_packet_unrefp) DnsPacket *reply = NULL; + DnsTransaction *t; + + transaction_env_setup(&env, DNSSEC_ALLOW_DOWNGRADE); + env.server->verified_feature_level = DNS_SERVER_FEATURE_LEVEL_DO; + env.server->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_DO; + + /* A query sent below DO while the server was degraded, whose reply arrives once the server is back + * at DO, e.g. because the grace period ended in between */ + transaction_setup_sent(&env, DNS_SERVER_FEATURE_LEVEL_EDNS0, /* bypass_do= */ -1, &t, &reply); + dns_transaction_process_reply(t, reply, /* encrypted= */ false); + + /* Accepted as unsigned, as it cannot be validated, but not cached, as later lookups go out with DO + * and get validated */ + ASSERT_EQ(t->state, DNS_TRANSACTION_SUCCESS); + ASSERT_EQ(t->answer_dnssec_result, DNSSEC_INCOMPATIBLE_SERVER); + ASSERT_EQ(dns_server_possible_feature_level(env.server), DNS_SERVER_FEATURE_LEVEL_DO); + ASSERT_EQ(dns_cache_size(&env.scope->cache), 0u); + + t->block_gc--; + ASSERT_NULL(dns_transaction_gc(t)); +} + +TEST(dns_transaction_process_reply_bypass_do_degraded_server) { + _cleanup_(transaction_env_teardown) TransactionEnv env = {}; + DnsTransaction *t; + + /* A client query with DO, passed on as is to a server that packet loss degraded below DO */ + t = process_unsigned_reply(&env, DNSSEC_ALLOW_DOWNGRADE, DNS_SERVER_FEATURE_LEVEL_EDNS0, + /* bypass_do= */ 1, /* server_incapable= */ false); + + /* The query went out with DO whatever the server is at, so the reply is validated as usual: the DS + * lookup is requested, and fails here */ + ASSERT_EQ(t->state, DNS_TRANSACTION_DNSSEC_FAILED); + ASSERT_EQ(t->answer_dnssec_result, DNSSEC_FAILED_AUXILIARY); + ASSERT_FALSE(set_isempty(t->dnssec_transactions)); + ASSERT_TRUE(dns_server_dnssec_supported(env.server)); + + t->block_gc--; + ASSERT_NULL(dns_transaction_gc(t)); +} + +TEST(dns_transaction_process_reply_bypass_no_do) { + _cleanup_(transaction_env_teardown) TransactionEnv env = {}; + DnsTransaction *t; + + /* A client query without DO, passed on as is to a server that is at DO */ + t = process_unsigned_reply(&env, DNSSEC_ALLOW_DOWNGRADE, DNS_SERVER_FEATURE_LEVEL_DO, + /* bypass_do= */ 0, /* server_incapable= */ false); + + /* The query went out without DO whatever the server is at, so the reply is accepted as unsigned, + * without auxiliary lookups, and without blaming the server */ + ASSERT_EQ(t->state, DNS_TRANSACTION_SUCCESS); + ASSERT_EQ(t->answer_dnssec_result, DNSSEC_INCOMPATIBLE_SERVER); + ASSERT_FALSE(FLAGS_SET(t->answer_query_flags, SD_RESOLVED_AUTHENTICATED)); + ASSERT_TRUE(set_isempty(t->dnssec_transactions)); + ASSERT_TRUE(dns_server_dnssec_supported(env.server)); + ASSERT_FALSE(env.server->warned_downgrade); + + /* But not cached, as later lookups would go out with DO and get validated */ + ASSERT_EQ(dns_cache_size(&env.scope->cache), 0u); + + t->block_gc--; + ASSERT_NULL(dns_transaction_gc(t)); +} + +TEST(dns_transaction_process_reply_do_cached_auxiliary) { + _cleanup_(transaction_env_teardown) TransactionEnv env = {}; + _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL; + _cleanup_(dns_packet_unrefp) DnsPacket *reply = NULL; + DnsTransaction *t, *aux = NULL; + + transaction_env_setup(&env, DNSSEC_ALLOW_DOWNGRADE); + env.server->verified_feature_level = DNS_SERVER_FEATURE_LEVEL_DO; + env.server->possible_feature_level = DNS_SERVER_FEATURE_LEVEL_DO; + + /* The DS lookup the reply needs, already answered from the cache on a retry, so that server and + * feature level are still those of an earlier attempt that went out below DO */ + key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_DS, "www.example.com"); + ASSERT_NOT_NULL(key); + ASSERT_OK(dns_transaction_new(&aux, env.scope, key, /* bypass= */ NULL, SD_RESOLVED_NO_NETWORK)); + aux->block_gc++; + aux->server = dns_server_ref(env.server); + aux->current_feature_level = DNS_SERVER_FEATURE_LEVEL_TCP; + aux->answer_source = DNS_TRANSACTION_CACHE; + aux->answer_rcode = DNS_RCODE_SUCCESS; + aux->state = DNS_TRANSACTION_SUCCESS; + + transaction_setup_sent(&env, DNS_SERVER_FEATURE_LEVEL_DO, /* bypass_do= */ -1, &t, &reply); + dns_transaction_process_reply(t, reply, /* encrypted= */ false); + + /* The stale feature level of the auxiliary transaction must not turn off validation */ + ASSERT_TRUE(set_contains(t->dnssec_transactions, aux)); + ASSERT_NE(t->answer_dnssec_result, DNSSEC_INCOMPATIBLE_SERVER); + ASSERT_FALSE(FLAGS_SET(t->answer_query_flags, SD_RESOLVED_AUTHENTICATED)); + + t->block_gc--; + ASSERT_NULL(dns_transaction_gc(t)); + aux->block_gc--; + ASSERT_NULL(dns_transaction_gc(aux)); +} + +DEFINE_TEST_MAIN(LOG_DEBUG);