On Sat, Jul 30, 2016 at 08:02:56PM +0200, Jeremie Courreges-Anglas wrote:
>
> Theo Buehler <t...@math.ethz.ch> writes:
>
> > 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.
>
> The fact that it is not portable doesn't mean that it isn't / it won't
> be available on other platforms. FreeBSD has it for example.
>
> Those ifdefs don't serve any purpose in the ports tree, will (should)
> be rejected upstream and will do more harm than good in the larger
> ecosystem. You don't want new unix code to look like unix code from the
> 80's.
>
> There are alternatives, eg use a macro that could look like
> CLEAR_PASSWORD(p, len) with a fallback implementation using memset.
Yes, such a macro would be good. There is a similar trivial
explicit_bzero.c fallback in opensmtpd-portable.
> Then, a better implementation would be used if the user appropriately
> adds -DHAVE_EXPLICIT_BZERO to CPPFLAGS in config.mk.
>
> Anyway, please don't add those #ifdef __OpenBSD__.
Agreed. Bad advise from me.
> >> > While there, add __dead and printf attributes to the die() function.
>
> The #ifdef __OpenBSD__ check is unrelated to the gcc attributes support.
> If you want ifdefs here, have a look at /usr/include/sys/cdefs.h.
>
> >> >
> >> >
> >> > 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
>
> Because slock.c starts with #define _XOPEN_SOURCE 500. See sys/cdefs.h.
>
> > and what the proper Makefile
> > incantations (or CFLAGS?) are in this situation.
> >
> > 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]);
> >
>
> --
> jca | PGP: 0x1524E7EE / 5135 92C1 AD36 5293 2BDF DDCC 0DFA 74AE 1524 E7EE
