Date: Sun, 28 Feb 1999 09:28:43 +0100
From: Thomas Roessler <roessler@GUUG.DE>
To: BUGTRAQ@netspace.org
Subject: [mutt security] tempfile race in mutt
Parts/Attachments:
   1.1 Shown    ~39 lines  Text
   1.2   OK    ~134 lines  Text
   2            475 bytes  Application
----------------------------------------

An anonymous Debian developer forwarded the following message from
debian-private to me:

> Date: Sun, 28 Feb 1999 01:14:14 +1100
> From: Hamish Moffatt <hamish@debian.org>
> To: security@debian.org
> Subject: mutt viewing html
>
> On my hamm system, when viewing text/html messages, mutt writes the
> text to /tmp/mutt.html then calls lynx on that file.
> 
> I found that if I made a symlink like
>
> /tmp/mutt.html -> /home/hamish/blah
>
> then viewing an HTML message, the file "blah" (which previously 
> did not exist) contained 4k of nulls. Mutt also deleted the symlink
> afterwards.

The behaviour Hamish describes exhibits two different problems:

- The temporary file name generator we use when interpreting
  nametemplates from the mailcap file was broken since it used
  access(2) to check for the existance of a file.  Obviously, this
  doesn't detect dangling symlinks.

- There are some attachment-handling functions which don't avoid
  race conditions by using our safe_fopen() function, but relied on
  stock libc fopen(3) instead.

The attached patch against mutt 0.95.3 is supposed to fix these
problems.  I plan to release mutt 0.95.4 tomorrow.

As a work-around, you can use a private $TMPDIR with mutt.  (This
may be a good idea in general.)

tlr
-- 
http://home.pages.de/~roessler/


Index: attach.c
===================================================================
RCS file: /home/roessler/cvsroot/mutt/attach.c,v
retrieving revision 2.1.4.4
diff -u -u -r2.1.4.4 attach.c
--- attach.c    1999/02/10 22:02:02     2.1.4.4
+++ attach.c    1999/02/28 08:06:08
@@ -707,9 +707,11 @@
       
       memset (&s, 0, sizeof (s));
       if (flags == M_SAVE_APPEND)
-       s.fpout = safe_fopen (path, "a");
-      else
+       s.fpout = fopen (path, "a");
+      else if (flags == M_SAVE_OVERWRITE)
        s.fpout = fopen (path, "w");
+      else
+       s.fpout = safe_fopen (path, "w");
       if (s.fpout == NULL)
       {
        mutt_perror ("fopen");
@@ -771,9 +773,12 @@
   s.flags = displaying ? M_DISPLAY : 0;
 
   if (flags == M_SAVE_APPEND)
-    s.fpout = safe_fopen (path, "a");
-  else
+    s.fpout = fopen (path, "a");
+  else if (flags == M_SAVE_OVERWRITE)
     s.fpout = fopen (path, "w");
+  else
+    s.fpout = safe_fopen (path, "w");
+
   if (s.fpout == NULL)
   {
     perror ("fopen");
Index: lib.c
===================================================================
RCS file: /home/roessler/cvsroot/mutt/lib.c,v
retrieving revision 2.2.4.5
diff -u -u -r2.2.4.5 lib.c
--- lib.c       1999/02/10 22:02:04     2.2.4.5
+++ lib.c       1999/02/28 08:06:08
@@ -803,8 +803,10 @@
 
       case 2: /* append */
         *append = M_SAVE_APPEND;
+        break;
       case 1: /* overwrite */
-       ;
+        *append = M_SAVE_OVERWRITE;
+        break;
     }
   }
   return 0;
Index: mutt.h
===================================================================
RCS file: /home/roessler/cvsroot/mutt/mutt.h,v
retrieving revision 2.1.4.6
diff -u -u -r2.1.4.6 mutt.h
--- mutt.h      1999/02/10 21:42:31     2.1.4.6
+++ mutt.h      1999/02/28 08:06:08
@@ -227,7 +227,8 @@
   M_NEW_SOCKET,
 
   /* Options for mutt_save_attachment */
-  M_SAVE_APPEND
+  M_SAVE_APPEND,
+  M_SAVE_OVERWRITE
 };
 
 /* possible arguments to set_quadoption() */
Index: rfc1524.c
===================================================================
RCS file: /home/roessler/cvsroot/mutt/rfc1524.c,v
retrieving revision 2.0.4.4
diff -u -u -r2.0.4.4 rfc1524.c
--- rfc1524.c   1999/02/10 22:02:06     2.0.4.4
+++ rfc1524.c   1999/02/28 08:06:09
@@ -29,11 +29,14 @@
 #include "mutt.h"
 #include "rfc1524.h"
 
-#include <ctype.h>
+#include <string.h>
 #include <stdlib.h>
-#include <unistd.h>
+#include <ctype.h>
+
+#include <sys/stat.h>
 #include <sys/wait.h>
-#include <string.h>
+#include <errno.h>
+#include <unistd.h>
 
 /* The command semantics include the following:
  * %s is the filename that contains the mail body data
@@ -445,6 +448,7 @@
   char tmp[_POSIX_PATH_MAX];
   char *period;
   size_t sl;
+  struct stat sb;
   
   strfcpy (buf, NONULL (Tempdir), sizeof (buf));
   mutt_expand_path (buf, sizeof (buf));
@@ -457,7 +461,7 @@
   {
     strfcpy (tmp, s, sizeof (tmp));
     snprintf (s, l, "%s/%s", buf, tmp);
-    if (access (s, F_OK) != 0)
+    if (lstat (s, &sb) == -1 && errno == ENOENT)
       return;
     if ((period = strrchr (tmp, '.')) != NULL)
       *period = 0;
@@ -610,6 +614,11 @@
  * This function returns 0 on successful move, 1 on old file doesn't exist,
  * 2 on new file already exists, and 3 on other failure.
  */
+
+/* note on access(2) use: No dangling symlink problems here due to
+ * safe_fopen().
+ */
+
 int mutt_rename_file (char *oldfile, char *newfile)
 {
   FILE *ofp, *nfp;