Bug 194204 - getentropy(2): sys call from openbsd
Summary: getentropy(2): sys call from openbsd
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Only Me
Assignee: Conrad Meyer
URL: https://reviews.freebsd.org/D14500
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-10-06 21:28 UTC by David CARLIER
Modified: 2018-03-21 01:18 UTC (History)
5 users (show)

See Also:


Attachments
Proposed patch (6.48 KB, patch)
2014-10-06 21:28 UTC, David CARLIER
no flags Details | Diff
Updated patch (12.89 KB, patch)
2014-11-05 15:34 UTC, David CARLIER
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David CARLIER 2014-10-06 21:28:16 UTC
Created attachment 148045 [details]
Proposed patch

it is a proposed simple FreeBSD version of the OpenBSD's
Comment 2 Mateusz Guzik freebsd_committer 2014-11-05 13:08:11 UTC
I'm confused with this patch.

OpenBSD implementation returns EIO for too big buffers, although apparently this was not the behaviour from the start. Previously it filled up to 256 bytes, but also indicated how many bytes were returned.

Your patch fills up to 256 bytes and does not tell the caller how many bytes it got, which is a big no-no.

We should stick to what is in OpenBSD, so please update the patch to return EIO when needed.

memset you put there is buggy. explicit_zero or equivalent is needed and if we don't have that in the kernel one will need to be ported as well.

Cannot comment on arc4rand usefulness for this purpose though.
Comment 3 David CARLIER 2014-11-05 13:17:22 UTC
Alright except that meanwhile I already made the change to use explicit_bzero in the github repo. Otherwise ok I ll update.
Comment 4 David CARLIER 2014-11-05 15:34:01 UTC
Created attachment 149067 [details]
Updated patch

As advised, it returns EIO if the len exceeds 256 bytes. Plus a light update of the related man page.

For reference the original OpenBSD man page.
http://www.openbsd.org/cgi-bin/man.cgi/OpenBSD-current/man2/getentropy.2
Comment 5 Mateusz Guzik freebsd_committer 2014-11-07 06:19:19 UTC
I'm no crypto expert.

Turns out FreeBSD's arc4rand uses RC4, while OpenBSD's version uses something based on ChaCha which is stronger than RC4.

I don't think we should provide a syscall with weaker randomness than one provided with original implementation.
Comment 6 David CARLIER 2014-11-07 06:22:15 UTC
I did as well an updated related to arc4random RC4 => Chacha 20
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=182610
Comment 7 Oliver Pinter freebsd_committer 2014-11-20 07:07:58 UTC
This is a part of private conversation with Theo de Raadt:

OP> Can we somehow prove / verify, that OpenBSD's implementation is fine?
OP> I know, that chacha based arc4random much better than (a)rc4 based,

TdR> It is not significantly better than rc4.
Comment 8 Ed Maste freebsd_committer 2018-02-15 02:28:45 UTC
(In reply to Mateusz Guzik from comment #5)
> I don't think we should provide a syscall with weaker randomness than one
> provided with original implementation.

I think that's the wrong way to look at it; if we have a weak CSPRNG that needs to be fixed, regardless of the mechanism by which userland obtains entropy from kernel.

I believe it is worthwhile for us to add the getentropy syscall, but the approach we use today (the kern.arandom sysctl) is in practice largely equivalent.
Comment 9 Conrad Meyer freebsd_committer 2018-02-15 17:50:07 UTC
Seems like it could be implemented as a thin shim around the sysctl, no?  Why do we need a syscall for it?
Comment 10 Conrad Meyer freebsd_committer 2018-02-15 19:06:02 UTC
Btw, our kernel random has used chacha20 since 2017 -- r317015.  I realize comment #5 dates from 2014, though.
Comment 11 Mateusz Guzik freebsd_committer 2018-02-15 19:34:59 UTC
> I think that's the wrong way to look at it; if we have a weak CSPRNG that needs to be fixed, regardless of the mechanism by which userland obtains entropy from kernel.

The way of looking at it was that if the compatibility mechanism is to be provided it has to be not weaker than OpenBSD equivalent.

Entropy for the rest of the kernel is a very different issue.

> Seems like it could be implemented as a thin shim around the sysctl, no?  Why do we need a syscall for it?

sysctls are very slow, but that may be a somewhat weak point given the nature of the request. given the abundance of free syscall numbers and the fact other systems (OpenBSD, Linux) do it as a syscall I don't see a good reason to do it differently.
Comment 12 David Carlier 2018-02-21 13:53:42 UTC
That was quite a time ago, I forgot in the meantime ... Is it still interesting enough to be considered ?
Comment 13 Conrad Meyer freebsd_committer 2018-02-21 20:43:36 UTC
(In reply to David Carlier from comment #12)
Yes, it's worth having.
Comment 14 Mateusz Guzik freebsd_committer 2018-02-21 21:03:24 UTC
So I had a closer look, linux added getrandom which is a slightly more elaborate version and getentropy can be emulated on top. I don't see any downsides to doing the equivalent.
Comment 15 David Carlier 2018-02-21 21:08:38 UTC
Alright I would not expect it would get any attraction in fact :-) curious now how it will end up.
Comment 16 Conrad Meyer freebsd_committer 2018-02-21 22:09:44 UTC
I'll take.
Comment 17 commit-hook freebsd_committer 2018-03-21 01:16:32 UTC
A commit references this bug:

Author: cem
Date: Wed Mar 21 01:15:47 UTC 2018
New revision: 331279
URL: https://svnweb.freebsd.org/changeset/base/331279

Log:
  Implement getrandom(2) and getentropy(3)

  The general idea here is to provide userspace programs with well-defined
  sources of entropy, in a fashion that doesn't require opening a new file
  descriptor (ulimits) or accessing paths (/dev/urandom may be restricted
  by chroot or capsicum).

  getrandom(2) is the more general API, and comes from the Linux world.
  Since our urandom and random devices are identical, the GRND_RANDOM flag
  is ignored.

  getentropy(3) is added as a compatibility shim for the OpenBSD API.

  truss(1) support is included.

  Tests for both system calls are provided.  Coverage is believed to be at
  least as comprehensive as LTP getrandom(2) test coverage.  Additionally,
  instructions for running the LTP tests directly against FreeBSD are provided
  in the "Test Plan" section of the Differential revision linked below.  (They
  pass, of course.)

  PR:		194204
  Reported by:	David CARLIER <david.carlier AT hardenedbsd.org>
  Discussed with:	cperciva, delphij, jhb, markj
  Relnotes:	maybe
  Differential Revision:	https://reviews.freebsd.org/D14500

Changes:
  head/include/unistd.h
  head/lib/libc/gen/Makefile.inc
  head/lib/libc/gen/Symbol.map
  head/lib/libc/gen/getentropy.3
  head/lib/libc/gen/getentropy.c
  head/lib/libc/sys/Makefile.inc
  head/lib/libc/sys/Symbol.map
  head/lib/libc/sys/getrandom.2
  head/lib/libc/tests/gen/Makefile
  head/lib/libc/tests/gen/getentropy_test.c
  head/sys/compat/freebsd32/syscalls.master
  head/sys/conf/files
  head/sys/kern/sys_getrandom.c
  head/sys/kern/syscalls.master
  head/sys/sys/random.h
  head/tests/sys/kern/Makefile
  head/tests/sys/kern/sys_getrandom.c
  head/usr.bin/truss/syscalls.c