On (31/05/2012 21:48), Pawel Jakub Dawidek wrote:> As learned on someone else's mistakes, I'd like to ask for a review
of
> those changes related to random data handling:
>
> http://people.freebsd.org/~pjd/patches/libc_arc4random.c.patch
> http://people.freebsd.org/~pjd/patches/openssl_rand_unix.c.patch
>
> The first patch changes arc4random() to use sysctl to obtain random data
> instead of opening /dev/random. The main reason here is to make it more
> sandbox-friendly. Once closed in sandbox, a process can no longer open
> files, so it has no access to proper random data. As a side-effect it
> should be a bit faster as instead of three system calls (open, read and
> close) we use only one (__sysctl).
>
> The second patch enables the use of libc's arc4random(3) in OpenSSL.
While at it, did you consider replacing default homegrown OpenSSL random
generator (ssleay_rand_*) with something standard (this "hash
uninitialized user buffer to increase entropy" thing makes me nervous,
which was also the source of well known Debian RSA key generation issue).
There is standard (ANSI X9.31 A.2.4) AES-based implementation under
openssl/fips/rand. Replacing fips_get_dt with our arc4random_buf() looks
straightforward. It may be performance improvement as well, considering
both OpenSSL and hardware support AESNI. Or simply replace the whole
thing with arc4random_*..
It's common practice to put internal/compat syscall declarations into .c
file itself in libc (like __sysctl you did). Handling such cases becomes
a disaster if syscall changes. Why not move declaration to
include/libc_private.h?
Patches are good to commit, IMHO.
> After implementing the first one I found that OpenBSD's arc4random(3)
> also uses sysctl, but without fall back to /dev/random.
>
> --
> Pawel Jakub Dawidek http://www.wheelsystems.com
> FreeBSD committer http://www.FreeBSD.org
> Am I Evil? Yes, I Am! http://tupytaj.pl