From: izzy Meyer Subject: Re: MAINTAINER REGRESSION FIX: x11/emwm-utils To: Lucas Gabriel Vuotto Cc: ports@openbsd.org Date: Fri, 23 May 2025 11:36:19 -0500 On Fri, 23 May 2025 02:44:38 +0000 Lucas Gabriel Vuotto wrote: > Hi izzy, > > On Thu, May 22, 2025 at 12:08:52PM -0500, izzy Meyer wrote: > > I said this in a previous mail [2], but I think it got overlooked: > > > > > Is it possible to backport this to -stable as the bsd-auth patch > > > and REBOOT_CMD fix breaking bugs in the port? Without these two > > > changes, the locking mechanism freezes up the xsession and > > > reboot/shutdown/halt do the same. I have a machine that I run > > > -stable on, and it'd be nice to have these fixes on that machine. > > > I understand port updates usually don't get backported to > > > -stable, but would this be a valid enough exception? > > > > Ideally, x11/emwm should be updated in tandem with x11/emwm-utils as > > they work as a team, so could that be backported as well? > > I sent my reply to you privately on Sun, 18 May 2025 06:33:36 +0000: > > | Backports to -stable are reserved for security or serious bugs. The > fix | freezings does fall in that category, but that's the work of > your patch | and not the release. In fact, the release is marked as > "small fixes". | > | Because of that, I'll need that you send me auth patches backported > to | 1.2 and then I can merge it. Thanks, I'll look into doing this for -stable. > > > In a previous mail, I was asked to suppress a warning about xmsm > > lacking suid root. [1] > > > > I patched too much in my #ifndef/#endif to suppress the warning, so > > the logic ended up assuming that the non SUID root binary can be > > ran as root, and exec's /usr/libexec/login_passwd as root when > > attempting a lock. This is incorrect. I narrowed down the > > #ifndef/#ifdef logic to just include the warning and the failure to > > lock is now fixed. > > > > This also bumps REVISION to 1. > > > > Tested on macppc/powerpc, arm64, and amd64. > > > > Good to merge? > > I have a few comments about this one. > > > diff --git a/Makefile b/Makefile > > index 6a5e5822b36..127fceca498 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -3,7 +3,7 @@ COMMENT = session manager and a > > toolchest-like application launcher V = 1.3 > > DISTNAME = emwm-utils-src-${V} > > PKGNAME = emwm-utils-${V} > > -REVISION = 0 > > +REVISION = 1 > > > > CATEGORIES = x11 > > HOMEPAGE = https://fastestcode.org/emwm.html > > diff --git a/patches/patch-src_smmain_c b/patches/patch-src_smmain_c > > index cf6ad7122bf..32ca62c615b 100644 > > --- a/patches/patch-src_smmain_c > > +++ b/patches/patch-src_smmain_c > > @@ -61,21 +61,18 @@ Index: src/smmain.c > > > > memset(pwb,0,strlen(pwb)); > > XmTextFieldSetString(wpasswd,""); > > -@@ -988,7 +1009,8 @@ static Boolean set_privileges(Boolean elevate) > > +@@ -988,10 +1009,13 @@ static Boolean set_privileges(Boolean > > elevate) if(!initialized){ > > orig_uid = geteuid(); > > orig_gid = getegid(); > > - > > -+ /* BSD-auth handles authentication, no SUID > > needed. */ -+#ifndef __OpenBSD__ > > if(orig_uid != 0){ > > ++ /* BSD-auth handles authentication, no > > SUID needed. */ ++#ifndef __OpenBSD__ > > log_msg("%s must be setuid root to enable > > " "screen locking capabilities.\n",bin_name); > > -@@ -996,6 +1018,7 @@ static Boolean set_privileges(Boolean elevate) > > ++#endif /* __OpenBSD__ */ > > ++ > > + initialized = True; > > can_elevate = False; > > return False; > > - } > > -+#endif /* __OpenBSD__ */ > > - initialized = True; > > - can_elevate = True; > > - } > > First of all, does this solve anything? It looks correct and makes > sense, but it doesn't really change the execution flow as it isn't > installed suid. orig_[gu]id get assigned the running user / group ID > and then the calls for sete[gu]id succeed. Notice how/where can_elevate is set. The conditional checks if we are*not* running as UID 0, or in other words, a regular user, so that block inside it will always run. If I wrap if #ifndef around the full conditional, it sets can_elevate to True, which means that it thinks SUID-root is set, which is wrong. If I just wrap it around the log_msg() call, can_elevate is set to False, this is correct, as yes, since its not SUID root, it cannot elevate. Additionally, if all I need to do is patch out the log_msg(), that's all I'd need to wrap around, right? > > diff --git a/patches/patch-src_smmain_c.orig > > b/patches/patch-src_smmain_c.orig new file mode 100644 > > index 00000000000..cf6ad7122bf > > --- /dev/null > > +++ b/patches/patch-src_smmain_c.orig > > Are you sure this is the one you wanted to include in here? It creates > an additional file, patches/patch-src_smmain_c.orig, which won't be > used for patching the port. Is there a part for > patches/patch-src_smmain_c missing in this mail or is this inclusion > an overlook? Oversight. I forgot to clean up after a make update-patches A fixed diff with this removed is attached. > > @@ -0,0 +1,81 @@ > > +# For bsd-auth support > > + > > +Index: src/smmain.c > > +--- src/smmain.c.orig > > ++++ src/smmain.c > > +@@ -60,6 +60,9 @@ > > + #if defined(__linux__) || defined(__svr4__) > > + #include > > + #include > > ++#elif defined(__OpenBSD__) > > ++#include > > ++#include > > + #endif > > + #include "smglobal.h" > > + #include "smconf.h" > > +@@ -419,6 +422,11 @@ static void lock_screen(void) > > + app_res.enable_locking = False; > > + return; > > + } > > ++ > > ++#ifdef __OpenBSD__ > > ++ /* BSD-auth handles authentication, no password hash > > check needed */ ++ can_auth = True; > > ++#else > > + > > + if(set_privileges(True)) { > > + > > +@@ -441,7 +449,8 @@ static void lock_screen(void) > > + > > + set_privileges(False); > > + } > > +- > > ++#endif /* __OpenBSD__ */ > > ++ > > + if(!can_auth){ > > + if(!app_res.silent) XBell(XtDisplay(wshell), 100); > > + log_msg("Cannot authenticate. Screen locking > > disabled!\n"); +@@ -861,6 +870,17 @@ static void > > passwd_enter_cb(Widget w, > > + char *upw = NULL; > > + > > + login = get_login(); > > ++ > > ++#ifdef __OpenBSD__ > > ++ int auth_result = auth_userokay(login, NULL, "auth-xmsm", > > pwb); ++ if (auth_result) { > > ++ unlock_screen(); > > ++ set_unlock_message(NULL); > > ++ } else { > > ++ if(!app_res.silent) XBell(XtDisplay(w), 100); > > ++ set_unlock_message(MSG_NOACCESS); > > ++ } > > ++#else > > + > > + set_privileges(True); > > + > > +@@ -896,6 +916,7 @@ static void passwd_enter_cb(Widget w, > > + if(!app_res.silent) XBell(XtDisplay(w),100); > > + set_unlock_message(MSG_NOACCESS); > > + } > > ++#endif /* __OpenBSD__ */ > > + > > + memset(pwb,0,strlen(pwb)); > > + XmTextFieldSetString(wpasswd,""); > > +@@ -988,7 +1009,8 @@ static Boolean set_privileges(Boolean elevate) > > + if(!initialized){ > > + orig_uid = geteuid(); > > + orig_gid = getegid(); > > +- > > ++ /* BSD-auth handles authentication, no SUID > > needed. */ ++#ifndef __OpenBSD__ > > + if(orig_uid != 0){ > > + log_msg("%s must be setuid root to enable > > " > > + "screen locking > > capabilities.\n",bin_name); +@@ -996,6 +1018,7 @@ static Boolean > > set_privileges(Boolean elevate) > > + can_elevate = False; > > + return False; > > + } > > ++#endif /* __OpenBSD__ */ > > + initialized = True; > > + can_elevate = True; > > + } > -- iz (she/her) > i like to say mundane things, > there are too many uninteresting things > that go unnoticed. izder456 (dot) neocities (dot) org diff --git a/Makefile b/Makefile index 6a5e5822b36..127fceca498 100644 --- a/Makefile +++ b/Makefile @@ -3,7 +3,7 @@ COMMENT = session manager and a toolchest-like application launcher V = 1.3 DISTNAME = emwm-utils-src-${V} PKGNAME = emwm-utils-${V} -REVISION = 0 +REVISION = 1 CATEGORIES = x11 HOMEPAGE = https://fastestcode.org/emwm.html diff --git a/patches/patch-src_smmain_c b/patches/patch-src_smmain_c index cf6ad7122bf..32ca62c615b 100644 --- a/patches/patch-src_smmain_c +++ b/patches/patch-src_smmain_c @@ -61,21 +61,18 @@ Index: src/smmain.c memset(pwb,0,strlen(pwb)); XmTextFieldSetString(wpasswd,""); -@@ -988,7 +1009,8 @@ static Boolean set_privileges(Boolean elevate) +@@ -988,10 +1009,13 @@ static Boolean set_privileges(Boolean elevate) if(!initialized){ orig_uid = geteuid(); orig_gid = getegid(); - -+ /* BSD-auth handles authentication, no SUID needed. */ -+#ifndef __OpenBSD__ if(orig_uid != 0){ ++ /* BSD-auth handles authentication, no SUID needed. */ ++#ifndef __OpenBSD__ log_msg("%s must be setuid root to enable " "screen locking capabilities.\n",bin_name); -@@ -996,6 +1018,7 @@ static Boolean set_privileges(Boolean elevate) ++#endif /* __OpenBSD__ */ ++ + initialized = True; can_elevate = False; return False; - } -+#endif /* __OpenBSD__ */ - initialized = True; - can_elevate = True; - }