From f6b1f8e7d642f82d830a71b78699a4349e0158e1 Mon Sep 17 00:00:00 2001 From: Jeremy Harris Date: Tue, 10 Oct 2023 23:03:28 +0100 Subject: [PATCH 3/3] Harden dnsdb against crafted DNS responses. Bug 3033 (cherry picked from commit 8787c8994f07c23c3664d76926e02f07314d699d) --- doc/ChangeLog | 3 ++ src/dns.c | 11 +++--- src/lookups/dnsdb.c | 78 +++++++++++++++++++++++++++-------------- 3 files changed, 59 insertions(+), 33 deletions(-) diff --git a/src/dns.c b/src/dns.c index 7d7ee0c04..8dc3695a1 100644 --- a/src/dns.c +++ b/src/dns.c @@ -300,15 +300,15 @@ return string_from_gstring(g); /* Increment the aptr in dnss, checking against dnsa length. 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; } /************************************************* * Get next DNS record from answer block * *************************************************/ /* Call this with reset == RESET_ANSWERS to scan the answer block, reset == @@ -384,15 +384,15 @@ if (reset != RESET_NEXT) namelen = dn_expand(dnsa->answer, dnsa->answer + dnsa->answerlen, dnss->aptr, (DN_EXPAND_ARG4_TYPE) &dnss->srr.name, DNS_MAXNAME); if (namelen < 0) goto null_return; /* skip name, type, class & TTL */ 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; } dnss->rrcount = reset == RESET_AUTHORITY ? ntohs(h->nscount) : ntohs(h->arcount); TRACE debug_printf("%s: reset (%s rrcount %d)\n", __FUNCTION__, reset == RESET_AUTHORITY ? "NS" : "AR", dnss->rrcount); @@ -424,18 +424,17 @@ if (dnss_inc_aptr(dnsa, dnss, namelen)) goto null_return; GETSHORT(dnss->srr.type, dnss->aptr); /* Record type */ TRACE trace = "R-class"; if (dnss_inc_aptr(dnsa, dnss, 2)) goto null_return; /* Don't want class */ 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. */ TRACE debug_printf("%s: return %s\n", __FUNCTION__, dns_text_type(dnss->srr.type)); return &dnss->srr; diff --git a/src/lookups/dnsdb.c b/src/lookups/dnsdb.c index 355be1b5d..020dc9a52 100644 --- a/src/lookups/dnsdb.c +++ b/src/lookups/dnsdb.c @@ -394,80 +394,101 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) /* Other kinds of record just have one piece of data each, but there may be several of them, of course. */ if (yield->ptr) yield = string_catn(yield, outsep, 1); 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; uschar s[264]; uschar * p = US rr->data; 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); sprintf(CS s, "%d%c%d%c%d%c", priority, *outsep2, weight, *outsep2, port, *outsep2); yield = string_cat(yield, s); 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); GETSHORT(port, p); if (priority != 1) continue; /* CSA version must be 1 */ @@ -510,15 +531,15 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) "domain=%s", dns_text_type(type), domain); break; } else yield = string_cat(yield, s); 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); rc = dn_expand(dnsa->answer, dnsa->answer + dnsa->answerlen, p, (DN_EXPAND_ARG4_TYPE)s, sizeof(s)); if (rc < 0) @@ -526,16 +547,19 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) log_write(0, LOG_MAIN, "responsible-mailbox truncated: type=%s " "domain=%s", dns_text_type(type), domain); break; } 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); yield = string_cat(yield, s); } } } /* Loop for list of returned records */ -- 2.42.0