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__.
>> > 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
