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

From: Markus Teich <markus.teich_AT_stusta.mhn.de>
Date: Tue, 20 Sep 2016 09:25:58 +0200

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
Received on Tue Sep 20 2016 - 09:25:58 CEST