Date: Thu, 13 Oct 2016 13:05:17 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Ed Maste <emaste@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r307148 - in head/lib/libc: gen stdlib Message-ID: <20161013110013.W925@besplex.bde.org> In-Reply-To: <201610121356.u9CDuF1q013531@repo.freebsd.org> References: <201610121356.u9CDuF1q013531@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 12 Oct 2016, Ed Maste wrote:
> Log:
> Add comment on use of abort() in libc
>
> Suggested by: jonathan (in review D8133)
It is almost easier to fix the bugs than add the comment.
> Modified: head/lib/libc/gen/arc4random.c
> ==============================================================================
> --- head/lib/libc/gen/arc4random.c Wed Oct 12 13:51:41 2016 (r307147)
> +++ head/lib/libc/gen/arc4random.c Wed Oct 12 13:56:14 2016 (r307148)
> @@ -144,8 +144,15 @@ arc4_stir(void)
> arc4_init();
> rs_initialized = 1;
> }
> - if (arc4_sysctl(rdat, KEYSIZE) != KEYSIZE)
> - abort(); /* Random sysctl cannot fail. */
> + if (arc4_sysctl(rdat, KEYSIZE) != KEYSIZE) {
> + /*
> + * The sysctl cannot fail. If it does fail on some FreeBSD
> + * derivative or after some future change, just abort so that
> + * the problem will be found and fixed. abort is not normally
> + * suitable for a library but makes sense here.
> + */
> + abort();
> + }
The comment starts by being just wrong:
1. "The" sysctl is not used here. Instead, a wrapper arc4_sysctl() is used.
The wrapper handles some but not all errors.
2. The sysctl can and does fail. It fails on:
- all old kernels mixed with new userlands
- with new kernels, at boot time, before the random device is seeded.
I couldn't get this to happen in practice, but it used to be a large
problem ("Dance fandago on keyboard to unblock"), and source code
still has large warnings about it. Apparently des's preseeding based
on device attach times fixes it for me. I use the new kernels with
3. The sysctl can, or at least used to, return short reads with nonzero
counts. The documentation for this is well hidden, but the
arc4_sysctl() wrapper exists to support short reads, or perhaps just
the special case of short reads of 0, which it handles poorly by
possibly spinning forever.
I couldn't get this to happen either. I think it takes O_NONBLOCK.
With interrupts but without O_NONBLOCK, I just got nice denial of
service attacks with simple dd tests like "dd bs=200m </dev/random
count=10". Each 200m read in this now takes 1.88 seconds on a 4GHz
system and is unkillable in the middile using keyboard SIGINT, but
dd now stops after the first unfilled read better than it used to.
The sysctl probably doesn't support O_NONBLOCK or interrupts, but
this is undocumented and the source code is well obfuscated to hide
this detail. I tried using sysctl(8) with a large count to get a
better/different denial of service using the sysctl, but sysctl(8)
-B <large> silently truncates to 256 bytes.
Then the excuse is wrong. abort() never makes sense in library functions.
Here it gives very confusing errors for the delicate boot-time fandago
case.
Style bugs:
- sentence breaks are 2 spaces in KNF, and all old code in this file follows
that rule.
- 'abort' is not marked up
> arc4_addrandom(rdat, KEYSIZE);
>
>
> Modified: head/lib/libc/stdlib/random.c
> ==============================================================================
> --- head/lib/libc/stdlib/random.c Wed Oct 12 13:51:41 2016 (r307147)
> +++ head/lib/libc/stdlib/random.c Wed Oct 12 13:56:14 2016 (r307148)
> @@ -279,8 +279,15 @@ srandomdev(void)
>
> mib[0] = CTL_KERN;
> mib[1] = KERN_ARND;
> - if (sysctl(mib, 2, state, &len, NULL, 0) == -1 || len != expected)
> + if (sysctl(mib, 2, state, &len, NULL, 0) == -1 || len != expected) {
> + /*
> + * The sysctl cannot fail. If it does fail on some FreeBSD
This is even more broken, since it doesn't have the wrapper.
All the old versions using [_]read() mishandled short reads, but this was
less broken when there was a fallback. For some reason the sysctl wrapper
in arc4random.c alone handled short reads.
> + * derivative or after some future change, just abort so that
> + * the problem will be found and fixed. abort is not normally
> + * suitable for a library but makes sense here.
> + */
> abort();
> + }
>
> if (rand_type != TYPE_0) {
> fptr = &state[rand_sep];
There are also gratuitous namespace differences and bugs. arc4random()
is less standard than random(), so it can use sysctl() without being
technically broken by namespace pollution. But it uses __sysctl(),
and has style bugs to declare this. OTOH, random() is now standard
in POSIX, so it has a reason to use __sysctl(), but it just uses sysctl().
Both should use _sysctlbyname(), and this should be declared in
namespace.h. Old code used _read() instead of read(), but that seems
to be nonsense since read() is more standard than random().
The old code with the fallback to read() was not wrong (except for the
missing short read heandling). It gave portability to old kernels. If
the sysctl were documented, then I think its documentation would say
that it acts like a special case of read (without O_NONBLOCK or EINTR
handling), perhaps on a slightly different device than /dev/random,
except for some technical differences for error reporting. So the read()
is just as secure as the sysctl. But with no documentation, we can't
tell what the differences are.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20161013110013.W925>