rrr:no dh_strip or strip loose setuid bit

Bug #1938886 reported by Dimitri John Ledkov
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
debhelper (Ubuntu)
Fix Released
High
Unassigned

Bug Description

Over at https://launchpadlibrarian.net/550715513/buildlog_ubuntu-hirsute-amd64.virtualbox_6.1.22-dfsg-2~ubuntu1.21.04.2_BUILDING.txt.gz

I have rebuilt an earlier version of virtualbox, that sets Rules-Requires-Root: no and added extra ls statements to find where/when/why setuid bits are getting lost after fixperms.

make[1]: Leaving directory '/<<PKGBUILDDIR>>'
   debian/rules override_dh_strip
make[1]: Entering directory '/<<PKGBUILDDIR>>'
ls -latr debian/virtualbox/usr/lib/virtualbox/VBoxSDL
-rwsr-sr-x 1 buildd buildd 406808 Jul 29 14:34 debian/virtualbox/usr/lib/virtualbox/VBoxSDL
...
dh_strip --dbgsym-migration='virtualbox-dbg'
debugedit: debian/virtualbox/usr/lib/virtualbox/VBoxSDL.so: Unknown DWARF DW_FORM_0x1f20
a7cf3c43c8b18c3261d2d4737a475bf730ad1554

ls -latr debian/virtualbox/usr/lib/virtualbox/VBoxSDL
-rwxr-xr-x 1 buildd buildd 166208 Jul 29 14:35 debian/virtualbox/usr/lib/virtualbox/VBoxSDL

It seems to me that either dh_strip or something it calls (strip, debugedit) looses the setuid permission in hirsute and up.

Revision history for this message
Gianfranco Costamagna (costamagnagianfranco) wrote :

I updated debugedit to 5.0 version, the changelog looks like adding some errors when chmod can't be done correctly

@@ -3419,7 +3431,8 @@
     }

   /* Make sure we can read and write */
- chmod (file, stat_buf.st_mode | S_IRUSR | S_IWUSR);
+ if (chmod (file, stat_buf.st_mode | S_IRUSR | S_IWUSR) != 0)
+ error (0, errno, "Failed to chmod input file '%s' to make sure we can read and write", file);

   fd = open (file, O_RDWR);
   if (fd < 0)
@@ -3635,7 +3648,8 @@
   close (fd);

   /* Restore old access rights */
- chmod (file, stat_buf.st_mode);
+ if (chmod (file, stat_buf.st_mode) != 0)
+ error (0, errno, "Failed to chmod input file '%s' to restore old access rights", file);

   free ((char *) dso->filename);
   destroy_strings (&dso->debug_str);

@@ -349,7 +352,8 @@
  }

       /* Make sure we can read and write */
- chmod (fname, stat_buf.st_mode | S_IRUSR | S_IWUSR);
+ if (chmod (fname, stat_buf.st_mode | S_IRUSR | S_IWUSR) != 0)
+ error (0, errno, _("cannot chmod \"%s\" to make sure we can read and write"), fname);

       bool failed = false;
       int fd = open64 (fname, O_RDWR);
@@ -386,7 +390,8 @@
  }

       /* Restore old access rights. Including any suid bits reset. */
- chmod (fname, stat_buf.st_mode);
+ if (chmod (fname, stat_buf.st_mode) != 0)
+ error (0, errno, _("cannot chmod \"%s\" to restore old access rights"), fname);

       if (failed)
  failed_count++;

and similar.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

With debugedit v5.0 things are still the same

https://launchpadlibrarian.net/551609214/buildlog_ubuntu-impish-amd64.virtualbox_6.1.26-dfsg-2ubuntu1_BUILDING.txt.gz

the setuid bit is lost.

I guess we need to dig deeper into dh_strip activities, to see what/where/how it is lost. Maybe it is lost before debugedit.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

ls -latr debian/virtualbox/usr/lib/virtualbox/VBoxSDL
-rwsr-sr-x 1 buildd buildd 406824 Aug 4 15:51 debian/virtualbox/usr/lib/virtualbox/VBoxSDL

dh_strip --dbgsym-migration='virtualbox-dbg'

ls -latr debian/virtualbox/usr/lib/virtualbox/VBoxSDL
-rwxr-xr-x 1 buildd buildd 166208 Aug 4 15:52 debian/virtualbox/usr/lib/virtualbox/VBoxSDL

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

dh_strip does

strip --remove-section=.comment --remove-section=.note --strip-unneeded -o /tmp/OdGxqpWWsW/stripeIrB_j debian/virtualbox/usr/lib/virtualbox/VBoxSDL.so
cat '/tmp/OdGxqpWWsW/stripeIrB_j' > 'debian/virtualbox/usr/lib/virtualbox/VBoxSDL.so'

which behaves differently under root and non-root.

specifically `cat anything > file` will strip setuid bits from file, irrespective of umask.

As root:
cat /dev/null > foo
chmod +s foo
ls -latr foo
-rwSr-Sr-- 1 root root 0 Aug 4 18:36 foo
cat /dev/null > foo
ls -latr foo
-rwSr-Sr-- 1 root root 0 Aug 4 18:36 foo

As mere mortal:

cat /dev/null > foo
chmod +s foo
ls -latr foo
-rwSr-Sr-- 1 xnox xnox 0 Aug 4 18:34 foo
cat /dev/null > foo
ls -latr foo
-rw-r-Sr-- 1 xnox xnox 0 Aug 4 18:34 foo

I really do not understand why mere-mortal strips user uid, keeps group uid, and root doesn't do that.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

- objcopy/strip changed in 2.36.1, not keeping file attributes of the
      original file. Work around that in dh_strip to write to a temporary
      file and cat'ing this to the original file to keep the original
      attributes.

which is broken for setuid files.

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

separately I'm not sure who/what/why stips setuid bits on file creation through redirect.

is it like some kind of a CVE in bash/dash? kernel protection?

Revision history for this message
Dimitri John Ledkov (xnox) wrote :

Based on https://elixir.bootlin.com/linux/latest/source/fs/inode.c#L1928 it seems that setuid and capabilities will be stipped, thus currently our implementation of dh_strip causes to loose setuid and capabilities.

No idea why this is working with fakeroot when Rules-Requires-Root is set to binary-targets.
And doesn't when it is set to "no".

chmod +s debian/virtualbox/usr/lib/virtualbox/VBoxSDL
ls -latr debian/virtualbox/usr/lib/virtualbox/VBoxSDL
-rwsr-sr-x 1 xnox xnox 166208 Aug 4 18:59 debian/virtualbox/usr/lib/virtualbox/VBoxSDL
$ cat debian/control | grep Rules
Rules-Requires-Root: no
$ fakeroot dh_strip -pvirtualbox
$ ls -latr debian/virtualbox/usr/lib/virtualbox/VBoxSDL
-rwxr-xr-x 1 xnox xnox 166208 Aug 4 18:59 debian/virtualbox/usr/lib/virtualbox/VBoxSDL

$ chmod +s debian/virtualbox/usr/lib/virtualbox/VBoxSDL
$ ls -latr debian/virtualbox/usr/lib/virtualbox/VBoxSDL
-rwsr-sr-x 1 xnox xnox 166208 Aug 4 18:59 debian/virtualbox/usr/lib/virtualbox/VBoxSDL
$ sed '/Rules-Requires-Root/s/no/binary-targets/' -i debian/control
$ cat debian/control | grep Rules
Rules-Requires-Root: binary-targets
$ fakeroot dh_strip -pvirtualbox
$ ls -latr debian/virtualbox/usr/lib/virtualbox/VBoxSDL
-rwxr-xr-x 1 xnox xnox 166208 Aug 4 19:01 debian/virtualbox/usr/lib/virtualbox/VBoxSDL

Changed in bash (Ubuntu):
status: New → Invalid
Changed in dash (Ubuntu):
status: New → Invalid
Changed in debhelper (Ubuntu):
status: New → Triaged
Changed in debugedit (Ubuntu):
status: New → Invalid
Changed in linux (Ubuntu):
status: New → Invalid
tags: added: patch
Revision history for this message
Dimitri John Ledkov (xnox) wrote :
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package debhelper - 13.3.4ubuntu2

---------------
debhelper (13.3.4ubuntu2) impish; urgency=medium

  * objcopy/strip changed in 2.36.1, not keeping file attributes of the
    original file. Work around that in dh_strip to write to a temporary
    file, copying attributes, cat'ing this to the original file, copying
    attributes again to keep the original attributes. LP: #1938886

 -- Dimitri John Ledkov <email address hidden> Wed, 04 Aug 2021 19:23:25 +0100

Changed in debhelper (Ubuntu):
status: Triaged → Fix Released
Mathew Hodson (mhodson)
no longer affects: virtualbox (Ubuntu)
no longer affects: linux (Ubuntu)
no longer affects: debugedit (Ubuntu)
no longer affects: dash (Ubuntu)
no longer affects: bash (Ubuntu)
Mathew Hodson (mhodson)
no longer affects: binutils (Ubuntu)
Changed in debhelper (Ubuntu):
importance: Undecided → High
tags: added: regression-release
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.