From a95acb1c19c2e3600ef327c71318e33316d34440 Mon Sep 17 00:00:00 2001 From: "Heiko Schlittermann (HS12-RIPE)" Date: Thu, 5 Oct 2023 22:49:57 +0200 Subject: [PATCH] fix: string_is_ip_address (CVE-2023-42117) Bug 3031 --- doc/doc-txt/ChangeLog | 206 ++++++++++++++++++++++++++++++++++++++++++ src/src/expand.c | 14 ++- src/src/functions.h | 1 + src/src/string.c | 200 +++++++++++++++++++++------------------- 4 files changed, 323 insertions(+), 98 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index 8c5b7fd9c..c36718d7e 100644 --- a/doc/doc-txt/ChangeLog +++ b/doc/doc-txt/ChangeLog @@ -20,6 +20,212 @@ JH/03 Bug 3001: Fix a possible OOB read in the SPA authenticator, which could CVE-2023-42114 +JH/04 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). + + +Exim version 4.next +------------------- + +HS/01 Fix string_is_ip_address() CVE-2023-42117 (Bug 3031) + + +Exim version 4.97 +----------------- + +JH/01 The hosts_connection_nolog main option now also controls "no MAIL in + SMTP connection" log lines. + +JH/02 Option default value updates: + - queue_fast_ramp (main) true (was false) + - remote_max_parallel (main) 4 (was 2) + +JH/03 Cache static regex pattern compilations, for use by ACLs. + +JH/04 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/05 Follow symlinks for placing a watch on TLS creds files. This means + (under Linux) we watch the dir containing the final file; previously + it would be the dir with the first symlink. We still do not monitor + the entire path. + +JH/06 Check for bad chars in rDNS for sender_host_name. The OpenBSD (at least) + dn_expand() is happy to pass them through. + +JH/07 OpenSSL Fix auto-reload of changed server OCSP proof. Previously, if + the file with the proof had an unchanged name, the new proof(s) were + loaded on top of the old ones (and nover used; the old ones were stapled). + +JH/08 Bug 2915: Fix use-after-free for $regex variables. Previously when + more than one message arrived in a single connection a reference from + the earlier message could be re-used. Often a sigsegv resulted. + These variables were introduced in Exim 4.87. + Debug help from Graeme Fowler. + +JH/09 Fix ${filter } for conditions that modify $value. Previously the + modified version would be used in construction the result, and a memory + error would occur. + +JH/10 GnuTLS: fix for (IOT?) clients offering no TLS extensions at all. + Find and fix by Jasen Betts. + +JH/11 OpenSSL: fix for ancient clients needing TLS support for versions earlier + than TLSv1,2, Previously, more-recent versions of OpenSSL were permitting + the systemwide configuration to override the Exim config. + +HS/01 Bug 2728: Introduce EDITME option "DMARC_API" to work around incompatible + API changes in libopendmarc. + +JH/12 Bug 2930: Fix daemon startup. When started from any process apart from + pid 1, in the normal "background daemon" mode, having to drop process- + group leadership also lost track of needing to create listener sockets. + +JH/13 Bug 2929: Fix using $recipients after ${run...}. A change made for 4.96 + resulted in the variable appearing empty. Find and fix by Ruben Jenster. + +JH/14 Bug 2933: Fix regex substring match variables for null matches. Since 4.96 + a capture group which obtained no text (eg. "(abc)*" matching zero + occurrences) could cause a segfault if the corresponding $ was + expanded. + +JH/15 Fix argument parsing for ${run } expansion. Previously, when an argument + included a close-brace character (eg. it itself used an expansion) an + error occurred. + +JH/16 Move running the smtp connect ACL to before, for TLS-on-connect ports, + starting TLS. Previously it was after, meaning that attackers on such + ports had to be screened using the host_reject_connection main config + option. The new sequence aligns better with the STARTTLS behaviour, and + permits defences against crypto-processing load attacks, even though it + is strictly an incompatible change. + Also, avoid sending any SMTP fail response for either the connect ACL + or host_reject_connection, for TLS-on-connect ports. + +JH/17 Permit the ACL "encrypted" condition to be used in a HELO/EHLO ACL, + Previously this was not permitted, but it makes reasonable sense. + While there, restore a restriction on using it from a connect ACL; given + the change JH/16 it could only return false (and before 4.91 was not + permitted). + +JH/18 Fix a fencepost error in logging. Previously (since 4.92) when a log line + was exactly sized compared to the log buffer, a crash occurred with the + misleading message "bad memory reference; pool not found". + Found and traced by Jasen Betts. + +JH/19 Bug 2911: Fix a recursion in DNS lookups. Previously, if the main option + dns_again_means_nonexist included an element causing a DNS lookup which + iteslf returned DNS_AGAIN, unbounded recursion occurred. Possible results + included (though probably not limited to) a process crash from stack + memory limit, or from excessive open files. Replace this with a paniclog + whine (as this is likely a configuration error), and returning + DNS_NOMATCH. + +JH/20 Bug 2954: (OpenSSL) Fix setting of explicit EC curve/group. Previously + this always failed, probably leading to the usual downgrade to in-clear + connections. + +JH/21 Fix TLSA lookups. Previously dns_again_means_nonexist would affect + SERVFAIL results, which breaks the downgrade resistance of DANE. Change + to not checking that list for these lookups. + +JH/22 Bug 2434: Add connection-elapsed "D=" element to more connection + closure log lines. + +JH/23 Fix crash in string expansions. Previously, if an empty variable was + immediately followed by an expansion operator, a null-indirection read + was done, killing the process. + +JH/24 Bug 2997: When built with EXPERIMENTAL_DSN_INFO, bounce messages can + include an SMTP response string which is longer than that supported + by the delivering transport. Alleviate by wrapping such lines before + column 80. + +JH/25 Bug 2827: Restrict size of References: header in bounce messages to 998 + chars (RFC limit). Previously a limit of 12 items was made, which with + a not-impossible References: in the message being bounced could still + be over-large and get stopped in the transport. + +JH/26 For a ${readsocket } in TLS mode, send a TLS Close Alert before the TCP + close. Previously a bare socket close was done. + +JH/27 Fix ${srs_encode ..}. Previously it would give a bad result for one day + every 1024 days. + +JH/28 Bug 2996: Fix a crash in the smtp transport. When finding that the + message being considered for delivery was already being handled by + another process, and having an SMTP connection already open, the function + to close it tried to use an uninitialized variable. This would afftect + high-volume sites more, especially when running mailing-list-style loads. + Pollution of logs was the major effect, as the other process delivered + the message. Found and partly investigated by Graeme Fowler. + +JH/29 Change format of the internal ID used for message identification. The old + version only supported 31 bits for a PID element; the new 64 (on systems + which can use Base-62 encoding, which is all currently supported ones + but not Darwin (MacOS) or Cygwin, which have case-insensitive filesystems + and must use Base-36). The new ID is 23 characters rather than 16, and is + visible in various places - notably logs, message headers, and spool file + names. Various of the ancillary utilities also have to know the format. + As well as the expanded PID portion, the sub-second part of the time + recorded in the ID is expanded to support finer precision. Theoretically + this permits a receive rate from a single comms channel of better than the + previous 2000/sec. + The major timestamp part of the ID is not changed; at 6 characters it is + usable until about year 3700. + Updating from previously releases is fully supported: old-format spool + files are still usable, and the utilities support both formats. New + message will use the new format. The one hints-DB file type which uses + message-IDs (the transport wait- DB) will be discarded if an old-format ID + is seen; new ones will be built with only new-format IDs. + Optionally, a utility can be used to convert spool files from old to new, + but this is only an efficiency measure not a requirement for operation + Downgrading from new to old requires running a provided utility, having + first stopped all operations. This will convert any spool files from new + back to old (losing time-precision and PID information) and remove any + wait- hints databases. + +JH/30 Bug 3006: Fix handling of JSON strings having embedded commas. Previously + we treated them as item separators when parsing for a list item, but they + need to be protected by the doublequotes. While there, add handling for + backslashes. + +JH/31 Bug 2998: Fix ${utf8clean:...} to disallow UTF-16 surrogate codepoints. + Found and fixed by Jasen Betts. No testcase for this as my usual text + editor insists on emitting only valid UTF-8. + +JH/32 Fix "tls_dhparam = none" under GnuTLS. At least with 3.7.9 this gave + a null-indirection SIGSEGV for the receive process. + +JH/33 Fix free for live variable $value created by a ${run ...} expansion during + -bh use. Internal checking would spot this and take a panic. + +JH/34 Bug 3013: Fix use of $recipients within arguments for ${run...}. + In 4.96 this would expand to empty. + +JH/35 Bug 3014: GnuTLS: fix expiry date for an auto-generated server + certificate. Find and fix by Andreas Metzler. + +JH/36 Add ARC info to DMARC hostory records. + +JH/37 Bug 3016: Avoid sending DSN when message was accepted under fakereject + or fakedefer. Previously the sender could discover that the message + had in fact been accepted. + +JH/38 Taint-track intermediate values from the peer in multi-stage authentation + sequences. Previously the input was not noted as being tainted; notably + this resulted in behaviour of LOGIN vs. PLAIN being inconsistent under + bad coding of authenticators. + +JH/39 Bug 3023: Fix crash induced by some combinations of zero-length strings + and ${tr...}. Found and diagnosed by Heiko Schlichting. + +JH/40 Support list of dkim results in the dkim_status ACL condition, making + it more usable in the data ACL. + + Exim version 4.96 ----------------- diff --git a/src/src/expand.c b/src/src/expand.c index 36c9f423b..4986e4657 100644 --- a/src/src/expand.c +++ b/src/src/expand.c @@ -2650,9 +2650,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 224666cb1..3c8104d25 100644 --- a/src/src/functions.h +++ b/src/src/functions.h @@ -556,6 +556,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/string.c b/src/src/string.c index a5161bb31..9aefc2b58 100644 --- a/src/src/string.c +++ b/src/src/string.c @@ -29,123 +29,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 */ -- 2.30.2