FS#69584 - [binutils][makepkg] strip command of binutils 2.36 changes ownership of binary, messing up packaging

Attached to Project: Arch Linux
Opened by AMM (amish) - Monday, 08 February 2021, 12:07 GMT
Last edited by Allan McRae (Allan) - Monday, 15 February 2021, 12:33 GMT
Task Type Bug Report
Category Packages: Core
Status Closed
Assigned To Allan McRae (Allan)
Levente Polyak (anthraxx)
Architecture All
Severity High
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 1
Private No

Details

Description:
I have a custom package, which has been working fine from 2-3 years.

PKGBUILD is roughly something like this:

package() {
make install
chown root:http "${pkgdir}/srv/http/foo"
}

In above, foo is a binary file which gets packaged inside a zst package with expected group as http.

With binutils 2.35, the file foo has correct permission in package file i.e. group as http.

But with binutils 2.36, the file foo gets group as root instead of http. (Breaking all my web scripts)

This change of permission happens exactly after makepkg calls strip_file() inside strip.sh

So clearly there is something that changed from binutils v2.35 and v2.36

This is most-likely going to affect packaging of all Arch packages where binary has owner or group other than root.

For example:
/usr/bin/dumpcap (owned by wireshark-cli where GID is changed to 150)
/usr/bin/locate (owned by mlocate where GID is set to 21)
/usr/bin/{wall,write} (owned by util-linux where group is set to tty)

I think this is a serious bug and needs attention.

I see that Allan McRae has already raised a query for very very similar issue at binutils upstream but this bug is slightly different.

This bug changes ownership from regular user to root, whereas issue that Allan has raised has ownership changed from root to the build user.

https://sourceware.org/pipermail/binutils/2021-February/115241.html

The bug that Allan reported is between binutils version 2.36 and 2.36.1 (which is not yet in Arch repo) but this bug that I am reporting is between binutils version 2.35 and 2.36 i.e. it has already landed in Arch repository.

Please have a look

Thank you


Additional info:
* package version(s)
2.36-3 (breaks packaging)
2.35 (works as expected)

* link to upstream bug report, if any
(Related) https://sourceware.org/pipermail/binutils/2021-February/115241.html


Steps to reproduce:
As above.
This task depends upon

Closed by  Allan McRae (Allan)
Monday, 15 February 2021, 12:33 GMT
Reason for closing:  Fixed
Additional comments about closing:  fakeroot-1.25.3-2
Comment by AMM (amish) - Monday, 08 February 2021, 12:23 GMT
As I look further, package kdesu changes Group ownership of /usr/lib/kf5/kdesud to nobody and sets a setgid permission.

Which means kdesud is supposed to run with group permission of nobody. (and not root)

But with above bug, the group will change to root. Which means instead of running as nobody, kdesud will now run as root!! And whoever runs kdesud is likely to have root privileges.

Ofcourse this will not happen right now, but it will happen the next time kdesu package is updated.
Comment by Eli Schwartz (eschwartz) - Monday, 08 February 2021, 12:24 GMT
Comment by Allan McRae (Allan) - Monday, 08 February 2021, 12:31 GMT
Should only be the binutls-2.36.1 package which had this issue, and was removed from [testing] very quickly. You should downgrade to the version in [core].
Comment by Allan McRae (Allan) - Monday, 08 February 2021, 12:33 GMT
In fact... 2.36.1 resulted in files owned by 1000:1000. Seems 2.36 just removes non-root ownership.

I'll update pacman with the backported patch.
Comment by AMM (amish) - Monday, 08 February 2021, 13:39 GMT
@Allan - you probably missed my note that the bug from 2.36 to 2.36.1 is different from the one I reported i.e. from 2.35 to 2.36

But I think the patch will resolve both the issues.
Comment by AMM (amish) - Monday, 08 February 2021, 14:24 GMT
About the new patch, if this whole exercise is done only for preserving file permissions and attributes. Then I would like to propose an alternate and faster method.

Instead of copying whole file twice, once by strip and again by cat, we can copy acl and attributes of $binary

1) Use getfacl, getfattr to store acl and attributes of $binary in two temporary files
2) Let strip command do whatever it does, directly on $binary (so double copying can be avoided)
3) Use setfacl, setfattr to restore acl and attributes of $binary

This will speed up the processing drastically instead of copying twice. Especially it will be very fast and efficient for large packages.
Comment by Levente Polyak (anthraxx) - Monday, 08 February 2021, 14:29 GMT
The distro repository has been scanned and no security issues with this root could be identified. Packages that would have been affected were not using 2.36 and all others were expected to have root:root
Comment by Eli Schwartz (eschwartz) - Monday, 08 February 2021, 14:37 GMT
strip already uses a temporary file, but mv's it in place to atomically overwrite the original file. and getfacl/getfattr were explicitly rejected in the timely commentary for that patch, as they aren't portable even on platforms that actually possess such a utility. An alternative patch used stat and chown, but was withdrawn in favor of the patch that handles xattr at all.
Comment by Evangelos Foutras (foutrelis) - Monday, 08 February 2021, 15:49 GMT
Working around this in makepkg might not be sufficient as package() itself could be using strip (e.g.: glibc's PKGBUILD, built with binutils 2.36.1 results in wrong ownership, regardless of the makepkg fix in pacman 5.2.2-2).

I think we need a fix in binutils and/or fakeroot instead of workarounds further up the stack.
Comment by Levente Polyak (anthraxx) - Monday, 08 February 2021, 15:53 GMT
foutrelis: this patch isn't just a workaround: it solves another purpose that was not possible before -- preserve XATTR which makes setcap install scriptlets obsolete in the future (once alpm sets the extract bits in the release version).
Comment by Allan McRae (Allan) - Monday, 08 February 2021, 16:01 GMT
Fakeroot is at fault, not binutils. It does not have a fix on the horizon (likely a few months).

strip in glibc's PKGBUILD remains an issue.
Comment by Eli Schwartz (eschwartz) - Monday, 08 February 2021, 16:31 GMT
For packages running strip in make install, the solution is "don't do that, it was always wrong as it breaks debug package generation".

For the one package where it's okay to run strip during package(), glibc, the workaround is to run chown after, to restore permissions. Or setfacl/getfacl if you really like ;) or hook into the same logic as strip_file.

Obviously the glibc workaround also works for packages which are doing it wrong and stripping during make install.
Comment by AMM (amish) - Tuesday, 09 February 2021, 04:07 GMT
Just an acknowledgement that I have tested this and the new pacman updated has fixed my packaging issue.
Comment by artoo (artoo) - Thursday, 11 February 2021, 15:37 GMT
As pointed out on the ML, this issue is not entirely fixed with binutils-2.36.1.

Among the packages with resulting wrong ownership are the kernel modules, and I noticed elfutils last night.
I am not entirely sure if it is worth to workaround these in the makepkg's tidy, or if a new fakeroot version is gonna address this.
Comment by artoo (artoo) - Thursday, 11 February 2021, 18:40 GMT
Small update.

Snippet from our bot, reproducible locally. This is with binutils-2.36.1

Checking libelf-0.183-3-x86_64.pkg.tar.zst
libelf E: File (usr/lib/libasm.a) is owned by builduser:builduser
libelf E: File (usr/lib/libdw.a) is owned by builduser:builduser
libelf E: File (usr/lib/libelf.a) is owned by builduser:builduser

Disabling the staticlibs option makes the problem go away. Stands to reason this needs to be fixed in strip.sh?

Checking libelf-0.183-3-x86_64.pkg.tar.zst
libelf W: Dependency gcc-libs included but already satisfied
libelf W: Dependency xz included but already satisfied
libelf W: Dependency zlib included but already satisfied
==> Running checkpkg
-> Checking packages

Comment by Doug Newgard (Scimmia) - Thursday, 11 February 2021, 18:45 GMT
With what version of pacman?
Comment by artoo (artoo) - Thursday, 11 February 2021, 18:54 GMT
It is pacman-5.2.2-2, so the strip patch from pacman.git repo is applied already.
The only difference compared to arch is binutils, we updated it from the hunch this issue isn't fixed.

https://sourceware.org/pipermail/binutils/2021-February/115240.html
Comment by Allan McRae (Allan) - Thursday, 11 February 2021, 21:25 GMT
Anything that strips files in the PKGBUILD (whether manually or by the build system) and not by makepkg will remain broken. This is even the case with binutils-2.36, which is just broken in a more subtle way.
Comment by artoo (artoo) - Thursday, 11 February 2021, 21:34 GMT
The stripping with enabled staticlibs seems broke on makepkg end with 2.36.1, while 2.36 is fine producing a libelf without *.a builduser owned?
Afaik, eg acpi_call doesn't strip in the PKGBUILD, but has module owned by builduser.

Edit: All kernel module PKGBUILDs that use strip in the PKGBUILD have builduser:builduser modules.

acpi_call & broadcom-wl are the only ones root owned, ie no manual strip.

Loading...