Content scanning: Fix locking on message spool files. Bug 2275

diff --git a/doc/doc-txt/ChangeLog b/doc/doc-txt/ChangeLog
index d99b2684ac79186ce49d508c95496bd96fc729e6..5ce54a24e8949d1603a9b15fb5fb8bdcbb9d9c87 100644 (file)
--- a/doc/doc-txt/ChangeLog
+++ b/doc/doc-txt/ChangeLog
@@ -32,6 +32,14 @@ JH/05 Bug 2273: Cutthrough delivery left a window where the received messsage
 PP/01 Refuse to open a spool data file (*-D) if it's a symlink.
       No known attacks, no CVE, this is defensive hardening.
 
 PP/01 Refuse to open a spool data file (*-D) if it's a symlink.
       No known attacks, no CVE, this is defensive hardening.
 

+JH/06 Bug 2275: The MIME ACL unlocked the received message files early, and
+      a queue-runner could start a delivery while other operations were ongoing.
+      Cutthrough delivery was a common victim, resulting in duplicate delivery.
+      Found and investigated by Tim Stewart.  Fix by using the open message data
+      file handle rather than opening another, and not locally closing it (which
+      releases a lock) for that case, while creating the temporary .eml format
+      file for the MIME ACL.  Also applies to "regex" and "spam" ACL conditions.
+

 
 Exim version 4.91
 -----------------
 
 Exim version 4.91
 -----------------

diff --git a/src/src/globals.c b/src/src/globals.c
index 7c63730aa1d40036dda8701551353a61d1f3881d..ec948151fc712a5e79c3ba338614ec25f74ea78f 100644 (file)
--- a/src/src/globals.c
+++ b/src/src/globals.c
@@ -1395,6 +1395,7 @@ uschar *spf_smtp_comment       = NULL;
 #endif
 
 BOOL    split_spool_directory  = FALSE;
 #endif
 
 BOOL    split_spool_directory  = FALSE;

+FILE   *spool_data_file               = NULL;

 uschar *spool_directory        = US SPOOL_DIRECTORY
                            "\0<--------------Space to patch spool_directory->";
 BOOL    spool_file_wireformat  = FALSE;
 uschar *spool_directory        = US SPOOL_DIRECTORY
                            "\0<--------------Space to patch spool_directory->";
 BOOL    spool_file_wireformat  = FALSE;

diff --git a/src/src/globals.h b/src/src/globals.h
index da0a09da3e7eb9e390252d0a724fd1ce0b00159f..9c7d8ccd9cf02659b69ba082c4c04f6c13f8ffa3 100644 (file)
--- a/src/src/globals.h
+++ b/src/src/globals.h
@@ -893,6 +893,7 @@ extern BOOL    spf_result_guessed;     /* spf result is of best-guess operation
 extern uschar *spf_smtp_comment;       /* spf comment to include in SMTP reply */
 #endif
 extern BOOL    split_spool_directory;  /* TRUE to use multiple subdirs */
 extern uschar *spf_smtp_comment;       /* spf comment to include in SMTP reply */
 #endif
 extern BOOL    split_spool_directory;  /* TRUE to use multiple subdirs */

+extern FILE   *spool_data_file;               /* handle for -D file */

 extern uschar *spool_directory;        /* Name of spool directory */
 extern BOOL    spool_file_wireformat;  /* current -D file has CRLF rather than NL */
 extern BOOL    spool_wireformat;       /* can write wireformat -D files */
 extern uschar *spool_directory;        /* Name of spool directory */
 extern BOOL    spool_file_wireformat;  /* current -D file has CRLF rather than NL */
 extern BOOL    spool_wireformat;       /* can write wireformat -D files */

diff --git a/src/src/receive.c b/src/src/receive.c
index b97f1d2b4f20d29e49b70ce825b58cb8ed215911..c3781420f3bef78f40f6c544b87204579b51e059 100644 (file)
--- a/src/src/receive.c
+++ b/src/src/receive.c
@@ -22,7 +22,6 @@ extern int dcc_ok;
 *                Local static variables          *
 *************************************************/
 
 *                Local static variables          *
 *************************************************/
 

-static FILE   *data_file = NULL;

 static int     data_fd = -1;
 static uschar *spool_name = US"";
 
 static int     data_fd = -1;
 static uschar *spool_name = US"";
 


@@ -343,10 +342,10 @@ if (spool_name[0] != '\0')
 
 /* Now close the file if it is open, either as a fd or a stream. */
 
 
 /* Now close the file if it is open, either as a fd or a stream. */
 

-if (data_file)
+if (spool_data_file)

   {
   {

-  (void)fclose(data_file);
-  data_file = NULL;
+  (void)fclose(spool_data_file);
+  spool_data_file = NULL;

   }
 else if (data_fd >= 0)
   {
   }
 else if (data_fd >= 0)
   {


@@ -1449,10 +1448,12 @@ if (rc == DISCARD)
   {
   recipients_count = 0;
   *blackholed_by_ptr = US"MIME ACL";
   {
   recipients_count = 0;
   *blackholed_by_ptr = US"MIME ACL";

+  cancel_cutthrough_connection(TRUE, US"mime acl discard");

   }
 else if (rc != OK)
   {
   Uunlink(spool_name);
   }
 else if (rc != OK)
   {
   Uunlink(spool_name);

+  cancel_cutthrough_connection(TRUE, US"mime acl not ok");

   unspool_mbox();
 #ifdef EXPERIMENTAL_DCC
   dcc_ok = 0;
   unspool_mbox();
 #ifdef EXPERIMENTAL_DCC
   dcc_ok = 0;


@@ -1710,7 +1711,7 @@ header names list to be the normal list. Indicate there is no data file open
 yet, initialize the size and warning count, and deal with no size limit. */
 
 message_id[0] = 0;
 yet, initialize the size and warning count, and deal with no size limit. */
 
 message_id[0] = 0;

-data_file = NULL;
+spool_data_file = NULL;

 data_fd = -1;
 spool_name = US"";
 message_size = 0;
 data_fd = -1;
 spool_name = US"";
 message_size = 0;


@@ -3050,7 +3051,7 @@ the first line of the file (containing the message ID) because otherwise there
 are problems when Exim is run under Cygwin (I'm told). See comments in
 spool_in.c, where the same locking is done. */
 
 are problems when Exim is run under Cygwin (I'm told). See comments in
 spool_in.c, where the same locking is done. */
 

-data_file = fdopen(data_fd, "w+");
+spool_data_file = fdopen(data_fd, "w+");

 lock_data.l_type = F_WRLCK;
 lock_data.l_whence = SEEK_SET;
 lock_data.l_start = 0;
 lock_data.l_type = F_WRLCK;
 lock_data.l_whence = SEEK_SET;
 lock_data.l_start = 0;


@@ -3067,12 +3068,12 @@ data line (which was read as a header but then turned out not to have the right
 format); write it (remembering that it might contain binary zeros). The result
 of fwrite() isn't inspected; instead we call ferror() below. */
 
 format); write it (remembering that it might contain binary zeros). The result
 of fwrite() isn't inspected; instead we call ferror() below. */
 

-fprintf(data_file, "%s-D\n", message_id);
+fprintf(spool_data_file, "%s-D\n", message_id);

 if (next)
   {
   uschar *s = next->text;
   int len = next->slen;
 if (next)
   {
   uschar *s = next->text;
   int len = next->slen;

-  if (fwrite(s, 1, len, data_file) == len) /* "if" for compiler quietening */
+  if (fwrite(s, 1, len, spool_data_file) == len) /* "if" for compiler quietening */

     body_linecount++;                 /* Assumes only 1 line */
   }
 
     body_linecount++;                 /* Assumes only 1 line */
   }
 


@@ -3080,19 +3081,19 @@ if (next)
 (indicated by '.'), or might have encountered an error while writing the
 message id or "next" line. */
 
 (indicated by '.'), or might have encountered an error while writing the
 message id or "next" line. */
 

-if (!ferror(data_file) && !(receive_feof)() && message_ended != END_DOT)
+if (!ferror(spool_data_file) && !(receive_feof)() && message_ended != END_DOT)

   {
   if (smtp_input)
     {
     message_ended = chunking_state <= CHUNKING_OFFERED
   {
   if (smtp_input)
     {
     message_ended = chunking_state <= CHUNKING_OFFERED

-      ? read_message_data_smtp(data_file)
+      ? read_message_data_smtp(spool_data_file)

       : spool_wireformat
       : spool_wireformat

-      ? read_message_bdat_smtp_wire(data_file)
-      : read_message_bdat_smtp(data_file);
+      ? read_message_bdat_smtp_wire(spool_data_file)
+      : read_message_bdat_smtp(spool_data_file);

     receive_linecount++;                /* The terminating "." line */
     }
   else
     receive_linecount++;                /* The terminating "." line */
     }
   else

-    message_ended = read_message_data(data_file);
+    message_ended = read_message_data(spool_data_file);

 
   receive_linecount += body_linecount;  /* For BSMTP errors mainly */
   message_linecount += body_linecount;
 
   receive_linecount += body_linecount;  /* For BSMTP errors mainly */
   message_linecount += body_linecount;


@@ -3139,10 +3140,10 @@ if (!ferror(data_file) && !(receive_feof)() && message_ended != END_DOT)
        }
       else
        {
        }
       else
        {

-       fseek(data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
+       fseek(spool_data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);

        give_local_error(ERRMESS_TOOBIG,
          string_sprintf("message too big (max=%d)", thismessage_size_limit),
        give_local_error(ERRMESS_TOOBIG,
          string_sprintf("message too big (max=%d)", thismessage_size_limit),

-         US"message rejected: ", error_rc, data_file, header_list);
+         US"message rejected: ", error_rc, spool_data_file, header_list);

        /* Does not return */
        }
       break;
        /* Does not return */
        }
       break;


@@ -3172,8 +3173,8 @@ we can then give up. Note that for SMTP input we must swallow the remainder of
 the input in cases of output errors, since the far end doesn't expect to see
 anything until the terminating dot line is sent. */
 
 the input in cases of output errors, since the far end doesn't expect to see
 anything until the terminating dot line is sent. */
 

-if (fflush(data_file) == EOF || ferror(data_file) ||
-    EXIMfsync(fileno(data_file)) < 0 || (receive_ferror)())
+if (fflush(spool_data_file) == EOF || ferror(spool_data_file) ||
+    EXIMfsync(fileno(spool_data_file)) < 0 || (receive_ferror)())

   {
   uschar *msg_errno = US strerror(errno);
   BOOL input_error = (receive_ferror)() != 0;
   {
   uschar *msg_errno = US strerror(errno);
   BOOL input_error = (receive_ferror)() != 0;


@@ -3201,8 +3202,8 @@ if (fflush(data_file) == EOF || ferror(data_file) ||
 
   else
     {
 
   else
     {

-    fseek(data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
-    give_local_error(ERRMESS_IOERR, msg, US"", error_rc, data_file,
+    fseek(spool_data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
+    give_local_error(ERRMESS_IOERR, msg, US"", error_rc, spool_data_file,

       header_list);
     /* Does not return */
     }
       header_list);
     /* Does not return */
     }


@@ -3243,7 +3244,7 @@ if (extract_recip && (bad_addresses || recipients_count == 0))
       }
     }
 
       }
     }
 

-  fseek(data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
+  fseek(spool_data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);

 
   /* If configured to send errors to the sender, but this fails, force
   a failure error code. We use a special one for no recipients so that it
 
   /* If configured to send errors to the sender, but this fails, force
   a failure error code. We use a special one for no recipients so that it


@@ -3257,7 +3258,7 @@ if (extract_recip && (bad_addresses || recipients_count == 0))
           (bad_addresses == NULL)?
             (extracted_ignored? ERRMESS_IGADDRESS : ERRMESS_NOADDRESS) :
           (recipients_list == NULL)? ERRMESS_BADNOADDRESS : ERRMESS_BADADDRESS,
           (bad_addresses == NULL)?
             (extracted_ignored? ERRMESS_IGADDRESS : ERRMESS_NOADDRESS) :
           (recipients_list == NULL)? ERRMESS_BADNOADDRESS : ERRMESS_BADADDRESS,

-          bad_addresses, header_list, data_file, FALSE))
+          bad_addresses, header_list, spool_data_file, FALSE))

       error_rc = (bad_addresses == NULL)? EXIT_NORECIPIENTS : EXIT_FAILURE;
     }
   else
       error_rc = (bad_addresses == NULL)? EXIT_NORECIPIENTS : EXIT_FAILURE;
     }
   else


@@ -3280,7 +3281,7 @@ if (extract_recip && (bad_addresses || recipients_count == 0))
   if (recipients_count == 0 || error_handling == ERRORS_STDERR)
     {
     Uunlink(spool_name);
   if (recipients_count == 0 || error_handling == ERRORS_STDERR)
     {
     Uunlink(spool_name);

-    (void)fclose(data_file);
+    (void)fclose(spool_data_file);

     exim_exit(error_rc, US"receiving");
     }
   }
     exim_exit(error_rc, US"receiving");
     }
   }


@@ -3603,9 +3604,9 @@ else
           /* Does not return */
         else
           {
           /* Does not return */
         else
           {

-          fseek(data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
+          fseek(spool_data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);

           give_local_error(ERRMESS_LOCAL_ACL, user_msg,
           give_local_error(ERRMESS_LOCAL_ACL, user_msg,

-            US"message rejected by non-SMTP ACL: ", error_rc, data_file,
+            US"message rejected by non-SMTP ACL: ", error_rc, spool_data_file,

               header_list);
           /* Does not return */
           }
               header_list);
           /* Does not return */
           }


@@ -3802,9 +3803,9 @@ else
     }
   else
     {
     }
   else
     {

-    fseek(data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
+    fseek(spool_data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);

     give_local_error(ERRMESS_LOCAL_SCAN, errmsg,
     give_local_error(ERRMESS_LOCAL_SCAN, errmsg,

-      US"message rejected by local scan code: ", error_rc, data_file,
+      US"message rejected by local scan code: ", error_rc, spool_data_file,

         header_list);
     /* Does not return */
     }
         header_list);
     /* Does not return */
     }


@@ -3878,8 +3879,8 @@ else
       }
     else
       {
       }
     else
       {

-      fseek(data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
-      give_local_error(ERRMESS_IOERR, errmsg, US"", error_rc, data_file,
+      fseek(spool_data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
+      give_local_error(ERRMESS_IOERR, errmsg, US"", error_rc, spool_data_file,

         header_list);
       /* Does not return */
       }
         header_list);
       /* Does not return */
       }


@@ -3905,7 +3906,7 @@ that is in the file, but we do add one extra for the notional blank line that
 precedes the data. This total differs from message_size in that it include the
 added Received: header and any other headers that got created locally. */
 
 precedes the data. This total differs from message_size in that it include the
 added Received: header and any other headers that got created locally. */
 

-if (fflush(data_file))
+if (fflush(spool_data_file))

   {
   errmsg = string_sprintf("Spool write error: %s", strerror(errno));
   log_write(0, LOG_MAIN, "%s\n", errmsg);
   {
   errmsg = string_sprintf("Spool write error: %s", strerror(errno));
   log_write(0, LOG_MAIN, "%s\n", errmsg);


@@ -3919,8 +3920,8 @@ if (fflush(data_file))
     }
   else
     {
     }
   else
     {

-    fseek(data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
-    give_local_error(ERRMESS_IOERR, errmsg, US"", error_rc, data_file,
+    fseek(spool_data_file, (long int)SPOOL_DATA_START_OFFSET, SEEK_SET);
+    give_local_error(ERRMESS_IOERR, errmsg, US"", error_rc, spool_data_file,

       header_list);
     /* Does not return */
     }
       header_list);
     /* Does not return */
     }


@@ -4259,10 +4260,13 @@ then we can think about properly declaring the message not-received. */
 
 TIDYUP:
 process_info[process_info_len] = 0;                    /* Remove message id */
 
 TIDYUP:
 process_info[process_info_len] = 0;                    /* Remove message id */

-if (data_file && cutthrough_done == NOT_TRIED)
-  if (fclose(data_file))                               /* Frees the lock */
+if (spool_data_file && cutthrough_done == NOT_TRIED)
+  {
+  if (fclose(spool_data_file))                         /* Frees the lock */

     log_write(0, LOG_MAIN|LOG_PANIC,
       "spoolfile error on close: %s", strerror(errno));
     log_write(0, LOG_MAIN|LOG_PANIC,
       "spoolfile error on close: %s", strerror(errno));

+  spool_data_file = NULL;
+  }

 
 /* Now reset signal handlers to their defaults */
 
 
 /* Now reset signal handlers to their defaults */
 


@@ -4349,8 +4353,11 @@ if (smtp_input)
       }
     if (cutthrough_done != NOT_TRIED)
       {
       }
     if (cutthrough_done != NOT_TRIED)
       {

-      if (data_file)
-       (void) fclose(data_file);  /* Frees the lock; do not care if error */
+      if (spool_data_file)
+       {
+       (void) fclose(spool_data_file);  /* Frees the lock; do not care if error */
+       spool_data_file = NULL;
+       }

       message_id[0] = 0;         /* Prevent a delivery from starting */
       cutthrough.delivery = cutthrough.callout_hold_only = FALSE;
       cutthrough.defer_pass = FALSE;
       message_id[0] = 0;         /* Prevent a delivery from starting */
       cutthrough.delivery = cutthrough.callout_hold_only = FALSE;
       cutthrough.defer_pass = FALSE;

diff --git a/src/src/spool_mbox.c b/src/src/spool_mbox.c
index 749484f2b0974bd891dffaf847033731de4f7b4d..05f90a819c57fbd520f312706bf7236ec5f08885 100644 (file)
--- a/src/src/spool_mbox.c
+++ b/src/src/spool_mbox.c
@@ -35,9 +35,7 @@ uschar message_subdir[2];
 uschar buffer[16384];
 uschar *temp_string;
 uschar *mbox_path;
 uschar buffer[16384];
 uschar *temp_string;
 uschar *mbox_path;

-FILE *mbox_file = NULL;
-FILE *data_file = NULL;
-FILE *yield = NULL;
+FILE *mbox_file = NULL, *l_data_file = NULL, *yield = NULL;

 header_line *my_headerlist;
 struct stat statbuf;
 int i, j;
 header_line *my_headerlist;
 struct stat statbuf;
 int i, j;


@@ -108,21 +106,25 @@ if (!spool_mbox_ok)
     goto OUT;
     }
 
     goto OUT;
     }
 

-  /* copy body file */
-  if (!source_file_override)
+  /* Copy body file.  If the main receive still has it open then it is holding
+  a lock, and we must not close it (which releases the lock), so just use the
+  global file handle. */
+  if (source_file_override)
+    l_data_file = Ufopen(source_file_override, "rb");
+  else if (spool_data_file)
+    l_data_file = spool_data_file;
+  else

     {
     message_subdir[1] = '\0';
     for (i = 0; i < 2; i++)
       {
       message_subdir[0] = split_spool_directory == (i == 0) ? message_id[5] : 0;
       temp_string = spool_fname(US"input", message_subdir, message_id, US"-D");
     {
     message_subdir[1] = '\0';
     for (i = 0; i < 2; i++)
       {
       message_subdir[0] = split_spool_directory == (i == 0) ? message_id[5] : 0;
       temp_string = spool_fname(US"input", message_subdir, message_id, US"-D");

-      if ((data_file = Ufopen(temp_string, "rb"))) break;
+      if ((l_data_file = Ufopen(temp_string, "rb"))) break;

       }
     }
       }
     }

-  else
-    data_file = Ufopen(source_file_override, "rb");

 
 

-  if (!data_file)
+  if (!l_data_file)

     {
     log_write(0, LOG_MAIN|LOG_PANIC, "Could not open datafile for message %s",
       message_id);
     {
     log_write(0, LOG_MAIN|LOG_PANIC, "Could not open datafile for message %s",
       message_id);


@@ -131,7 +133,7 @@ if (!spool_mbox_ok)
 
   /* The code used to use this line, but it doesn't work in Cygwin.
 
 
   /* The code used to use this line, but it doesn't work in Cygwin.
 

-      (void)fread(data_buffer, 1, 18, data_file);
+      (void)fread(data_buffer, 1, 18, l_data_file);

 
      What's happening is that spool_mbox used to use an fread to jump over the
      file header. That fails under Cygwin because the header is locked, but
 
      What's happening is that spool_mbox used to use an fread to jump over the
      file header. That fails under Cygwin because the header is locked, but


@@ -139,23 +141,23 @@ if (!spool_mbox_ok)
      explicitly, because the one in the file is parted of the locked area.  */
 
   if (!source_file_override)
      explicitly, because the one in the file is parted of the locked area.  */
 
   if (!source_file_override)

-    (void)fseek(data_file, SPOOL_DATA_START_OFFSET, SEEK_SET);
+    (void)fseek(l_data_file, SPOOL_DATA_START_OFFSET, SEEK_SET);

 
   do
     {
     uschar * s;
 
     if (!spool_file_wireformat || source_file_override)
 
   do
     {
     uschar * s;
 
     if (!spool_file_wireformat || source_file_override)

-      j = fread(buffer, 1, sizeof(buffer), data_file);
+      j = fread(buffer, 1, sizeof(buffer), l_data_file);

     else                                               /* needs CRLF -> NL */
     else                                               /* needs CRLF -> NL */

-      if ((s = US fgets(CS buffer, sizeof(buffer), data_file)))
+      if ((s = US fgets(CS buffer, sizeof(buffer), l_data_file)))

        {
        uschar * p = s + Ustrlen(s) - 1;
 
        if (*p == '\n' && p[-1] == '\r')
          *--p = '\n';
        else if (*p == '\r')
        {
        uschar * p = s + Ustrlen(s) - 1;
 
        if (*p == '\n' && p[-1] == '\r')
          *--p = '\n';
        else if (*p == '\r')

-         ungetc(*p--, data_file);
+         ungetc(*p--, l_data_file);

 
        j = p - buffer;
        }
 
        j = p - buffer;
        }


@@ -190,7 +192,7 @@ else
   *mbox_file_size = statbuf.st_size;
 
 OUT:
   *mbox_file_size = statbuf.st_size;
 
 OUT:

-if (data_file) (void)fclose(data_file);
+if (l_data_file && !spool_data_file) (void)fclose(l_data_file);

 if (mbox_file) (void)fclose(mbox_file);
 store_reset(reset_point);
 return yield;
 if (mbox_file) (void)fclose(mbox_file);
 store_reset(reset_point);
 return yield;