For #6 - pointers are for trailing fragments of names or whole names,…

… not labels - mistaken implementation.
mistakes-consortium · Aug 15, 2015 · 956e0a2 · 956e0a2
1 parent 21486f9
commit 956e0a2
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 20 deletions.
diff --git a/tests/testsuite.c b/tests/testsuite.c
@@ -8,8 +8,12 @@
 unsigned char dns_response[] = {
     0, 0, 0, 0, 0, 0, 255, 255, 255, 255, 255, 255, // 12 bytes of random stuff for fun..
     1, 'a', 7, 'e', 'x', 'a', 'm', 'p', 'l', 'e', 3, 'c', 'o', 'm', 0,
-    1, 'a', 0xc0, 14, 3, 'o', 'r', 'g', 0,
-    0xc0, 36, 0
+    1, 'b', 0xc0, 14
+};
+
+unsigned char evil_dns_response_selfreference[] = {
+    0, 0,
+    0xc0, 2
 };
 
 // this one is too short.
@@ -37,7 +41,7 @@ void test_dnsname_no_compression()
 
 void test_malicious_input_rejected() {
     // no infinite loop....
-    ASSERT_EQUALS(1, dns_compare_name(".", dns_response, sizeof(dns_response), 36));
+    ASSERT_EQUALS(1, dns_compare_name(".", evil_dns_response_selfreference, sizeof(evil_dns_response_selfreference), 2));
 }
 
 void test_bounds_truncate() {
@@ -50,15 +54,14 @@ void test_bounds_truncate() {
 void test_dnsname_with_compression()
 {
     // pointer to the part of the message that used DNS compression properly. False positive in stock LWIP...
-    ASSERT_EQUALS(0, dns_compare_name("a.example.org", dns_response, sizeof(dns_response), 27));
+    ASSERT_EQUALS(0, dns_compare_name("b.example.com", dns_response, sizeof(dns_response), 27));
     // it should work when case insensitive also, see RFC 4343 sec 4.1
-    ASSERT_EQUALS(0, dns_compare_name("a.Example.org", dns_response, sizeof(dns_response), 27));
+    ASSERT_EQUALS(0, dns_compare_name("b.Example.com", dns_response, sizeof(dns_response), 27));
 
     // below here is where things failed before my substantive changes.  This code code from lwip had
     // anything that has dns compression matching everything as soon as the compression kicks in..
     // pointer to the compressed a.example.com in the message, but we aren't comparing that
-    //FIXME re-enable test once working...
-    ASSERT_EQUALS(1, dns_compare_name("a.bing.com", dns_response, sizeof(dns_response), 27));
+    ASSERT_EQUALS(1, dns_compare_name("b.bing.com", dns_response, sizeof(dns_response), 27));
 }
 
 /* test runner */

diff --git a/user/dns_util.h b/user/dns_util.h
@@ -76,8 +76,7 @@ dns_compare_name(unsigned char *query, unsigned char *response_ptr, uint16_t res
     unsigned char* response = response_ptr;
     unsigned char* end = response_ptr + response_size;
     //response += offset;
-
-    uint16_t offset_bookmark = 0;
+    uint8_t compression_jumped = 1;
 
     //printf(">>>>>> begin!\n");
 
@@ -88,15 +87,23 @@ dns_compare_name(unsigned char *query, unsigned char *response_ptr, uint16_t res
         if ((n & 0xc0) == 0xc0) {
             /** @see RFC 1035 - 4.1.4. Message compression */
             /* Compressed name */
-            if (offset_bookmark != 0) {
-                //printf("<<<<<< this would be an infinite loop. no match, invalid formatting.\n");
+
+            if ((offset >= response_size) || (offset == 0)) {
+                //printf("<<<<<<<< pointer out of range (offset=%d). No match.\n", offset);
+                return 1;
+            }
+
+            if (compression_jumped == 0) {
+                //printf("<<<<<< you can only do one compression jump per name.\n");
                 return 1;
             }
-            offset_bookmark = offset + 1;
+            compression_jumped = 0;
+
+            uint16_t current_offset = offset - 1;
             offset = 255 * (0xc0 ^ n) + response[offset++];
             //printf("found offset to be %d after decoding pointer.\n", n);
-            if ((offset >= response_size) || (offset == 0)) {
-                //printf("<<<<<<<< pointer out of range (offset=%d). No match.\n", offset);
+            if (offset >= current_offset) {
+                //printf("<<<<<<< forward pointers disallowed. No match. current offset=%d, jump=%d\n", current_offset, offset);
                 return 1;
             }
         } else {
@@ -117,12 +124,6 @@ dns_compare_name(unsigned char *query, unsigned char *response_ptr, uint16_t res
             };
             ++query;
 
-            if (offset_bookmark != 0) {
-                offset = offset_bookmark;
-                offset_bookmark = 0;
-                //printf("restored the offset bookmark to %d\n", offset);
-            }
-
             if (offset >= response_size) {
                 //printf("<<<<<< offset has grown larger than size of response. no match.\n");
                 return 1;