Bugzilla Bug 154310

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.

On Wed, Oct 04, 2006 at 11:19:26AM -0400, Kyle Wheeler wrote:
>     char tmpfile[POSIX_PATH_MAX] = "/tmp/muttXXXXXX";
>     char tmpfile2[POSIX_PATH_MAX];
>     char *extension = "foo";
>     // mkstemp should come first
>     fd = mkstemp(tmpfile);
>     sprintf(tmpfile2, "%s.%s", tmpfile, extension);
>     rename(tmpfile, tmpfile2);

The trouble with this is that while tmpfile is "guaranteed" unique,
tmpfile.foo may well not be.  :(  Granted, it would be hard to
exploit, but theoretically not impossible.  You still need to be
careful to open the file with O_CREAT|O_EXCL and expect that it could
fail (which I believe mutt does, though I didn't verify every possible
case).  

Unfortunately even that isn't really enough given that lots of people
set TMPDIR to something in their home directory, and have home
directories on NFS.  Opening a file this way (with O_EXCL) on NFS is
broken (and thus so are any implementations of mkstemp() family
functions that rely on it). You can do the link(2) trick too, but
AFAICT there's no way to guarantee that the file you're creating is
unique on NFS without encountering a race condition.  This is a
weakness in the NFS protocol IIRC, not in the implementation of
mkstemp(). [This may not be the case with NFSv4, I am uncertain.]
The man page for open(2) mentions this.  That's problem #1.

The scheme that mutt currently uses for most of its temp files
(_mutt_mktemp()/safe_open()) involves using the host name, UID, pid,
and an internal counter maintained by mutt.  The trouble is, it's
fairly likely that a user will go through the exact same steps when
they boot their workstation (i.e. when starting mutt immediately after
booting the system), so for any particular user the likelihood is
surprisingly high that all of these pieces (including the PID) except
for the counter will frequently be the same for any given user, thus
very guessable.  Only the counter will change, and it's always
initialized to zero, so that also is not hard to guess.  At a glance,
I don't see mutt doing anything to check to see if for such files, the
user is the only one who can read and/or write to the file.  Certainly
functions that call safe_open() use O_EXCL, but due to the problems
with that working on NFS, that may not be enough.  

Also NFS or no NFS, with regard to mutt_adv_mktemp() for attachments,
calls to safe_fopen() generally use "w+" which means the file will be
truncated if it exists, but that doesn't do anything to ensure that
the file so created couldn't be moved aside and replaced with a
malicious file should the directory be writable by someone else; nor
does it do anything to prevent the race where someone else creates the
file in between the calls to mktemp() and safe_fopen().  In the latter
case, the file will be truncated, but the file is still writable by
the owner and can potentially be modified in a malicious way before
mutt runs the MIME type's helper program on it. 

Is this exploitable?  It would seem so, though executing such a
maneuver is probably exceedingly difficult.  An attacker would need to
be able to guess the temp file name in advance, know what kind of data
was going into it, and be able to craft malicious data which would
exploit some problem in the program it was going to be fed into...

Still, mutt really ought to be creating a unique directory in which to
create its temp files, and before doing so it should make sure the
permissions on that directory are 700 and owned by the user.  It
should also make sure that the parent directory either allows access
only to the user, or that it has the sticky bit set.  There's still a
possible race condition involving creating a unique directory name,
but fortunately mkdir(2) always fails if the directory already exists,
which largely solves the problem.  Further, checking the permissions
and ownership on the directory before creating the temp file should
eliminate any remaining doubt.  


On Wed, Oct 04, 2006 at 11:36:43AM -0400, Kyle Wheeler wrote:
> On the other hand, mkstemps() would solve the problem, at least for 
> the OS's that support it, i.e. all modern Unixes (it works on Tru64 
> Unix, Solaris, FreeBSD, OpenBSD, NetBSD, MacOS X, OpenSolaris, 
> Solaris, and Linux). 

$ uname -smr
Linux 2.4.20-20.9 i686
$ man mkstemps
No manual entry for mkstemps
$ gcc mkstemps.c
/home/ddm/tmp/ccvsG8nh.o(.text+0x17): In function `main':
: undefined reference to `mkstemps'
collect2: ld returned 1 exit status

You were saying?

This function is not sufficiently portable to rely upon its presence.
At best you'd need to provide an alternate implementation, in which
case there's no obvious benefit to using it... mutt_adv_mktemp()
basically does the same thing already.

Note that mkstemps also relies on O_EXCL so it also suffers from a
race condition on NFS.

-- 
Derek D. Martin    http://www.pizzashack.org/   GPG Key ID: 0xDFBEAD02
-=-=-=-=-
This message is posted from an invalid address.  Replying to it will result in
undeliverable mail.  Sorry for the inconvenience.  Thank the spammers.

OWL has a large patch reworking tmp file handling in mutt:
http://cvsweb.openwall.com/cgi/cvsweb.cgi/Owl/packages/mutt/mutt-1.4.2.1-owl-tmp.diff

opening the bug since the mail can be found at 
<http://marc.theaimsgroup.com/?l=mutt-dev&m=115999486426292&w=2>

<http://secunia.com/advisories/22613/>

CC'ing maintainers

Ubuntu announcement can be found at
http://www.ubuntu.com/usn/usn-373-1
so there should be patches available too

Pulling in herd for advise/bumping.

Ok,

I've digged in the recent mutt commits and the only reference to temp files /
security is the attached one.

I'll apply it later today if any of you can ACK the patch.

- ferdy

Created an attachment (id=102446) [edit]
paranoid-temp-file.patch

Proposed patch from upstream.

That patch is being applied in mail-client/mutt-1.5.13-r2. If it is not enough
to fix this bug please state so and I'll try to get in touch with upstream.

- ferdy

Vorlon is this patch enough?

could someone pls verify the patch? (tavis/...?)

mutt-1.5.14 is in the tree now (with this patch included too). I haven't seen
more patches related to this bug.

Is this bug still considered valid? Do I have to do anything else?

- ferdy

Tavis please advise.

Tavis please advise.

Tavis please advise.

Tavis please advise.

>     char tmpfile[POSIX_PATH_MAX] = "/tmp/muttXXXXXX";
>     char tmpfile2[POSIX_PATH_MAX];
>     char *extension = "foo";
>     // mkstemp should come first
>     fd = mkstemp(tmpfile);
>     sprintf(tmpfile2, "%s.%s", tmpfile, extension);
>     rename(tmpfile, tmpfile2);

rename() is not vulnerable to a symlink attack, unlike open(). The race
conditions explained in the mail needs other successful attacks, or
administrator misconfigurations. Non-issue for me.


For the O_EXCL which doesn't work on NFS, that may be a weakness, but this
would affect all other apps using O_EXCL. It's not mutt specific and it can't
simply be solved. Furthermore, /tmp on a NFS... sounds... eccentric. (why not,
but in that case it is also a misconfiguration since that will affect many
applications). Non-issue for me. 

I'm an addicted mutt user and despite of that, I think we can close this bug as
Invalid.

mmm after a deeper look to the mailing list, the syscall to rename() should be
checked (return value equals 0 or -1). Indeed, if the files are:

-rw------- 1 falco   falco    0 Jun  9 23:46 muttjlNOGW
-rw-r--r-- 1 foo     foo     5 Jun  9 23:46 muttjlNOGW.foo

Then the rename() call fails, mutt continues, and read/write from/to the
crafted muttjlNOGW.foo file. Yes, that's a bug (another user could view the
mails). Not a security issue (DoS, code exec, or so).

according to the CVE refs, this affects versions 1.5.12 and earlier, and we
have version .13 stable and versions .14 and .15 unstable in the tree, so what
should we do here? Ferdy/Aggrifis/net-mail, please advise.

I just commited mutt-1.5.16. There is no point in stabling .15 so I'd go for
.16 myself once arch people deem it appropiate. We can start removing
vulnerable unused versions though.

- ferdy

Handling stable marking on bug #178003

finally closing without GLSA wrt the discussion on bug 178003, feel free to
reopen  if you disagree.