Re: [hackers] [slock] CVE-2016-6866 NULL Pointer Dereference Denial of Service Vulnerability(2016-08-19)

From: Hiltjo Posthuma <hiltjo_AT_codemadness.org>
Date: Mon, 22 Aug 2016 20:38:01 +0200

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
_AT_@ -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;
_AT_@ -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
Received on Mon Aug 22 2016 - 20:38:01 CEST

This archive was generated by hypermail 2.3.0 : Mon Aug 22 2016 - 20:48:15 CEST