Pull request #5044: dns: add fix infinite recursion vulnerability

Merge in SNORT/snort3 from ~SHIBOSE/snort3:dnsloop to master

Squashed commit of the following:

commit deba44f95fa2318eecae06366136a29801478ab1
Author: shibose <shibose@cisco.com>
Date:   Wed Dec 10 00:09:05 2025 +0530

    dns: add fix infinite recursion vulnerability

diff --git a/src/service_inspectors/dns/dns.cc b/src/service_inspectors/dns/dns.cc
index 9a3439edfe80d02b30ca22fe77502661b8c9d3d7..5f15111f615832dfe74095a8145d90a860c8eed6 100644 (file)
--- a/src/service_inspectors/dns/dns.cc
+++ b/src/service_inspectors/dns/dns.cc
@@ -326,8 +326,14 @@ static uint16_t ParseDNSHeader(
 }
 
 static uint16_t ParseDNSName(
-    const unsigned char* data, uint16_t bytes_unused, DNSData* dnsSessionData, bool parse_dns_name = false)
+    const unsigned char* data, uint16_t bytes_unused, DNSData* dnsSessionData, bool parse_dns_name = false,
+    int32_t start_offset = -1)
 {
+    // For initial call (start_offset == -1), calculate position from bytes_unused.
+    // For recursive calls following pointers, start_offset is explicitly provided.
+    if (start_offset < 0)
+        start_offset = dnsSessionData->bytes_unused - bytes_unused;
+
     uint16_t bytes_required = dnsSessionData->curr_txt.txt_len -
         dnsSessionData->curr_txt.txt_bytes_seen;
 
@@ -387,13 +393,21 @@ static uint16_t ParseDNSName(
                     if (dnsSessionData->length > 0)
                         dnsSessionData->curr_txt.offset += 2; // first two bytes are length in TCP
 
-                    if (parse_dns_name && dnsSessionData->data.size() > dnsSessionData->curr_txt.offset)
+                    if (parse_dns_name)
                     {
-                        // If the name field is a pointer, then parse the name field at that offset only if
-                        // the offset is within the bounds of the data buffer.
-                        dnsSessionData->curr_txt.name_state = DNS_RESP_STATE_NAME_SIZE;
-                        return ParseDNSName(&dnsSessionData->data[0] + dnsSessionData->curr_txt.offset,
-                            dnsSessionData->bytes_unused, dnsSessionData, parse_dns_name);
+                        // Per RFC 1035, compression pointers must point to a prior occurrence.
+                        // The pointer target (curr_txt.offset) must be less than where we started parsing (start_offset).
+                        // This prevents forward pointers and loops.
+                        if (dnsSessionData->data.size() > dnsSessionData->curr_txt.offset
+                            && static_cast<int32_t>(dnsSessionData->curr_txt.offset) < start_offset)
+                        {
+                            // If the name field is a pointer, then parse the name field at that offset only if
+                            // the offset is within the bounds of the data buffer and points backwards.
+                            dnsSessionData->curr_txt.name_state = DNS_RESP_STATE_NAME_SIZE;
+                            return ParseDNSName(&dnsSessionData->data[0] + dnsSessionData->curr_txt.offset,
+                                dnsSessionData->bytes_unused - dnsSessionData->curr_txt.offset, dnsSessionData,
+                                parse_dns_name, static_cast<int32_t>(dnsSessionData->curr_txt.offset));
+                        }
                     }
                 }
 
@@ -445,6 +459,12 @@ static uint16_t ParseDNSQuestion(
     if (dnsSessionData->curr_rec_state < DNS_RESP_STATE_Q_NAME_COMPLETE)
     {
         uint16_t new_bytes_unused = ParseDNSName(data, bytes_unused, dnsSessionData, true);
+
+        // Sanity check: new_bytes_unused should never exceed bytes_unused.
+        // If it does, the DNS data is malformed - return 0 to stop processing.
+        if (new_bytes_unused > bytes_unused)
+            return 0;
+
         uint16_t bytes_used = bytes_unused - new_bytes_unused;
 
         if (dnsSessionData->curr_txt.name_state == DNS_RESP_STATE_NAME_COMPLETE)
@@ -1115,6 +1135,14 @@ SfIp DnsResponseIp::get_ip()
     return ip;
 }
 
+DnsResponseFqdn::DnsResponseFqdn(const unsigned char* data, uint16_t bytes_unused, DNSData* dnsSessionData) :
+    data(data), bytes_unused(bytes_unused), dnsSessionData(std::make_shared<DNSData>(*dnsSessionData))
+{
+    // Clear cur_fqdn_event in the copied DNSData to prevent nested object chains
+    // that cause stack exhaustion during destruction.
+    this->dnsSessionData->cur_fqdn_event = DnsResponseFqdn();
+}
+
 FqdnTtl DnsResponseFqdn::get_fqdn()
 {
     std::string dns_name;

diff --git a/src/service_inspectors/dns/dns.h b/src/service_inspectors/dns/dns.h
index d0cb9741528780777dcec5d8cca1c9dc66b659f4..8230a9fd00708b36e0ad69dfad0a05b16776357e 100644 (file)
--- a/src/service_inspectors/dns/dns.h
+++ b/src/service_inspectors/dns/dns.h
@@ -196,9 +196,7 @@ public:
     DnsResponseFqdn()
     {}
 
-    DnsResponseFqdn(const unsigned char* data, uint16_t bytes_unused, DNSData* dnsSessionData) :
-        data(data), bytes_unused(bytes_unused), dnsSessionData(std::make_shared<DNSData>(*dnsSessionData))
-    {}
+    DnsResponseFqdn(const unsigned char* data, uint16_t bytes_unused, DNSData* dnsSessionData);
 
     FqdnTtl get_fqdn();
     void update_ttl(uint32_t ttl);