dns: parser reads into garbage on misreported packet size (mfontanini…

…#468) Co-authored-by: Bill Willcox <billwcorp@gmail.com>
laudrup · Nov 23, 2022 · 7661e80 · 7661e80
1 parent 633caad
commit 7661e80
Show file tree

Hide file tree

Showing 3 changed files with 64 additions and 6 deletions.
diff --git a/include/tins/dns.h b/include/tins/dns.h
@@ -1034,7 +1034,8 @@ class TINS_API DNS : public PDU {
     uint32_t compose_name(const uint8_t* ptr, char* out_ptr) const;
     void convert_records(const uint8_t* ptr, 
                          const uint8_t* end,
-                         resources_type& res) const;
+                         resources_type& res,
+                         const uint16_t rr_count) const;
     void skip_to_section_end(Memory::InputMemoryStream& stream, 
                              const uint32_t num_records) const;
     void skip_to_dname_end(Memory::InputMemoryStream& stream) const;

diff --git a/src/dns.cpp b/src/dns.cpp
@@ -414,10 +414,11 @@ void DNS::inline_convert_v4(uint32_t value, char* output) {
 // Parses records in some section.
 void DNS::convert_records(const uint8_t* ptr,
                           const uint8_t* end,
-                          resources_type& res) const {
+                          resources_type& res,
+                          const uint16_t rr_count) const {
     InputMemoryStream stream(ptr, end - ptr);
     char dname[256], small_addr_buf[256];
-    while (stream) {
+    while (stream && (res.size() < rr_count)) {
         string data;
         bool used_small_buffer = false;
         // Retrieve the record's domain name.
@@ -577,7 +578,8 @@ DNS::resources_type DNS::answers() const {
         convert_records(
             &records_data_[0] + answers_idx_, 
             &records_data_[0] + authority_idx_, 
-            res
+            res,
+            answers_count()
         );
     }
     return res;
@@ -589,7 +591,8 @@ DNS::resources_type DNS::authority() const {
         convert_records(
             &records_data_[0] + authority_idx_, 
             &records_data_[0] + additional_idx_, 
-            res
+            res,
+            authority_count()
         );
     }
     return res;
@@ -601,7 +604,8 @@ DNS::resources_type DNS::additional() const {
         convert_records(
             &records_data_[0] + additional_idx_, 
             &records_data_[0] + records_data_.size(), 
-            res
+            res,
+            additional_count()
         );
     }
     return res;

diff --git a/tests/src/dns_test.cpp b/tests/src/dns_test.cpp
@@ -602,3 +602,56 @@ TEST_F(DNSTest, BadLabelSize) {
         SUCCEED();
     }
 }
+
+TEST_F(DNSTest, BadPacketLength) {
+
+    // valid response packet with RR's in all sections
+    const uint8_t payload[] = {
+        0x74,0xa9,0x85,0x80,0x00,0x01,0x00,0x02,0x00,0x01,0x00,0x04,0x08,0x5f,0x73,0x65,0x72,
+        0x76,0x69,0x63,0x65,0x04,0x5f,0x74,0x63,0x70,0x05,0x77,0x69,0x66,0x69,0x36,0x03,
+        0x6c,0x61,0x6e,0x00,0x00,0x21,0x00,0x01,0xc0,0x0c,0x00,0x21,0x00,0x01,0x00,0x01,
+        0x51,0x80,0x00,0x16,0x00,0x00,0x00,0x03,0x00,0x09,0x04,0x66,0x61,0x73,0x74,0x05,
+        0x77,0x69,0x66,0x69,0x36,0x03,0x6c,0x61,0x6e,0x00,0xc0,0x0c,0x00,0x21,0x00,0x01,
+        0x00,0x01,0x51,0x80,0x00,0x16,0x00,0x00,0x00,0x01,0x00,0x09,0x04,0x73,0x6c,0x6f,
+        0x77,0x05,0x77,0x69,0x66,0x69,0x36,0x03,0x6c,0x61,0x6e,0x00,0xc0,0x62,0x00,0x02,
+        0x00,0x01,0x00,0x01,0x51,0x80,0x00,0x05,0x02,0x70,0x69,0xc0,0x62,0xc0,0x5d,0x00,
+        0x01,0x00,0x01,0x00,0x01,0x51,0x80,0x00,0x04,0x0a,0x18,0x00,0x02,0xc0,0x3b,0x00,
+        0x01,0x00,0x01,0x00,0x01,0x51,0x80,0x00,0x04,0x0a,0x18,0x00,0x02,0xc0,0x79,0x00,
+        0x01,0x00,0x01,0x00,0x01,0x51,0x80,0x00,0x04,0x0a,0x18,0x00,0x02,0x00,0x00,0x29,
+        0x10,0x00,0x00,0x00,0x00,0x00,0x00,0x1c,0x00,0x0a,0x00,0x18,0x86,0x1f,0x14,0x0f,
+        0x41,0xfa,0xf3,0x95,0x48,0x6e,0x79,0x61,0x61,0x78,0x32,0x0f,0x44,0x5d,0x21,0x47,
+        0x85,0x83,0x9a,0x95
+    };
+
+    // valid DNS message but misreport packet size;
+    // before fix, parser headed into uncharted waters on requesting additional section
+
+    // buffer with space for valid packet plus garbage bytes
+    const size_t bigsz{512};
+    uint8_t big_packet[bigsz];
+
+    // copy valid packet
+    std::copy(payload, 
+              payload + sizeof(payload), 
+              big_packet);
+
+    // fill additional bytes with junk 
+    std::fill(big_packet + sizeof(payload), 
+              big_packet + bigsz, 
+              0x5A);
+
+    // initial packet parse ok
+    const DNS packet(big_packet, bigsz);
+
+    // RR's parse ok now
+    EXPECT_EQ(packet.questions_count(), 1);
+    EXPECT_EQ(packet.answers_count(), 2);
+    EXPECT_EQ(packet.authority_count(), 1);
+    EXPECT_EQ(packet.additional_count(), 4);
+    EXPECT_EQ(packet.queries().size(), 1U);
+    EXPECT_EQ(packet.answers().size(), 2U);
+    EXPECT_EQ(packet.authority().size(), 1U);
+    EXPECT_EQ(packet.additional().size(), 4U);
+}
+
+