[PATCH] D93881: [llvm-objcopy] preserve file ownership when overwritten

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

> GNU objcopy and strip preserve the ownership of a file when it's overwritten, e.g. while "sudo objcopy foo -o bar" creates a new file bar with root access, "sudo objcopy foo" will keep the owner and group of foo unchanged. Currently llvm-objcopy and llvm-strip behave differently, always changing the ownership to root.

`objcopy foo -o bar` => `objcopy foo bar`

I am unsure that we want to adopt the GNU behavior because

(1) there were security vulnerabilities
(2) the code is subtle and untestable
(3) the behavior is self inconsistent. The behaviors of `sudo objcopy foo` and `sudo -u mail objcopy foo` (if user `mail` has write permission to the directory) are different.
(4) I believe very few packages require the behavior. It can only be used by root (CAP_CHOWN), so it is a very minor use case.

> The discrepancy prevents Chrome OS from migrating to llvm-objcopy and llvm-strip as Portage (the build system) builds binaries following recommended steps documented at the man page of GNU strip as follows,

Not having a behavior has pros and cons and I understand that it may sometimes cause friction.
But from https://chromium-review.googlesource.com/q/gnu+strip , it seems that very few crbug.com bugs require the GNU fchown (was: chown) behavior.
Could you just update the build instructions for these few packages?

In D93881#2483249 <https://reviews.llvm.org/D93881#2483249>, @manojgupta wrote:

> In D93881#2483184 <https://reviews.llvm.org/D93881#2483184>, @MaskRay wrote:
>
>> I don't think we want to introduce a filename based chmod API. Unless it is essential (very unlikely), it is easy for developers to make time-of-check time-of-use (TOCTOU) race condition allowing bypass of protection mechanism due to symlink attacks.
>>
>> binutils 2.36 will fix the issue https://sourceware.org/bugzilla/show_bug.cgi?id=26945
>
> Can you please clarify what is being fixed, the use of internal renaming APIs or the current behavior?

GNU objcopy before 2.36 had the symlink attach vulnerability. The patch would introduce the similar vulnerability to llvm-objcopy.

>> Regarding whether llvm-objcopy should adopt the GNU objcopy `smart_rename` behavior, I am a bit hesitant, somewhat inclining to no.
>>
>> It seems that very few packages actually require the behavior - most packages should use something like `install -o ... -g ... -m ... ` to copy files, the owner/group is not too useful.
>
> We are hitting this problem in Portage (used in Gentoo Linux and Chrome OS) which is both a build system and a package manager. One of the steps after building a package is to install the files with the required permissions. And another action is to strip the files and add the debuginfo link etc. These steps execute as root privilege in portage as they modify the file system and we do not have control over it.
>
> Because of the deviation from GNU objcopy/strip, stripping or just adding debuginfo link causes the "security sensitive" binaries to lose the intended permissions. We already had to revert twice switching to llvm objcopy/strip for this reason in Chrome OS before we figured out the problem. Therefore, I'd argue that this change is needed for GNU compatibility.
> It is also what most tools already do when dealing with existing files e.g. "sudo vi foo.c" or "echo foo |& sudo tee a.c" does not change file ownership or group.

It's different. `vi foo.c` and `tee a.c` edits the file in place while strip/llvm-strip creates a temporary file and renames it to the original name.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93881/new/

https://reviews.llvm.org/D93881