On 2006-10-04 18:16:19 -0400, Derek Martin wrote: > On Wed, Oct 04, 2006 at 10:58:40PM +0200, Thomas Roessler wrote: > > On 2006-10-04 16:39:29 -0400, Derek Martin wrote: > > > > > It would seem that Mutt's temp file creation mechanisms all > > > suffer from a potentially exploitable race condition. Actually > > > there are two: one in functions that call safe_open(), which only > > > affects users creating temp files on NFS file systems (due to the > > > O_EXCL problem), and one in functions that make use of > > > safe_fopen(), because the resulting file is not adequately > > > checked to determine if other users can modify it before it is > > > written to. > > > > Well, umask (077) is one of the first things mutt does, so the > > second problem looks like a non-issue to me. (I.e., other users > > can't modify the temporary file unless the owner deliberately > > changed the permissions. Oh well.) > > Not so. An attacker could create the file first, in between the > call to mktemp() and fopen(). As I said in my last message, the > call to fopen(blah, "w+") will truncate the file if it exists, > but it does nothing to ensure the modes of any existing file, nor > will your umask. Where do you see the fopen (blah, "w+")? A safe_fopen (blah, "w+") dispatches to an safe_open with O_EXCL. So, if at all, problem 1 can occur in the NFS context when O_EXCL is broken. O_EXCL is the crucial one here... >> I'll admit that my memory of NFS subtleties is getting rusty at >> this point. Mind to elaborate on what safe_open ought to do >> besides the check of comparing fstat and lstat results? > The scheme I outlined in the previous message of creating a > temporary directory with permissions 700 and creating temp files > in there, checking permissions before trusting temp files, is > suitable to fix both problems. Best of all, using that solution > makes temp file creation consistent for all uses of temp files, > which currently is not the case. I'll note that mutt should be > careful to remove any such temp directory upon exit or receiving > a signal which would normally terminate it. Agree for temporary files. > Anywhere mutt needs to do this for files which are intended to > hang around, e.g. new mbox folders, you can do that and then > link(2) it to the temp file. I'm trying to parse what you are suggesting here: Do you mean that mutt should create all new files in the temporary directory and then use a hard link to "move" (abuse of language, sorry) them to the right place? Sounds like a nice idea in theory, but it will fail as soon as the temporary directory is on a file system different from the target directory. We could go to the length of creating a new temporary directory around every new file that we create, and then do the linking dance, but that sounds a bit like overkill. Then again... That could actually localize the change to safe_open, no? Patch attached; comments very welcome. This one does keep the failure mode of somebody putting a file in place and mutt not succeeding to create a temporary file. At the same time, it takes care of the failure mode of somebody attacking an NFS-using mutt and corrupting folders, or wreaking other havoc on the data that mutt processes. Not yet committed. -- Thomas Roessler <roessler@xxxxxxxxxxxxxxxxxx>
Index: lib.c
===================================================================
RCS file: /cvs/mutt/mutt/lib.c,v
retrieving revision 3.20
diff -u -r3.20 lib.c
--- lib.c 18 May 2006 17:35:29 -0000 3.20
+++ lib.c 5 Oct 2006 08:20:38 -0000
@@ -481,14 +481,85 @@
return 0;
}
+/* Create a temporary directory next to a file name */
+
+int mutt_mkwrapdir (const char *path, char *newfile, size_t nflen,
+ char *newdir, size_t ndlen)
+{
+ const char *basename;
+ char parent[_POSIX_PATH_MAX];
+ char *p;
+ int rv;
+
+ strfcpy (parent, NONULL (path), sizeof (parent));
+
+ if ((p = strrchr (parent, '/')))
+ {
+ *p = '\0';
+ basename = p + 1;
+ }
+ else
+ {
+ strfcpy (parent, ".", sizeof (parent));
+ basename = path;
+ }
+
+ do
+ {
+ snprintf (newdir, ndlen, "%s/%s", parent, ".muttXXXXXX");
+ mktemp (newdir);
+ }
+ while ((rv = mkdir (newdir, 0700)) == -1 && errno == EEXIST);
+
+ if (rv == -1)
+ return -1;
+
+ snprintf (newfile, nflen, "%s/%s", newdir, NONULL(basename));
+ return 0;
+}
+
+int mutt_put_file_in_place (const char *path, const char *safe_file, const
char *safe_dir)
+{
+ int rv;
+
+ rv = safe_rename (safe_file, path);
+ unlink (safe_file);
+ rmdir (safe_dir);
+ return rv;
+}
+
int safe_open (const char *path, int flags)
{
struct stat osb, nsb;
int fd;
- if ((fd = open (path, flags, 0600)) < 0)
- return fd;
+ if (flags & O_EXCL)
+ {
+ char safe_file[_POSIX_PATH_MAX];
+ char safe_dir[_POSIX_PATH_MAX];
+ if (mutt_mkwrapdir (path, safe_file, sizeof (safe_file),
+ safe_dir, sizeof (safe_dir)) == -1)
+ return -1;
+
+ if ((fd = open (safe_file, flags, 0600)) < 0)
+ {
+ rmdir (safe_dir);
+ return fd;
+ }
+
+ if (mutt_put_file_in_place (path, safe_file, safe_dir) == -1)
+ {
+ close (fd);
+ return -1;
+ }
+ }
+ else
+ {
+ if ((fd = open (path, flags, 0600)) < 0)
+ return fd;
+ }
+
/* make sure the file is not symlink */
if (lstat (path, &osb) < 0 || fstat (fd, &nsb) < 0 ||
compare_stat(&osb, &nsb) == -1)
Attachment:
pgpK1FK8O26Xi.pgp
Description: PGP signature