- Fix bounds check in packed_rr_to_string, it checked the (#1488)

assembled rr length against the output string length
  dest_len, instead of against the size of the rr buffer it
  writes into. Callers in cachedump.c and remote.c pass a
  dest_len larger than that buffer.
- Unit test for packed_rr_to_string.

diff --git a/doc/Changelog b/doc/Changelog
index 383992393e7d7a24cb4f7696b26890f6c92717ea..d4b145673111767cdfa84a34f077226ee3ffa54f 100644 (file)
--- a/doc/Changelog
+++ b/doc/Changelog
@@ -1,3 +1,11 @@
+6 August 2026: Alex Khanin
+       - Fix bounds check in packed_rr_to_string, it checked the
+         assembled rr length against the output string length
+         dest_len, instead of against the size of the rr buffer it
+         writes into. Callers in cachedump.c and remote.c pass a
+         dest_len larger than that buffer.
+       - Unit test for packed_rr_to_string.
+
 6 August 2026: Wouter
        - Fix #1485: the list_forwards command omits port numbers.
          The list_forwards and list_stubs commands for

diff --git a/testcode/unitmain.c b/testcode/unitmain.c
index 985a9d5522e8ac69e1828ff0efa72e8dd1215e4a..a3f2f12a838b8c6961fab2e7d730d155e3abe2e7 100644 (file)
--- a/testcode/unitmain.c
+++ b/testcode/unitmain.c
@@ -1337,6 +1337,89 @@ static void mesh_test(void)
        free(c1);
 }
 
+#include "util/data/packed_rrset.h"
+#include "sldns/sbuffer.h"
+/** packed_rrset unit tests */
+static void packed_rrset_test(void)
+{
+       /* packed_rr_to_string assembles the dname, type, class, ttl and
+        * rdata of one rr into a buffer of 65535 bytes.  Check that it
+        * refuses an rr that does not fit in there, also when the caller
+        * passes a dest_len that is larger than that, like the callers in
+        * daemon/cachedump.c and daemon/remote.c do.  Without the check it
+        * writes past the end of the assembly buffer. */
+       uint8_t smalldname[] = "\003www\007example\003com";
+       uint8_t smallrdata[] = {0, 4, 1, 2, 3, 4};
+       uint8_t maxdname[LDNS_MAX_DOMAINLEN];
+       struct ub_packed_rrset_key rrk;
+       struct packed_rrset_data d;
+       uint8_t* rr_data[1];
+       size_t rr_len[1];
+       time_t rr_ttl[1];
+       size_t dest_len = 65535*4+2048; /* the size daemon/cachedump.c uses */
+       char* dest = (char*)malloc(dest_len);
+       int i;
+
+       unit_show_func("util/data/packed_rrset.c", "packed_rr_to_string");
+       if(!dest) fatal_exit("out of memory");
+       memset(&rrk, 0, sizeof(rrk));
+       memset(&d, 0, sizeof(d));
+       rrk.entry.data = &d;
+       rrk.rk.rrset_class = htons(LDNS_RR_CLASS_IN);
+       d.count = 1;
+       d.rr_len = rr_len;
+       d.rr_ttl = rr_ttl;
+       d.rr_data = rr_data;
+       rr_ttl[0] = 3600;
+
+       /* an ordinary rr is printed, also with the large dest_len */
+       rrk.rk.dname = smalldname;
+       rrk.rk.dname_len = sizeof(smalldname);
+       rrk.rk.type = htons(LDNS_RR_TYPE_A);
+       rr_data[0] = smallrdata;
+       rr_len[0] = sizeof(smallrdata);
+       unit_assert(packed_rr_to_string(&rrk, 0, 0, dest, dest_len) == 1);
+       unit_assert(strstr(dest, "1.2.3.4") != NULL);
+
+       /* a dname of the maximum length, 127 labels of one character */
+       for(i=0; i<127; i++) {
+               maxdname[i*2] = 1;
+               maxdname[i*2+1] = (uint8_t)'a';
+       }
+       maxdname[254] = 0;
+       rrk.rk.dname = maxdname;
+       rrk.rk.dname_len = sizeof(maxdname);
+       rrk.rk.type = htons(LDNS_RR_TYPE_TXT);
+
+       /* 255+2+2+4+65272 is exactly 65535, that still fits */
+       rr_len[0] = 65535 - 255 - 8;
+       rr_data[0] = (uint8_t*)calloc(1, rr_len[0]);
+       if(!rr_data[0]) fatal_exit("out of memory");
+       sldns_write_uint16(rr_data[0], (uint16_t)(rr_len[0]-2));
+       unit_assert(packed_rr_to_string(&rrk, 0, 0, dest, dest_len) == 1);
+       free(rr_data[0]);
+
+       /* one more byte of rdata does not fit and must be refused */
+       rr_len[0] = 65535 - 255 - 8 + 1;
+       rr_data[0] = (uint8_t*)calloc(1, rr_len[0]);
+       if(!rr_data[0]) fatal_exit("out of memory");
+       sldns_write_uint16(rr_data[0], (uint16_t)(rr_len[0]-2));
+       unit_assert(packed_rr_to_string(&rrk, 0, 0, dest, dest_len) == 0);
+       unit_assert(dest[0] == 0);
+       free(rr_data[0]);
+
+       /* the largest rdata an rr can hold, well over the buffer */
+       rr_len[0] = 2 + 65535;
+       rr_data[0] = (uint8_t*)calloc(1, rr_len[0]);
+       if(!rr_data[0]) fatal_exit("out of memory");
+       sldns_write_uint16(rr_data[0], 65535);
+       unit_assert(packed_rr_to_string(&rrk, 0, 0, dest, dest_len) == 0);
+       unit_assert(dest[0] == 0);
+       free(rr_data[0]);
+
+       free(dest);
+}
+
 void unit_show_func(const char* file, const char* func)
 {
        printf("test %s:%s\n", file, func);
@@ -1409,6 +1492,7 @@ main(int argc, char* argv[])
        zonemd_test();
        tcpreuse_test();
        msgparse_test();
+       packed_rrset_test();
        edns_ede_answer_encode_test();
        localzone_test();
        mesh_test();

diff --git a/util/data/packed_rrset.c b/util/data/packed_rrset.c
index 3b0330c5560b90766eb37d88b295b15d80eedee7..598d606b4a7db84c8ddf4b759133a778e589c1f9 100644 (file)
--- a/util/data/packed_rrset.c
+++ b/util/data/packed_rrset.c
@@ -280,7 +280,9 @@ int packed_rr_to_string(struct ub_packed_rrset_key* rrset, size_t i,
        size_t rlen = rrset->rk.dname_len + 2 + 2 + 4 + d->rr_len[i];
        time_t adjust = 0;
        log_assert(dest_len > 0 && dest);
-       if(rlen > dest_len) {
+       /* rlen is the length written into rr, dest_len bounds the output
+        * string; check both, callers can pass a dest_len over sizeof(rr). */
+       if(rlen > dest_len || rlen > sizeof(rr)) {
                dest[0] = 0;
                return 0;
        }