Implement privilege separation in xlock(1).

With feedback from stsp@, florian@, op@ ok florian@ op@.

9 files changed, 363 insertions, 10 deletions

diff --git a/app/xlockmore/Makefile.bsd-wrapper b/app/xlockmore/Makefile.bsd-wrapper
index ab89b4e34..4c8361287 100644
--- a/app/xlockmore/Makefile.bsd-wrapper
+++ b/app/xlockmore/Makefile.bsd-wrapper
@@ -1,4 +1,4 @@
-# $OpenBSD: Makefile.bsd-wrapper,v 1.14 2019/12/03 17:44:29 deraadt Exp $
+# $OpenBSD: Makefile.bsd-wrapper,v 1.15 2022/06/26 14:09:51 matthieu Exp $
 
 .include <bsd.xconf.mk>
 
@@ -18,6 +18,7 @@ config.status:
 		--enable-syslog --without-motif --without-ttf \
 		--without-gtk2 --without-gtk --without-esound \
 		--without-rplay --without-ftgl \
-		--without-opengl --without-mesa
+		--without-opengl --without-mesa \
+		--enable-privsep
 
 .include <bsd.xorg.mk>
diff --git a/app/xlockmore/config.h.in b/app/xlockmore/config.h.in
index ff00ecd8e..cd8b0ed1b 100644
--- a/app/xlockmore/config.h.in
+++ b/app/xlockmore/config.h.in
@@ -210,6 +210,9 @@
 /* Define this one when using Esound */
 #undef DEFAULT_SOUND_DIR
 
+/* Define to enable privilege separation */
+#undef USE_PRIVSEP
+
 /* Allows xlock to run in root window (some window managers have problems) */
 #undef USE_VROOT
 
diff --git a/app/xlockmore/configure b/app/xlockmore/configure
index 63dbaa9c4..2a2203c0d 100644
--- a/app/xlockmore/configure
+++ b/app/xlockmore/configure
@@ -793,6 +793,7 @@ enable_maptype
 enable_def_play
 enable_vroot
 enable_allow_root
+enable_privsep
 enable_vtlock
 enable_syslog
 enable_multiple_user
@@ -1492,6 +1493,7 @@ Optional Features:
                           (some window managers have problems)
   --disable-allow-root    allows users to turn off allowroot (default is to
                           always allow root to be able to unlock xlock)
+  --enable-privsep        enable privilege separation
   --enable-vtlock         allows to turn on VT switch lock (default is to be
                           able to switch to another VT)
   --enable-syslog         enable syslog logging
@@ -9455,7 +9457,7 @@ if test "${ac_cv_mesa_version_string+set}" = set; then
   $as_echo_n "(cached) " >&6
 else
   cat > conftest.$ac_ext <<EOF
-#line 9458 "configure"
+#line 9460 "configure"
 #include "confdefs.h"
 #include <GL/xmesa.h>
 configure: XMESA_MAJOR_VERSION XMESA_MINOR_VERSION
@@ -12736,6 +12738,23 @@ _ACEOF
 
 fi
 
+# Check whether --enable-privsep was given.
+if test "${enable_privsep+set}" = set; then
+  enableval=$enable_privsep; use_privsep=$enableval
+else
+  use_privsep=no
+fi
+
+if test "$use_privsep" = yes; then
+  { $as_echo "$as_me:$LINENO: result: enabling privsep" >&5
+$as_echo "enabling privsep" >&6; }
+  cat >>confdefs.h <<\_ACEOF
+#define USE_PRIVSEP 1
+_ACEOF
+
+  XLOCKLIBS="${XLOCKLIBS} -lutil"
+fi
+
 case ${canonical} in
     *-*-linux* | *-*-freebsd* | *-*-openbsd* | *-*-netbsd* | *-*-dragonfly* )
       # Check whether --enable-vtlock was given.
diff --git a/app/xlockmore/configure.in b/app/xlockmore/configure.in
index 5bfe256c6..920641bf5 100644
--- a/app/xlockmore/configure.in
+++ b/app/xlockmore/configure.in
@@ -3747,6 +3747,13 @@ if test "$allow_root" = yes; then
   AC_DEFINE(ALWAYS_ALLOW_ROOT)
 fi
 
+AC_ARG_ENABLE(privsep, [  --enable-privsep        enable privilege separation], use_privsep=$enableval,use_privsep=no)
+if test "$use_privsep" = yes; then
+  AC_MSG_RESULT([enabling privsep])
+  AC_DEFINE(USE_PRIVSEP)
+  XLOCKLIBS="${XLOCKLIBS} -lutil"
+fi
+
 case ${canonical} in
     *-*-linux* | *-*-freebsd* | *-*-openbsd* | *-*-netbsd* | *-*-dragonfly* )
       AC_ARG_ENABLE(vtlock, [  --enable-vtlock         allows to turn on VT switch lock (default is to be
diff --git a/app/xlockmore/modes/Makefile.in b/app/xlockmore/modes/Makefile.in
index 512a54c9e..969ec46b9 100644
--- a/app/xlockmore/modes/Makefile.in
+++ b/app/xlockmore/modes/Makefile.in
@@ -48,7 +48,7 @@ CXG = $(CX) $(DG)
 # S as the separator for object code
 
 # List of object files
-XLOCKUTILOBJS = $(DOU)xlock$(OU)passwd$(OU)resource$(OU)parsecmd$(O)$(S)\
+XLOCKUTILOBJS = $(DOU)xlock$(OU)passwd$(OU)privsep$(OU)resource$(OU)parsecmd$(O)$(S)\
 $(DOU)util$(OU)logout$(OU)mode$(OU)xlockimage$(OU)ras$(OU)xbm$(O)$(S)\
 $(DOU)vis$(OU)visgl$(OU)color$(OU)random$(OU)iostuff$(OU)automata$(O)$(S)\
 $(DOU)spline$(OU)sound$(OU)erase$(OU)magick$(O)$(S)\
diff --git a/app/xlockmore/xlock/Makefile.in b/app/xlockmore/xlock/Makefile.in
index 2195919e4..41fde0818 100644
--- a/app/xlockmore/xlock/Makefile.in
+++ b/app/xlockmore/xlock/Makefile.in
@@ -38,7 +38,7 @@ CU = $(C) $(DU)
 @CHECK@XLOCKCHECKOBJS = $(DOU)memcheck$(O)
 
 # List of object files
-XLOCKUTILOBJS = $(DOU)xlock$(OU)passwd$(OU)resource$(OU)parsecmd$(O)$(S)\
+XLOCKUTILOBJS = $(DOU)xlock$(OU)passwd$(OU)privsep$(OU)resource$(OU)parsecmd$(O)$(S)\
 $(DOU)util$(OU)logout$(OU)mode$(OU)xlockimage$(OU)ras$(OU)xbm$(O)$(S)\
 $(DOU)vis$(OU)visgl$(OU)color$(OU)random$(OU)iostuff$(OU)automata$(O)$(S)\
 $(DOU)spline$(OU)sound$(OU)erase$(OU)magick$(O)$(S)\
@@ -48,7 +48,7 @@ XLOCKCHECKSRCS = $(DU)memcheck$(C)
 
 XLOCKUTILHDRS = xlock.h mode.h vroot.h xlockimage.h ras.h \
 version.h config.h
-XLOCKUTILSRCS = $(DU)xlock$(CU)passwd$(CU)resource$(CU)parsecmd$(C) \
+XLOCKUTILSRCS = $(DU)xlock$(CU)passwd$(CU)privsep$(CU)resource$(CU)parsecmd$(C) \
 $(DU)util$(CU)logout$(CU)mode$(CU)xlockimage$(CU)ras$(CU)xbm$(C) \
 $(DU)vis$(CU)visgl$(CU)color$(CU)random$(CU)iostuff$(CU)automata$(C) \
 $(DU)spline$(CU)sound$(CU)erase$(CU)magick$(C) \
diff --git a/app/xlockmore/xlock/passwd.c b/app/xlockmore/xlock/passwd.c
index e24b97dbc..914db414f 100644
--- a/app/xlockmore/xlock/passwd.c
+++ b/app/xlockmore/xlock/passwd.c
@@ -72,10 +72,14 @@ extern char *cpasswd;
 #include <pwd.h>
 #endif
 
-#if defined( __bsdi__ ) && _BSDI_VERSION >= 199608 || defined(__OpenBSD__)
+#if defined( __bsdi__ ) && _BSDI_VERSION >= 199608
 #define       BSD_AUTH
 #endif
 
+#ifdef USE_PRIVSEP
+extern int priv_pw_check(char *, char *, char *);
+#endif
+
 #ifdef        BSD_AUTH
 #include <login_cap.h>
 #include <bsd_auth.h>
@@ -1272,7 +1276,20 @@ checkPasswd(char *buffer)
 
 #else /* !ultrix */
 
-#ifdef BSD_AUTH
+#ifdef USE_PRIVSEP
+	char	*pass;
+	char	*style;
+
+	/* buffer can be in the form style:pass */
+	if ((pass = strchr(buffer, ':')) != NULL) {
+	    *pass++ = '\0';
+	    style = buffer;
+	} else {
+	    pass = buffer;
+	    style = NULL;
+	}
+	return priv_pw_check(user, pass, style);
+#elif defined(BSD_AUTH)
 	char       *pass;
 	char       *style;
 	char       *name;
diff --git a/app/xlockmore/xlock/privsep.c b/app/xlockmore/xlock/privsep.c
new file mode 100644
index 000000000..7fdf4f11d
--- /dev/null
+++ b/app/xlockmore/xlock/privsep.c
@@ -0,0 +1,295 @@
+/* $OpenBSD: privsep.c,v 1.1 2022/06/26 14:09:51 matthieu Exp $ */
+/*
+ * Copyright 2001 Niels Provos <provos@citi.umich.edu>
+ * All rights reserved.
+ *
+ * Copyright (c) 2021 Matthieu Herrb
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ *    - Redistributions of source code must retain the above copyright
+ *	notice, this list of conditions and the following disclaimer.
+ *    - Redistributions in binary form must reproduce the above
+ *	copyright notice, this list of conditions and the following
+ *	disclaimer in the documentation and/or other materials provided
+ *	with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
+ * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
+ * COPYRIGHT HOLDERS OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT,
+ * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING,
+ * BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT
+ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN
+ * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+ * POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#ifdef HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#ifdef USE_PRIVSEP
+#include <sys/queue.h>
+#include <sys/socket.h>
+#include <err.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <imsg.h>
+#include <paths.h>
+#include <pwd.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syslog.h>
+#include <unistd.h>
+
+#include <login_cap.h>
+#include <bsd_auth.h>
+
+
+/* Command */
+enum xlock_cmd {
+	XLOCK_CHECKPW_CMD,
+	XLOCK_CHECKPW_RESULT
+};
+
+struct priv_cmd_hdr {
+	size_t namelen;
+	size_t passlen;
+	size_t stylelen;
+};
+
+static struct imsgbuf parent_ibuf, child_ibuf;
+static int priv_inited = 0;
+
+static int
+pw_check(char *name, char *pass, char *style)
+{
+	int authok;
+
+	authok = auth_userokay(name, style, "auth-xlock", pass) ||
+		auth_userokay("root", style, "auth-xlock", pass);
+	if (authok)
+		syslog(LOG_NOTICE, "%s: %s unlocked screen", "xlock", name);
+	return authok;
+}
+
+static int
+send_cmd(struct imsgbuf *ibuf, char *user, char *pass, char *style)
+{
+	size_t n, datalen = 0;
+	struct ibuf *wbuf;
+	struct priv_cmd_hdr hdr;
+
+	memset(&hdr, 0, sizeof(struct priv_cmd_hdr));
+	hdr.namelen = strlen(user) + 1;
+	hdr.passlen = strlen(pass) + 1;
+	if (style != NULL)
+		hdr.stylelen = strlen(style) + 1;
+
+	datalen = sizeof(struct priv_cmd_hdr) + hdr.namelen +
+		hdr.passlen + hdr.stylelen;
+
+	wbuf = imsg_create(ibuf, XLOCK_CHECKPW_CMD, 0, 0, datalen);
+	if (wbuf == NULL) {
+		warn("imsg_create");
+		return -1;
+	}
+	if (imsg_add(wbuf, &hdr, sizeof(struct priv_cmd_hdr)) == -1) {
+		warn("imsg_add");
+		return -1;
+	}
+	if (imsg_add(wbuf, user, hdr.namelen) == -1) {
+		warn("imsg_add");
+		return -1;
+	}
+	if (imsg_add(wbuf, pass, hdr.passlen) == -1) {
+		warn("imsg_add");
+		return -1;
+	}
+	if (style != NULL)
+		if (imsg_add(wbuf, style, hdr.stylelen) == -1) {
+			warn("imsg_add");
+			return -1;
+		}
+	wbuf->fd = -1;
+	imsg_close(ibuf, wbuf);
+
+	if ((n = msgbuf_write(&ibuf->w)) == -1 && errno != EAGAIN) {
+		warn("imsg_write");
+		return -1;
+	}
+	if (n == 0)
+		return -1;
+	return 0;
+}
+
+static int
+receive_cmd(struct imsgbuf *ibuf, char **name, char **pass, char **style)
+{
+	struct imsg imsg;
+	struct priv_cmd_hdr hdr;
+	ssize_t n, nread, datalen;
+	void *data;
+
+	if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN) {
+		warn("imsg_read");
+		return -1;
+	}
+	if (nread == 0) {
+		/* parent exited */
+		exit(0);
+	}
+	if ((n = imsg_get(ibuf, &imsg)) == -1) {
+		warnx("imsg_get");
+		return -1;
+	}
+	if (imsg.hdr.type != XLOCK_CHECKPW_CMD) {
+		warnx("invalid command");
+		imsg_free(&imsg);
+		return -1;
+	}
+	datalen = imsg.hdr.len - IMSG_HEADER_SIZE;
+	data = imsg.data;
+	if (datalen < sizeof(struct priv_cmd_hdr)) {
+		warnx("truncated header");
+		imsg_free(&imsg);
+		return -1;
+	}
+	memcpy(&hdr, data, sizeof(struct priv_cmd_hdr));
+	if (datalen != sizeof(hdr) + hdr.namelen + hdr.passlen + hdr.stylelen) {
+		warnx("truncated strings");
+		imsg_free(&imsg);
+		return -1;
+	}
+	data += sizeof(struct priv_cmd_hdr);
+	*name = strndup(data, hdr.namelen);
+	if (*name == NULL)
+		goto nomem;
+	data += hdr.namelen;
+	*pass = strndup(data, hdr.passlen);
+	if (*pass == NULL)
+		goto  nomem;
+	data += hdr.passlen;
+	if (hdr.stylelen != 0) {
+		*style = strndup(data, hdr.passlen);
+		if (*style == NULL)
+			goto nomem;
+	} else
+		*style = NULL;
+	imsg_free(&imsg);
+	return 0;
+nomem:
+	warn("strndup");
+	imsg_free(&imsg);
+	return -1;
+}
+
+static int
+send_result(struct imsgbuf *ibuf, int result)
+{
+	imsg_compose(ibuf, XLOCK_CHECKPW_RESULT, 0, 0, -1,
+		     &result, sizeof(int));
+	return msgbuf_write(&ibuf->w);
+}
+
+static int
+receive_result(struct imsgbuf *ibuf, int *presult)
+{
+	ssize_t nread, n;
+	int retval = 0;
+	struct imsg imsg;
+
+	if ((nread = imsg_read(ibuf)) == -1 && errno != EAGAIN)
+		return -1;
+	if (nread == 0)
+		return -1;
+	if ((n = imsg_get(ibuf, &imsg)) == -1)
+		return -1;
+	if (imsg.hdr.len - IMSG_HEADER_SIZE != sizeof(int))
+		retval = -1;
+	if (imsg.hdr.type != XLOCK_CHECKPW_RESULT)
+		retval = -1;
+	memcpy(presult, imsg.data, sizeof(int));
+	imsg_free(&imsg);
+	return retval;
+}
+
+int
+priv_init(gid_t gid)
+{
+	pid_t pid;
+	int socks[2], result;
+	char *user, *pass, *style;
+
+	/* Create sockets */
+	if (socketpair(AF_LOCAL, SOCK_STREAM, PF_UNSPEC, socks) == -1) {
+		return -1;
+	}
+	pid = fork();
+	if (pid < 0) {
+		/* can't fork */
+		return -1;
+	}
+	if (pid != 0) {
+		/* parent - drop setgid privileges and return */
+		if (gid != -1) {
+			if (setresgid(gid, gid, gid) == -1)
+				return -1;
+		}
+		close(socks[0]);
+		imsg_init(&parent_ibuf, socks[1]);
+		priv_inited = 1;
+		return 0;
+	}
+	/* child */
+	close(socks[1]);
+
+	setproctitle("[priv]");
+
+	imsg_init(&child_ibuf, socks[0]);
+
+	if (unveil(_PATH_AUTHPROGDIR, "rx") == -1)
+		err(1, "unveil");
+	if (pledge("stdio rpath getpw proc exec", NULL) == -1)
+		err(1, "pledge");
+
+	while (1) {
+		if (receive_cmd(&child_ibuf, &user, &pass, &style) == -1) {
+			warn("receive_cmd");
+			result = 0;
+		} else
+			result = pw_check(user, pass, style);
+		freezero(user, strlen(user));
+		freezero(pass, strlen(pass));
+		free(style);
+		if (send_result(&child_ibuf, result) == -1)
+			warn("send_result");
+	}
+}
+
+int
+priv_pw_check(char *user, char *pass, char *style)
+{
+	int result;
+
+	if (priv_inited != 0) {
+		if (send_cmd(&parent_ibuf, user, pass, style) == -1) {
+			warn("send_cmd");
+			return 0;
+		}
+		if (receive_result(&parent_ibuf, &result) == -1) {
+			warn("receive_result");
+			return 0;
+		}
+		return (result);
+	} else
+		return pw_check(user, pass, style);
+}
+#endif
diff --git a/app/xlockmore/xlock/xlock.c b/app/xlockmore/xlock/xlock.c
index e14563d95..5d4ebaf6b 100644
--- a/app/xlockmore/xlock/xlock.c
+++ b/app/xlockmore/xlock/xlock.c
@@ -649,6 +649,11 @@ extern int  dpmsoff;
 
 #endif
 
+#ifdef USE_PRIVSEP
+extern int priv_init(gid_t);
+#endif
+	
+
 #ifdef HAVE_KRB5
 #include <krb5.h>
 #endif /* HAVE_KRB5 */
@@ -2628,6 +2633,7 @@ getPassword(void)
 			(void) printf("failed unlock attempt on user %s\n", user);
 			syslog(SYSLOG_NOTICE, "%s: failed unlock attempt on user %s\n",
 			       ProgramName, user);
+			explicit_bzero(buffer, strlen(buffer));
 #endif
 		}
 #endif
@@ -3206,6 +3212,10 @@ main(int argc, char **argv)
 
 	ruid = getuid();
 	rgid = getgid();
+#ifdef USE_PRIVSEP
+	if (priv_init(rgid) ==  -1)
+		exit(2);
+#else
 #ifdef HAVE_SETEUID
 	/* save effective uid and gid for later */
 	euid = geteuid();
@@ -3226,7 +3236,7 @@ main(int argc, char **argv)
 
 #endif
 #endif
-
+#endif
 	ProgramName = strrchr(argv[0], '/');
 	if (ProgramName)
 		ProgramName++;
@@ -3245,6 +3255,7 @@ main(int argc, char **argv)
 
 	getResources(&dsp, argc, argv);
 
+#ifndef USE_PRIVSEP
 #ifdef HAVE_SETEUID
 	/* become root to get the password */
 	(void) setegid(egid);
@@ -3257,7 +3268,7 @@ main(int argc, char **argv)
 
 #endif
 #endif
-
+#endif
 	initPasswd();
 
 /* revoke root privs, if there were any */