On Sat, Jul 30, 2016 at 07:03:56PM +0200, Theo Buehler wrote:
> On Sat, Jul 30, 2016 at 06:30:40PM +0200, Joerg Jung wrote:
> >
> > > Am 30.07.2016 um 18:07 schrieb Theo Buehler <t...@math.ethz.ch>:
> > >
> > > Currently, slock only clears the entered passwd buffer as part of
> > > auth_userokay(3). If the user aborts the password entry with ESC or
> > > clears the entered password with multiple backspaces, a cleartext
> > > copy of the entered password is kept in memory. Use explicit_bzero()
> > > to avoid this.
> >
> > I think this is a useful addition.
> >
> > You should try to bring this patch upstream (but be
> > prepared for flames).
>
> Thanks, I will try to do that when I have a patch that compiles without
> additional warnings.
>
> > slock already contains various ifdef's so maybe add
> > an #ifdef __OpenBSD__ around the explicit_bzero()
> > because AFAIK it is non-portable.
>
> Right, added.
As noted by jca and theo, my that was bad advice and you should handle
that different.
> > > While there, add __dead and printf attributes to the die() function.
> > >
> > >
> > > I've been running with this patch for quite a while (> 3 months) with no
> > > issues on amd64 and macppc.
> > >
> > > The port runs just fine, but when I compile it, I get the following
> > > warning which I don't know how to fix properly:
> >
> > > CFLAGS = -O2 -pipe -std=c99 -pedantic -Wall -I/usr/X11R6/include
> > > -DVERSION="1.3" -DHAVE_BSD_AUTH
> > > LDFLAGS = -L/usr/X11R6/lib -lX11 -lXext -lXrandr
> > > CC = cc
> > > creating config.h from config.def.h
> > > cc -c -O2 -pipe -std=c99 -pedantic -Wall -I/usr/X11R6/include
> > > -DVERSION=\"1.3\" -DHAVE_BSD_AUTH slock.c
> > > In file included from /usr/X11R6/include/X11/Xlib.h:47,
> > > from /usr/X11R6/include/X11/extensions/Xrender.h:28,
> > > from /usr/X11R6/include/X11/extensions/Xrandr.h:33,
> > > from slock.c:16:
> > > /usr/X11R6/include/X11/Xfuncproto.h:159:24: warning: ISO C does not
> > > permit named variadic macros
> > > slock.c: In function 'readpw':
> > > slock.c:175: warning: implicit declaration of function 'explicit_bzero'
> > >
> > > Could someone help me with fixing that, please?
> >
> > The explicit prototype for the function is missing,
> > try adding #include <strings.h>
>
> Thanks. That's what I first thought as well, I should have mentioned
> that. The prototype of explicit_bzero is in <string.h> which is already
> included by slock.c. The problem seems to be that it is guarded by
>
> #if __BSD_VISIBLE
>
> and I don't know why that isn't defined and what the proper Makefile
> incantations (or CFLAGS?) are in this situation.
AFAIK, that should be: -D_BSD_SOURCE in CFLAGS then.
> Index: Makefile
> ===================================================================
> RCS file: /cvs/ports/x11/slock/Makefile,v
> retrieving revision 1.14
> diff -u -p -r1.14 Makefile
> --- Makefile 6 Jul 2016 21:34:15 -0000 1.14
> +++ Makefile 30 Jul 2016 16:59:04 -0000
> @@ -3,7 +3,7 @@
> COMMENT= simple X screen locker
>
> DISTNAME= slock-1.3
> -REVISION= 0
> +REVISION= 1
>
> CATEGORIES= x11
>
> Index: patches/patch-slock_c
> ===================================================================
> RCS file: patches/patch-slock_c
> diff -N patches/patch-slock_c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ patches/patch-slock_c 30 Jul 2016 16:59:04 -0000
> @@ -0,0 +1,40 @@
> +$OpenBSD$
> +--- slock.c.orig Fri Feb 12 20:29:02 2016
> ++++ slock.c Sat Jul 30 18:48:34 2016
> +@@ -48,6 +48,9 @@ static int rrevbase;
> + static int rrerrbase;
> +
> + static void
> ++#ifdef __OpenBSD__
> ++ __attribute__((__noreturn__, __format__(printf, 1, 2)))
> ++#endif
> + die(const char *errstr, ...)
> + {
> + va_list ap;
> +@@ -172,10 +175,16 @@ readpw(Display *dpy, const char *pws)
> + break;
> + case XK_Escape:
> + len = 0;
> ++#ifdef __OpenBSD__
> ++ explicit_bzero(passwd, sizeof(passwd));
> ++#endif
> + break;
> + case XK_BackSpace:
> + if (len)
> + --len;
> ++#ifdef __OpenBSD__
> ++ explicit_bzero(passwd + len, 1);
> ++#endif
> + break;
> + default:
> + if (num && !iscntrl((int)buf[0]) && (len + num
> < sizeof(passwd))) {
> +@@ -185,6 +194,9 @@ readpw(Display *dpy, const char *pws)
> + break;
> + }
> + color = len ? INPUT : (failure || failonclear ? FAILED
> : INIT);
> ++#ifdef __OpenBSD__
> ++ explicit_bzero(buf, sizeof(buf));
> ++#endif
> + if (running && oldc != color) {
> + for (screen = 0; screen < nscreens; screen++) {
> + XSetWindowBackground(dpy,
> locks[screen]->win, locks[screen]->colors[color]);
