Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tmpfiles: chown after chmod removes setuid bit #12354

Closed
SebastianS90 opened this issue Apr 19, 2019 · 10 comments · Fixed by #12431
Closed

tmpfiles: chown after chmod removes setuid bit #12354

SebastianS90 opened this issue Apr 19, 2019 · 10 comments · Fixed by #12431
Labels
bug 🐛 Programming errors, that need preferential fixing tmpfiles
Milestone

Comments

@SebastianS90
Copy link

systemd version the issue has been seen with

systemd 241 (241.93-1-arch)

Used distribution

Arch Linux

Expected behaviour you didn't see

setuid bit is set as defined by my tmpfile configuration

Unexpected behaviour you saw

setuid bit was not set

Steps to reproduce the problem

  • Configure a tmpfile for mode 4755 (setuid bit set) and non-root owner.
  • Create that file as root.
  • Run systemd-tmpfiles --create. The tmpfile now has the correct owner, but no setuid bit.
  • Run systemd-tmpfiles --create again. Now also the setuid bit is set.
root@seb-laptop ~ # echo "z /run/test123 4755 sebastian sebastian - -" > /etc/tmpfiles.d/test123.conf
root@seb-laptop ~ # touch /run/test123
root@seb-laptop ~ # stat /run/test123 
  File: /run/test123
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: 17h/23d Inode: 1753768     Links: 1
Access: (0644/-rw-r--r--)  Uid: (    0/    root)   Gid: (    0/    root)
Access: 2019-04-19 21:53:26.658664394 +0200
Modify: 2019-04-19 21:53:26.658664394 +0200
Change: 2019-04-19 21:53:26.658664394 +0200
 Birth: -
root@seb-laptop ~ # systemd-tmpfiles --create
root@seb-laptop ~ # stat /run/test123        
  File: /run/test123
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: 17h/23d Inode: 1753768     Links: 1
Access: (0755/-rwxr-xr-x)  Uid: ( 1000/sebastian)   Gid: ( 1000/sebastian)
Access: 2019-04-19 21:53:26.658664394 +0200
Modify: 2019-04-19 21:53:26.658664394 +0200
Change: 2019-04-19 21:53:46.331796167 +0200
 Birth: -
root@seb-laptop ~ # systemd-tmpfiles --create
root@seb-laptop ~ # stat /run/test123        
  File: /run/test123
  Size: 0               Blocks: 0          IO Block: 4096   regular empty file
Device: 17h/23d Inode: 1753768     Links: 1
Access: (4755/-rwsr-xr-x)  Uid: ( 1000/sebastian)   Gid: ( 1000/sebastian)
Access: 2019-04-19 21:53:26.658664394 +0200
Modify: 2019-04-19 21:53:26.658664394 +0200
Change: 2019-04-19 21:53:53.238395435 +0200
 Birth: -

The interesting excerpt from strace is:

openat(4, "test123", O_RDONLY|O_NOFOLLOW|O_CLOEXEC|O_PATH) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
close(4)                                = 0
fstat(3, {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
chmod("/proc/self/fd/3", 04755)         = 0
fchownat(3, "", 1000, 1000, AT_EMPTY_PATH) = 0
close(3)

As you can see, systemd-tmpfiles first sets the permissions and then changes the owner. But chown implicitly resets the setuid bit.

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Apr 19, 2019

# cat test.conf 
f /tmp/test                4755 nobody -        - -
# ls -l /tmp/test
ls: cannot access '/tmp/test': No such file or directory
# systemd-tmpfiles --create $PWD/test.conf
# ls -l /tmp/test
-rwxr-xr-x 1 nobody root 0 Apr 19 16:31 /tmp/test
# systemd-tmpfiles --create $PWD/test.conf
# ls -l /tmp/test
-rwsr-xr-x 1 nobody root 0 Apr 19 16:31 /tmp/test

Crude workaround is to list your desired changes twice in the file (or wait for a reboot).

@floppym
Copy link
Contributor

floppym commented Apr 19, 2019

Using tmpfiles to change the suid bit seems pretty strange. What's the real use case here?

@SebastianS90
Copy link
Author

Nullmailer package on Arch: https://git.archlinux.org/svntogit/community.git/tree/trunk/nullmailer.tmpfiles?h=packages/nullmailer

It is a simple relay only mailer that has a local queue. The executable must be run as the user owning the spool folder for the queue.

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Apr 19, 2019

The use case is distribution packaging for non-root binaries. There are two ways to package a non-root binary: reserve a UID for all of eternity, or use a post-install process via sysusers.d to create the user, then tmpfiles.d to correct the ownership after the user was dynamically allocated.

If that binary is also meant to be suid, then the suid bit needs to be reset too.

...

Is this a general "why would you ever use suid, ever" question?

@floppym
Copy link
Contributor

floppym commented Apr 19, 2019

I see.

Generally people use tmpfiles.d for stuff that needs to happen on every boot. It would be very strange to change the suid bit on some file on every boot.

For stuff that only needs to be done once per package install, package managers usually have their own hooks you can utilize to call arbitrary commands (like chmod/chown). I don't think I have ever seen tmpfiles.d used in quite this way.

@eli-schwartz
Copy link
Contributor

Well, it is already there to create a bunch of directories in /var... and there is already a hook for tmpfiles.d, this means a single unified hook can handle many actions, rather than executing a shellscript for each package.

@floppym
Copy link
Contributor

floppym commented Apr 19, 2019

Sure, it just didn't occur to me.

It seems like it would be fairly simple to re-order the chown before the chmod, but I'm not sure if that introduces any security risks, or would cause any other problems. That probably requires a little bit of thought.

@eli-schwartz
Copy link
Contributor

Since the whole problem is that chown clears the suid bit, it should still clear it when the action is reordered. So no one else should be able to surprisingly run things with suid the-new-user, if the suid it is meant to be cleared anyway.

As for read/write permissions, the owner can already do what they want. But if the group permission is being cleared, I guess that would be a problem.

Maybe it could always set the permissions twice, once before and once after. Or do better accounting, e.g. detect if the permission bit includes suid and save that in order to perform the operation after the chown.

@SebastianS90
Copy link
Author

Relevant security example: The file is initially owned by root:root and has permissions 660. The tmpfiles entry says it should be owned by root:adm with permissions 640.

Currently we first change the permissions to 640 and then the group to adm. No write access ever for group adm. If we reorder that then there is a short moment where the adm group can write to the file.

@poettering poettering added bug 🐛 Programming errors, that need preferential fixing tmpfiles labels Apr 23, 2019
@poettering poettering added this to the v243 milestone Apr 23, 2019
poettering added a commit to poettering/systemd that referenced this issue Apr 29, 2019
Otherwise chown() will drop SUID/SGID bits again, which users might want
to set explicitly.

Fixes: systemd#12354
poettering added a commit to poettering/systemd that referenced this issue Apr 29, 2019
chown() might drop the suid/sgid bit from files. hence let's chmod()
after chown().

But also, let's tighten the transition a bit: before issuing chown()
let's set the file mask to the minimum of the old and new access
bitmask, so that at no point in time additional privs are available on
the file with a non-matching ownership.

Fixes: systemd#12354
@poettering
Copy link
Member

Fix waiting in #12431. ptal

poettering added a commit to poettering/systemd that referenced this issue Apr 29, 2019
chown() might drop the suid/sgid bit from files. hence let's chmod()
after chown().

But also, let's tighten the transition a bit: before issuing chown()
let's set the file mask to the minimum of the old and new access
bitmask, so that at no point in time additional privs are available on
the file with a non-matching ownership.

Fixes: systemd#12354
poettering added a commit to poettering/systemd that referenced this issue Apr 30, 2019
chown() might drop the suid/sgid bit from files. hence let's chmod()
after chown().

But also, let's tighten the transition a bit: before issuing chown()
let's set the file mask to the minimum of the old and new access
bitmask, so that at no point in time additional privs are available on
the file with a non-matching ownership.

Fixes: systemd#12354
anthraxx added a commit to anthraxx/arch-pkgbuilds that referenced this issue May 8, 2019
edevolder pushed a commit to edevolder/systemd that referenced this issue Jun 26, 2019
chown() might drop the suid/sgid bit from files. hence let's chmod()
after chown().

But also, let's tighten the transition a bit: before issuing chown()
let's set the file mask to the minimum of the old and new access
bitmask, so that at no point in time additional privs are available on
the file with a non-matching ownership.

Fixes: systemd#12354
svenstaro pushed a commit to archlinux/svntogit-community that referenced this issue Jul 22, 2020
work around FS#62404 and systemd/systemd#12354

git-svn-id: file:///srv/repos/svn-community/svn@452050 9fca08f4-af9d-4005-b8df-a31f2cc04f65
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Programming errors, that need preferential fixing tmpfiles
Development

Successfully merging a pull request may close this issue.

4 participants