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.

Sign up for GitHub

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

Jump to bottom

hooks/post-install: add check setuid/setgid hook #33011

Open
wants to merge 58 commits into
base: master
Choose a base branch
from
Open

hooks/post-install: add check setuid/setgid hook #33011

wants to merge 58 commits into from

Conversation

paper42
Copy link
Member

@paper42 paper42 commented Sep 18, 2021

Closes #32156

cc @ericonr

@paper42 paper42 changed the title hooks/post-install: add check setuid/setgid hook [WIP]: hooks/post-install: add check setuid/setgid hook Sep 18, 2021
@0x5c
Copy link
Contributor

0x5c commented Apr 3, 2022 •
edited

If we'll need an xlint for the set{u,g}id, would it make sense to have it require an explanatory comment (like for make_check=no)? If so, we should already start adding those.

@0x5c
Copy link
Contributor

0x5c commented Apr 4, 2022 •
edited

Going through the list, there's some odd ones I'm not sure what to do with

  • Powermanga, uses sgid root:games, except that we don't have a games group, it is not being created, and the package has no INSTALL script to chown it.
    See Arch Wiki and Debian Wiki.
  • containers, has suid binaries /usr/bin/{contain,pseudo} as root:root, but the README mentions situations where suid should not be installed.

There's also some packages I barely comprehend in the first place and will not attempt to allow/fix

  • arcan, a "Combined display server, multimedia framework and game engine". (Wouldn't a game engine preferably not run as root?)
  • all instances of chrome-sandbox, since I do not know how those work
  • all mount.* tools, hard to test and no idea how they work in relation to mount itself being suid

Packages I can tell need setuid/setgid are being marked as allowed and pushed to 0x5c:suid as I go through the list.

I'll be making PRs to void-packages directly for packages that appear to not need suid/sgid, as was the case for vpsm: #36489. I'll list those here if there's more.

@paper42
Copy link
Member Author

paper42 commented Apr 6, 2022

If we'll need an xlint for the set{u,g}id, would it make sense to have it require an explanatory comment (like for make_check=no)? If so, we should already start adding those.

I don't think we should require a comment. If something is not obvious, we can always add the comment.

0x5c
0x5c reviewed Apr 20, 2022
View reviewed changes
common/hooks/post-install/15-check-setuid-setgid.sh
fi
done
if [ -n "$matched" ]; then
echo "$2 file: ${setidfile#$PKGDESTDIR}"
Copy link
Contributor

@0x5c 0x5c Apr 20, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I observe in other hooks that printed text tends to be prefixed by 3 spaces. Should that be the case for that print?

@0x5c
Copy link
Contributor

0x5c commented Apr 20, 2022

In 0x5c@9cd5290 I allowed thttpd's makeweb command. However, it's not configurable (except at compile-time) and allows normal users to create a subdirectory of the main www dir. This seems somewhat dangerous and like something that should not be part of the main package.

@0x5c
Copy link
Contributor

0x5c commented Apr 20, 2022

mit-krb5-client is now the first example of this working on subpackages

@paper42 paper42 self-assigned this May 30, 2022
@paper42 paper42 force-pushed the setuid-setgid-hook branch 2 times, most recently from dd42f6d to 97bf7c9 Compare February 25, 2023 11:30
@paper42 paper42 marked this pull request as ready for review March 12, 2023 07:41
@paper42 paper42 changed the title [WIP]: hooks/post-install: add check setuid/setgid hook hooks/post-install: add check setuid/setgid hook Mar 12, 2023
@classabbyamp classabbyamp added the xbps-src xbps-src related label Jun 24, 2023
paper42 and others added 27 commits August 6, 2023 20:36
@paper42 @classabbyamp
lxc: add $setuid
a5f0dab
@paper42 @classabbyamp
dma: add $setuid and $setgid
a12f3ab
@paper42 @classabbyamp
xorg-server: add $setuid
b26a53b
@paper42 @classabbyamp
opensmtpd: add $setuid and $setgid
3ae9d50
@paper42 @classabbyamp
arcan: add $setuid
35a476d
@paper42 @classabbyamp
davfs2: add $setuid
e24eae9
@paper42 @classabbyamp
hikari: add $setuid
11ed08e
@paper42 @classabbyamp
squid: add $setuid
8217190
@paper42 @classabbyamp
libutempter: add $setgid
6ecce3f
@paper42 @classabbyamp
qemu: add $setuid
54cbdc1
@paper42 @classabbyamp
plocate: add $setgid
5d71909
@paper42 @classabbyamp
electron19: add $setuid
de0f380
@paper42 @classabbyamp
firejail: add $setuid
f088d53
@paper42 @classabbyamp
hiawatha: add $setuid
bcb5f39
@paper42 @classabbyamp
kismet: add $setuid
e93043c
@paper42 @classabbyamp
schroot: add $setuid
d3e82ad
@paper42 @classabbyamp
x2goserver: add $setgid
b264718
@paper42 @classabbyamp
virtualbox-ose: add $setuid
ade33eb
@0x5c @classabbyamp
at: explicitly allow setuid
3621efb
@0x5c @classabbyamp
9mount: explicitly allow setuid
8e5ff9c
@0x5c @classabbyamp
xlockmore: explicitly allow setuid
aacd141 
Setuid root appears required by xlock on systems that use shadow passwords,
according to the README.

Requires confirmation
@0x5c @classabbyamp
util-linux: explicitly allow setuid
06ed1d7
@0x5c @classabbyamp
shadow: explicitly allow setuid
4e1175d
@0x5c @classabbyamp
sudo: explicitly allow setuid
000c311
@0x5c @classabbyamp
udevil: explicitly allow setuid
9e5716d
@0x5c @classabbyamp
thttpd: explicitly allow setuid
0f45e96 
The makeweb tool needs sgid. However, thttpd is configured at compile time, and
it's unclear if our config (the default) allows usage of `makeweb`. That tool
also doesn't look like a superb thing to inconditionally ship in the main
package since it can't be configured by the system admin. Perhaps it should be
split into a subpackage?
@0x5c @classabbyamp
mit-krb5: explicitly allow setuid
f889f1e
@0x5c
Copy link
Contributor

0x5c commented Sep 20, 2023 •
edited

I don't think that 9cb2e7b is safe to have, considering that that sgid is supposed to be for group :games which we don't have. This results in the game having sgid :root.

@0x5c
Copy link
Contributor

0x5c commented Sep 20, 2023

For electron's chrome-sandbox, this comment suggests that they should not be packaged anymore.
#32156 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Duncaen Duncaen Duncaen left review comments

@0x5c 0x5c 0x5c left review comments

Assignees

@paper42 paper42

Labels
xbps-src xbps-src related
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RFC] explicitly allow setuid and setgid permissions in templates
4 participants
@paper42 @0x5c @Duncaen @classabbyamp