Re: [hackers] [slock] [PATCHSET] Some improvements and more security

[FRIGN]

On Sun, 11 Sep 2016 23:24:20 +0200
FRIGN  wrote:

> See attached. Most important is the patch which removes the
> abomination of user $USER which actually poses quite a risk and only
> is done on part of the systems.

So you can test this, do the following

$ unset USER
$ slock
Segmentation fault
$

This is due to getenv() returning NULL which in turn is not well-taken
by the getpwnam_user-function. We don't want it to segfault!

-- 
FRIGN 

[hackers] [slock] [PATCHSET] Some improvements and more security

[FRIGN]

Good evening fellow hackers,

I sat down this evening to write down some patches that have been
floating around in my head for a while.

See attached. Most important is the patch which removes the abomination
of user $USER which actually poses quite a risk and only is done on
part of the systems. The largest one is the one removing global state
of the program to make code audits simpler.

Cheers

FRIGN

-- 
FRIGN 
>From 86c4a4edc72958461c9142d218ac0a285751e708 Mon Sep 17 00:00:00 2001
From: FRIGN 
Date: Sun, 11 Sep 2016 23:08:19 +0200
Subject: [PATCH 1/3] Localize and rework data structures

For a project like slock there is no need to carry around global state.
By moving the three structures to main() it is now clear which functions
modify which state, greatly improving the readability of the code,
especially given slock is a suid program.
---
 slock.c | 62 ++
 1 file changed, 34 insertions(+), 28 deletions(-)

diff --git a/slock.c b/slock.c
index 7127ebe..83f6810 100644
--- a/slock.c
+++ b/slock.c
@@ -33,18 +33,18 @@ enum {
 
 #include "config.h"
 
-typedef struct {
+struct lock {
 	int screen;
 	Window root, win;
 	Pixmap pmap;
 	unsigned long colors[NUMCOLS];
-} Lock;
+};
 
-static Lock **locks;
-static int nscreens;
-static Bool rr;
-static int rrevbase;
-static int rrerrbase;
+struct xrandr {
+	int active;
+	int evbase;
+	int errbase;
+};
 
 static void
 die(const char *errstr, ...)
@@ -123,7 +123,8 @@ getpw(void)
 }
 
 static void
-readpw(Display *dpy, const char *pws)
+readpw(Display *dpy, struct xrandr *rr, struct lock **locks, int nscreens,
+   const char *pws)
 {
 	char buf[32], passwd[256], *encrypted;
 	int num, screen, running, failure;
@@ -194,7 +195,7 @@ readpw(Display *dpy, const char *pws)
 }
 oldc = color;
 			}
-		} else if (rr && ev.type == rrevbase + RRScreenChangeNotify) {
+		} else if (rr->active && ev.type == rr->evbase + RRScreenChangeNotify) {
 			XRRScreenChangeNotifyEvent *rre = (XRRScreenChangeNotifyEvent*)
 			for (screen = 0; screen < nscreens; screen++) {
 if (locks[screen]->win == rre->window) {
@@ -208,7 +209,7 @@ readpw(Display *dpy, const char *pws)
 }
 
 static void
-unlockscreen(Display *dpy, Lock *lock)
+unlockscreen(Display *dpy, struct lock *lock)
 {
 	if(dpy == NULL || lock == NULL)
 		return;
@@ -223,28 +224,31 @@ unlockscreen(Display *dpy, Lock *lock)
 }
 
 static void
-cleanup(Display *dpy)
+cleanup(Display **dpy, struct lock ***locks, int *nscreens)
 {
 	int s;
 
-	for (s = 0; s < nscreens; ++s)
-		unlockscreen(dpy, locks[s]);
+	for (s = 0; s < *nscreens; ++s)
+		unlockscreen(*dpy, (*locks)[s]);
+	*nscreens = 0;
 
-	free(locks);
-	XCloseDisplay(dpy);
+	free(*locks);
+	*locks = NULL;
+	XCloseDisplay(*dpy);
+	*dpy = NULL;
 }
 
-static Lock *
-lockscreen(Display *dpy, int screen)
+static struct lock *
+lockscreen(Display *dpy, struct xrandr *rr, int screen)
 {
 	char curs[] = {0, 0, 0, 0, 0, 0, 0, 0};
 	int i, ptgrab, kbgrab;
-	Lock *lock;
+	struct lock *lock;
 	XColor color, dummy;
 	XSetWindowAttributes wa;
 	Cursor invisible;
 
-	if (dpy == NULL || screen < 0 || !(lock = malloc(sizeof(Lock
+	if (dpy == NULL || screen < 0 || !(lock = malloc(sizeof(struct lock
 		return NULL;
 
 	lock->screen = screen;
@@ -281,7 +285,7 @@ lockscreen(Display *dpy, int screen)
 		/* input is grabbed: we can lock the screen */
 		if (ptgrab == GrabSuccess && kbgrab == GrabSuccess) {
 			XMapRaised(dpy, lock->win);
-			if (rr)
+			if (rr->active)
 XRRSelectInput(dpy, lock->win, RRScreenChangeNotifyMask);
 
 			XSelectInput(dpy, lock->root, SubstructureNotifyMask);
@@ -312,13 +316,15 @@ usage(void)
 
 int
 main(int argc, char **argv) {
+	struct xrandr rr;
+	struct lock **locks;
 	struct passwd *pwd;
 	struct group *grp;
 	uid_t duid;
 	gid_t dgid;
 	const char *pws;
 	Display *dpy;
-	int s, nlocks;
+	int s, nlocks, nscreens;
 
 	ARGBEGIN {
 	case 'v':
@@ -360,16 +366,16 @@ main(int argc, char **argv) {
 		die("slock: setuid: %s\n", strerror(errno));
 
 	/* check for Xrandr support */
-	rr = XRRQueryExtension(dpy, , );
+	rr.active = XRRQueryExtension(dpy, , );
 
 	/* get number of screens in display "dpy" and blank them */
 	nscreens = ScreenCount(dpy);
-	if (!(locks = calloc(nscreens, sizeof(Lock * {
+	if (!(locks = calloc(nscreens, sizeof(struct lock * {
 		XCloseDisplay(dpy);
 		die("slock: out of memory\n");
 	}
 	for (nlocks = 0, s = 0; s < nscreens; s++) {
-		if ((locks[s] = lockscreen(dpy, s)) != NULL)
+		if ((locks[s] = lockscreen(dpy, , s)) != NULL)
 			nlocks++;
 		else
 			break;
@@ -378,7 +384,7 @@ main(int argc, char **argv) {
 
 	/* did we manage to lock everything? */
 	if (nlocks != nscreens) {
-		cleanup(dpy);
+		cleanup(, , );
 		return 1;
 	}
 
@@ -386,7 +392,7 @@ main(int argc, char **argv) {
 	if (argc > 0) {
 		switch (fork()) {
 		case -1:
-			cleanup(dpy);
+			cleanup(, , );
 			die("slock: fork failed: %s\n", strerror(errno));
 		case 

Re: [hackers] [xssstate] Refactor xssstate.c

[Russ]

> does it really simplify the code? I think it makes it less readable
> without arg.h.
 
I agree it does some, but using strcmp allows matching the flags exactly, and 
prevents things like "xssstate -v" being equivilant to "xssstate -vxyz".  And 
since you would only ever use one flag at a time, arg.h seemed like it was 
excessive, but perhaps I was thinking about simplify the wrong way.  I also like
being able to use only one switch statement,  but that's mostly a personal 
preference.  Thanks for the input!

Russ

[hackers] [xssstate] Refactor xssstate.c

[Russ]

Hello all,

I'd like to propose a refactoring of xssstate.c.  It simplifies the
code, removing the need for arg.h, and has strict error checking and
usage rules.  Thoughts?

___
diff --git a/arg.h b/arg.h
deleted file mode 100644
index ba3fb3f..000
--- a/arg.h
+++ /dev/null
@@ -1,48 +0,0 @@
-/*
- * Copy me if you can.
- * by 20h
- */
-
-#ifndef ARG_H__
-#define ARG_H__
-
-extern char *argv0;
-
-/* use main(int argc, char *argv[]) */
-#define ARGBEGIN   for (argv0 = *argv, argv++, argc--;\
-   argv[0] && argv[0][0] == '-'\
-   && argv[0][1];\
-   argc--, argv++) {\
-   char argc_;\
-   char **argv_;\
-   int brk_;\
-   if (argv[0][1] == '-' && argv[0][2] == '\0') {\
-   argv++;\
-   argc--;\
-   break;\
-   }\
-   for (brk_ = 0, argv[0]++, argv_ = argv;\
-   argv[0][0] && !brk_;\
-   argv[0]++) {\
-   if (argv_ != argv)\
-   break;\
-   argc_ = argv[0][0];\
-   switch (argc_)
-#define ARGEND }\
-   }
-
-#define ARGC() argc_
-
-#define EARGF(x)   ((argv[0][1] == '\0' && argv[1] == NULL)?\
-   ((x), abort(), (char *)0) :\
-   (brk_ = 1, (argv[0][1] != '\0')?\
-   ([0][1]) :\
-   (argc--, argv++, argv[0])))
-
-#define ARGF() ((argv[0][1] == '\0' && argv[1] == NULL)?\
-   (char *)0 :\
-   (brk_ = 1, (argv[0][1] != '\0')?\
-   ([0][1]) :\
-   (argc--, argv++, argv[0])))
-
-#endif
diff --git a/xssstate.c b/xssstate.c
index f6e63a8..ad603e1 100644
--- a/xssstate.c
+++ b/xssstate.c
@@ -1,106 +1,68 @@
-/*
- * See LICENSE file for copyright and license details.
- */
-
-#include 
-#include 
-#include 
+/* See LICENSE file for copyright and license details. */
 #include 
-#include 
-#include 
+#include 
+#include 
+#include 
 #include 
 
-#include "arg.h"
-
-char *argv0;
-
-void
-die(const char *errstr, ...) {
+static void
+die(const char *fmt, ...) 
+{
va_list ap;
 
-   va_start(ap, errstr);
-   vfprintf(stderr, errstr, ap);
+   va_start(ap, fmt);
+   vfprintf(stderr, fmt, ap);
va_end(ap);
-   exit(EXIT_FAILURE);
-}
+   fputc('\n', stderr);
 
-void
-usage(void)
-{
-   die("usage: %s [-istv]\n", basename(argv0));
+   exit(1);
 }
 
 int
-main(int argc, char *argv[]) {
-   XScreenSaverInfo *info;
+main(int argc, char *argv[])
+{
+   int base, errbase, idle, state;
Display *dpy;
-   int base, errbase;
-   Bool showstate, showtill, showidle;
-
-   showstate = false;
-   showtill = false;
-   showidle = false;
-
-   ARGBEGIN {
-   case 'i':
-   showidle = true;
-   break;
-   case 's':
-   showstate = true;
-   break;
-   case 't':
-   showtill = true;
-   break;
-   case 'v':
-   die("xssstate-"VERSION", © 2008-2016 xssstate engineers"
-   ", see LICENSE for details.\n");
-   default:
-   usage();
-   } ARGEND;
-
-   if (!showstate && !showtill && !showidle)
-   usage();
-
-   if (!(dpy = XOpenDisplay(0)))
-   die("Cannot open display.\n");
+   XScreenSaverInfo *info;
 
+   if (argc != 2)
+   die("usage: xssstate [-i] [-s] [-t] [-v]");
+   if (!strcmp("-v", argv[1]))
+   die("xssstate-"VERSION);
+   idle = 0;
+   state = 0;
+   if (!strcmp("-i", argv[1])) {
+   idle = 1;
+   } else if (!strcmp("-s", argv[1])) {
+   state = 1;
+   } else if (strcmp("-t", argv[1])) {
+   die("usage: xssstate [-i] [-s] [-t] [-v]");
+   }
+   if (!(dpy = XOpenDisplay(NULL)))
+   die("xssstate: cannot open display");
if (!XScreenSaverQueryExtension(dpy, , ))
-   die("Screensaver extension not activated.\n");
-
-   info = XScreenSaverAllocInfo();
-   XScreenSaverQueryInfo(dpy, DefaultRootWindow(dpy), info);
-
-   if (showstate) {
-   switch(info->state) {
-   case ScreenSaverOn:
-   printf("on\n");
-   break;
-   case ScreenSaverOff:
-