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

Manoj Gupta via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jan 6 18:26:37 PST 2021


manojgupta added a comment.

In D93881#2483542 <https://reviews.llvm.org/D93881#2483542>, @MaskRay wrote:

>> 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.

While I can understand the hesitation, we have a real need for this. And IIUC, even the trunk GNU tools has the existing old behavior, though they may have changed the internal implementation.

> (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.

I think I already provided examples of usage in Gentoo and Chrome OS that llvm objcopy breaks. This does not seem to be a case of minor use case to me.

>> 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.

Please see the query links below for a more accurate count.

> Could you just update the build instructions for these few packages?

I see over 600 uses of of fowners in Gentoo and over 55 in Chrome OS (public). Not all uses are related to binary files but asking to fix the build system instead of fixing the incompatibility is a non-trivial and a no-go.

[1] https://github.com/gentoo/gentoo/search?q=fowners&type=code
[2] https://source.chromium.org/search?q=fowners&sq=&ss=chromiumos

> 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.

I am simply trying to point out the normal behavior of tools when modifying existing files by various. The internal implementation obviously will vary between tools. From user point of view, it does not matter if the tool does a copy/rename or in place modification to do so.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93881



More information about the llvm-commits mailing list