[#4652] Limit DNS name compression pointer hops

Reject cyclic or excessively long compression pointer chains with
FORMERR so Name wire parsing cannot loop or burn unbounded CPU.

Closes #4652

Co-authored-by: Cursor <cursoragent@cursor.com>

diff --git a/changelog_unreleased/4652-name-wire-parser-has-no-compression-pointer-hop-limit-cyclic-pointers-hang-forever b/changelog_unreleased/4652-name-wire-parser-has-no-compression-pointer-hop-limit-cyclic-pointers-hang-forever
new file mode 100644 (file)
index 0000000..b02bfba
--- /dev/null
+++ b/changelog_unreleased/4652-name-wire-parser-has-no-compression-pointer-hop-limit-cyclic-pointers-hang-forever
@@ -0,0 +1,5 @@
+[bug]          wlodek
+       The DNS Name wire parser now limits compression pointer
+       hops, rejecting cyclic or excessively long pointer chains
+       with FORMERR instead of looping.
+       (Gitlab #4652)

diff --git a/src/lib/dns/name.cc b/src/lib/dns/name.cc
index 47d9b8fa4d829b3f5d75aa808c8dd1c4caa0db0d..6c4a99bd94efdb9fcbec081ceb1e51219d3c5ab5 100644 (file)
--- a/src/lib/dns/name.cc
+++ b/src/lib/dns/name.cc
@@ -403,6 +403,7 @@ Name::Name(InputBuffer& buffer, bool downcase) {
     unsigned int current = buffer.getPosition();
     unsigned int pos_begin = current;
     unsigned int biggest_pointer = current;
+    unsigned int ptr_hops = 0;
 
     // Make the compiler happy; this is not required.
     // XXX: bad style in that we initialize it with a dummy value and define
@@ -463,6 +464,10 @@ Name::Name(InputBuffer& buffer, bool downcase) {
             if (--n != 0) {
                 break;
             }
+            if (++ptr_hops > Name::MAX_WIRE) {
+                isc_throw(DNSMessageFORMERR,
+                          "compression pointer loop or too many hops");
+            }
             if (new_current >= biggest_pointer) {
                 isc_throw(DNSMessageFORMERR,
                           "bad compression pointer (out of range): " <<

diff --git a/src/lib/dns/tests/name_unittest.cc b/src/lib/dns/tests/name_unittest.cc
index 757977fec4e7ddfa661e45e0bcfba79cdfcbb8b3..cb4b16c15be148f66a98ebb49c49a1dc9ddf3920 100644 (file)
--- a/src/lib/dns/tests/name_unittest.cc
+++ b/src/lib/dns/tests/name_unittest.cc
@@ -454,6 +454,50 @@ TEST_F(NameTest, fromWire) {
     EXPECT_EQ(3U, nameFactoryFromWire("name_fromWire1", 25).getLabelCount());
 }
 
+// Build a wire buffer with a root label at offset 0 and a chain of
+// compression pointers at offsets 2, 4, ..., 2*hops.  Parsing from the
+// last pointer follows exactly `hops` compression pointers to ".".
+std::vector<uint8_t>
+makeCompressionPointerChain(unsigned int hops) {
+    std::vector<uint8_t> data((hops * 2) + 2, 0);
+    data[0] = 0x00; // root
+    for (unsigned int i = 1; i <= hops; ++i) {
+        const size_t pos = static_cast<size_t>(i) * 2;
+        const uint16_t target = static_cast<uint16_t>((i - 1) * 2);
+        data[pos] = 0xc0 | ((target >> 8) & 0x3f);
+        data[pos + 1] = target & 0xff;
+    }
+    return (data);
+}
+
+TEST_F(NameTest, fromWireCompressionPointerLoop) {
+    // Cyclic compression pointers must not hang; they must fail fast.
+    const uint8_t cycle[] = { 0xc0, 0x02, 0xc0, 0x00 };
+    InputBuffer buf(cycle, sizeof(cycle));
+    EXPECT_THROW({ Name name(buf); }, DNSMessageFORMERR);
+}
+
+TEST_F(NameTest, fromWireCompressionPointerHopLimit) {
+    // Exactly MAX_WIRE hops is still accepted (matches BIND-style limit).
+    {
+        std::vector<uint8_t> data = makeCompressionPointerChain(Name::MAX_WIRE);
+        InputBuffer buf(&data[0], data.size());
+        buf.setPosition(Name::MAX_WIRE * 2);
+        Name name(buf);
+        EXPECT_EQ(".", name.toText());
+    }
+
+    // One hop beyond MAX_WIRE must be rejected to bound decompression work
+    // and defeat pointer-loop DoS even if other checks are bypassed.
+    {
+        std::vector<uint8_t> data =
+            makeCompressionPointerChain(Name::MAX_WIRE + 1);
+        InputBuffer buf(&data[0], data.size());
+        buf.setPosition((Name::MAX_WIRE + 1) * 2);
+        EXPECT_THROW({ Name name(buf); }, DNSMessageFORMERR);
+    }
+}
+
 TEST_F(NameTest, copyConstruct) {
     Name copy(example_name);
     EXPECT_EQ(copy, example_name);