On Wed, Aug 22, 2018 at 11:12:39AM -0700, Kevin J. McCarthy wrote: > On Wed, Aug 22, 2018 at 10:04:12AM -0500, Derek Martin wrote: > > I actually think that the MH folder code is at fault here, not the > > stat() call in safe_rename(), despite potential issues with the latter. > > If safe_rename() fails with EEXIST, what logic suggests retrying > > forever is a good idea? > > Thank you Derek. I first thought about changing the mh.c code, but the > code is actually incrementing a counter and regenerating time() as part > of the filename for each retry (take a quick look at > _maildir_commit_message()). I didn't want to put an arbitrary limit, > because Murphy's Law says eventually someone would hit it, and logically > there is no need.
What I was actually suggesting is to not call out the EEXIST error case differently from other error cases. Making that change has no other effect on the retry logic in that function. But it does eliminate the illogical retry forever if the message file already exists for some reason. > Steffen's cautions apply to dotlock code, which is a different case and > is not affected by this change. It's fundamentally the same thing though. The mechanism for dotlock works like this: - create a secure temporary file (with O_EXCL). This ensures that the file we're opening for the lock has not been subverted by another process, potentially an attacker. - stat the file - link the file to canonical name If the link succeeds, we have the lock, but the rc from link is unreliable, so... - stat the file again using the new link Here, we compare the inode and/or make sure the link count has increased, to ensure we're really dealing with the same file... - write the PID to the lock file - unlink the temporary file Exact details may vary slightly, but that's the essence of it. This is almost exactly what _maildir_commit_message() (and safe_rename()) does, for largely the same reasons, though the purpose of the file is different. > I'm inclined to keep the commit unless a regression appears or someone > can show proof the change is unsafe. So, I just read safe_rename(), and I think there are a few problems here: - The stat comparison is supposed to catch the case where the return value from link() incorrectly indicates failure, but it does not do that because it only happens when link() succeeds. That defeats the purpose of doing the check at all. - If you fixed that, your change would obviously again neuter that check. - link() can follow symlinks on some systems. But safe_rename() uses lstat() (which does not follow symlinks) to get the struct stats for original and new links. If you fix this check, it should probably use stat() for the original, and lstat() for the target. This will fix safe_rename() for the case where the OS follows symlinks, and catch the case where the new target is a hard link to the original symlink (rather than the file). The only likely scenario this would happen in real life is if someone was trying to execute a symlink attack on the user's message file, which seems a bit far-fetched, but it would be more correct. - Minor issue of comment on unlink not checking the return value, but it actually does... FWIW the version I have handy is a bit out of date so it's possible my comments aren't valid any longer, but I'd be surprised if this function had changed much. It's also very late here, and I could be misreading the code. =8^) -- 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.
pgpmptZpHogoK.pgp
Description: PGP signature
