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.
> 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__.
Thanks for this input, all this makes a lot of sense. I went with your
idea of adding a CLEAR_PASSWORD() macro.
> >> > 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 removed the ifdef's here.
[...]
> > 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.
Okay, now I understand why __BSD_VISIBLE isn't defined, but I'm still
unsure about the proper way of dealing with this. For now I went with
jung@'s suggestion of adding -D_BSD_SOURCE to config.mk, which seems to
be a legitimate way of asking explicitly for __BSD_VISIBLE, if I read
sys/cdefs.h correctly.
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 19:07:26 -0000
@@ -3,7 +3,7 @@
COMMENT= simple X screen locker
DISTNAME= slock-1.3
-REVISION= 0
+REVISION= 1
CATEGORIES= x11
Index: patches/patch-config_mk
===================================================================
RCS file: /cvs/ports/x11/slock/patches/patch-config_mk,v
retrieving revision 1.8
diff -u -p -r1.8 patch-config_mk
--- patches/patch-config_mk 5 Mar 2016 19:17:01 -0000 1.8
+++ patches/patch-config_mk 30 Jul 2016 19:07:26 -0000
@@ -1,7 +1,7 @@
$OpenBSD: patch-config_mk,v 1.8 2016/03/05 19:17:01 jung Exp $
--- config.mk.orig Fri Feb 12 20:29:02 2016
-+++ config.mk Sat Feb 27 10:43:40 2016
-@@ -4,28 +4,28 @@ VERSION = 1.3
++++ config.mk Sat Jul 30 21:05:27 2016
+@@ -4,28 +4,29 @@ VERSION = 1.3
# Customize below to fit your system
# paths
@@ -25,7 +25,8 @@ $OpenBSD: patch-config_mk,v 1.8 2016/03/
-CPPFLAGS = -DVERSION=\"${VERSION}\" -DHAVE_SHADOW_H
-CFLAGS = -std=c99 -pedantic -Wall -Os ${INCS} ${CPPFLAGS}
-LDFLAGS = -s ${LIBS}
-+CPPFLAGS += -DVERSION=\"${VERSION}\" -DHAVE_BSD_AUTH
++CPPFLAGS += -DVERSION=\"${VERSION}\" -DHAVE_BSD_AUTH \
++ -DHAVE_EXPLICIT_BZERO -D_BSD_SOURCE
+CFLAGS += -std=c99 -pedantic -Wall ${INCS} ${CPPFLAGS}
+LDFLAGS += ${LIBS}
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 19:07:26 -0000
@@ -0,0 +1,46 @@
+$OpenBSD$
+--- slock.c.orig Fri Feb 12 20:29:02 2016
++++ slock.c Sat Jul 30 21:04:33 2016
+@@ -23,6 +23,12 @@
+ #include <bsd_auth.h>
+ #endif
+
++#ifdef HAVE_EXPLICIT_BZERO
++#define CLEAR_PASSWORD(p, len) explicit_bzero((p), (len))
++#else
++#define CLEAR_PASSWORD(p, len) memset((p), 0, (len))
++#endif
++
+ enum {
+ INIT,
+ INPUT,
+@@ -47,7 +53,7 @@ static Bool rr;
+ static int rrevbase;
+ static int rrerrbase;
+
+-static void
++static void __attribute__((__noreturn__, __format__(printf, 1, 2)))
+ die(const char *errstr, ...)
+ {
+ va_list ap;
+@@ -172,10 +178,12 @@ readpw(Display *dpy, const char *pws)
+ break;
+ case XK_Escape:
+ len = 0;
++ CLEAR_PASSWORD(passwd, sizeof(passwd));
+ break;
+ case XK_BackSpace:
+ if (len)
+ --len;
++ CLEAR_PASSWORD(passwd + len, 1);
+ break;
+ default:
+ if (num && !iscntrl((int)buf[0]) && (len + num
< sizeof(passwd))) {
+@@ -185,6 +193,7 @@ readpw(Display *dpy, const char *pws)
+ break;
+ }
+ color = len ? INPUT : (failure || failonclear ? FAILED
: INIT);
++ CLEAR_PASSWORD(buf, sizeof(buf));
+ if (running && oldc != color) {
+ for (screen = 0; screen < nscreens; screen++) {
+ XSetWindowBackground(dpy,
locks[screen]->win, locks[screen]->colors[color]);
