From: Heiko Schlittermann (HS12-RIPE) Date: Sun, 15 Oct 2023 17:53:25 +0000 (+0200) Subject: Merge branch 'exim-4.96+security' into master+security X-Git-Tag: exim-4.97-RC3~6 X-Git-Url: https://git.exim.org/exim.git/commitdiff_plain/3857519629ca8fbcf3466c3fc761a5bb6ed32d53?hp=4f07f38374f8662c318699fb30432273ffcfe0d3 Merge branch 'exim-4.96+security' into master+security * exim-4.96+security: docs: Changelog Harden dnsdb against crafted DNS responses. Bug 3033 SPF: harden against crafted DNS responses fix: string_is_ip_address (CVE-2023-42117) Bug 3031 Testsuite: Add testcases for string_is_ip_address (CVE-2023-42117) --- diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index a78ec386f..4306cabc0 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -205,6 +205,14 @@ JH/42 Bug 3001: Fix a possible OOB read in the SPA authenticator, which could be triggered by externally-controlled input. Found by Trend Micro. CVE-2023-42114 +JH/43 Bug 2903: avoid exit on an attempt to rewrite a malformed address. + Make the rewrite never match and keep the logging. Trust the + admin to be using verify=header-syntax (to actually reject the message). + +JH/44 Bug 3033: Harden dnsdb lookups against crafted DNS responses. + CVE-2023-42219 + +HS/02 Fix string_is_ip_address() CVE-2023-42117 (Bug 3031) Exim version 4.96 ----------------- diff --git a/src/src/dns.c b/src/src/dns.c index d39b4b590..db566f2e8 100644 --- a/src/src/dns.c +++ b/src/src/dns.c @@ -305,7 +305,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; } /************************************************* @@ -389,7 +389,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; } @@ -429,10 +429,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/expand.c b/src/src/expand.c index 1d0ddec2a..40cc8d73a 100644 --- a/src/src/expand.c +++ b/src/src/expand.c @@ -2763,9 +2763,17 @@ switch(cond_type = identify_operator(&s, &opname)) case ECOND_ISIP: case ECOND_ISIP4: case ECOND_ISIP6: - rc = string_is_ip_address(sub[0], NULL); - *yield = ((cond_type == ECOND_ISIP)? (rc != 0) : - (cond_type == ECOND_ISIP4)? (rc == 4) : (rc == 6)) == testfor; + { + const uschar *errp; + const uschar **errpp; + DEBUG(D_expand) errpp = &errp; else errpp = 0; + if (0 == (rc = string_is_ip_addressX(sub[0], NULL, errpp))) + DEBUG(D_expand) debug_printf("failed: %s\n", errp); + + *yield = ( cond_type == ECOND_ISIP ? rc != 0 : + cond_type == ECOND_ISIP4 ? rc == 4 : rc == 6) == testfor; + } + break; /* Various authentication tests - all optionally compiled */ diff --git a/src/src/functions.h b/src/src/functions.h index 4222c623a..8f85165e7 100644 --- a/src/src/functions.h +++ b/src/src/functions.h @@ -576,6 +576,7 @@ extern uschar *string_dequote(const uschar **); extern uschar *string_format_size(int, uschar *); extern int string_interpret_escape(const uschar **); extern int string_is_ip_address(const uschar *, int *); +extern int string_is_ip_addressX(const uschar *, int *, const uschar **); #ifdef SUPPORT_I18N extern BOOL string_is_utf8(const uschar *); #endif diff --git a/src/src/lookups/dnsdb.c b/src/src/lookups/dnsdb.c index 1563eda56..35a946447 100644 --- a/src/src/lookups/dnsdb.c +++ b/src/src/lookups/dnsdb.c @@ -394,38 +394,55 @@ while ((domain = string_nextinlist(&keystring, &sep, NULL, 0))) if (type == T_TXT || type == T_SPF) { if (!outsep2) /* output only the first item of data */ - yield = string_catn(yield, US (rr->data+1), (rr->data)[0]); + { + 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 for (unsigned data_offset = 0; data_offset < rr->size; ) { 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; @@ -435,17 +452,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); @@ -455,6 +475,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); @@ -505,7 +526,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); @@ -521,8 +542,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); diff --git a/src/src/string.c b/src/src/string.c index 52b1d2fb5..055a37fd6 100644 --- a/src/src/string.c +++ b/src/src/string.c @@ -30,123 +30,133 @@ Arguments: maskptr NULL if no mask is permitted to follow otherwise, points to an int where the offset of '/' is placed if there is no / followed by trailing digits, *maskptr is set 0 + errp NULL if no diagnostic information is required, and if the netmask + length should not be checked. Otherwise it is set pointing to a short + descriptive text. Returns: 0 if the string is not a textual representation of an IP address 4 if it is an IPv4 address 6 if it is an IPv6 address -*/ +The legacy string_is_ip_address() function follows below. +*/ int -string_is_ip_address(const uschar *s, int *maskptr) -{ -int yield = 4; +string_is_ip_addressX(const uschar *ip_addr, int *maskptr, const uschar **errp) { + struct addrinfo hints; + struct addrinfo *res; -/* If an optional mask is permitted, check for it. If found, pass back the -offset. */ + uschar *slash, *percent; -if (maskptr) + uschar *endp = 0; + long int mask = 0; + const uschar *addr = 0; + + /* If there is a slash, but we didn't request a (optional) netmask, + we return failure, as we do if the mask isn't a pure numerical value, + or if it is negative. The actual length is checked later, once we know + the address family. */ + if (slash = Ustrchr(ip_addr, '/')) { - const uschar *ss = s + Ustrlen(s); - *maskptr = 0; - if (s != ss && isdigit(*(--ss))) + if (!maskptr) { - while (ss > s && isdigit(ss[-1])) ss--; - if (ss > s && *(--ss) == '/') *maskptr = ss - s; + if (errp) *errp = "netmask found, but not requested"; + return 0; } - } - -/* A colon anywhere in the string => IPv6 address */ - -if (Ustrchr(s, ':') != NULL) - { - BOOL had_double_colon = FALSE; - BOOL v4end = FALSE; - - yield = 6; - - /* An IPv6 address must start with hex digit or double colon. A single - colon is invalid. */ - - if (*s == ':' && *(++s) != ':') return 0; - - /* Now read up to 8 components consisting of up to 4 hex digits each. There - may be one and only one appearance of double colon, which implies any number - of binary zero bits. The number of preceding components is held in count. */ - for (int count = 0; count < 8; count++) + uschar *rest; + mask = Ustrtol(slash+1, &rest, 10); + if (*rest || mask < 0) { - /* If the end of the string is reached before reading 8 components, the - address is valid provided a double colon has been read. This also applies - if we hit the / that introduces a mask or the % that introduces the - interface specifier (scope id) of a link-local address. */ - - if (*s == 0 || *s == '%' || *s == '/') return had_double_colon ? yield : 0; - - /* If a component starts with an additional colon, we have hit a double - colon. This is permitted to appear once only, and counts as at least - one component. The final component may be of this form. */ - - if (*s == ':') - { - if (had_double_colon) return 0; - had_double_colon = TRUE; - s++; - continue; - } - - /* If the remainder of the string contains a dot but no colons, we - can expect a trailing IPv4 address. This is valid if either there has - been no double-colon and this is the 7th component (with the IPv4 address - being the 7th & 8th components), OR if there has been a double-colon - and fewer than 6 components. */ - - if (Ustrchr(s, ':') == NULL && Ustrchr(s, '.') != NULL) - { - if ((!had_double_colon && count != 6) || - (had_double_colon && count > 6)) return 0; - v4end = TRUE; - yield = 6; - break; - } - - /* Check for at least one and not more than 4 hex digits for this - component. */ - - if (!isxdigit(*s++)) return 0; - if (isxdigit(*s) && isxdigit(*(++s)) && isxdigit(*(++s))) s++; - - /* If the component is terminated by colon and there is more to - follow, skip over the colon. If there is no more to follow the address is - invalid. */ - - if (*s == ':' && *(++s) == 0) return 0; + if (errp) *errp = "netmask not numeric or <0"; + return 0; } - /* If about to handle a trailing IPv4 address, drop through. Otherwise - all is well if we are at the end of the string or at the mask or at a percent - sign, which introduces the interface specifier (scope id) of a link local - address. */ + *maskptr = slash - ip_addr; /* offset of the slash */ + endp = slash; + } else if (maskptr) *maskptr = 0; /* no slash found */ - if (!v4end) - return (*s == 0 || *s == '%' || - (*s == '/' && maskptr != NULL && *maskptr != 0))? yield : 0; + /* The interface-ID suffix (%) is optional (for IPv6). If it + exists, we check it syntactically. Later, if we know the address + family is IPv4, we might reject it. + The interface-ID is mutually exclusive with the netmask, to the + best of my knowledge. */ + if (percent = Ustrchr(ip_addr, '%')) + { + if (slash) + { + if (errp) *errp = "interface-ID and netmask are mutually exclusive"; + return 0; + } + for (uschar *p = percent+1; *p; p++) + if (!isalnum(*p) && !ispunct(*p)) + { + if (errp) *errp = "interface-ID must match [[:alnum:][:punct:]]"; + return 0; + } + endp = percent; } -/* Test for IPv4 address, which may be the tail-end of an IPv6 address. */ - -for (int i = 0; i < 4; i++) + /* inet_pton() can't parse netmasks and interface IDs, so work on a shortened copy + allocated on the current stack */ + if (endp) { + ptrdiff_t l = endp - ip_addr; + if (l > 255) + { + if (errp) *errp = "rudiculous long ip address string"; + return 0; + } + addr = alloca(l+1); /* *BSD does not have strndupa() */ + Ustrncpy((uschar *)addr, ip_addr, l); + ((uschar*)addr)[l] = '\0'; + } else addr = ip_addr; + + int af; + union { /* we do not need this, but inet_pton() needs a place for storage */ + struct in_addr sa4; + struct in6_addr sa6; + } sa; + + af = Ustrchr(addr, ':') ? AF_INET6 : AF_INET; + if (!inet_pton(af, addr, &sa)) { - long n; - uschar * end; - - if (i != 0 && *s++ != '.') return 0; - n = strtol(CCS s, CSS &end, 10); - if (n > 255 || n < 0 || end <= s || end > s+3) return 0; - s = end; + if (errp) *errp = af == AF_INET6 ? "IP address string not parsable as IPv6" + : "IP address string not parsable IPv4"; + return 0; } + /* we do not check the values of the mask here, as + this is done on the callers side (but I don't understand why), so + actually I'd like to do it here, but it breaks at least 0002 */ + switch (af) + { + case AF_INET6: + if (errp && mask > 128) + { + *errp = "IPv6 netmask value must not be >128"; + return 0; + } + return 6; + case AF_INET: + if (percent) + { + if (errp) *errp = "IPv4 address string must not have an interface-ID"; + return 0; + } + if (errp && mask > 32) { + *errp = "IPv4 netmask value must not be >32"; + return 0; + } + return 4; + default: + if (errp) *errp = "unknown address family (should not happen)"; + return 0; + } +} -return !*s || (*s == '/' && maskptr && *maskptr != 0) ? yield : 0; +int +string_is_ip_address(const uschar *ip_addr, int *maskptr) { + return string_is_ip_addressX(ip_addr, maskptr, 0); } + #endif /* COMPILE_UTILITY */ diff --git a/test/scripts/0000-Basic/0002 b/test/scripts/0000-Basic/0002 index b4f2341bb..c1fa1bdb5 100644 --- a/test/scripts/0000-Basic/0002 +++ b/test/scripts/0000-Basic/0002 @@ -458,6 +458,7 @@ ge: ${if ge{ABC}{abc}{y}{n}} gei: ${if gei{ABC}{abc}{y}{n}} isip: ${if isip {1.2.3.4}{y}{n}} 1.2.3.4 +isip: ${if isip {1.2.3}{y}{n}} 1.2.3 isip4: ${if isip4{1.2.3.4}{y}{n}} 1.2.3.4 isip6: ${if isip6{1.2.3.4}{y}{n}} 1.2.3.4 isip: ${if isip {::1.2.3.256}{y}{n}} ::1.2.3.256 @@ -475,6 +476,9 @@ isip: ${if isip {fe80::1.2.3.4}{y}{n}} fe80::1.2.3.4 isip: ${if isip {rhubarb}{y}{n}} rhubarb isip4: ${if isip4{rhubarb}{y}{n}} rhubarb isip6: ${if isip6{rhubarb}{y}{n}} rhubarb +isip6: ${if isip6{::/100}{y}{n}} ::/100 +isip6: ${if isip6{::/foo}{y}{n}} ::/foo +isip6: ${if isip6{::/f o}{y}{n}} ::/f o match: ${if match{abcd}{\N^([ab]+)(\w+)$\N}{$2$1}fail} match: ${if match{abcd}{^\N([ab]+)(\w+)$\N}{$2$1}fail} diff --git a/test/stdout/0002 b/test/stdout/0002 index 2d7c82838..d5bb0605c 100644 --- a/test/stdout/0002 +++ b/test/stdout/0002 @@ -452,6 +452,7 @@ newline tab\134backslash ~tilde\177DEL\200\201. > gei: y > > isip: y 1.2.3.4 +> isip: n 1.2.3 > isip4: y 1.2.3.4 > isip6: n 1.2.3.4 > isip: n ::1.2.3.256 @@ -469,6 +470,9 @@ newline tab\134backslash ~tilde\177DEL\200\201. > isip: n rhubarb > isip4: n rhubarb > isip6: n rhubarb +> isip6: n ::/100 +> isip6: n ::/foo +> isip6: n ::/f o > > match: cdab > match: cdab