Hello fellow hackers, I noticed a few weeks ago that slock did not drop privileges on OpenBSD until the last commit. Now, still, the privdropping is horrible and thus I wrote this patch to make it very strict and actually allow the user to specifiy what to drop to and also override the supplementary groups. There really should not be any security compromises with a setuid application like slock.
Cheers FRIGN -- FRIGN <[email protected]>
>From dbb170d18380a4c20c5a074e68a4f481f17d3d5e Mon Sep 17 00:00:00 2001 From: FRIGN <[email protected]> Date: Wed, 7 Sep 2016 13:32:29 +0200 Subject: [PATCH] Ensure Polyphemus-Mitigation and properly drop privileges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don't hide privilege drops inside readpw() and actually make it configurable what you are dropping to in config.h. The privilege drop comes after opening the Display because the user "nobody" with "nogroup" can't do that. So why do I call this strategy the Polyphemus-Mitigation? """ After the giant returns in the evening and eats two more of the men, Odysseus offers Polyphemus some strong and undiluted wine given to him earlier on his journey. Drunk and unwary, the giant asks Odysseus his name, promising him a guest-gift if he answers. Odysseus tells him "Îá½ÏιÏ", which means "nobody" and Polyphemus promises to eat this "Nobody" last of all. With that, he falls into a drunken sleep. Odysseus had meanwhile hardened a wooden stake in the fire and now drives it into Polyphemus' eye. When Polyphemus shouts for help from his fellow giants, saying that "Nobody" has hurt him, they think Polyphemus is being afflicted by divine power and recommend prayer as the answer. """ (source: https://en.wikipedia.org/wiki/Polyphemus) --- config.def.h | 3 +++ config.mk | 2 +- slock.c | 24 +++++++++++++++++++----- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/config.def.h b/config.def.h index eae2d9a..20c1291 100644 --- a/config.def.h +++ b/config.def.h @@ -1,3 +1,6 @@ +static const char *user = "nobody"; +static const char *group = "nogroup"; + static const char *colorname[NUMCOLS] = { "black", /* after initialization */ "#005577", /* during input */ diff --git a/config.mk b/config.mk index 049305c..11357a7 100644 --- a/config.mk +++ b/config.mk @@ -15,7 +15,7 @@ INCS = -I. -I/usr/include -I${X11INC} LIBS = -L/usr/lib -lc -lcrypt -L${X11LIB} -lX11 -lXext -lXrandr # flags -CPPFLAGS = -DVERSION=\"${VERSION}\" -DHAVE_SHADOW_H +CPPFLAGS = -DVERSION=\"${VERSION}\" -D_DEFAULT_SOURCE -DHAVE_SHADOW_H CFLAGS = -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS} LDFLAGS = -s ${LIBS} COMPATSRC = explicit_bzero.c diff --git a/slock.c b/slock.c index da4b099..9d4086d 100644 --- a/slock.c +++ b/slock.c @@ -6,6 +6,7 @@ #include <ctype.h> #include <errno.h> +#include <grp.h> #include <pwd.h> #include <stdarg.h> #include <stdlib.h> @@ -83,7 +84,6 @@ dontkillme(void) } #endif -/* only run as root */ static const char * getpw(void) { @@ -119,10 +119,6 @@ getpw(void) } #endif /* HAVE_SHADOW_H */ - /* drop privileges */ - if (geteuid() == 0 && - ((getegid() != pw->pw_gid && setgid(pw->pw_gid) < 0) || setuid(pw->pw_uid) < 0)) - die("slock: cannot drop privileges\n"); return rval; } @@ -316,6 +312,8 @@ usage(void) int main(int argc, char **argv) { + struct passwd *pwd; + struct group *grp; const char *pws; Display *dpy; int s, nlocks; @@ -328,6 +326,14 @@ main(int argc, char **argv) { usage(); } ARGEND + /* validate drop-user and -group */ + errno = 0; + if (!(pwd = getpwnam(user))) + die("slock: getpwnam: %s\n", strerror(errno)); + errno = 0; + if (!(grp = getgrnam(group))) + die("slock: getgrnam: %s\n", strerror(errno)); + #ifdef __linux__ dontkillme(); #endif @@ -339,6 +345,14 @@ main(int argc, char **argv) { if (!(dpy = XOpenDisplay(NULL))) die("slock: cannot open display\n"); + /* drop privileges */ + if (setgroups(1, &(grp->gr_gid)) < 0) + die("slock: setgroups: %s\n", strerror(errno)); + if (setgid(grp->gr_gid) < 0) + die("slock: setgid: %s\n", strerror(errno)); + if (setuid(pwd->pw_uid) < 0) + die("slock: setuid: %s\n", strerror(errno)); + /* check for Xrandr support */ rr = XRRQueryExtension(dpy, &rrevbase, &rrerrbase); -- 2.7.3
