On Fri, 23 May 2025 02:44:38 +0000
Lucas Gabriel Vuotto <lucas@sexy.is> 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 <crypt.h>
> > + #include <shadow.h>
> > ++#elif defined(__OpenBSD__)
> > ++#include <bsd_auth.h>
> > ++#include <login_cap.h>
> > + #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;
- 	}

• emwm-utils-1.3p1.diff (1K)