Merge pull request #34391 from poettering/dns-long-label-fix

resolved: fixes when trying to serialize overly long DNS names

src/resolve/resolved-dns-packet.c

@@ -557,12 +557,19 @@ int dns_packet_append_name(
                 bool canonical_candidate,
                 size_t *start) {
 
-        size_t saved_size;
+        _cleanup_free_ char **added_entries = NULL; /* doesn't own the strings! this is just regular pointer array, not a NULL-terminated strv! */
+        size_t n_added_entries = 0, saved_size;
         int r;
 
         assert(p);
         assert(name);
 
+        r = dns_name_is_valid(name);
+        if (r < 0)
+                return r;
+        if (r == 0)
+                return -EINVAL;
+
         if (p->refuse_compression)
                 allow_compression = false;
 
@@ -598,6 +605,11 @@ int dns_packet_append_name(
                 if (allow_compression) {
                         _cleanup_free_ char *s = NULL;
 
+                        if (!GREEDY_REALLOC(added_entries, n_added_entries + 1)) {
+                                r = -ENOMEM;
+                                goto fail;
+                        }
+
                         s = strdup(z);
                         if (!s) {
                                 r = -ENOMEM;
@@ -608,7 +620,8 @@ int dns_packet_append_name(
                         if (r < 0)
                                 goto fail;
 
-                        TAKE_PTR(s);
+                        /* Keep track of the entries we just added (note that the string is owned by the hashtable, not this array!) */
+                        added_entries[n_added_entries++] = TAKE_PTR(s);
                 }
         }
 
@@ -623,6 +636,12 @@ done:
         return 0;
 
 fail:
+        /* Remove all label compression names we added again */
+        FOREACH_ARRAY(s, added_entries, n_added_entries) {
+                hashmap_remove(p->names, *s);
+                free(*s);
+        }
+
         dns_packet_truncate(p, saved_size);
         return r;
 }
@@ -1506,7 +1525,7 @@ int dns_packet_read_name(
         size_t after_rindex = 0, jump_barrier = p->rindex;
         _cleanup_free_ char *name = NULL;
         bool first = true;
-        size_t n = 0;
+        size_t n = 0, m = 0;
         int r;
 
         if (p->refuse_compression)
@@ -1535,14 +1554,21 @@ int dns_packet_read_name(
 
                         if (first)
                                 first = false;
-                        else
+                        else {
                                 name[n++] = '.';
+                                m++;
+                        }
 
                         r = dns_label_escape(label, c, name + n, DNS_LABEL_ESCAPED_MAX);
                         if (r < 0)
                                 return r;
 
                         n += r;
+                        m += c;
+
+                        if (m > DNS_HOSTNAME_MAX)
+                                return -EBADMSG;
+
                         continue;
                 } else if (allow_compression && FLAGS_SET(c, 0xc0)) {
                         uint16_t ptr;

src/resolve/test-dns-packet-append.c

@@ -1267,4 +1267,34 @@ TEST(packet_append_answer_single_svcb) {
         ASSERT_EQ(memcmp(DNS_PACKET_DATA(packet), data, sizeof(data)), 0);
 }
 
+static void dump_packet_data(DnsPacket *packet) {
+        assert(packet);
+        fprintf(stderr, "packet bytes:");
+        for (size_t i = 0; i < packet->size; i++)
+                fprintf(stderr, " %x", DNS_PACKET_DATA(packet)[i]);
+        fprintf(stderr, "\n");
+}
+
+TEST(packet_append_key_name_too_long) {
+        _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL;
+        _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL;
+        int r;
+
+        ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX));
+
+        DNS_PACKET_ID(packet) = htobe16(42);
+        DNS_PACKET_HEADER(packet)->flags = htobe16(DNS_PACKET_MAKE_FLAGS(0, 0, 0, 0, 1, 0, 0, 0, 0));
+        DNS_PACKET_HEADER(packet)->qdcount = htobe16(1);
+
+        key = ASSERT_PTR(dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A, "www.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.com"));
+        r = dns_packet_append_key(packet, key, 0, NULL);
+
+        log_debug("r = %d, size = %zu", r, packet->size);
+        log_debug("key name = <%s>", dns_resource_key_name(key));
+        dump_packet_data(packet);
+
+        ASSERT_EQ(r, -EINVAL);
+        ASSERT_EQ(packet->size, 12U);
+}
+
 DEFINE_TEST_MAIN(LOG_DEBUG)

src/resolve/test-dns-packet-extract.c

@@ -806,12 +806,7 @@ TEST(packet_query_single_long_domain) {
                         0x10, 'n', 'i', 't', 'r', 'o', 's', 'y', 'l', 's', 'u', 'l', 'f', 'u', 'r', 'i', 'c',
                         0x10, 'o', 'b', 'j', 'e', 'c', 't', 'l', 'e', 's', 's', 'n', 'e', 's', 's', 'e', 's',
                         0x10, 'p', 'a', 'r', 't', 'r', 'i', 'd', 'g', 'e', 'b', 'e', 'r', 'r', 'i', 'e', 's',
-                        0x10, 'r', 'e', 'a', 's', 'o', 'n', 'l', 'e', 's', 's', 'n', 'e', 's', 's', 'e', 's',
+                        0x0F, 'r', 'e', 'a', 's', 'o', 'n', 'l', 'e', 's', 's', 'n', 'e', 's', 's', 'e',
-                        0x10, 's', 'e', 'm', 'i', 'p', 'a', 't', 'h', 'o', 'l', 'o', 'g', 'i', 'c', 'a', 'l',
-                        0x10, 't', 'o', 'm', 'f', 'o', 'o', 'l', 'i', 's', 'h', 'n', 'e', 's', 's', 'e', 's',
-                        0x10, 'u', 'n', 'd', 'e', 'r', 'c', 'a', 'p', 'i', 't', 'a', 'l', 'i', 'z', 'e', 'd',
-                        0x10, 'v', 'e', 'c', 't', 'o', 'r', 'c', 'a', 'r', 'd', 'i', 'o', 'g', 'r', 'a', 'm',
-                        0x10, 'w', 'e', 'a', 't', 'h', 'e', 'r', 'p', 'r', 'o', 'o', 'f', 'n', 'e', 's', 's',
                         0x00,
         /* A */         0x00, 0x01,
         /* IN */        0x00, 0x01
@@ -823,12 +818,12 @@ TEST(packet_query_single_long_domain) {
         ASSERT_EQ(dns_question_size(packet->question), 1u);
         ASSERT_EQ(dns_answer_size(packet->answer), 0u);
 
-        key = dns_resource_key_new(DNS_CLASS_IN, DNS_TYPE_A,
+        key = dns_resource_key_new(
-                "absorptivenesses.calligraphically.deacidifications.ecophysiological."
+                        DNS_CLASS_IN, DNS_TYPE_A,
-                "falsifiabilities.heterochromatism.icositetrahedron.journalistically."
+                        "absorptivenesses.calligraphically.deacidifications.ecophysiological."
-                "kinaesthetically.lactovegetarians.misinterpretable.nitrosylsulfuric."
+                        "falsifiabilities.heterochromatism.icositetrahedron.journalistically."
-                "objectlessnesses.partridgeberries.reasonlessnesses.semipathological."
+                        "kinaesthetically.lactovegetarians.misinterpretable.nitrosylsulfuric."
-                "tomfoolishnesses.undercapitalized.vectorcardiogram.weatherproofness");
+                        "objectlessnesses.partridgeberries.reasonlessnesse");
 
         ASSERT_NOT_NULL(key);
         ASSERT_TRUE(dns_question_contains_key(packet->question, key));
@@ -4777,4 +4772,78 @@ TEST(format_dns_svc_param_key) {
         ASSERT_STREQ(str, "ohttp");
 }
 
+TEST(overlong_domain) {
+        _cleanup_(dns_packet_unrefp) DnsPacket *packet = NULL;
+
+        ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX));
+        ASSERT_NOT_NULL(packet);
+        dns_packet_truncate(packet, 0);
+
+        const uint8_t data[] = {
+                0x00, 0x42,     0x00, 0x00,
+                0x00, 0x01,     0x00, 0x00,     0x00, 0x00,     0x00, 0x00,
+
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                0x00,
+                0x00, DNS_TYPE_A,
+                0x00, DNS_CLASS_IN,
+        };
+
+        ASSERT_OK(dns_packet_append_blob(packet, data, sizeof(data), NULL));
+        ASSERT_OK(dns_packet_validate_query(packet));
+        _cleanup_(dns_resource_key_unrefp) DnsResourceKey *key = NULL;
+        ASSERT_ERROR(dns_packet_read_key(packet, &key, NULL, NULL), EBADMSG);
+
+        const uint8_t data2[] = {
+                0x00, 0x42,     0x00, 0x00,
+                0x00, 0x01,     0x00, 0x00,     0x00, 0x00,     0x00, 0x00,
+
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                62, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1',
+                0x00,
+                0x00, DNS_TYPE_A,
+                0x00, DNS_CLASS_IN,
+        };
+
+        packet = dns_packet_unref(packet);
+        ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX));
+        ASSERT_NOT_NULL(packet);
+        dns_packet_truncate(packet, 0);
+        ASSERT_OK(dns_packet_append_blob(packet, data2, sizeof(data2), NULL));
+        ASSERT_OK(dns_packet_validate_query(packet));
+        ASSERT_ERROR(dns_packet_read_key(packet, &key, NULL, NULL), EBADMSG);
+
+        const uint8_t data3[] = {
+                0x00, 0x42,     0x00, 0x00,
+                0x00, 0x01,     0x00, 0x00,     0x00, 0x00,     0x00, 0x00,
+
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                63, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2',
+                61, '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9', '0',
+                0x00,
+                0x00, DNS_TYPE_A,
+                0x00, DNS_CLASS_IN,
+        };
+
+        packet = dns_packet_unref(packet);
+        ASSERT_OK(dns_packet_new(&packet, DNS_PROTOCOL_DNS, 0, DNS_PACKET_SIZE_MAX));
+        ASSERT_NOT_NULL(packet);
+        dns_packet_truncate(packet, 0);
+        ASSERT_OK(dns_packet_append_blob(packet, data3, sizeof(data3), NULL));
+        ASSERT_OK(dns_packet_validate_query(packet));
+        ASSERT_OK(dns_packet_read_key(packet, &key, NULL, NULL));
+
+        ASSERT_STREQ(dns_resource_key_name(key),
+                     "012345678901234567890123456789012345678901234567890123456789012."
+                     "012345678901234567890123456789012345678901234567890123456789012."
+                     "012345678901234567890123456789012345678901234567890123456789012."
+                     "0123456789012345678901234567890123456789012345678901234567890");
+}
+
 DEFINE_TEST_MAIN(LOG_DEBUG)

src/shared/dns-domain.c

@@ -480,17 +480,17 @@ finish:
         return 0;
 }
 
-void dns_name_hash_func(const char *p, struct siphash *state) {
+void dns_name_hash_func(const char *name, struct siphash *state) {
         int r;
 
-        assert(p);
+        assert(name);
 
-        for (;;) {
+        for (const char *p = name;;) {
                 char label[DNS_LABEL_MAX+1];
 
                 r = dns_label_unescape(&p, label, sizeof label, 0);
                 if (r < 0)
-                        break;
+                        return string_hash_func(p, state); /* fallback for invalid DNS names */
                 if (r == 0)
                         break;
 
@@ -516,13 +516,13 @@ int dns_name_compare_func(const char *a, const char *b) {
         for (;;) {
                 char la[DNS_LABEL_MAX+1], lb[DNS_LABEL_MAX+1];
 
-                if (x == NULL && y == NULL)
+                if (!x && !y)
                         return 0;
 
                 r = dns_label_unescape_suffix(a, &x, la, sizeof(la));
                 q = dns_label_unescape_suffix(b, &y, lb, sizeof(lb));
                 if (r < 0 || q < 0)
-                        return CMP(r, q);
+                        return strcmp(a, b); /* if not valid DNS labels, then let's compare the whole strings as is */
 
                 r = ascii_strcasecmp_nn(la, r, lb, q);
                 if (r != 0)