From f6b1f8e7d642f82d830a71b78699a4349e0158e1 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Tue, 10 Oct 2023 23:03:28 +0100 Subject: [PATCH] Harden dnsdb against crafted DNS responses. Bug 3033 (cherry picked from commit 8787c8994f07c23c3664d76926e02f07314d699d) --- doc/doc-txt/ChangeLog | 3 ++ src/src/dns.c | 11 +++--- src/src/lookups/dnsdb.c | 78 +++++++++++++++++++++++++++-------------- 3 files changed, 59 insertions(+), 33 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index c36718d7e..f7ab3c005 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -225,6 +225,9 @@ JH/39 Bug 3023: Fix crash induced by some combinations of zero-length strings JH/40 Support list of dkim results in the dkim_status ACL condition, making it more usable in the data ACL. +JH/43 Bug 3033: Harden dnsdb lookups against crafted DNS responses. + CVE-2023-42219 + Exim version 4.96 ----------------- diff --git a/src/src/dns.c b/src/src/dns.c index 7d7ee0c04..8dc3695a1 100644 --- a/src/src/dns.c +++ b/src/src/dns.c @@ -304,7 +304,7 @@ Return: TRUE for a bad result static BOOL dnss_inc_aptr(const dns_answer * dnsa, dns_scan * dnss, unsigned delta) { -return (dnss->aptr += delta) >= dnsa->answer + dnsa->answerlen; +return (dnss->aptr += delta) > dnsa->answer + dnsa->answerlen; } /************************************************* @@ -388,7 +388,7 @@ if (reset != RESET_NEXT) TRACE trace = "A-hdr"; if (dnss_inc_aptr(dnsa, dnss, namelen+8)) goto null_return; GETSHORT(dnss->srr.size, dnss->aptr); /* size of data portion */ - /* skip over it */ + /* skip over it, checking for a bogus size */ TRACE trace = "A-skip"; if (dnss_inc_aptr(dnsa, dnss, dnss->srr.size)) goto null_return; } @@ -428,10 +428,9 @@ GETLONG(dnss->srr.ttl, dnss->aptr); /* TTL */ GETSHORT(dnss->srr.size, dnss->aptr); /* Size of data portion */ dnss->srr.data = dnss->aptr; /* The record's data follows */ -/* Unchecked increment ok here since no further access on this iteration; -will be checked on next at "R-name". */ - -dnss->aptr += dnss->srr.size; /* Advance to next RR */ +/* skip over it, checking for a bogus size */ +if (dnss_inc_aptr(dnsa, dnss, dnss->srr.size)) + goto null_return; /* Return a pointer to the dns_record structure within the dns_answer. This is for convenience so that the scans can use nice-looking for loops. */ diff --git a/src/src/lookups/dnsdb.c b/src/src/lookups/dnsdb.c index 355be1b5d..020dc9a52 100644 --- a/src/src/lookups/dnsdb.c +++ b/src/src/lookups/dnsdb.c @@ -398,43 +398,60 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) if (type == T_TXT || type == T_SPF) { - if (outsep2 == NULL) /* output only the first item of data */ - yield = string_catn(yield, US (rr->data+1), (rr->data)[0]); + if (!outsep2) /* output only the first item of data */ + { + uschar n = (rr->data)[0]; + /* size byte + data bytes must not excced the RRs length */ + if (n + 1 <= rr->size) + yield = string_catn(yield, US (rr->data+1), n); + } else { /* output all items */ int data_offset = 0; while (data_offset < rr->size) { - uschar chunk_len = (rr->data)[data_offset++]; - if (outsep2[0] != '\0' && data_offset != 1) + uschar chunk_len = (rr->data)[data_offset]; + int remain = rr->size - data_offset; + + /* Apparently there are resolvers that do not check RRs before passing + them on, and glibc fails to do so. So every application must... + Check for chunk len exceeding RR */ + + if (chunk_len > remain) + chunk_len = remain; + + if (*outsep2 && data_offset != 0) yield = string_catn(yield, outsep2, 1); - yield = string_catn(yield, US ((rr->data)+data_offset), chunk_len); + yield = string_catn(yield, US ((rr->data) + ++data_offset), --chunk_len); data_offset += chunk_len; } } } else if (type == T_TLSA) - { - uint8_t usage, selector, matching_type; - uint16_t payload_length; - uschar s[MAX_TLSA_EXPANDED_SIZE]; - uschar * sp = s; - uschar * p = US rr->data; + if (rr->size < 3) + continue; + else + { + uint8_t usage, selector, matching_type; + uint16_t payload_length; + uschar s[MAX_TLSA_EXPANDED_SIZE]; + uschar * sp = s; + uschar * p = US rr->data; + + usage = *p++; + selector = *p++; + matching_type = *p++; + /* What's left after removing the first 3 bytes above */ + payload_length = rr->size - 3; + sp += sprintf(CS s, "%d%c%d%c%d%c", usage, *outsep2, + selector, *outsep2, matching_type, *outsep2); + /* Now append the cert/identifier, one hex char at a time */ + while (payload_length-- > 0 && sp-s < (MAX_TLSA_EXPANDED_SIZE - 4)) + sp += sprintf(CS sp, "%02x", *p++); - usage = *p++; - selector = *p++; - matching_type = *p++; - /* What's left after removing the first 3 bytes above */ - payload_length = rr->size - 3; - sp += sprintf(CS s, "%d%c%d%c%d%c", usage, *outsep2, - selector, *outsep2, matching_type, *outsep2); - /* Now append the cert/identifier, one hex char at a time */ - while (payload_length-- > 0 && sp-s < (MAX_TLSA_EXPANDED_SIZE - 4)) - sp += sprintf(CS sp, "%02x", *p++); - - yield = string_cat(yield, s); - } + yield = string_cat(yield, s); + } else /* T_CNAME, T_CSA, T_MX, T_MXH, T_NS, T_PTR, T_SOA, T_SRV */ { int priority, weight, port; @@ -444,17 +461,20 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) switch (type) { case T_MXH: + if (rr->size < sizeof(u_int16_t)) continue; /* mxh ignores the priority number and includes only the hostnames */ GETSHORT(priority, p); break; case T_MX: + if (rr->size < sizeof(u_int16_t)) continue; GETSHORT(priority, p); sprintf(CS s, "%d%c", priority, *outsep2); yield = string_cat(yield, s); break; case T_SRV: + if (rr->size < 3*sizeof(u_int16_t)) continue; GETSHORT(priority, p); GETSHORT(weight, p); GETSHORT(port, p); @@ -464,6 +484,7 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) break; case T_CSA: + if (rr->size < 3*sizeof(u_int16_t)) continue; /* See acl_verify_csa() for more comments about CSA. */ GETSHORT(priority, p); GETSHORT(weight, p); @@ -514,7 +535,7 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) if (type == T_SOA && outsep2 != NULL) { - unsigned long serial, refresh, retry, expire, minimum; + unsigned long serial = 0, refresh = 0, retry = 0, expire = 0, minimum = 0; p += rc; yield = string_catn(yield, outsep2, 1); @@ -530,8 +551,11 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) else yield = string_cat(yield, s); p += rc; - GETLONG(serial, p); GETLONG(refresh, p); - GETLONG(retry, p); GETLONG(expire, p); GETLONG(minimum, p); + if (rr->size >= p - rr->data - 5*sizeof(u_int32_t)) + { + GETLONG(serial, p); GETLONG(refresh, p); + GETLONG(retry, p); GETLONG(expire, p); GETLONG(minimum, p); + } sprintf(CS s, "%c%lu%c%lu%c%lu%c%lu%c%lu", *outsep2, serial, *outsep2, refresh, *outsep2, retry, *outsep2, expire, *outsep2, minimum); -- 2.30.2