Re: MAINTAINER FIX: x11/emwm-utils add SUID to xmsm and xmsession

Thanks for the pointers on bsd-auth and why we should avoid setuid root. I did a little research on this, admittedly I am not super technical in the security side of things. Sorry if I came off as presumptuous or rude out of my ignorance.

I tried to put my user in _shutdown, and patched the #define but it still attempts to run those commands as su even in cases where the user can run it su-less. (see .xsession-errors yourself).

I suppose for now, I'll try to patch it up to not hard-require setuid root and let users in the _shutdown group work. I plan to write up a pkg-readme about these quirks if/when I get this working.

Incorporating bsd-auth here is far beyond my skillset, as you may have noticed. So any assistance or pointers would be appreciated.

Thanks.


-------- Original Message --------
From: Stuart Henderson <stu@spacehopper.org>
Sent: February 28, 2025 3:51:45 AM CST
To: izzy Meyer <izder456@disroot.org>
Cc: Lucas Gabriel Vuotto <lucas@sexy.is>, ports@openbsd.org
Subject: Re: MAINTAINER FIX: x11/emwm-utils add SUID to xmsm and xmsession

On 2025/02/28 01:45, izzy Meyer wrote:
> On Fri, 28 Feb 2025 03:13:53 +0000
> Lucas Gabriel Vuotto <lucas@sexy.is> wrote:
>
> > Hello,
> >
> > On Thu, Feb 27, 2025 at 07:52:30PM -0600, izzy Meyer wrote:
> > > Hello ports@
> > >
> > > Attached is a diff to set SUID on xmsm and xmsession which fixes the
> > > locking/logout/poweroff/shutdown errors. This also bumps REVISION to
> > > 1.

shutdown should work already, as long as the user is in group _shutdown.

if you change the REBOOT_CMD #define to "/sbin/shutdown -r now" then the
same should apply for reboot.

> > > Index: x11/emwm-utils/Makefile
> > > ===================================================================
> > > RCS file: /cvs/ports/x11/emwm-utils/Makefile,v
> > > diff -u -r1.1.1.1 Makefile
> > > --- x11/emwm-utils/Makefile 23 Aug 2024 06:03:38
> > > -0000 1.1.1.1 +++ x11/emwm-utils/Makefile 28 Feb 2025
> > > 01:49:29 -0000 @@ -1,6 +1,7 @@
> > > COMMENT = session manager and a toolchest-like application
> > > launcher
> > > V = 1.2
> > > +REVISION = 1
> >
> > REVISION starts at 0.
>
> Whoops, bit of an oversight on my part. Thanks!
>
> > > DISTNAME = emwm-utils-src-${V}
> > > PKGNAME = emwm-utils-${V}
> > >
> > > @@ -28,5 +29,9 @@
> > > RCDIR=${WRKINST}${PREFIX}/lib/X11
> > >
> > > NO_TEST = Yes
> > > +
> > > +# xmsession and xmsm need suid to lock properly
> > > +post-install:
> > > + chmod u+s ${PREFIX}/bin/{xmsm,xmsession}
> >
> > Ports usually handle this differently, by hooking into the BSD
> > Authentication system (man authenticate). Take a look at x11/slock,
> > which might be one of the simplest implementations.
> >
>
> I looked into slock.c under the WRKSRC of x11/slock, and found no
> mention of the functions shown in authenticate(3). BUT- What I *do* see
> is an explicit mention os SUID/SGID:
>
> #ifdef __OpenBSD__
> if (!(pw = getpwuid_shadow(getuid())))
> die("slock: getpwnam_shadow: cannot retrieve
> shadow entry. " "Make sure to suid or sgid slock.\n");
> hash = pw->pw_passwd;
>
> Additionally, the Makefile in the WRKSRC of x11/slock specifically sets
> SUID:
>
> install: all
> @echo installing executable file to ${DESTDIR}${PREFIX}/bin
> @mkdir -p ${DESTDIR}${PREFIX}/bin
> @cp -f slock ${DESTDIR}${PREFIX}/bin
> @chmod 755 ${DESTDIR}${PREFIX}/bin/slock
> @chmod u+s ${DESTDIR}${PREFIX}/bin/slock # here
> [..snip]
>
> I must be missing something.

use 'make patch', not 'make extract'.

> Any specific hard-need for using authenticate(3) here? as simply
> setting SUID seems to do the trick fine.

if you don't understand the difference you definitely shouldn't be using
setuid root.

the buffer size calculations for sprintf do not give me a warm and fuzzy
feeling but might be ok. the expand_env_vars() implementation with its
strcat, multiplications for calloc, etc, should stay the hell away from
setuid root.

certainly should be no more than setgid _shadow and even that's a
stretch. converting to bsd-auth would be an improvement.

> Minor question, I actually don't see SUID being set with my diff when
> installed on my system, but inside of the fake install, it is SUID? Does
> this not get preserved when packaging or something?

correct, it needs to be annotated in the PLIST. it is simply too
dangerous to have these things easy to apply based on the port build,
it must at least be an explicit decision by the port maintainer and
whoever commits such a diff.


--
Sent from a mobile, please excuse shitty formatting.