safe_rename() and verifying the result of link(2)

[Derek Martin]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mutt.org/pipermail/mutt-dev/attachments/20180823/36165e41/attachment.asc>