On Mon, Aug 22, 2016 at 03:15:07PM +0200, Markus Teich wrote: > Heyho, > > FRIGN pointed me to this[0] yesterday. > > > Vendor was notified about this issue on 2015-11-13. > > I could not find a mail related to this on the suckless mailing lists for that > date, but hiro mentioned an unspecific issue which might be related on > 2015-09-09: > > > I tried slock on a computer with some crazy network credential system and > > entering a password results in segfault when the network has an outage. It's > > horrible. > > Now to the proposed fix[1]: It surely solves the issue in this specific case, > but a too short sp_pwdp as argument is not the only way for crypt() to return > NULL. > > The salt might contain bytes not from the valid range [a-zA-Z0-9./]. Should we > check for that? What about the EPERM failure? > > I think a good compromise is to check the return of the crypt() call and if it > is NULL, we handle it as if the wrong password was entered and print out a > warning. This way the screen still stays locked on failure and the user has > the > chance to discover the failure if he is logging the output somewhere. > > The check if the salt is 2 byte and those are in the valid range can of course > be done in advance before actually locking the screen. > > --Markus > > 0: http://seclists.org/oss-sec/2016/q3/333 > 1: http://s1m0n.dft-labs.eu/files/slock/slock.c.patch >
I agree, it's better to check if crypt() returns NULL. Maybe something like this (untested): diff --git a/slock.c b/slock.c index a00fbb9..1209f20 100644 --- a/slock.c +++ b/slock.c @@ -121,7 +121,7 @@ readpw(Display *dpy) readpw(Display *dpy, const char *pws) #endif { - char buf[32], passwd[256]; + char buf[32], passwd[256], *encrypted; int num, screen; unsigned int len, color; KeySym ksym; @@ -157,7 +157,11 @@ readpw(Display *dpy, const char *pws) #ifdef HAVE_BSD_AUTH running = !auth_userokay(getlogin(), NULL, "auth-xlock", passwd); #else - running = !!strcmp(crypt(passwd, pws), pws); + errno = 0; + if (!(encrypted = crypt(passwd, pws))) + fprintf(stderr, "slock: crypt: %s\n", strerror(errno)); + else + running = !!strcmp(encrypted, pws); #endif if (running) { XBell(dpy, 100); Unrelated, but I also noticed, should we change: running = !auth_userokay(getlogin(), NULL, "auth-xlock", passwd); to: running = !auth_userokay(getlogin(), NULL, "auth-slock", passwd); ? -- Kind regards, Hiltjo