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

[Markus Teich]

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

Heyho,

thanks for the patches.

> Subject: [PATCH 1/3] Localize and rework data structures
> …
> -typedef struct {
> +struct lock {
>   int screen;
>   Window root, win;
>   Pixmap pmap;
>   unsigned long colors[NUMCOLS];
> -} Lock;
> +};

Removing the typedef is against the coding style, but I like it for slock. If I
remember correctly the arguments were purely cosmetic: brevity (pro typedef) vs.
clarity (explicitly stating `struct bla` each time). Since slock is a security
relevant program I prefer clarity.

>  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;
>  }

The new cleanup function seems to only be a half measure. If you think zeroing
out nscreens and the dpy and locks pointers is worth it even though currently(!)
cleanup() is only called right before die() or return from main, why not do it
right after XOpenDisplay()? There are three die() calls which don't clean dpy
and one which doesn't clean nscreens. Either leave it as is or go all the way,
add checks like `if (*locks)` before cleaning and always use this state
resistant cleanup function.

> Subject: [PATCH 2/3] Rename getpw() and pws to gethash() and hash
> …
> - if (!(encrypted = crypt(passwd, pws)))
> + if (!(encrypted = crypt(passwd, hash)))

Additionally I'd rename `encrypted` to `hashed` to make it clear that crypt() is
just recomputing the hash.

> Subject: [PATCH 3/3] Stop using $USER for shadow entries
> …
> - const char *rval;
> + const char *hash;

This rename belongs in the second patch.

--Markus

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

[FRIGN]

On Mon, 12 Sep 2016 13:13:39 +0200
Quentin Rameau  wrote:

Hey Quentin,

> That was just one example of the rest of the complexity it brings in
> here.

it just stops hiding the fact that readpw() does many things at once.
It would make sense to split this function apart a bit, maybe separating
the hash-checking from the actual X stuff. Having global state skews
your vision and the biggest sin is having functions which do too many
things at once.

> > because it would be inconsistent with the xrandr-struct.
> It would be inconsistent with… An inconsistency you're bringing in? ^^

what other choice do I have? I could of course typedef the xrandr-struct

typedef struct _xrandr {
...
} Xrandr;

but this would be confusing as hell, given people could almost start
believing this struct was part of X.

> As I said, personal taste.

We had the discussion already. Markus decides in the end.

> Not *that* reasoning, as I already said, I already told you I was for
> getting rid of it.
> I mean the reasoning of why it was introduced in the first place, to
> be sure something is not missed here.

It was added in commit 0f1157d[0] and carried through history since
then. I bet back then it was laziness not wanting to run getpwuid to
get the pw_name entry. There is no reasoning and we should definitely
move away from the $USER environment variable, especially because it
makes slock's behaviour inconsistent across operating systems (in some
cases it is sensitive to $USER, in others it is not).

> Get rid of it.

I'll send in a small patch when I have the time.

Cheers

FRIGN

[0]:http://git.suckless.org/slock/commit/slock.c?id=0f1157d7e6cabe0394ffc35a0a58a30507af8ebd

-- 
FRIGN 

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

[Quentin Rameau]

> > First patch:
> > - I agree with the structuring of the xrandr part, good!
> > - But not with the localization of every data, they're used across
> > the whole program and you're just over-complicating functions here
> > imho. The program is simple and clear enough not to have to get
> > triple pointers passed as function parameters.  
> 
> the triple pointer is just used once in the cleanup function, as after
> freeing the locks we set the locks double pointer to NULL.
That was just one example of the rest of the complexity it brings in
here.

> > - un-typedef, that's not an issue here, why changing it besides
> >   satisfying your personal taste?  
> 
> because it would be inconsistent with the xrandr-struct.
It would be inconsistent with… An inconsistency you're bringing in? ^^
> There is also
> no reason to typedef struct anyway and just hides the underlying
> machinery for no good reason.
As I said, personal taste.

> > Second patch:
> > - fine by me, renaming variables patch ;p  
> 
> You wouldn't believe how much difference it makes to improve variable
> naming.
No I wouldn't! I have no clue!

> > Third patch:
> > - the rval renaming belongs to the second patch  
> 
> It's debatable if you think of the second patch as a renaming patch,
Is it not? Does it make more sense to have it in a patch which changes
where we get the user login from?
> but in the end I don't see it as a big issue.
Not really indeed (either way).

> > - I agree with the removal of the $USER, we discussed it before this
> >   patch on IRC. But maybe we should have the reasoning behind that
> > from the original author who put it here.  
> 
> The reasoning is that we don't want a bloody seduid program depending
> so strongly on environment variables. For some things, it's sufficient
> to just kick them out because they are so obviously wrong.
Not *that* reasoning, as I already said, I already told you I was for
getting rid of it.
I mean the reasoning of why it was introduced in the first place, to be
sure something is not missed here.

> What has been bugging me for quite a while is this DPMS comment that
> was added there for no reason.
[…]
Get rid of it.

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

[FRIGN]

On Mon, 12 Sep 2016 12:20:51 +0200
Quentin Rameau  wrote:

Hey Quentin,

> First patch:
> - I agree with the structuring of the xrandr part, good!
> - But not with the localization of every data, they're used across the
>   whole program and you're just over-complicating functions here imho.
>   The program is simple and clear enough not to have to get triple
>   pointers passed as function parameters.

the triple pointer is just used once in the cleanup function, as after
freeing the locks we set the locks double pointer to NULL.

> - un-typedef, that's not an issue here, why changing it besides
>   satisfying your personal taste?

because it would be inconsistent with the xrandr-struct. There is also
no reason to typedef struct anyway and just hides the underlying
machinery for no good reason.

> Second patch:
> - fine by me, renaming variables patch ;p

You wouldn't believe how much difference it makes to improve variable
naming.

> Third patch:
> - the rval renaming belongs to the second patch

It's debatable if you think of the second patch as a renaming patch,
but in the end I don't see it as a big issue.

> - I agree with the removal of the $USER, we discussed it before this
>   patch on IRC. But maybe we should have the reasoning behind that
> from the original author who put it here.

The reasoning is that we don't want a bloody seduid program depending
so strongly on environment variables. For some things, it's sufficient
to just kick them out because they are so obviously wrong.
What has been bugging me for quite a while is this DPMS comment that
was added there for no reason. Every sane mind would agree that
fiddling with DPMS makes no sense whatsoever. When I slock, my screen
turns off after 10 minutes. So, if I don't like that, I disable DPMS.
If I do, I just fiddle around with my mouse a bit and get the slock
promt.
The only issue I see there is if the screen is not locked and one
assumes it is locked and does not wait for the screen to turn back on
and enter his password, possibly unleashing it to the public (IRC chat,
whatever). But this is a minor thing imho. DPMS stays out and there's
no need imho for such a long comment to reflect that.

> Anyway, thanks for the work!

I was happy to do it! :)

Cheers

FRIGN

-- 
FRIGN 

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