From: Alan T. DeKok Date: Fri, 15 Oct 2021 18:36:25 +0000 (-0400) Subject: better sanity checker for DNS packets X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=f084dbd47880a41b6b13f46592ae0d19f6147504;p=thirdparty%2Ffreeradius-server.git better sanity checker for DNS packets --- diff --git a/src/protocols/dns/base.c b/src/protocols/dns/base.c index 4a7ea76755b..8a233f8ac79 100644 --- a/src/protocols/dns/base.c +++ b/src/protocols/dns/base.c @@ -79,10 +79,8 @@ fr_dict_attr_autoload_t dns_dict_attr[] = { 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 + int qdcount, count, expected; if (packet_len <= DNS_HDR_LEN) { DECODE_FAIL(MIN_LENGTH_PACKET); @@ -97,11 +95,13 @@ bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query, fr_d return false; } + qdcount = fr_net_to_uint16(packet + 4); + if (query) { /* * There should be at least one query, and no replies in the query! */ - if (fr_net_to_uint16(packet + 4) == 0) { + if (!qdcount) { DECODE_FAIL(NO_QUESTIONS); return false; } @@ -122,26 +122,150 @@ bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query, fr_d */ } -#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 for wildly fake packets, by making rough + * estimations. This way we don't actually have to walk + * the packet. + */ + if (p + (qdcount * 5) > end) { + DECODE_FAIL(TOO_MANY_RRS); + return false; + } + p += (qdcount * 5); + + if ((p + ((expected - qdcount) * (1 + 8 + 2))) > end) { + DECODE_FAIL(TOO_MANY_RRS); + return false; + } + + /* + * The counts are at least vaguely OK, let's walk over the whole packet. + */ + p = packet + DNS_HDR_LEN; + /* * Check that lengths of RRs match. */ while (p < end) { - uint16_t len; + uint16_t len = 0; + uint8_t const *start = p; + /* - * @todo - get DNS label, followed by 2 octets of - * type, 2 of class, 4 of TTL. Then check the RR. + * Simple DNS label decoder */ + while (p < end) { + /* + * 0x00 is "end of label" + */ + if (!*p) { + p++; + break; + } + + /* + * 2 octets of 14-bit pointer, which must + * be at least somewhat sane. + */ + if (*p >= 0xc0) { + size_t offset; + + if ((p + 2) >= end) { + DECODE_FAIL(INVALID_RR_LABEL); + return false; + } + + offset = p[1]; + offset += ((*p & ~0xc0) << 8); + + /* + * Can't point to the header. + */ + if (offset < 12) { + DECODE_FAIL(INVALID_RR_LABEL); + return false; + } + + /* + * Can't point to the current label. + */ + if ((packet + offset) >= start) { + DECODE_FAIL(INVALID_RR_LABEL); + return false; + } + + /* + * A compressed pointer is the end of the current label. + */ + p += 2; + break; + } + + /* + * 0b10 and 0b10 are forbidden + */ + if (*p > 63) { + DECODE_FAIL(INVALID_RR_LABEL); + return false; + } + + /* + * It must be a length byte, which doesn't cause overflow. + */ + if ((p + *p + 1) >= end) { + DECODE_FAIL(INVALID_RR_LABEL); + return false; + } + + /* + * Total length of labels can't be too high. + */ + len += *p; + if (len >= 256) { + DECODE_FAIL(INVALID_RR_LABEL); + return false; + } + + /* + * Go to the next label. + */ + p += *p + 1; + } + + if (qdcount) { + /* + * qtype + qclass + */ + if ((p + 4) > end) { + DECODE_FAIL(MISSING_RR_HEADER); + return false; + } + + p += 4; + qdcount--; + goto next; + } + /* + * type + class + TTL + */ + if ((p + 8) > end) { + DECODE_FAIL(MISSING_RR_HEADER); + return false; + } + p += 8; + + /* + * rr_len + */ if ((p + 2) >= end) { - DECODE_FAIL(MISSING_RRLEN); + DECODE_FAIL(MISSING_RR_LEN); return false; } @@ -153,6 +277,8 @@ bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query, fr_d } p+= len; + +next: count++; if (count > expected) { @@ -165,7 +291,6 @@ bool fr_dns_packet_ok(uint8_t const *packet, size_t packet_len, bool query, fr_d 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 4a777e90f3d..83148871a2e 100644 --- a/src/protocols/dns/decode.c +++ b/src/protocols/dns/decode.c @@ -668,7 +668,9 @@ static fr_table_num_ordered_t reason_fail_table[] = { { 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("invalid label for resource record"), DECODE_FAIL_INVALID_RR_LABEL }, + { L("missing resource record header"), DECODE_FAIL_MISSING_RR_HEADER }, + { L("missing resource record length field"), DECODE_FAIL_MISSING_RR_LEN }, { 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 }, diff --git a/src/protocols/dns/dns.h b/src/protocols/dns/dns.h index 3994d3f4a68..dc8ab2ef0ef 100644 --- a/src/protocols/dns/dns.h +++ b/src/protocols/dns/dns.h @@ -112,7 +112,9 @@ typedef enum { DECODE_FAIL_NO_QUESTIONS, DECODE_FAIL_ANSWERS_IN_QUESTION, DECODE_FAIL_NS_IN_QUESTION, - DECODE_FAIL_MISSING_RRLEN, + DECODE_FAIL_INVALID_RR_LABEL, + DECODE_FAIL_MISSING_RR_HEADER, + DECODE_FAIL_MISSING_RR_LEN, DECODE_FAIL_RR_OVERFLOWS_PACKET, DECODE_FAIL_TOO_MANY_RRS, DECODE_FAIL_TOO_FEW_RRS, diff --git a/src/tests/unit/protocols/dns/error.txt b/src/tests/unit/protocols/dns/error.txt index ea472ffa819..bdf45bcbdcd 100644 --- a/src/tests/unit/protocols/dns/error.txt +++ b/src/tests/unit/protocols/dns/error.txt @@ -5,12 +5,12 @@ proto dns proto-dictionary dns # 0x6000 questions -decode-proto 44 81 9a 97 00 00 00 00 60 00 00 00 00 00 01 00 2d 00 00 dc dc 23 -match Failed decoding NS - ns structure at count 1/24576 overflows the packet +decode-proto 44 81 9a 97 00 00 00 00 60 00 00 00 00 00 01 00 2d 00 00 dc dc 23 +match DNS packet malformed - missing resource record length field -# 256 bytes of data +# 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 +match DNS packet malformed - missing resource record header decode-proto 20 20 20 20 20 20 20 20 20 20 20 20 ff match DNS packet malformed - unexpected NS records in query packet @@ -19,7 +19,7 @@ decode-proto 2020 2020 0000 2020 2020 2020 012d 0000 0c20 2020 2020 2000 2520 20 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 +match DNS packet malformed - invalid label for resource record count match 12