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/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
 -----------------
 
 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:
 they are delivered.
 
 Arguments:

-  tp            the transort
+  tp            the transport

   addr1         the first address
   addr2         the second address
 
   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        *
 *************************************************/
 /*************************************************
 *      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
   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)
     {
 
   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_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 &&
         (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 */
     FALSE,                    /* overrides_hosts */
     100,                      /* max_addresses */
     500,                      /* connection_max_messages */

+    NULL,                     /* ti_same_local_identity */

     FALSE,                    /* deliver_as_creator */
     FALSE,                    /* disable_logging */
     FALSE,                    /* initgroups */
     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    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 */
                                   /**************************************/
   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 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                  *
 
 /*************************************************
 *             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);
 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                  *
 *************************************************/
 /*************************************************
 *              Main entry point                  *
 *************************************************/