Bugzilla – Bug 1169238
AUDIT-STALE: enlightenment: changes to binaries requiring suid bits
Last modified: 2022-07-05 10:58:23 UTC
The enlightenment team is redoing and simplify and condensing the number of binaries that have suid bits set. The new binary enlightenment_system [1], does the following replacing the previous suid bit binaries that performed these tasks mount/unmount storage, rfkill, shutdown/suspend/reboot/suspend/hibernate execution (support not used if systemd is found first which is the case in openSUSE), ddc monitor controls (libddcutil), cpufreq controls and backlight controls For now the package is still building in a testing repository which you can find here https://build.opensuse.org/package/show/X11:Enlightenment:Testing/enlightenment 1. http://git.enlightenment.org/core/enlightenment.git/tree/src/bin/e_system.c [ 443s] enlightenment.x86_64: E: permissions-file-setuid-bit (Badness: 10) /usr/lib64/enlightenment/utils/enlightenment_system is packaged with setuid/setgid bits (04555) [ 443s] If the package is intended for inclusion in any SUSE product [ 443s] please open a bug report to request review of the program by the [ 443s] security team. Please refer to [ 443s] https://en.opensuse.org/openSUSE:Package_security_guidelines#audit_bugs for [ 443s] more information.
Thank you for opening the bug for us early on and providing as information. We will schedule the review and let you know the outcome.
I will have a look at this.
I'm done with the review. I'm sorry to say that I can't accept the setuid program the way it currently is. It's quite a catastrophe as far as setuid-root programs go. I will create sub-bugs with the individual major issues I found. But first I will publish a complete report on the upstream mailing list. The Enlightenment project does not seem to want private reporting of security issues is what they told me on their IRC channel. So no EMBARGO and no CRD then.
(In reply to Matthias Gerstner from comment #3) > I'm done with the review. I'm sorry to say that I can't accept the setuid > program the way it currently is. It's quite a catastrophe as far as > setuid-root programs go. > > I will create sub-bugs with the individual major issues I found. But first I > will publish a complete report on the upstream mailing list. The > Enlightenment project does not seem to want private reporting of security > issues is what they told me on their IRC channel. So no EMBARGO and no CRD > then. Thanks, I don't think embargo's and CRD's apply in this case because it is still pre release software I intentionally wanted to get the review done before the upstream release so that issues were addressed beforehand :-)
(In reply to simonf.lees@suse.com from comment #4) > I don't think embargo's and CRD's apply in this case because it is still pre > release software I intentionally wanted to get the review done before the > upstream release so that issues were addressed beforehand :-) Yes I figured that much. Still I always feel better when there's a defined security reporting process. I comment 0 you said: > The enlightenment team is redoing and simplify and condensing the number of > binaries that have suid bits set. So are there any setuid-root binaries in current Enlightenment releases? I looked into Leap 15.1 but couldn't find any. Since this new setuid-root binary is quite a security disaster I wondered if any of that was already existing before ... Ah I'm just seeing that we're removing the setuid bits in our spec file there. So we should at least not be affected if anything is bad in there.
I just posted my detailed security report on the enlightenment-devel mailing list here: https://sourceforge.net/p/enlightenment/mailman/message/36988890/
(In reply to Matthias Gerstner from comment #5) > (In reply to simonf.lees@suse.com from comment #4) > > I don't think embargo's and CRD's apply in this case because it is still pre > > release software I intentionally wanted to get the review done before the > > upstream release so that issues were addressed beforehand :-) > > Yes I figured that much. Still I always feel better when there's a defined > security reporting process. > > I comment 0 you said: > > > The enlightenment team is redoing and simplify and condensing the number of > > binaries that have suid bits set. > > So are there any setuid-root binaries in current Enlightenment releases? I > looked into Leap 15.1 but couldn't find any. Since this new setuid-root > binary > is quite a security disaster I wondered if any of that was already existing > before ... > > Ah I'm just seeing that we're removing the setuid bits in our spec file > there. > So we should at least not be affected if anything is bad in there. We are for atleast most, at one point there was one or two because I remember getting an audit done many years ago. But there implementation was completely different, we have also likely dropped some due to being able to use systemd for shutdown, reboot etc. But there was some like setting the CPU frequency that we never set the suid bit on. This new implementation is meant to totally replace all those existing ones. Thanks for your time and effort on this.
I'm done with creating the sub-bugs. Sorry for all the noise but this is the best way to keep track of all findings when we revisit a new version of the program. Apart from the concrete security findings I have one further comment: The setuid-root program tries to load one "libddcutil.so.2" library during runtime for the DDC control features implemented in `e_system_ddc.c`. This library version is not currently available on Tumbleweed, thus these features will not work at the moment.
(In reply to Matthias Gerstner from comment #8) > I'm done with creating the sub-bugs. Sorry for all the noise but this is the > best way to keep track of all findings when we revisit a new version of the > program. > > Apart from the concrete security findings I have one further comment: > > The setuid-root program tries to load one "libddcutil.so.2" library during > runtime for the DDC control features implemented in `e_system_ddc.c`. This > library version is not currently available on Tumbleweed, thus these features > will not work at the moment. That's ok for now, its a new feature, enlightenment will use that library to control brightness on a large number of external monitors that support it as well as for laptop monitors etc when available.
I will make a second pass over all the findings in a while when I have dealt with some other things I'm currently working on. There are sign on the upstream mailing list that they're making a new release. Will this include the setuid program? In any case I would like to have a look at the released version of the tool and maybe also follow-up releases until the program is feature complete. There's a rather high danger that regressions are introduced.
(In reply to Matthias Gerstner from comment #10) > I will make a second pass over all the findings in a while when I have dealt > with some other things I'm currently working on. > > There are sign on the upstream mailing list that they're making a new > release. > Will this include the setuid program? In any case I would like to have a look > at the released version of the tool and maybe also follow-up releases until > the program is feature complete. There's a rather high danger that > regressions > are introduced. The libraries were released yesterday they are independent and don't include it. Enlightenment will likely be released within the coming month including the binary. The current plan is Alpha this weekend, then Beta next weekend and release after that if there are no issues. This release will include the new suid binary.
I'm changing the bug type to TRACKER since there's currently nothing else to do. Once you can point me to a new package containing the official release I can make a second pass over the code.
(In reply to Matthias Gerstner from comment #12) > I'm changing the bug type to TRACKER since there's currently nothing else to > do. Once you can point me to a new package containing the official release I > can make a second pass over the code. The final version has now been released, there is a package at https://build.opensuse.org/package/show/X11:Enlightenment:Testing/enlightenment ready for approval.
So bug 1170161 and bug 1170162 are left open. Upstream cut off the mount logic in the source code but not the umount logic. The umount logic is still disabled via the configuration file. I'm not all that happy about it, though. I can whitelist the setuid program but please approach upstream about the two bugs left. I have a bad feeling about this mounting logic.
(In reply to Matthias Gerstner from comment #14) > So bug 1170161 and bug 1170162 are left open. Upstream cut off the mount > logic > in the source code but not the umount logic. The umount logic is still > disabled via the configuration file. > > I'm not all that happy about it, though. I can whitelist the setuid program > but please approach upstream about the two bugs left. I have a bad feeling > about this mounting logic. I'll follow up with upstream, if you can add it to the whitelist that would be great i'd like to try and get these changes into Leap 15.2 as well
Before I can add the whitelisting you need to add a call to '%set_permissions' to your spec file. You can see how this is done e.g. in openSUSE:Factory/fuse3 or similar packages shipping setuid binaries. Please tell me when this is done.
(In reply to Matthias Gerstner from comment #16) > Before I can add the whitelisting you need to add a call to > '%set_permissions' > to your spec file. You can see how this is done e.g. in > openSUSE:Factory/fuse3 or similar packages shipping setuid binaries. > > Please tell me when this is done. It should be done now https://build.opensuse.org/request/show/808997
Thank you for adjusting the spec file. The whitelisting is now in Base:System/permissions. It will take some time before it will hit Factory, though. A larger change is pending to be accepted in the permissions package in Factory. I don't want to interrupt the review process there. Once this is through we should be able to get the whitelisting quickly into Factory.
The big permissions/Factory change is through by now. I've submitted the enlightenment Factory whitelisting via sr#810755. You can already submit Enlightenment, too, and specify that it should be handled together with this sr#. I've also submitted a whitelisting for SLE-15-SP2 via IBS sr#219362.
Changing bug type to "STALE" since there has been no progress for a couple of months.
FYI, due to bug 1196609 the enlightenment system.conf was changed to no longer require membership of the "users" group to allow running the setuid helper(s).
(In reply to Fabian Vogt from comment #21) > FYI, due to bug 1196609 the enlightenment system.conf was changed to no > longer require membership of the "users" group to allow running the setuid > helper(s). Only for some of the helpers, ones that were disabled before are still disabled for all users other then root. On the weekend i'll see if I can try and patch these out of the build completely so we can close off the remaining issues from an openSUSE side. An interesting question is whether its worth adding the complexity to the suid bit application to hook into logind and only allow a user with an active seat to run the commands which would reduce the footprint in some areas but increase complexity in others.
(In reply to simonf.lees@suse.com from comment #22) > An interesting question is whether its worth adding the complexity to the suid bit application to hook into logind and only allow a user with an active seat to run the commands which would reduce the footprint in some areas but increase complexity in others. This could be achieved by not using setuid-root binaries at all, but rely on Polkit and pkexec. pkexec runs the program as root after authenticating the calling user. Polkit can be configured to allow execution for users in an active session without entering a password. A weakness of pkexec is that the restriction of command line parameters that are allowed to be passed to the programm is not easily possible based on configuration, except for the first command line argument. This has to be kept in mind if this route should be followed.
All sub-bugs have been addressed, so I'm closing this tracker bug as well. Thanks for working on this.