Prevent poor connection reuse (bug 1141).

Safety checks against connection reuse where transport options present
a different identity to a remote host for two different messages (in
expansion of interface, helo_data, tls_certificate).

fixes bug 1141.

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index de7c178787dc74360993014d73e164f17afe127a..5111d565a0a6fe17900aa53ad1f4dd4eec040cda 100644 (file)
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -93,6 +93,11 @@ PP/21 Defaulting "accept_8bitmime" to true, not false.
 
 PP/22 Added -bw for inetd wait mode support.
 
+PP/23 Safety checks against connection reuse where transport options present
+      a different identity to a remote host for two different messages (in
+      expansion of interface, helo_data, tls_certificate).
+      Bugzilla 1141.
+
 
 Exim version 4.77
 -----------------

diff --git a/src/src/deliver.c b/src/src/deliver.c
index 10b63397efdadad24bdad409e2562c146b1c6888..579c3376d3a23cfe12fc473c9379187e9986e7b5 100644 (file)
--- a/src/src/deliver.c
+++ b/src/src/deliver.c
@@ -517,7 +517,7 @@ uid/gid/initgroups settings for the two addresses are going to be the same when
 they are delivered.
 
 Arguments:
-  tp            the transort
+  tp            the transport
   addr1         the first address
   addr2         the second address
 
@@ -549,6 +549,43 @@ return TRUE;
 
 
 
+/*************************************************
+*  Check identity variants on an smtp transport  *
+*************************************************/
+
+/* This host might present as multiple identities to a remote host; for
+instance, different source IP addresses, different HELO data, anything
+which differs as a connection characteristic, not just a per-message
+characteristic.  If it is evaluated per-message (DKIM) then that's fine.
+
+We care about characteristics of the connection as presented to the remote
+host.  TLS parameters of this host (a client cert) are in-scope.
+
+The caller will already have verified that the same actual transport is used
+for both addresses.  We only need to worry about expansion variables.
+
+We delegate this to be set per-transport driver, but only implement it for
+SMTP (at time of writing).
+
+Arguments:
+  tp            the transport
+  addr1         the first address
+  addr2         the second address
+
+Returns:        TRUE or FALSE
+*/
+
+static BOOL
+same_local_identity(transport_instance *tp, address_item *addr1, address_item *addr2)
+{
+if (tp->ti_same_local_identity == NULL)
+  return TRUE;
+return tp->ti_same_local_identity(tp, addr1, addr2);
+}
+
+
+
+
 /*************************************************
 *      Record that an address is complete        *
 *************************************************/
@@ -3570,7 +3607,13 @@ for (delivery_count = 0; addr_remote != NULL; delivery_count++)
   entirely different domains. The host list pointers can be NULL in the case
   where the hosts are defined in the transport. There is also a configured
   maximum limit of addresses that can be handled at once (see comments above
-  for how it is computed). */
+  for how it is computed).
+
+  In addition, if the transport is smtp and specifies the interface option,
+  and it's an expanded string, then it must expand to the same value.
+  Similarly for any other connection characteristics.  same_local_identity()
+  will dispatch this to a per-transport-driver check (only defined for SMTP).
+  */
 
   while ((next = *anchor) != NULL && address_count < address_count_max)
     {
@@ -3586,6 +3629,8 @@ for (delivery_count = 0; addr_remote != NULL; delivery_count++)
         &&
         same_ugid(tp, next, addr)
         &&
+        same_local_identity(tp, next, addr)
+        &&
         (next->p.remove_headers == addr->p.remove_headers ||
           (next->p.remove_headers != NULL &&
            addr->p.remove_headers != NULL &&

diff --git a/src/src/globals.c b/src/src/globals.c
index b68544e54d590335f6e644623c98409695c9a3b3..c5d439306e4b4862bb1b45363b5889cff4537b9d 100644 (file)
--- a/src/src/globals.c
+++ b/src/src/globals.c
@@ -1224,6 +1224,7 @@ transport_instance  transport_defaults = {
     FALSE,                    /* overrides_hosts */
     100,                      /* max_addresses */
     500,                      /* connection_max_messages */
+    NULL,                     /* ti_same_local_identity */
     FALSE,                    /* deliver_as_creator */
     FALSE,                    /* disable_logging */
     FALSE,                    /* initgroups */

diff --git a/src/src/structs.h b/src/src/structs.h
index 9b51d0b7c113b8e257e34260c3421236a8542625..fa7468a006c687bff27c0b24316a1d156fde231e 100644 (file)
--- a/src/src/structs.h
+++ b/src/src/structs.h
@@ -149,6 +149,10 @@ typedef struct transport_instance {
   BOOL    overrides_hosts;        /* ) Used only for remote transports  */
   int     max_addresses;          /* )                                  */
   int     connection_max_messages;/* )                                  */
+  BOOL (*ti_same_local_identity)( /* ) For seeing if this transport     */
+    struct transport_instance *,  /* ) presents as the same local       */
+    struct address_item *,        /* ) identity when evaluated for both */
+    struct address_item *);       /* ) address items                    */
                                   /**************************************/
   BOOL    deliver_as_creator;     /* Used only by pipe at present */
   BOOL    disable_logging;        /* For very weird requirements */

diff --git a/src/src/transports/smtp.c b/src/src/transports/smtp.c
index 6ec94d5da45bb0a2fae2369095f6678710d73d14..1a49239d855db66dd500c3cd0023afc26cf15693 100644 (file)
--- a/src/src/transports/smtp.c
+++ b/src/src/transports/smtp.c
@@ -213,6 +213,9 @@ static uschar *smtp_command;   /* Points to last cmd for error messages */
 static uschar *mail_command;   /* Points to MAIL cmd for error messages */
 static BOOL    update_waiting; /* TRUE to update the "wait" database */
 
+static BOOL smtp_same_local_identity(  /* safety of connection reuse check */
+    struct transport_instance *, struct address_item *, struct address_item *);
+
 
 /*************************************************
 *             Setup entry point                  *
@@ -330,6 +333,13 @@ if (ob->hosts_override && ob->hosts != NULL) tblock->overrides_hosts = TRUE;
 for them, but do not do any lookups at this time. */
 
 host_build_hostlist(&(ob->fallback_hostlist), ob->fallback_hosts, FALSE);
+
+/* Set up the verifier that the local identity presented to the remote
+host is the same, to ensure multiple messages delivered down one connection
+have the same connection-level identity. */
+
+if (strcmpic(ob->protocol, US"lmtp") != 0)
+  tblock->ti_same_local_identity = smtp_same_local_identity;
 }
 
 
@@ -2210,6 +2220,100 @@ return first_addr;
 
 
 
+
+/*************************************************
+*   Check identity variants at connection level  *
+*************************************************/
+
+/* When do_remote_deliveries() is determining which messages to send over one
+connection, it's not enough to check per-address characteristics.  Attributes
+of the transport might evaluate differently (eg, "interface"); some are fine,
+they vary per-message and the connection doesn't matter (eg, DKIM signing
+identity).  But anything which affects the identity of the connection, as
+perceived by the remote host, is in scope.
+
+This check checks various connection-level options to see if they vary
+per-message.
+
+Returns:    TRUE if the transport presents the same identity
+            FALSE if the messages should be sent down different connections
+*/
+
+static BOOL
+smtp_same_local_identity(
+    struct transport_instance *tblock,
+    struct address_item *addr1,
+    struct address_item *addr2)
+{
+smtp_transport_options_block *ob =
+  (smtp_transport_options_block *)(tblock->options_block);
+uschar *if1, *if2, *helo1, *helo2;
+BOOL need_interface_check = FALSE;
+BOOL need_helo_check = FALSE;
+#ifdef SUPPORT_TLS
+uschar *tlsc1, *tlsc2;
+BOOL need_cert_check = FALSE;
+#endif
+
+if (ob->interface && Ustrchr(ob->interface, '$'))
+  need_interface_check = TRUE;
+if (ob->helo_data && Ustrchr(ob->helo_data, '$'))
+  need_helo_check = TRUE;
+#ifdef SUPPORT_TLS
+if (ob->tls_certificate && Ustrchr(ob->tls_certificate, '$'))
+  need_cert_check = TRUE;
+#endif
+
+if (!(need_interface_check || need_helo_check
+#ifdef SUPPORT_TLS
+      || need_cert_check
+#endif
+      ))
+  return TRUE;
+
+/* silence bogus compiler warnings */
+if1 = if2 = helo1 = helo2 = NULL;
+#ifdef SUPPORT_TLS
+tlsc1 = tlsc2 = NULL;
+#endif
+
+deliver_set_expansions(addr1);
+if (need_interface_check)
+  if1 = expand_string(ob->interface);
+if (need_helo_check)
+  helo1 = expand_string(ob->helo_data);
+#ifdef SUPPORT_TLS
+if (need_cert_check)
+  tlsc1 = expand_string(ob->tls_certificate);
+#endif
+
+deliver_set_expansions(addr2);
+if (need_interface_check)
+  if2 = expand_string(ob->interface);
+if (need_helo_check)
+  helo2 = expand_string(ob->helo_data);
+#ifdef SUPPORT_TLS
+if (need_cert_check)
+  tlsc2 = expand_string(ob->tls_certificate);
+#endif
+
+deliver_set_expansions(NULL);
+
+if (need_interface_check && (Ustrcmp(if1, if2) != 0))
+  return FALSE;
+if (need_helo_check && (Ustrcmp(helo1, helo2) != 0))
+  return FALSE;
+#ifdef SUPPORT_TLS
+if (need_cert_check && (Ustrcmp(tlsc1, tlsc2) != 0))
+  return FALSE;
+#endif
+
+return TRUE;
+}
+
+
+
+
 /*************************************************
 *              Main entry point                  *
 *************************************************/