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
