From 9d38e6c3845871c8774cd6494163b1625235080a Mon Sep 17 00:00:00 2001 From: Phil Pennock Date: Sat, 12 May 2012 08:27:49 -0400 Subject: [PATCH 1/1] 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. --- doc/doc-txt/ChangeLog | 5 ++ src/src/deliver.c | 49 +++++++++++++++++- src/src/globals.c | 1 + src/src/structs.h | 4 ++ src/src/transports/smtp.c | 104 ++++++++++++++++++++++++++++++++++++++ 5 files changed, 161 insertions(+), 2 deletions(-) diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog index de7c17878..5111d565a 100644 --- 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 10b63397e..579c3376d 100644 --- 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 b68544e54..c5d439306 100644 --- 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 9b51d0b7c..fa7468a00 100644 --- 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 6ec94d5da..1a49239d8 100644 --- 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 * *************************************************/ -- 2.30.2