Bug 1607044 - eu-elfcompress removes suid bit
Summary: eu-elfcompress removes suid bit
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: elfutils
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Mark Wielaard
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-07-21 08:21 UTC by Igor Raits
Modified: 2018-07-31 08:10 UTC (History)
6 users (show)

Fixed In Version: elfutils-0.173-6.fc29
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-07-24 22:42:47 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Igor Raits 2018-07-21 08:21:48 UTC
It can be seen here:

https://semaphoreci.com/rpm-ecosystem/rpm/branches/pull-request-473/builds/1

If we call eu-elfcompress -q -p -t none "$f", it removes suid bit.

Comment 1 Mark Wielaard 2018-07-21 11:38:41 UTC
Simpler replicator:

[root@tarox tmp]# cp /bin/echo ./echo-test
[root@tarox tmp]# chmod +s ./echo-test 
[root@tarox tmp]# ls -l ./echo-test 
-rwsr-sr-x. 1 root root 33128 Jul 21 13:12 ./echo-test
[root@tarox tmp]# eu-elfcompress -v -p -t none ./echo-test 
processing: ./echo-test
[root@tarox tmp]# ls -l ./echo-test 
-rwxr-xr-x. 1 root root 33128 Jul 21 13:13 ./echo-test

This is probably caused by the order eu-elfcompress sets the mode/owner of the file. fchown says:

       When the owner or group of an executable file are changed by an unpriv‐
       ileged user the S_ISUID and S_ISGID mode bits are cleared.  POSIX  does
       not specify whether this also should happen when root does the chown();
       the Linux behavior depends on the kernel version.  In case  of  a  non-
       group-executable  file (i.e., one for which the S_IXGRP bit is not set)
       the S_ISGID bit indicates mandatory locking, and is not  cleared  by  a
       chown().

and fchmod says:

       If  the  calling  process  is  not privileged (Linux: does not have the
       CAP_FSETID capability), and the group of the file does  not  match  the
       effective  group  ID  of  the process or one of its supplementary group
       IDs, the S_ISGID bit will be turned off, but this  will  not  cause  an
       error to be returned.

So even with eu-elfcompress -v you won't see any errors, but the permission bit might be dropped anyway.

This patch solves the issue by swapping the fchmod and fchown calls:

diff --git a/src/elfcompress.c b/src/elfcompress.c
index bdb0e3b..1a0f984 100644
--- a/src/elfcompress.c
+++ b/src/elfcompress.c
@@ -1235,13 +1235,16 @@ process_file (const char *fname)
   elf_end (elfnew);
   elfnew = NULL;
 
-  /* Try to match mode and owner.group of the original file.  */
-  if (fchmod (fdnew, st.st_mode & ALLPERMS) != 0)
-    if (verbose >= 0)
-      error (0, errno, "Couldn't fchmod %s", fnew);
+  /* Try to match mode and owner.group of the original file.
+     Note to set suid bits we have to make sure the owner is setup
+     correctly first. Otherwise fchmod will drop them silently
+     or fchown may clear them.  */
   if (fchown (fdnew, st.st_uid, st.st_gid) != 0)
     if (verbose >= 0)
       error (0, errno, "Couldn't fchown %s", fnew);
+  if (fchmod (fdnew, st.st_mode & ALLPERMS) != 0)
+    if (verbose >= 0)
+      error (0, errno, "Couldn't fchmod %s", fnew);
 
   /* Finally replace the old file with the new file.  */
   if (foutput == NULL)

Comment 2 Igor Raits 2018-07-21 11:46:26 UTC
Why does it even fchmod()/fchown()?

Comment 3 Igor Raits 2018-07-21 11:48:32 UTC
why not to do something like stat() + chmod()?

Comment 4 Mark Wielaard 2018-07-21 12:26:52 UTC
(In reply to Igor Gnatenko from comment #2)
> Why does it even fchmod()/fchown()?

To make sure the new file has the same mode and owner as the original.

Comment 5 Mark Wielaard 2018-07-21 12:28:00 UTC
(In reply to Igor Gnatenko from comment #3)
> why not to do something like stat() + chmod()?

That is what it does, it stats the original file and then fchown/fchmod the new file.

Comment 6 Mark Wielaard 2018-07-24 08:43:40 UTC
This should be fixed by:

commit 15973e2b9bab41a0851627863f37a877851f5610
Author: Mark Wielaard <mjw>
Date:   Sat Jul 21 17:20:57 2018 +0200

    0.173-4 - Add elfutils-0.173-elfcompress.patch (#1607044)

But also needs:

commit 896e01dee76f9a08594e75ecda324fc99e03fce2
Author: Mark Wielaard <mjw>
Date:   Sat Jul 21 23:58:40 2018 +0200

    0.173-5
    
    - Add BuildRequires gcc-c++ for demangle support.
    - Add elfutils-0.173-annobingroup.patch.

It still doesn't build because of a (latent?) bug on i386 handling the new annobin ELF group support in ET_REL files in eu-unstrip:

https://koji.fedoraproject.org/koji/taskinfo?taskID=28503802

Other arches are fine, but this doesn't really look like it should be arch specific.

Comment 7 Mark Wielaard 2018-07-24 22:42:47 UTC
It needed another patch to eu-unstrip:

commit 47092416243d54e4cff3cd2558ece8b93695d54e
Author: Mark Wielaard <mark>
Date:   Tue Jul 24 23:34:19 2018 +0200

    unstrip: Also check sh_size in compare_unalloc_sections.
    
    compare_unalloc_sections only checked sh_flags and the section names.
    This would cause stripped/debug section mismatches when there were
    multiple sections with the same name and flags. Fix this by also checking
    the size of the section matches.
    
    Add a testcase that has two ".group" sections created on i386 with the
    gcc annobin plugin.
    
    Signed-off-by: Mark Wielaard <mark>

Added to elfutils-0.173-6.fc29.


Note You need to log in before you can comment on or make changes to this bug.