Merge pull request #690 from teg/resolved-fixes-2

resolved: assorted fixes v2

6 files changed, 101 insertions, 17 deletions

diff --git a/src/resolve/dns-type.c b/src/resolve/dns-type.c
index a3e740896f..e1087b3219 100644
--- a/src/resolve/dns-type.c
+++ b/src/resolve/dns-type.c
@@ -43,3 +43,8 @@ int dns_type_from_string(const char *s) {
 
         return sc->id;
 }
+
+/* XXX: find an authorotative list of all pseudo types? */
+bool dns_type_is_pseudo(int n) {
+        return IN_SET(n, DNS_TYPE_ANY, DNS_TYPE_AXFR, DNS_TYPE_IXFR, DNS_TYPE_OPT);
+}
diff --git a/src/resolve/dns-type.h b/src/resolve/dns-type.h
index 86951d233a..950af36ee3 100644
--- a/src/resolve/dns-type.h
+++ b/src/resolve/dns-type.h
@@ -25,6 +25,7 @@
 
 const char *dns_type_to_string(int type);
 int dns_type_from_string(const char *s);
+bool dns_type_is_pseudo(int n);
 
 /* DNS record types, taken from
  * http://www.iana.org/assignments/dns-parameters/dns-parameters.xhtml.
diff --git a/src/resolve/resolved-dns-packet.c b/src/resolve/resolved-dns-packet.c
index 773691f066..649e8b74e1 100644
--- a/src/resolve/resolved-dns-packet.c
+++ b/src/resolve/resolved-dns-packet.c
@@ -275,7 +275,7 @@ static void dns_packet_truncate(DnsPacket *p, size_t sz) {
         if (p->size <= sz)
                 return;
 
-        HASHMAP_FOREACH_KEY(s, n, p->names, i) {
+        HASHMAP_FOREACH_KEY(n, s, p->names, i) {
 
                 if (PTR_TO_SIZE(n) < sz)
                         continue;
@@ -761,7 +761,7 @@ int dns_packet_append_rr(DnsPacket *p, const DnsResourceRecord *rr, size_t *star
                 if (r < 0)
                         goto fail;
 
-                r = dns_packet_append_blob(p, rr->sshfp.key, rr->sshfp.key_size, NULL);
+                r = dns_packet_append_blob(p, rr->sshfp.fingerprint, rr->sshfp.fingerprint_size, NULL);
                 break;
 
         case DNS_TYPE_DNSKEY:
@@ -1208,9 +1208,12 @@ static int dns_packet_read_type_window(DnsPacket *p, Bitmap **types, size_t *sta
                         if (bitmap[i] & bitmask) {
                                 uint16_t n;
 
-                                /* XXX: ignore pseudo-types? see RFC4034 section 4.1.2 */
                                 n = (uint16_t) window << 8 | (uint16_t) bit;
 
+                                /* Ignore pseudo-types. see RFC4034 section 4.1.2 */
+                                if (dns_type_is_pseudo(n))
+                                        continue;
+
                                 r = bitmap_set(*types, n);
                                 if (r < 0)
                                         goto fail;
@@ -1233,6 +1236,38 @@ fail:
         return r;
 }
 
+static int dns_packet_read_type_windows(DnsPacket *p, Bitmap **types, size_t size, size_t *start) {
+        size_t saved_rindex;
+        int r;
+
+        saved_rindex = p->rindex;
+
+        while (p->rindex < saved_rindex + size) {
+                r = dns_packet_read_type_window(p, types, NULL);
+                if (r < 0)
+                        goto fail;
+
+                /* don't read past end of current RR */
+                if (p->rindex > saved_rindex + size) {
+                        r = -EBADMSG;
+                        goto fail;
+                }
+        }
+
+        if (p->rindex != saved_rindex + size) {
+                r = -EBADMSG;
+                goto fail;
+        }
+
+        if (start)
+                *start = saved_rindex;
+
+        return 0;
+fail:
+        dns_packet_rewind(p, saved_rindex);
+        return r;
+}
+
 int dns_packet_read_key(DnsPacket *p, DnsResourceKey **ret, size_t *start) {
         _cleanup_free_ char *name = NULL;
         uint16_t class, type;
@@ -1513,6 +1548,13 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) {
                 if (r < 0)
                         goto fail;
 
+                if (rr->ds.digest_size <= 0) {
+                        /* the accepted size depends on the algorithm, but for now
+                           just ensure that the value is greater than zero */
+                        r = -EBADMSG;
+                        goto fail;
+                }
+
                 break;
         case DNS_TYPE_SSHFP:
                 r = dns_packet_read_uint8(p, &rr->sshfp.algorithm, NULL);
@@ -1524,8 +1566,16 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) {
                         goto fail;
 
                 r = dns_packet_read_memdup(p, rdlength - 2,
-                                           &rr->sshfp.key, &rr->sshfp.key_size,
+                                           &rr->sshfp.fingerprint, &rr->sshfp.fingerprint_size,
                                            NULL);
+
+                if (rr->sshfp.fingerprint_size <= 0) {
+                        /* the accepted size depends on the algorithm, but for now
+                           just ensure that the value is greater than zero */
+                        r = -EBADMSG;
+                        goto fail;
+                }
+
                 break;
 
         case DNS_TYPE_DNSKEY: {
@@ -1557,6 +1607,14 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) {
                 r = dns_packet_read_memdup(p, rdlength - 4,
                                            &rr->dnskey.key, &rr->dnskey.key_size,
                                            NULL);
+
+                if (rr->dnskey.key_size <= 0) {
+                        /* the accepted size depends on the algorithm, but for now
+                           just ensure that the value is greater than zero */
+                        r = -EBADMSG;
+                        goto fail;
+                }
+
                 break;
         }
 
@@ -1596,6 +1654,14 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) {
                 r = dns_packet_read_memdup(p, offset + rdlength - p->rindex,
                                            &rr->rrsig.signature, &rr->rrsig.signature_size,
                                            NULL);
+
+                if (rr->rrsig.signature_size <= 0) {
+                        /* the accepted size depends on the algorithm, but for now
+                           just ensure that the value is greater than zero */
+                        r = -EBADMSG;
+                        goto fail;
+                }
+
                 break;
 
         case DNS_TYPE_NSEC:
@@ -1603,11 +1669,12 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) {
                 if (r < 0)
                         goto fail;
 
-                while (p->rindex != offset + rdlength) {
-                        r = dns_packet_read_type_window(p, &rr->nsec.types, NULL);
-                        if (r < 0)
-                                goto fail;
-                }
+                r = dns_packet_read_type_windows(p, &rr->nsec.types, offset + rdlength - p->rindex, NULL);
+                if (r < 0)
+                        goto fail;
+
+                /* NSEC RRs with empty bitmpas makes no sense, but the RFC does not explicitly forbid them
+                   so we allow it */
 
                 break;
 
@@ -1626,6 +1693,7 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) {
                 if (r < 0)
                         goto fail;
 
+                /* this may be zero */
                 r = dns_packet_read_uint8(p, &size, NULL);
                 if (r < 0)
                         goto fail;
@@ -1638,14 +1706,21 @@ int dns_packet_read_rr(DnsPacket *p, DnsResourceRecord **ret, size_t *start) {
                 if (r < 0)
                         goto fail;
 
+                if (size <= 0) {
+                        r = -EBADMSG;
+                        goto fail;
+                }
+
                 r = dns_packet_read_memdup(p, size, &rr->nsec3.next_hashed_name, &rr->nsec3.next_hashed_name_size, NULL);
                 if (r < 0)
                         goto fail;
 
-                r = dns_packet_append_types(p, rr->nsec3.types, NULL);
+                r = dns_packet_read_type_windows(p, &rr->nsec.types, offset + rdlength - p->rindex, NULL);
                 if (r < 0)
                         goto fail;
 
+                /* empty non-terminals can have NSEC3 records, so empty bitmaps are allowed */
+
                 break;
         }
         default:
diff --git a/src/resolve/resolved-dns-rr.c b/src/resolve/resolved-dns-rr.c
index 9efe4b3c08..2bc9f2b520 100644
--- a/src/resolve/resolved-dns-rr.c
+++ b/src/resolve/resolved-dns-rr.c
@@ -276,7 +276,7 @@ DnsResourceRecord* dns_resource_record_unref(DnsResourceRecord *rr) {
                         break;
 
                 case DNS_TYPE_SSHFP:
-                        free(rr->sshfp.key);
+                        free(rr->sshfp.fingerprint);
                         break;
 
                 case DNS_TYPE_DNSKEY:
@@ -434,8 +434,8 @@ int dns_resource_record_equal(const DnsResourceRecord *a, const DnsResourceRecor
         case DNS_TYPE_SSHFP:
                 return a->sshfp.algorithm == b->sshfp.algorithm &&
                        a->sshfp.fptype == b->sshfp.fptype &&
-                       a->sshfp.key_size == b->sshfp.key_size &&
-                       memcmp(a->sshfp.key, b->sshfp.key, a->sshfp.key_size) == 0;
+                       a->sshfp.fingerprint_size == b->sshfp.fingerprint_size &&
+                       memcmp(a->sshfp.fingerprint, b->sshfp.fingerprint, a->sshfp.fingerprint_size) == 0;
 
         case DNS_TYPE_DNSKEY:
                 return a->dnskey.zone_key_flag == b->dnskey.zone_key_flag &&
@@ -687,7 +687,7 @@ int dns_resource_record_to_string(const DnsResourceRecord *rr, char **ret) {
                 break;
 
         case DNS_TYPE_SSHFP:
-                t = hexmem(rr->sshfp.key, rr->sshfp.key_size);
+                t = hexmem(rr->sshfp.fingerprint, rr->sshfp.fingerprint_size);
                 if (!t)
                         return -ENOMEM;
 
diff --git a/src/resolve/resolved-dns-rr.h b/src/resolve/resolved-dns-rr.h
index dc51126f97..0f40f3ceef 100644
--- a/src/resolve/resolved-dns-rr.h
+++ b/src/resolve/resolved-dns-rr.h
@@ -117,11 +117,12 @@ struct DnsResourceRecord {
                         size_t digest_size;
                 } ds;
 
+                /* https://tools.ietf.org/html/rfc4255#section-3.1 */
                 struct {
                         uint8_t algorithm;
                         uint8_t fptype;
-                        void *key;
-                        size_t key_size;
+                        void *fingerprint;
+                        size_t fingerprint_size;
                 } sshfp;
 
                 /* http://tools.ietf.org/html/rfc4034#section-2.1 */
diff --git a/src/resolve/resolved-dns-transaction.c b/src/resolve/resolved-dns-transaction.c
index e468f245f7..3d46c99df8 100644
--- a/src/resolve/resolved-dns-transaction.c
+++ b/src/resolve/resolved-dns-transaction.c
@@ -417,8 +417,10 @@ void dns_transaction_process_reply(DnsTransaction *t, DnsPacket *p) {
 
         /* Only consider responses with equivalent query section to the request */
         if (!dns_question_is_superset(p->question, t->question) ||
-            !dns_question_is_superset(t->question, p->question))
+            !dns_question_is_superset(t->question, p->question)) {
                 dns_transaction_complete(t, DNS_TRANSACTION_INVALID_REPLY);
+                return;
+        }
 
         /* According to RFC 4795, section 2.9. only the RRs from the answer section shall be cached */
         dns_cache_put(&t->scope->cache, p->question, DNS_PACKET_RCODE(p), p->answer, DNS_PACKET_ANCOUNT(p), 0, p->family, &p->sender);