From: akhanin-dnsf <106987117+akhanin-dnsf@users.noreply.github.com>
Date: Thu, 6 Aug 2026 15:04:05 +0000 (-0500)
Subject: - Fix bounds check in packed_rr_to_string, it checked the (#1488)
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=307fc6f062f8e02ca1edb0ccfce4cdf427a99691;p=thirdparty%2Funbound.git

- 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 383992393..d4b145673 100644
--- 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 985a9d552..a3f2f12a8 100644
--- 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 3b0330c55..598d606b4 100644
--- 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;
 	}