Invalid signed wildcard records were being accepted

An RRSIG whose Labels field indicates fewer labels than its signer
name requires was being accepted.  When such a record covers a
wildcard, the validator reconstructs a wildcard owner name above the
signer's zone and caches it as secure.  RFC 8198 cache synthesis
(synth-from-dnssec) then serves that forged wildcard for unrelated
names, poisoning the cache.

These records are now rejected, both when an RRSIG is parsed and when
its signature is verified.

diff --git a/lib/dns/dnssec.c b/lib/dns/dnssec.c
index 3b4c6672a6a404600b1c7ea9617e86d3cc61eeb8..d71d1e68b9720276fa472b012889b5ccf7b9b6ee 100644 (file)
--- a/lib/dns/dnssec.c
+++ b/lib/dns/dnssec.c
@@ -127,7 +127,7 @@ dns_dnssec_keyfromrdata(const dns_name_t *name, const dns_rdata_t *rdata,
        isc_buffer_t b;
        isc_region_t r;
 
-       INSIST(name != NULL);
+       INSIST(DNS_NAME_VALID(name));
        INSIST(rdata != NULL);
        INSIST(mctx != NULL);
        INSIST(key != NULL);
@@ -180,12 +180,14 @@ dns_dnssec_sign(const dns_name_t *name, dns_rdataset_t *set, dst_key_t *key,
        isc_result_t result;
        isc_buffer_t *databuf = NULL;
        char data[256 + 8];
+       unsigned int labels;
        unsigned int sigsize;
        dns_fixedname_t fnewname;
        dns_fixedname_t fsigner;
 
-       REQUIRE(name != NULL);
-       REQUIRE(dns_name_countlabels(name) <= 255);
+       REQUIRE(DNS_NAME_VALID(name));
+       labels = dns_name_countlabels(name);
+       REQUIRE(labels <= 255 && labels > 0);
        REQUIRE(set != NULL);
        REQUIRE(key != NULL);
        REQUIRE(inception != NULL);
@@ -213,7 +215,7 @@ dns_dnssec_sign(const dns_name_t *name, dns_rdataset_t *set, dst_key_t *key,
 
        sig.covered = set->type;
        sig.algorithm = dst_algorithm_tosecalg(dst_key_alg(key));
-       sig.labels = dns_name_countlabels(name) - 1;
+       sig.labels = labels - 1;
        if (dns_name_iswildcard(name)) {
                sig.labels--;
        }
@@ -353,10 +355,13 @@ dns_dnssec_verify(const dns_name_t *name, dns_rdataset_t *set, dst_key_t *key,
        isc_result_t result;
        unsigned char data[300];
        dst_context_t *ctx = NULL;
-       int labels = 0;
+       unsigned int labels;
+       unsigned int siglabels;
        bool downcase = false;
 
-       REQUIRE(name != NULL);
+       REQUIRE(DNS_NAME_VALID(name));
+       labels = dns_name_countlabels(name);
+       REQUIRE(labels > 0);
        REQUIRE(set != NULL);
        REQUIRE(key != NULL);
        REQUIRE(mctx != NULL);
@@ -368,6 +373,21 @@ dns_dnssec_verify(const dns_name_t *name, dns_rdataset_t *set, dst_key_t *key,
                return DNS_R_SIGINVALID;
        }
 
+       /*
+        * The RRSIG labels field can't indicate fewer labels than the
+        * signer.  Also the labels shouldn't be greater than that of
+        * the owner name.
+        *
+        * sig.labels doesn't include the root label, so add 1 to account
+        * for it.
+        */
+       siglabels = sig.labels + 1;
+       if (siglabels < dns_name_countlabels(&sig.signer) || siglabels > labels)
+       {
+               inc_stat(dns_dnssecstats_fail);
+               return DNS_R_SIGINVALID;
+       }
+
        if (isc_serial_lt(sig.timeexpire, sig.timesigned)) {
                inc_stat(dns_dnssecstats_fail);
                return DNS_R_SIGINVALID;
@@ -434,12 +454,11 @@ again:
         * If the name is an expanded wildcard, use the wildcard name.
         */
        dns_fixedname_init(&fnewname);
-       labels = dns_name_countlabels(name) - 1;
        RUNTIME_CHECK(dns_name_downcase(name, dns_fixedname_name(&fnewname)) ==
                      ISC_R_SUCCESS);
-       if (labels - sig.labels > 0) {
-               dns_name_split(dns_fixedname_name(&fnewname), sig.labels + 1,
-                              NULL, dns_fixedname_name(&fnewname));
+       if (labels > siglabels) {
+               dns_name_split(dns_fixedname_name(&fnewname), siglabels, NULL,
+                              dns_fixedname_name(&fnewname));
        }
 
        dns_name_toregion(dns_fixedname_name(&fnewname), &r);
@@ -448,7 +467,7 @@ again:
         * Create an envelope for each rdata: <name|type|class|ttl>.
         */
        isc_buffer_init(&envbuf, data, sizeof(data));
-       if (labels - sig.labels > 0) {
+       if (labels > siglabels) {
                isc_buffer_putuint8(&envbuf, 1);
                isc_buffer_putuint8(&envbuf, '*');
                memmove(data + 2, r.base, r.length);
@@ -543,7 +562,7 @@ cleanup_struct:
                inc_stat(dns_dnssecstats_fail);
        }
 
-       if (result == ISC_R_SUCCESS && labels - sig.labels > 0) {
+       if (result == ISC_R_SUCCESS && labels > siglabels) {
                if (wild != NULL) {
                        RUNTIME_CHECK(dns_name_concatenate(
                                              dns_wildcardname,

diff --git a/lib/dns/rdata/generic/rrsig_46.c b/lib/dns/rdata/generic/rrsig_46.c
index 647c45b519f60bea9b27ef3269910bb2919f5e6c..a4f9b4916b5291a70918db35585513a87d1aeb17 100644 (file)
--- a/lib/dns/rdata/generic/rrsig_46.c
+++ b/lib/dns/rdata/generic/rrsig_46.c
@@ -23,14 +23,16 @@
 static isc_result_t
 fromtext_rrsig(ARGS_FROMTEXT) {
        isc_token_t token;
-       unsigned char alg, c;
+       unsigned char alg, labels;
        long i;
        dns_rdatatype_t covered;
-       char *e;
+       char *e = NULL;
        isc_result_t result;
        isc_buffer_t buffer;
        uint32_t time_signed, time_expire;
        unsigned int used;
+       dns_fixedname_t fixed;
+       dns_name_t *signer = dns_fixedname_initname(&fixed);
 
        REQUIRE(type == dns_rdatatype_rrsig);
 
@@ -72,8 +74,8 @@ fromtext_rrsig(ARGS_FROMTEXT) {
        if (token.value.as_ulong > 0xffU) {
                RETTOK(ISC_R_RANGE);
        }
-       c = (unsigned char)token.value.as_ulong;
-       RETERR(mem_tobuffer(target, &c, 1));
+       labels = (unsigned char)token.value.as_ulong;
+       RETERR(mem_tobuffer(target, &labels, 1));
 
        /*
         * Original ttl.
@@ -148,7 +150,16 @@ fromtext_rrsig(ARGS_FROMTEXT) {
        if (origin == NULL) {
                origin = dns_rootname;
        }
-       RETTOK(dns_name_wirefromtext(&buffer, origin, options, target));
+       RETTOK(dns_name_fromtext(signer, &buffer, origin, options));
+
+       /*
+        * (RRSIG labels doesn't include the root label, so add one
+        * to normalize it before checking against the signer.)
+        */
+       if ((labels + 1) < dns_name_countlabels(signer)) {
+               RETTOK(ISC_R_RANGE);
+       }
+       RETERR(mem_tobuffer(target, signer->ndata, signer->length));
 
        /*
         * Sig.
@@ -295,6 +306,7 @@ fromwire_rrsig(ARGS_FROMWIRE) {
        isc_region_t sr;
        dns_name_t name;
        unsigned char algorithm;
+       unsigned char labels;
 
        REQUIRE(type == dns_rdatatype_rrsig);
 
@@ -318,6 +330,7 @@ fromwire_rrsig(ARGS_FROMWIRE) {
        }
 
        algorithm = sr.base[2];
+       labels = sr.base[3];
 
        isc_buffer_forward(source, 18);
        RETERR(mem_tobuffer(target, sr.base, 18));
@@ -328,6 +341,14 @@ fromwire_rrsig(ARGS_FROMWIRE) {
        dns_name_init(&name);
        RETERR(dns_name_fromwire(&name, source, dctx, target));
 
+       /*
+        * (RRSIG labels doesn't include the root label, so add one
+        * to normalize it before checking against the signer.)
+        */
+       if ((labels + 1) < dns_name_countlabels(&name)) {
+               RETERR(DNS_R_FORMERR);
+       }
+
        /*
         * Sig.
         */