fix: string_is_ip_address (CVE-2023-42117) Bug 3031
authorHeiko Schlittermann (HS12-RIPE) <hs@schlittermann.de>
Thu, 5 Oct 2023 20:49:57 +0000 (22:49 +0200)
committerHeiko Schlittermann (HS12-RIPE) <hs@schlittermann.de>
Sat, 14 Oct 2023 21:45:43 +0000 (23:45 +0200)
doc/doc-txt/ChangeLog
src/src/expand.c
src/src/functions.h
src/src/string.c

index 8c5b7fd9cb45b7192cdfa44ed5cd10626bbcb226..c36718d7efeab21ea28d542ab21f47790442208e 100644 (file)
@@ -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<n> 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 $<n> 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
 -----------------
 
index 36c9f423bedf06e844bb84a2a9aece8344a57472..4986e4657ac52ea9927a761174b391a3ee1e4160 100644 (file)
@@ -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 */
index 224666cb121131582deadca58a8bca987032274b..3c8104d25795d65bdbb41cf9c2d3950b72be6959 100644 (file)
@@ -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
index a5161bb31e057d3592294a7a9bd0ad57e4b101f4..9aefc2b581114be5db73a06a0d029ca2e3f27512 100644 (file)
@@ -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 (%<id>) 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 */