From: Alan T. DeKok Date: Fri, 15 Oct 2021 17:32:50 +0000 (-0400) Subject: make fr_dns_ok() return a reason why the packet is bad X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=313e7587037f1a1137f2f319ed0aece7d988c989;p=thirdparty%2Ffreeradius-server.git 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 7063565d164..81ba4cd324b 100644 --- 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 eccb7c602e0..4a7ea76755b 100644 --- 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 3356aa0e3dc..4a777e90f3d 100644 --- 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 34bee267bb5..3994d3f4a68 100644 --- 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 a2cc49a7a16..a91f7ce41c9 100644 --- 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 924afc44379..ea472ffa819 100644 --- 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