[Bug binutils/26945] Unsafe chown+chmod in smart_rename, possibly elsewhere

[bugdal at aerifal dot cx]

https://sourceware.org/bugzilla/show_bug.cgi?id=26945

--- Comment #13 from Rich Felker <bugdal at aerifal dot cx> ---
This patch keeps expanding way beyond the scope of what I can commit to review,
and I don't think it's nearing something that fixes the problem. It's not clear
to me if bfd_stat uses fstat on the open file (safe) or stat on the name
(unsafe) and I could go look this up, but this is ballooning into something
much larger than I can take on and after I look that up there will be something
else and something else. You really need someone who understands both this code
and temp file/writable-dir TOCTOU race vulnerabilities to work with you on the
fix. Short of that, the only thing I can say with confidence is that the
chmod/chown behavior should be removed entirely (and even then I'm not sure
it'd be safe to run in directories other users can write to, but at least it
wouldn't explicitly be offering a feature meant only for that use case and
encouraging users to use it that way).

On major bug I will comment on individually -- in smart_rename you added a
line:

  to_fd = open (to, O_WRONLY | O_TRUNC | O_BINARY, 0777);

I don't understand the motivation for that at all, but it's creating a
*completely new vulnerability* of the type we're discussing here. (And even
when it's not being maliciously exploited, it will cause data loss whenever the
file is a hardlink!)

It looks like you're trying to avoid calling stat on the target because I told
you that's unsafe, but instead you're truncating the target, which might be a
symlink or hardlink to any file on the whole filesystem, *then* checking, after
truncating it, whether it has any hardlinks. Here you really do want to stick
with lstat. You're not using the result to copy permissions, just to see if the
target you're about to replace would be a hard link or symlink, in which case I
think the idea is to overwrite the existing file rather than replacing it so
the linked status is preserved (at the cost of atomicity). There inherently
will be races where you do "the wrong thing" here, and opening the file first
doesn't do anything to avoid that. But they should not be "exploitable" for
anything beyond annoyance.

-- 
You are receiving this mail because:
You are on the CC list for the bug.