Fix heavy-pipeline SMTP command input corruption. Bug 2250

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index 6fb4402030d3da27e26091e5413dc094793c3fd8..eb0e1a346f21f0e3c3604b0d6ba4265eb45129e8 100644 (file)
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -149,6 +149,17 @@ JH/28 Ensure that variables possibly set during message acceptance are marked
       message files.  Do the same for the SMTP per-message loop, for certain
       variables indirectly set in ACL operations.
 
+JH/29 Bug 2250: Fix a longstanding bug in heavily-pipelined SMTP input (such
+      as a multi-recipient message from a mailinglist manager).  The coding had
+      an arbitrary cutoff number of characters while checking for more input;
+      enforced by writing a NUL into the buffer.  This corrupted long / fast
+      input.   The problem was exposed more widely when more pipelineing of SMTP
+      responses was introduced, and one Exim system was feeding another.
+      The symptom is log complaints of SMTP syntax error (NUL chars) on the
+      receiving system, and refused recipients seen by the sending system
+      (propating to people being dropped from mailing lists).
+      Discovered and pinpointed by David Carter.
+
 
 Exim version 4.90
 -----------------

diff --git a/src/src/smtp_in.c b/src/src/smtp_in.c
index c9fe792201a3e541424992d278184fa84b6cce66..2d213305ece036b724da3c186a00327e6ce7c961 100644 (file)
--- a/src/src/smtp_in.c
+++ b/src/src/smtp_in.c
@@ -359,16 +359,13 @@ rc = smtp_getc(GETC_BUFFER_UNLIMITED);
 if (rc < 0) return TRUE;      /* End of file or error */
 
 smtp_ungetc(rc);
-rc = smtp_inend - smtp_inptr;
-if (rc > 150) rc = 150;
-smtp_inptr[rc] = 0;
 return FALSE;
 }
 
 static BOOL
 check_sync(void)
 {
-if (!smtp_enforce_sync || sender_host_address == NULL || sender_host_notsocket)
+if (!smtp_enforce_sync || !sender_host_address || sender_host_notsocket)
   return TRUE;
 
 return wouldblock_reading();
@@ -439,9 +436,10 @@ if (!smtp_out) return FALSE;
 fflush(smtp_out);
 if (smtp_receive_timeout > 0) alarm(smtp_receive_timeout);
 
-/* Limit amount read, so non-message data is not fed to DKIM */
+/* Limit amount read, so non-message data is not fed to DKIM.
+Take care to not touch the safety NUL at the end of the buffer. */
 
-rc = read(fileno(smtp_in), smtp_inbuffer, MIN(IN_BUFFER_SIZE, lim));
+rc = read(fileno(smtp_in), smtp_inbuffer, MIN(IN_BUFFER_SIZE-1, lim));
 save_errno = errno;
 alarm(0);
 if (rc <= 0)
@@ -1614,11 +1612,11 @@ if (proxy_session && proxy_session_failed)
 
 /* Enforce synchronization for unknown commands */
 
-if (smtp_inptr < smtp_inend &&                     /* Outstanding input */
-    check_sync &&                                  /* Local flag set */
-    smtp_enforce_sync &&                           /* Global flag set */
-    sender_host_address != NULL &&                 /* Not local input */
-    !sender_host_notsocket)                        /* Really is a socket */
+if (  smtp_inptr < smtp_inend          /* Outstanding input */
+   && check_sync                       /* Local flag set */
+   && smtp_enforce_sync                        /* Global flag set */
+   && sender_host_address              /* Not local input */
+   && !sender_host_notsocket)          /* Really is a socket */
   return BADSYN_CMD;
 
 return OTHER_CMD;
@@ -2419,10 +2417,12 @@ else
     (sender_host_address ? protocols : protocols_local) [pnormal];
 
 /* Set up the buffer for inputting using direct read() calls, and arrange to
-call the local functions instead of the standard C ones. */
+call the local functions instead of the standard C ones.  Place a NUL at the
+end of the buffer to safety-stop C-string reads from it. */
 
 if (!(smtp_inbuffer = US malloc(IN_BUFFER_SIZE)))
   log_write(0, LOG_MAIN|LOG_PANIC_DIE, "malloc() failed for SMTP input buffer");
+smtp_inbuffer[IN_BUFFER_SIZE-1] = '\0';
 
 receive_getc = smtp_getc;
 receive_getbuf = smtp_getbuf;
@@ -5669,8 +5669,8 @@ while (done <= 0)
 
     case BADCHAR_CMD:
     done = synprot_error(L_smtp_syntax_error, 0, NULL,       /* Just logs */
-      US"NULL character(s) present (shown as '?')");
-    smtp_printf("501 NULL characters are not allowed in SMTP commands\r\n", FALSE);
+      US"NUL character(s) present (shown as '?')");
+    smtp_printf("501 NUL characters are not allowed in SMTP commands\r\n", FALSE);
     break;
 
 
@@ -5679,7 +5679,7 @@ while (done <= 0)
     if (smtp_inend >= smtp_inbuffer + IN_BUFFER_SIZE)
       smtp_inend = smtp_inbuffer + IN_BUFFER_SIZE - 1;
     c = smtp_inend - smtp_inptr;
-    if (c > 150) c = 150;
+    if (c > 150) c = 150;      /* limit logged amount */
     smtp_inptr[c] = 0;
     incomplete_transaction_log(US"sync failure");
     log_write(0, LOG_MAIN|LOG_REJECT, "SMTP protocol synchronization error "

diff --git a/test/stderr/0435 b/test/stderr/0435
index 1ec601657b17604621da221665c1cfb8f1bf47b6..ad55c7e8f63ccad07230f82d37011719c729575b 100644 (file)
--- a/test/stderr/0435
+++ b/test/stderr/0435
@@ -15,8 +15,8 @@ SMTP>> 220 myhost.test.ex ESMTP Exim x.yz Tue, 2 Mar 1999 09:44:33 +0000
 smtp_setup_msg entered
 SMTP<< HELO    ?
 LOG: smtp_syntax_error MAIN
-  SMTP syntax error in "HELO    ?" U=CALLER NULL character(s) present (shown as '?')
-SMTP>> 501 NULL characters are not allowed in SMTP commands
+  SMTP syntax error in "HELO    ?" U=CALLER NUL character(s) present (shown as '?')
+SMTP>> 501 NUL characters are not allowed in SMTP commands
 SMTP<< quit
 SMTP>> 221 myhost.test.ex closing connection
 LOG: smtp_connection MAIN

diff --git a/test/stdout/0435 b/test/stdout/0435
index 2b974e2cf5d2f02474df8c312b2e1b6b5d6b49d4..c86a5083756e06cd7ab2c04ae24ef3ca6226fdbe 100644 (file)
--- a/test/stdout/0435
+++ b/test/stdout/0435
@@ -1,3 +1,3 @@
 220 myhost.test.ex ESMTP Exim x.yz Tue, 2 Mar 1999 09:44:33 +0000\r
-501 NULL characters are not allowed in SMTP commands\r
+501 NUL characters are not allowed in SMTP commands\r
 221 myhost.test.ex closing connection\r