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 <[email protected]>
>From 86c4a4edc72958461c9142d218ac0a285751e708 Mon Sep 17 00:00:00 2001 From: FRIGN <[email protected]> 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*)&ev; 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, &rrevbase, &rrerrbase); + rr.active = XRRQueryExtension(dpy, &rr.evbase, &rr.errbase); /* 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, &rr, 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(&dpy, &locks, &nscreens); return 1; } @@ -386,7 +392,7 @@ main(int argc, char **argv) { if (argc > 0) { switch (fork()) { case -1: - cleanup(dpy); + cleanup(&dpy, &locks, &nscreens); die("slock: fork failed: %s\n", strerror(errno)); case 0: if (close(ConnectionNumber(dpy)) < 0) @@ -399,10 +405,10 @@ main(int argc, char **argv) { } /* everything is now blank. Wait for the correct password */ - readpw(dpy, pws); + readpw(dpy, &rr, locks, nscreens, pws); /* password ok, unlock everything and quit */ - cleanup(dpy); + cleanup(&dpy, &locks, &nscreens); return 0; } -- 2.7.3
>From 6f284f5719cf06d2197f71f9e97d31a127cae62d Mon Sep 17 00:00:00 2001 From: FRIGN <[email protected]> Date: Sun, 11 Sep 2016 23:10:57 +0200 Subject: [PATCH 2/3] Rename getpw() and pws to gethash() and hash --- slock.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/slock.c b/slock.c index 83f6810..e231ce6 100644 --- a/slock.c +++ b/slock.c @@ -85,7 +85,7 @@ dontkillme(void) #endif static const char * -getpw(void) +gethash(void) { const char *rval; struct passwd *pw; @@ -124,7 +124,7 @@ getpw(void) static void readpw(Display *dpy, struct xrandr *rr, struct lock **locks, int nscreens, - const char *pws) + const char *hash) { char buf[32], passwd[256], *encrypted; int num, screen, running, failure; @@ -161,10 +161,10 @@ readpw(Display *dpy, struct xrandr *rr, struct lock **locks, int nscreens, case XK_Return: passwd[len] = 0; errno = 0; - if (!(encrypted = crypt(passwd, pws))) + if (!(encrypted = crypt(passwd, hash))) fprintf(stderr, "slock: crypt: %s\n", strerror(errno)); else - running = !!strcmp(encrypted, pws); + running = !!strcmp(encrypted, hash); if (running) { XBell(dpy, 100); failure = True; @@ -322,7 +322,7 @@ main(int argc, char **argv) { struct group *grp; uid_t duid; gid_t dgid; - const char *pws; + const char *hash; Display *dpy; int s, nlocks, nscreens; @@ -350,8 +350,8 @@ main(int argc, char **argv) { dontkillme(); #endif - pws = getpw(); - if (strlen(pws) < 2) + hash = gethash(); + if (strlen(hash) < 2) die("slock: failed to get user password hash.\n"); if (!(dpy = XOpenDisplay(NULL))) @@ -405,7 +405,7 @@ main(int argc, char **argv) { } /* everything is now blank. Wait for the correct password */ - readpw(dpy, &rr, locks, nscreens, pws); + readpw(dpy, &rr, locks, nscreens, hash); /* password ok, unlock everything and quit */ cleanup(&dpy, &locks, &nscreens); -- 2.7.3
>From a503906a983239fbe297d4aac8a2d4b6fb604cf9 Mon Sep 17 00:00:00 2001 From: FRIGN <[email protected]> Date: Sun, 11 Sep 2016 23:17:53 +0200 Subject: [PATCH 3/3] Stop using $USER for shadow entries This was extremely bad practice, effectively making the program behave different depending on which architecture you are running it on. OpenBSD offers getpwuid_shadow, but there is no getspuid for getspnam, so we resort to using the pw_name entry in the struct passwd we filled earlier. This prevents slock from crashing when $USER is empty (easy to do). If you want to run slock as a different user, don't use $ USER="tom" slock but doas or sudo which were designed for this purpose. --- slock.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/slock.c b/slock.c index e231ce6..5348863 100644 --- a/slock.c +++ b/slock.c @@ -87,7 +87,7 @@ dontkillme(void) static const char * gethash(void) { - const char *rval; + const char *hash; struct passwd *pw; /* Check if the current user has a password entry */ @@ -98,28 +98,28 @@ gethash(void) else die("slock: cannot retrieve password entry\n"); } - rval = pw->pw_passwd; + hash = pw->pw_passwd; #if HAVE_SHADOW_H - if (rval[0] == 'x' && rval[1] == '\0') { + if (hash[0] == 'x' && hash[1] == '\0') { struct spwd *sp; - if (!(sp = getspnam(getenv("USER")))) + if (!(sp = getspnam(pw->pw_name))) die("slock: getspnam: cannot retrieve shadow entry (make sure to suid or sgid slock)\n"); - rval = sp->sp_pwdp; + hash = sp->sp_pwdp; } #else - if (rval[0] == '*' && rval[1] == '\0') { + if (hash[0] == '*' && hash[1] == '\0') { #ifdef __OpenBSD__ - if (!(pw = getpwnam_shadow(getenv("USER")))) + if (!(pw = getpwuid_shadow(getuid()))) die("slock: getpwnam_shadow: cannot retrieve shadow entry (make sure to suid or sgid slock)\n"); - rval = pw->pw_passwd; + hash = pw->pw_passwd; #else die("slock: getpwuid: cannot retrieve shadow entry (make sure to suid or sgid slock)\n"); #endif /* __OpenBSD__ */ } #endif /* HAVE_SHADOW_H */ - return rval; + return hash; } static void -- 2.7.3
