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