<<< Date Index >>>     <<< Thread Index >>>

Re: [patch] Re: security problem with temp files [was Re: mutt_adv_mktemp() ?]



On Thu, Oct 05, 2006 at 10:25:49AM +0200, Thomas Roessler wrote:
> Where do you see the fopen (blah, "w+")?
> 
> A safe_fopen (blah, "w+") dispatches to an safe_open with O_EXCL.

Oops, yeah...  I didn't read the code for safe_fopen(); just made
assumptions.  My bad.

> So, if at all, problem 1 can occur in the NFS context when O_EXCL is
> broken.  O_EXCL is the crucial one here...

Yes.

> > 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.
> 
> 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.

If you make the directory in the same place you were going to make the
original file, it will always work.

> Then again...  That could actually localize the change to safe_open,
> no?

It seems so.  

The patch appears to be sufficient, though one could argue that
mutt_mkwrapdir should check to make sure the permissions on parent
preclude the possibility that your new, safe temporary directory could
be moved out of the way and recreated by a malicious user, and then
after opening the file verify the permissions on your temp dir are
still what you thought they were.  The flip side of the coin is at
some point, you have to stop trying to protect the user from himself
in order to get useful work done; if people don't take the time to
understand basic file permissions, they probably deserve what they
get.

This kind of decision is one that I sometimes wrestle with in my own
code.  Most users understand basic file permissions in theory, but
most newer users who are not particularly security-minded tend not to
really grasp the ramifications until a permissions problem bites them
in the a$$.  Actually thinking about it more, I'm inclined to think
that mutt should at least verify the permissions and ownership on the
temporary directory AFTER the new file has been opened (and before the
call to link()).  If they're not 0700 and owned by the user, something
bad is definitely going on.

I like that you used a hidden directory for this; I was going to
suggest it if you hadn't.

I will also note that both unlink() and rmdir() can fail in
mutt_put_file_in_place.  The call to unlink fails trivially (i.e. in
the case of hardware failure, generally), but the call to rmdir fails
less trivially.  In theory, no other file should be created in that
directory, since mutt has created it for its own, one-time-only use
for a specific temporary file.  If the rmdir fails, it could signal
something fishy is going on...  It might be worth notifying the user
about that.  If nothing else it gives them a chance to manually clean
up the mess that mutt wasn't able to clean up.

This turned out to be a lot easier to fix than I'd first thought; I'm
suddenly curious about Solar Designer's 46kB patch.  He's
unquestionably better than me with code security; but he's also more
paranoid, sometimes needlessly so I think.

-- 
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.

Attachment: pgp0S2Pf7xigZ.pgp
Description: PGP signature