> Yep, it does what's on the tin and people plain used it wrong, > decades of wrong, bad wrong. So it got deprecated and turned into a > warn to wake people up :)
The problem with deprecating mktemp() it is that it serves a perfectly valid and useful function, which in and of itself is not insecure. Mutt is using it in exactly that way. I'm not saying there are NO weaknesses in safe_open()... or in code that calls it; but this ISN'T one of them. > Given O_EXCL (if it works, and checking the result) is what actually > neuters the race, then the X's exist not for security but give: That's false. O_EXCL is not the end of the story. Using a randomly generated name IS IMPORTANT, because it makes it harder for an attacker to effect a collision. If the attacker can either predict the filename, or can effectively create files that exhaust the set of possible file names, then the attacker can DoS your application. It's important that the universe of possible names be large, to make it as hard as possible for such an attack to be effected. Using a generator with poor randomness, or with a small universe, is BAD. Now, at a quick glance, it doesn't appear that Mutt tries to deal with this, i.e. safe_open() does not detect failures and retry, but it probably should make an attempt. But I didn't look at the calling code, it may be handled there (though I think it shouldn't be). One possible way to address this would be, at start-up, create a randomly generated temporary directory which persists for the life of Mutt, which has only write perms for the owner, and write all your temp files there. Mut does something like this... but not quite. The advantage here is you can try to do this before Mutt tries to do anything "important" with temp files, in a loop until TMP_MAX, and then if you fail, you can be absolutely certain that something bad is happening that requires your investigation. Under such conditions, mutt should complain loudly and exit. But, once the directory is created, you never need to worry about the collision problem again (except within your own program), for the life of your program. Ideally, the library function would register a call with atexit() to make sure this directory gets cleaned up upon exit of the program, though the user (the programmer) should explicitly do this when it makes sense to do so. You should absolutely not roll your own random temp file generator, unless you're prepared to have a bona fide security expert review it. Doing this purely to avoid a bogus warning is a horrible, horrible idea. You should just put this idea out of your mind. > I think I saw a stat() coded, then later on, the O_EXCL, which There is an lstat(), which exists to establish that the target is not a symlink. That is also an important part of the process, and serves to foil a different class of attacks than a simple race. It is remarkably, astonishingly hard to create temp files safely. -- 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 due to spam prevention. Sorry for the inconvenience.
pgpC1fLOoj0vL.pgp
Description: PGP signature
