resolved: when validating, first strip revoked trust anchor keys from validated keys list

When validating a transaction we initially collect DNSKEY, DS, SOA RRs
in the "validated_keys" list, that we need for the proofs. This includes
DNSKEY and DS data from our trust anchor database. Quite possibly we
learn that some of these DNSKEY/DS RRs have been revoked between the
time we request and collect those additional RRs and we begin the
validation step. In this case we need to make sure that the respective
DS/DNSKEY RRs are removed again from our list. This patch adds that, and
strips known revoked trust anchor RRs from the validated list before we
begin the actual validation proof, and each time we add more DNSKEY
material to it while we are doing the proof.

src/resolve/resolved-dns-rr.c

@@ -1085,6 +1085,156 @@ int dns_resource_record_to_wire_format(DnsResourceRecord *rr, bool canonical) {
         return 0;
 }
 
+static void dns_resource_record_hash_func(const void *i, struct siphash *state) {
+        const DnsResourceRecord *rr = i;
+
+        assert(rr);
+
+        dns_resource_key_hash_func(rr->key, state);
+
+        switch (rr->unparseable ? _DNS_TYPE_INVALID : rr->key->type) {
+
+        case DNS_TYPE_SRV:
+                siphash24_compress(&rr->srv.priority, sizeof(rr->srv.priority), state);
+                siphash24_compress(&rr->srv.weight, sizeof(rr->srv.weight), state);
+                siphash24_compress(&rr->srv.port, sizeof(rr->srv.port), state);
+                dns_name_hash_func(rr->srv.name, state);
+                break;
+
+        case DNS_TYPE_PTR:
+        case DNS_TYPE_NS:
+        case DNS_TYPE_CNAME:
+        case DNS_TYPE_DNAME:
+                dns_name_hash_func(rr->ptr.name, state);
+                break;
+
+        case DNS_TYPE_HINFO:
+                string_hash_func(rr->hinfo.cpu, state);
+                string_hash_func(rr->hinfo.os, state);
+                break;
+
+        case DNS_TYPE_TXT:
+        case DNS_TYPE_SPF: {
+                DnsTxtItem *j;
+
+                LIST_FOREACH(items, j, rr->txt.items) {
+                        siphash24_compress(j->data, j->length, state);
+
+                        /* Add an extra NUL byte, so that "a" followed by "b" doesn't result in the same hash as "ab" followed by "". */
+                        siphash24_compress((const uint8_t[]) { 0 }, 1, state);
+                }
+                break;
+        }
+
+        case DNS_TYPE_A:
+                siphash24_compress(&rr->a.in_addr, sizeof(rr->a.in_addr), state);
+                break;
+
+        case DNS_TYPE_AAAA:
+                siphash24_compress(&rr->aaaa.in6_addr, sizeof(rr->aaaa.in6_addr), state);
+                break;
+
+        case DNS_TYPE_SOA:
+                dns_name_hash_func(rr->soa.mname, state);
+                dns_name_hash_func(rr->soa.rname, state);
+                siphash24_compress(&rr->soa.serial, sizeof(rr->soa.serial), state);
+                siphash24_compress(&rr->soa.refresh, sizeof(rr->soa.refresh), state);
+                siphash24_compress(&rr->soa.retry, sizeof(rr->soa.retry), state);
+                siphash24_compress(&rr->soa.expire, sizeof(rr->soa.expire), state);
+                siphash24_compress(&rr->soa.minimum, sizeof(rr->soa.minimum), state);
+                break;
+
+        case DNS_TYPE_MX:
+                siphash24_compress(&rr->mx.priority, sizeof(rr->mx.priority), state);
+                dns_name_hash_func(rr->mx.exchange, state);
+                break;
+
+        case DNS_TYPE_LOC:
+                siphash24_compress(&rr->loc.version, sizeof(rr->loc.version), state);
+                siphash24_compress(&rr->loc.size, sizeof(rr->loc.size), state);
+                siphash24_compress(&rr->loc.horiz_pre, sizeof(rr->loc.horiz_pre), state);
+                siphash24_compress(&rr->loc.vert_pre, sizeof(rr->loc.vert_pre), state);
+                siphash24_compress(&rr->loc.latitude, sizeof(rr->loc.latitude), state);
+                siphash24_compress(&rr->loc.longitude, sizeof(rr->loc.longitude), state);
+                siphash24_compress(&rr->loc.altitude, sizeof(rr->loc.altitude), state);
+                break;
+
+        case DNS_TYPE_SSHFP:
+                siphash24_compress(&rr->sshfp.algorithm, sizeof(rr->sshfp.algorithm), state);
+                siphash24_compress(&rr->sshfp.fptype, sizeof(rr->sshfp.fptype), state);
+                siphash24_compress(rr->sshfp.fingerprint, rr->sshfp.fingerprint_size, state);
+                break;
+
+        case DNS_TYPE_DNSKEY:
+                siphash24_compress(&rr->dnskey.flags, sizeof(rr->dnskey.flags), state);
+                siphash24_compress(&rr->dnskey.protocol, sizeof(rr->dnskey.protocol), state);
+                siphash24_compress(&rr->dnskey.algorithm, sizeof(rr->dnskey.algorithm), state);
+                siphash24_compress(rr->dnskey.key, rr->dnskey.key_size, state);
+                break;
+
+        case DNS_TYPE_RRSIG:
+                siphash24_compress(&rr->rrsig.type_covered, sizeof(rr->rrsig.type_covered), state);
+                siphash24_compress(&rr->rrsig.algorithm, sizeof(rr->rrsig.algorithm), state);
+                siphash24_compress(&rr->rrsig.labels, sizeof(rr->rrsig.labels), state);
+                siphash24_compress(&rr->rrsig.original_ttl, sizeof(rr->rrsig.original_ttl), state);
+                siphash24_compress(&rr->rrsig.expiration, sizeof(rr->rrsig.expiration), state);
+                siphash24_compress(&rr->rrsig.inception, sizeof(rr->rrsig.inception), state);
+                siphash24_compress(&rr->rrsig.key_tag, sizeof(rr->rrsig.key_tag), state);
+                dns_name_hash_func(rr->rrsig.signer, state);
+                siphash24_compress(rr->rrsig.signature, rr->rrsig.signature_size, state);
+                break;
+
+        case DNS_TYPE_NSEC:
+                dns_name_hash_func(rr->nsec.next_domain_name, state);
+                /* FIXME: we leave out the type bitmap here. Hash
+                 * would be better if we'd take it into account
+                 * too. */
+                break;
+
+        case DNS_TYPE_DS:
+                siphash24_compress(&rr->ds.key_tag, sizeof(rr->ds.key_tag), state);
+                siphash24_compress(&rr->ds.algorithm, sizeof(rr->ds.algorithm), state);
+                siphash24_compress(&rr->ds.digest_type, sizeof(rr->ds.digest_type), state);
+                siphash24_compress(rr->ds.digest, rr->ds.digest_size, state);
+                break;
+
+        case DNS_TYPE_NSEC3:
+                siphash24_compress(&rr->nsec3.algorithm, sizeof(rr->nsec3.algorithm), state);
+                siphash24_compress(&rr->nsec3.flags, sizeof(rr->nsec3.flags), state);
+                siphash24_compress(&rr->nsec3.iterations, sizeof(rr->nsec3.iterations), state);
+                siphash24_compress(rr->nsec3.salt, rr->nsec3.salt_size, state);
+                siphash24_compress(rr->nsec3.next_hashed_name, rr->nsec3.next_hashed_name_size, state);
+                /* FIXME: We leave the bitmaps out */
+                break;
+
+        default:
+                siphash24_compress(rr->generic.data, rr->generic.size, state);
+                break;
+        }
+}
+
+static int dns_resource_record_compare_func(const void *a, const void *b) {
+        const DnsResourceRecord *x = a, *y = b;
+        int ret;
+
+        ret = dns_resource_key_compare_func(x->key, y->key);
+        if (ret != 0)
+                return ret;
+
+        if (dns_resource_record_equal(x, y))
+                return 0;
+
+        /* This is a bit dirty, we don't implement proper odering, but
+         * the hashtable doesn't need ordering anyway, hence we don't
+         * care. */
+        return x < y ? -1 : 1;
+}
+
+const struct hash_ops dns_resource_record_hash_ops = {
+        .hash = dns_resource_record_hash_func,
+        .compare = dns_resource_record_compare_func,
+};
+
 DnsTxtItem *dns_txt_item_free_all(DnsTxtItem *i) {
         DnsTxtItem *n;
 

src/resolve/resolved-dns-rr.h

@@ -300,6 +300,7 @@ DnsTxtItem *dns_txt_item_free_all(DnsTxtItem *i);
 bool dns_txt_item_equal(DnsTxtItem *a, DnsTxtItem *b);
 
 extern const struct hash_ops dns_resource_key_hash_ops;
+extern const struct hash_ops dns_resource_record_hash_ops;
 
 int dnssec_algorithm_to_string_alloc(int i, char **ret);
 int dnssec_algorithm_from_string(const char *s) _pure_;

src/resolve/resolved-dns-transaction.c

@@ -2247,6 +2247,39 @@ static int dns_transaction_check_revoked_trust_anchors(DnsTransaction *t) {
         return 0;
 }
 
+static int dns_transaction_invalidate_revoked_keys(DnsTransaction *t) {
+        bool changed;
+        int r;
+
+        assert(t);
+
+        /* Removes all DNSKEY/DS objects from t->validated_keys that
+         * our trust anchors database considers revoked. */
+
+        do {
+                DnsResourceRecord *rr;
+
+                changed = false;
+
+                DNS_ANSWER_FOREACH(rr, t->validated_keys) {
+                        r = dns_trust_anchor_is_revoked(&t->scope->manager->trust_anchor, rr);
+                        if (r < 0)
+                                return r;
+                        if (r > 0) {
+                                r = dns_answer_remove_by_rr(&t->validated_keys, rr);
+                                if (r < 0)
+                                        return r;
+
+                                assert(r > 0);
+                                changed = true;
+                                break;
+                        }
+                }
+        } while (changed);
+
+        return 0;
+}
+
 int dns_transaction_validate_dnssec(DnsTransaction *t) {
         _cleanup_(dns_answer_unrefp) DnsAnswer *validated = NULL;
         bool dnskeys_finalized = false;
@@ -2287,16 +2320,26 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) {
 
         log_debug("Validating response from transaction %" PRIu16 " (%s).", t->id, dns_transaction_key_string(t));
 
-        /* First, see if this response contains any revoked trust anchors we care about */
+        /* First, see if this response contains any revoked trust
+         * anchors we care about */
         r = dns_transaction_check_revoked_trust_anchors(t);
         if (r < 0)
                 return r;
 
-        /* Second see if there are DNSKEYs we already know a validated DS for. */
+        /* Second, see if there are DNSKEYs we already know a
+         * validated DS for. */
         r = dns_transaction_validate_dnskey_by_ds(t);
         if (r < 0)
                 return r;
 
+        /* Third, remove all DNSKEY and DS RRs again that our trust
+         * anchor says are revoked. After all we might have marked
+         * some keys revoked above, but they might still be lingering
+         * in our validated_keys list. */
+        r = dns_transaction_invalidate_revoked_keys(t);
+        if (r < 0)
+                return r;
+
         for (;;) {
                 bool changed = false;
 
@@ -2324,6 +2367,14 @@ int dns_transaction_validate_dnssec(DnsTransaction *t) {
                                         r = dns_answer_copy_by_key(&t->validated_keys, t->answer, rr->key, DNS_ANSWER_AUTHENTICATED);
                                         if (r < 0)
                                                 return r;
+
+                                        /* some of the DNSKEYs we just
+                                         * added might already have
+                                         * been revoked, remove them
+                                         * again in that case. */
+                                        r = dns_transaction_invalidate_revoked_keys(t);
+                                        if (r < 0)
+                                                return r;
                                 }
 
                                 /* Add the validated RRset to the new

src/resolve/resolved-dns-trust-anchor.c

@@ -511,13 +511,18 @@ int dns_trust_anchor_load(DnsTrustAnchor *d) {
 
 void dns_trust_anchor_flush(DnsTrustAnchor *d) {
         DnsAnswer *a;
+        DnsResourceRecord *rr;
 
         assert(d);
 
         while ((a = hashmap_steal_first(d->positive_by_key)))
                 dns_answer_unref(a);
-
         d->positive_by_key = hashmap_free(d->positive_by_key);
+
+        while ((rr = set_steal_first(d->revoked_by_rr)))
+                dns_resource_record_unref(rr);
+        d->revoked_by_rr = set_free(d->revoked_by_rr);
+
         d->negative_by_name = set_free_free(d->negative_by_name);
 }
 
@@ -547,11 +552,35 @@ int dns_trust_anchor_lookup_negative(DnsTrustAnchor *d, const char *name) {
         return set_contains(d->negative_by_name, name);
 }
 
+static int dns_trust_anchor_revoked_put(DnsTrustAnchor *d, DnsResourceRecord *rr) {
+        int r;
+
+        assert(d);
+
+        r = set_ensure_allocated(&d->revoked_by_rr, &dns_resource_record_hash_ops);
+        if (r < 0)
+                return r;
+
+        r = set_put(d->revoked_by_rr, rr);
+        if (r < 0)
+                return r;
+        if (r > 0)
+                dns_resource_record_ref(rr);
+
+        return r;
+}
+
 static int dns_trust_anchor_remove_revoked(DnsTrustAnchor *d, DnsResourceRecord *rr) {
         _cleanup_(dns_answer_unrefp) DnsAnswer *new_answer = NULL;
         DnsAnswer *old_answer;
         int r;
 
+        /* Remember that this is a revoked trust anchor RR */
+        r = dns_trust_anchor_revoked_put(d, rr);
+        if (r < 0)
+                return r;
+
+        /* Remove this from the positive trust anchor */
         old_answer = hashmap_get(d->positive_by_key, rr->key);
         if (!old_answer)
                 return 0;
@@ -610,6 +639,9 @@ static int dns_trust_anchor_check_revoked_one(DnsTrustAnchor *d, DnsResourceReco
                         if (anchor->dnskey.key_size != revoked_dnskey->dnskey.key_size)
                                 continue;
 
+                        /* Note that we allow the REVOKE bit to be
+                         * different! It will be set in the revoked
+                         * key, but unset in our version of it */
                         if (((anchor->dnskey.flags ^ revoked_dnskey->dnskey.flags) | DNSKEY_FLAG_REVOKE) != DNSKEY_FLAG_REVOKE)
                                 continue;
 
@@ -629,6 +661,10 @@ static int dns_trust_anchor_check_revoked_one(DnsTrustAnchor *d, DnsResourceReco
 
                 DNS_ANSWER_FOREACH(anchor, a) {
 
+                        /* We set mask_revoke to true here, since our
+                         * DS fingerprint will be the one of the
+                         * unrevoked DNSKEY, but the one we got passed
+                         * here has the bit set. */
                         r = dnssec_verify_dnskey(revoked_dnskey, anchor, true);
                         if (r < 0)
                                 return r;
@@ -698,3 +734,12 @@ int dns_trust_anchor_check_revoked(DnsTrustAnchor *d, DnsResourceRecord *dnskey,
 
         return 0;
 }
+
+int dns_trust_anchor_is_revoked(DnsTrustAnchor *d, DnsResourceRecord *rr) {
+        assert(d);
+
+        if (!IN_SET(rr->key->type, DNS_TYPE_DS, DNS_TYPE_DNSKEY))
+                return 0;
+
+        return set_contains(d->revoked_by_rr, rr);
+}

src/resolve/resolved-dns-trust-anchor.h

@@ -32,6 +32,7 @@ typedef struct DnsTrustAnchor DnsTrustAnchor;
 struct DnsTrustAnchor {
         Hashmap *positive_by_key;
         Set *negative_by_name;
+        Set *revoked_by_rr;
 };
 
 int dns_trust_anchor_load(DnsTrustAnchor *d);
@@ -41,3 +42,4 @@ int dns_trust_anchor_lookup_positive(DnsTrustAnchor *d, const DnsResourceKey* ke
 int dns_trust_anchor_lookup_negative(DnsTrustAnchor *d, const char *name);
 
 int dns_trust_anchor_check_revoked(DnsTrustAnchor *d, DnsResourceRecord *dnskey, DnsAnswer *rrs);
+int dns_trust_anchor_is_revoked(DnsTrustAnchor *d, DnsResourceRecord *rr);