make fr_dns_ok() return a reason why the packet is bad

and push that through the rest of the code, and the test cases

diff --git a/src/listen/dns/proto_dns_udp.c b/src/listen/dns/proto_dns_udp.c
index 7063565d16480524d7cfc265e149be912abb719c..81ba4cd324bb2c422ce3772bfce25a3b742cafcb 100644 (file)
--- a/src/listen/dns/proto_dns_udp.c
+++ b/src/listen/dns/proto_dns_udp.c
@@ -127,6 +127,7 @@ static ssize_t mod_read(fr_listen_t *li, void **packet_ctx, fr_time_t *recv_time
        size_t                          packet_len;
        uint32_t                        xid;
        fr_dns_packet_t                 *packet;
+       fr_dns_decode_fail_t            reason;
 
        *leftover = 0;          /* always for UDP */
 
@@ -161,8 +162,8 @@ static ssize_t mod_read(fr_listen_t *li, void **packet_ctx, fr_time_t *recv_time
         */
        packet = (fr_dns_packet_t *) buffer;
 
-       if (!fr_dns_packet_ok(buffer, packet_len, true)) {
-               RATE_LIMIT_GLOBAL(WARN, "Invalid DNS packet - ignoring");
+       if (!fr_dns_packet_ok(buffer, packet_len, true, &reason)) {
+               RATE_LIMIT_GLOBAL(WARN, "Invalid DNS packet failed with reason %d - ignoring", reason);
                return 0;
        }
 

diff --git a/src/protocols/dns/base.c b/src/protocols/dns/base.c
index eccb7c602e030c1e453c6869cb3ae0f1e2291122..4a7ea76755b347e1db66e0b12822bde7375bc76b 100644 (file)
--- a/src/protocols/dns/base.c
+++ b/src/protocols/dns/base.c
@@ -75,9 +75,17 @@ fr_dict_attr_autoload_t dns_dict_attr[] = {
        [FR_DNS_STATEFUL_OP] = "stateful-operations",
 };
 
-bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query)
+#define DECODE_FAIL(_reason) if (reason) *reason = DECODE_FAIL_ ## _reason
+
+bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query, fr_dns_decode_fail_t *reason)
 {
+#if 0
+       uint8_t const *p, *end;
+       int count, expected;
+#endif
+
        if (packet_len <= DNS_HDR_LEN) {
+               DECODE_FAIL(MIN_LENGTH_PACKET);
                return false;
        }
 
@@ -85,6 +93,7 @@ bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query)
         *      query=0, response=1
         */
        if (((packet[2] & 0x80) == 0) != query) {
+               DECODE_FAIL(UNEXPECTED);
                return false;
        }
 
@@ -93,16 +102,19 @@ bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query)
                 *      There should be at least one query, and no replies in the query!
                 */
                if (fr_net_to_uint16(packet + 4) == 0) {
+                       DECODE_FAIL(NO_QUESTIONS);
                        return false;
                }
                if (fr_net_to_uint16(packet + 6) != 0) {
+                       DECODE_FAIL(NS_IN_QUESTION);
                        return false;
                }
                if (fr_net_to_uint16(packet + 8) != 0) {
+                       DECODE_FAIL(ANSWERS_IN_QUESTION);
                        return false;
                }
-
                // additional records can exist!
+
        } else {
                /*
                 *      Replies _usually_ copy the query.  But not
@@ -110,6 +122,52 @@ bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query)
                 */
        }
 
+#if 0
+       expected = fr_net_to_uint16(packet + 4) + fr_net_to_uint16(packet + 6) + fr_net_to_uint16(packet + 8) + fr_net_to_uint16(packet + 10);
+       count = 0;
+
+       p = packet + DNS_HDR_LEN;
+       end = packet + packet_len;
+
+       /*
+        *      Check that lengths of RRs match.
+        */
+       while (p < end) {
+               uint16_t len;
+
+               /*
+                *      @todo - get DNS label, followed by 2 octets of
+                *      type, 2 of class, 4 of TTL.  Then check the RR.
+                */
+
+               if ((p + 2) >= end) {
+                       DECODE_FAIL(MISSING_RRLEN);
+                       return false;
+               }
+
+               len = fr_net_to_uint16(p);
+               p += 2;
+               if ((p + len) > end) {
+                       DECODE_FAIL(RR_OVERFLOWS_PACKET);
+                       return false;
+               }
+
+               p+= len;
+               count++;
+
+               if (count > expected) {
+                       DECODE_FAIL(TOO_MANY_RRS);
+                       return false;
+               }
+       }
+
+       if (count != expected) {
+               DECODE_FAIL(TOO_FEW_RRS);
+               return false;
+       }
+#endif
+
+       DECODE_FAIL(NONE);
        return true;
 }
 

diff --git a/src/protocols/dns/decode.c b/src/protocols/dns/decode.c
index 3356aa0e3dc1ea77995ad84b5bb0555a30318a95..4a777e90f3da48b579145d3a0437d2820c887e3b 100644 (file)
--- a/src/protocols/dns/decode.c
+++ b/src/protocols/dns/decode.c
@@ -661,26 +661,40 @@ static int decode_test_ctx(void **out, TALLOC_CTX *ctx)
        return 0;
 }
 
+static fr_table_num_ordered_t reason_fail_table[] = {
+       { L("none"),                                            DECODE_FAIL_NONE                },
+       { L("packet is smaller than DNS header"),               DECODE_FAIL_MIN_LENGTH_PACKET   },
+       { L("expected query / answer, got answer / query"),     DECODE_FAIL_UNEXPECTED          },
+       { L("no 'questions' in query packet"),                  DECODE_FAIL_NO_QUESTIONS        },
+       { L("unexprected answers in query packet"),             DECODE_FAIL_ANSWERS_IN_QUESTION },
+       { L("unexpected NS records in query packet"),           DECODE_FAIL_NS_IN_QUESTION      },
+       { L("missing resource record length field"),            DECODE_FAIL_MISSING_RRLEN       },
+       { L("resource record length overflows the packet"),     DECODE_FAIL_RR_OVERFLOWS_PACKET },
+       { L("more resource records than indicated in header"),  DECODE_FAIL_TOO_MANY_RRS        },
+       { L("fewer resource records than indicated in header"), DECODE_FAIL_TOO_FEW_RRS         },
+};
+static size_t reason_fail_table_len = NUM_ELEMENTS(reason_fail_table);
+
 static ssize_t fr_dns_decode_proto(TALLOC_CTX *ctx, fr_pair_list_t *out, uint8_t const *data, size_t data_len, void *proto_ctx)
 {
        fr_dns_ctx_t *packet_ctx = proto_ctx;
+       fr_dns_decode_fail_t reason;
 
        if (data_len > 65535) return -1; /* packet is too big */
 
        /*
         *      Allow queries or answers
         */
-#if 0
-       if (!fr_dns_packet_ok(data, data_len, true)) {
-               FR_PROTO_TRACE("FAIL %d", __LINE__);
-               if (!fr_dns_packet_ok(data, data_len, false)) {
-                       FR_PROTO_TRACE("FAIL %d", __LINE__);
+       if (!fr_dns_packet_ok(data, data_len, true, &reason)) {
+               if (reason != DECODE_FAIL_UNEXPECTED) goto fail;
+
+               if (!fr_dns_packet_ok(data, data_len, false, &reason)) {
+               fail:
+                       fr_strerror_printf("DNS packet malformed - %s",
+                                          fr_table_str_by_value(reason_fail_table, reason, "???"));
                        return -1;
                }
-
-               FR_PROTO_TRACE("FAIL %d", __LINE__);
        }
-#endif
 
        packet_ctx->packet = data;
        packet_ctx->packet_len = data_len;

diff --git a/src/protocols/dns/dns.h b/src/protocols/dns/dns.h
index 34bee267bb506a78d0cb3199bc00c929f8cf3b69..3994d3f4a6810ccb48c96cf51fd530de8f46ed27 100644 (file)
--- a/src/protocols/dns/dns.h
+++ b/src/protocols/dns/dns.h
@@ -105,13 +105,27 @@ typedef enum {
        FR_DNS_DO_NOT_RESPOND = 256,
 } fr_dns_packet_code_t;
 
+typedef enum {
+       DECODE_FAIL_NONE = 0,
+       DECODE_FAIL_MIN_LENGTH_PACKET,
+       DECODE_FAIL_UNEXPECTED,
+       DECODE_FAIL_NO_QUESTIONS,
+       DECODE_FAIL_ANSWERS_IN_QUESTION,
+       DECODE_FAIL_NS_IN_QUESTION,
+       DECODE_FAIL_MISSING_RRLEN,
+       DECODE_FAIL_RR_OVERFLOWS_PACKET,
+       DECODE_FAIL_TOO_MANY_RRS,
+       DECODE_FAIL_TOO_FEW_RRS,
+       DECODE_FAIL_MAX
+} fr_dns_decode_fail_t;
+
 #define FR_DNS_PACKET_CODE_VALID(_code) (((_code) < FR_DNS_CODE_MAX) || (((_code & 0x10) != 0) && ((_code & ~0x10) < FR_DNS_CODE_MAX)))
 
 #define DNS_HDR_LEN (12)
 
 extern char const *fr_dns_packet_codes[FR_DNS_CODE_MAX];
 
-bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query);
+bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query, fr_dns_decode_fail_t *reason);
 
 fr_dns_labels_t *fr_dns_labels_get(uint8_t const *packet, size_t packet_len, bool init_mark);
 

diff --git a/src/tests/unit/protocols/dns/base.txt b/src/tests/unit/protocols/dns/base.txt
index a2cc49a7a16918db2b3665ecf17785a54fbdfce7..a91f7ce41c94071926bffb722e8a3a1e48fe4c40 100644 (file)
--- a/src/tests/unit/protocols/dns/base.txt
+++ b/src/tests/unit/protocols/dns/base.txt
@@ -14,12 +14,12 @@ proto-dictionary dns
 #  A record of '.', class Internet, TTL 16
 #  length 4, with 127.0.0.1 as the IP address
 
-#                                                 Z  type   class  ttl
-decode-proto 00 00 00 00 00 00 00 01 00 00 00 00  00 00 01  00 01  00 00 00 10  00 04 7f 00 00 01
-match packet = { id = 0, query = query, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 1, nscount = 0, arcount = 0 }, rr = { name = ".", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
+#                                                 Z  type   class   ttl
+decode-proto 00 00 80 00 00 00 00 01 00 00 00 00  00 00 01   00 01  00 00 00 10  00 04 7f 00 00 01
+match packet = { id = 0, query = response, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 1, nscount = 0, arcount = 0 }, rr = { name = ".", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
 
 encode-proto -
-match  00 00 00 00 00 00 00 01 00 00 00 00 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
+match  00 00 80 00 00 00 00 01 00 00 00 00 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
 
 #  Really "decode RR".
 #           Z  type   class  ttl          length  IPaddr
@@ -32,24 +32,24 @@ match 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
 #
 #  And a complex label
 #
-encode-proto packet = { id = 0, query = query, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 1, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
-match 00 00 00 00 00 00 00 01 00 00 00 00 03 77 77 77 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
+encode-proto packet = { id = 0, query = response, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 1, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
+match 00 00 80 00 00 00 00 01 00 00 00 00 03 77 77 77 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
 
 decode-proto -
-match packet = { id = 0, query = query, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 1, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
+match packet = { id = 0, query = response, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 1, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
 
 #
 #  multiple labels (2)
 #
-encode-proto packet = { id = 0, query = query, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 2, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ftp.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
-match 00 00 00 00 00 00 00 02 00 00 00 00 03 77 77 77 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01 03 66 74 70 c0 10 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
+encode-proto packet = { id = 0, query = response, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 2, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ftp.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
+match 00 00 80 00 00 00 00 02 00 00 00 00 03 77 77 77 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01 03 66 74 70 c0 10 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
 
 #
 #  We encode "www.example.com"
 #  and then "ftp" with a pointer c010 to "example.com"
 #
 decode-proto -
-match packet = { id = 0, query = query, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 2, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ftp.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
+match packet = { id = 0, query = response, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 2, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ftp.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
 
 #
 #  multiple labels (3), but with all counts removed.  The counts will
@@ -60,15 +60,15 @@ match packet = { id = 0, query = query, opcode = query, authoritative = no, trun
 #  about every bit of RFC correctness here, we just care to test the
 #  encoders and decoders for formatting.
 #
-encode-proto packet = { id = 0, query = query, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 3, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ftp.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ns.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
-match 00 00 00 00 00 00 00 03 00 00 00 00 03 77 77 77 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01 03 66 74 70 c0 10 00 01 00 01 00 00 00 10 00 04 7f 00 00 01 02 6e 73 c0 10 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
+encode-proto packet = { id = 0, query = response, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 3, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ftp.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ns.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
+match 00 00 80 00 00 00 00 03 00 00 00 00 03 77 77 77 07 65 78 61 6d 70 6c 65 03 63 6f 6d 00 00 01 00 01 00 00 00 10 00 04 7f 00 00 01 03 66 74 70 c0 10 00 01 00 01 00 00 00 10 00 04 7f 00 00 01 02 6e 73 c0 10 00 01 00 01 00 00 00 10 00 04 7f 00 00 01
 
 #
 #  We encode "www.example.com"
 #  and then "ftp" with a pointer c010 to "example.com"
 #
 decode-proto -
-match packet = { id = 0, query = query, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 3, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ftp.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ns.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
+match packet = { id = 0, query = response, opcode = query, authoritative = no, truncated-response = no, recursion-desired = no, recursion-available = no, reserved = no, authentic-data = no, checking-disabled = no, rcode = no-error, qdcount = 0, ancount = 3, nscount = 0, arcount = 0 }, rr = { name = "www.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ftp.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }, rr = { name = "ns.example.com", type = a, class = 1, ttl = 16, type.a = { ip-address = 127.0.0.1 } }
 
 count
 match 22

diff --git a/src/tests/unit/protocols/dns/error.txt b/src/tests/unit/protocols/dns/error.txt
index 924afc44379ae25d847c086e43f66ec0c9413595..ea472ffa81933e1a0eb45b21b72e4b2fb97f9a46 100644 (file)
--- a/src/tests/unit/protocols/dns/error.txt
+++ b/src/tests/unit/protocols/dns/error.txt
@@ -13,10 +13,13 @@ decode-proto 44 81 9a 97 00 00 00 00 00 01 00 00  01 00 00 00 2d 00 00 dc dc 23
 match packet = { id = 17537, query = response, opcode = 3, authoritative = no, truncated-response = yes, recursion-desired = no, recursion-available = yes, reserved = no, authentic-data = no, checking-disabled = yes, rcode = yx-rr-set, qdcount = 0, ancount = 0, nscount = 1, arcount = 0 }, raw.ns = 0x010000002d0000dcdc23
 
 decode-proto 20 20 20 20 20 20 20 20 20 20 20 20 ff
-match Failed decoding questions - question structure at count 1/8224 overflows the packet
+match DNS packet malformed - unexpected NS records in query packet
 
 decode-proto 2020 2020 0000 2020 2020 2020 012d 0000 0c20 2020 2020 2000 2520 2020 2020 ff20 2020 2020 2020 ff20 2020 2020 2020 20202020 2020 2020 2020 2020 20ff 2020 c00d
+match DNS packet malformed - no 'questions' in query packet
+
+decode-proto 2020 A020 0000 2020 2020 2020 012d 0000 0c20 2020 2020 2000 2520 2020 2020 ff20 2020 2020 2020 ff20 2020 2020 2020 20202020 2020 2020 2020 2020 20ff 2020 c00d
 match Failed decoding RRs - rr structure at count 2/8224 overflows the packet
 
 count
-match 10
+match 12