Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

entropy: Add support for BSD sysctl(KERN_ARND) #3423

Merged
merged 5 commits into from
Jun 25, 2020
Merged

entropy: Add support for BSD sysctl(KERN_ARND) #3423

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9f5312c
entropy: Add support for BSD sysctl(KERN_ARND)
alarixnia Jun 11, 2020
6777dcb
Add ChangeLog.d entry for kern.arandom support.
alarixnia Jun 11, 2020
f4d9f21
entropy: Rename sysctl_wrapper to sysctl_arnd_wrapper
alarixnia Jun 19, 2020
e3fdcfa
entropy: Avoid arithmetic on void pointer
alarixnia Jun 23, 2020
8373c86
entropy: Adjust parameter type of internal function to avoid a cast
alarixnia Jun 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ChangeLog.d/sysctl-arnd-support.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Features
* Added support to entropy_poll for the kern.arandom syscall supported on some BSD systems. Contributed by Nia Alarie in #3423.
45 changes: 45 additions & 0 deletions library/entropy_poll.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,41 @@ static int getrandom_wrapper( void *buf, size_t buflen, unsigned int flags )
#endif /* SYS_getrandom */
#endif /* __linux__ || __midipix__ */

/*
* Some BSD systems provide KERN_ARND.
* This is equivalent to reading from /dev/urandom, only it doesn't require an
* open file descriptor, and provides up to 256 bytes per call (basically the
* same as getentropy(), but with a longer history).
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage of KERN_ARND over arc4random_buf?

Apparently arc4random_buf ha been using ChaCha20 with backtracking resistance (replacing an older implementation based on RC4) since:

So arc4random_buf looks fine to me on non-antique NetBSD, non-antique OpenBSD and the latest major release of FreeBSD, but wouldn't be ok on FreeBSD 11.x which is still supported.

So why not call arc4random_buf on modern BSD, and fall back to /dev/urandom on FreeBSD <12?

Copy link
Contributor Author

@alarixnia alarixnia Jun 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've discussed this with other projects and the response has been mixed. Certain libraries expressed a preference towards using a system call because it's harder to reconstruct the RNG state than it is for arc4random_buf where the state lives in libc and is per-process.

If this was simply about providing an endless stream of random numbers to applications, I'd use arc4random without question, but since this is an "entropy" API that is intended for seeding other RNGs, it seemed more prudent to use a system call.

Using the sysctl will be safe on antique releases and using it doesn't involve having to check for the presence of a function in libc, just the presence of a #define (the sysctl API is ancient BSD stuff). OpenSSL has an explicit policy of supporting OS versions that even we don't support, so they had a preference for this API.

Speaking from the NetBSD side, I'd like to see more software using arc4random_buf (other developers and users may disagree with me), but it's up to you which you prefer to use in mbedtls. It's certainly safe in all versions I care about, and provides some very nice performance and simplicity — at the expense of keeping its state in the application, and an awkward name and possibly fatal insecurity on some OS versions. I can resubmit a PR that uses arc4random on a set of allowed operating system versions, if that's what is preferred. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm starting to wonder if a three-layer approach would make sense:

  1. If we're on a set of known-good OS and versions use arc4random_buf.
  2. Otherwise use KERN_ARND if available.
  3. Otherwise fall back to open()ing /dev/urandom.

If we go for this approach, we could begin by finalizing and merging this PR, and then have another one adding the arc4random layer.

The question is: what platforms would benefit from the KERN_ARND layer in such an approach?

  • FreeBSD 11 still uses ARC4 for arc4random and has KERN_ARND but it will be EOL in a bit more than a year (end of September 2021).
  • Dragonfly BSD looks like it's still using ARC4 for arc4random, according to this man page, but doesn't have KERN_ARND so can't benefit from this layer anyway.

So maybe three layers are not necessary, and just having arc4random() when it's known to be safe, and /dev/urandom as a fallback would be enough.

Wdyt?

Copy link
Contributor Author

@alarixnia alarixnia Jun 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not strongly opinionated about which approach gets chosen — I really just want a better default choice than reading from /dev/urandom for NetBSD. Other than obvious performance differences, the only meaningful difference on all supported versions of NetBSD is that the application can't access the state of the KERN_ARND RNG.

If you're happy using arc4random_buf in an entropy function, I will submit a new PR after this one is closed or merged.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so there is a minor security advantage to the sysctl approach: if the arc4random state is compromised by a memory read vulnerability before it's used to seed the entropy for mbedtls, that compromises the mbedtls entropy. (If the arc4random state is compromised later, that shouldn't affect the mbedtls entropy since modern arc4random has backtracking resistance.) It's not such a big advantage that I'd reject arc4random, which is simpler, but since the sysctl code is there, I'm happy to use it.

Regarding OS version support, we don't have a well-defined policy, but we do try to accommodate people who run Mbed TLS on “oddball” platforms, including out-of-support platforms. But we can only promise best effort, not results, when it comes to platforms for which we don't have technical knowledge on the team and a CI system in place.

*
* Documentation: https://netbsd.gw.com/cgi-bin/man-cgi?sysctl+7
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I also found it useful to have a look as sysctl(3) as well, but since it's prominently linked from the above, I'm not requesting any change here.

*/
#if (defined(__FreeBSD__) || defined(__NetBSD__)) && !defined(HAVE_GETRANDOM)
#include <sys/param.h>
#include <sys/sysctl.h>
#if defined(KERN_ARND)
#define HAVE_SYSCTL_ARND

static int sysctl_wrapper ( void *buf, size_t buflen )
alarixnia marked this conversation as resolved.
Show resolved Hide resolved
{
int name[2];
size_t len;

name[0] = CTL_KERN;
name[1] = KERN_ARND;

while( buflen > 0 )
{
len = buflen > 256 ? 256 : buflen;
if( sysctl(name, 2, buf, &len, NULL, 0) == -1 )
return( -1 );
buflen -= len;
buf += len;
alarixnia marked this conversation as resolved.
Show resolved Hide resolved
}
return( 0 );
}
#endif /* KERN_ARND */
#endif /* __FreeBSD__ || __NetBSD__ */

#include <stdio.h>

int mbedtls_platform_entropy_poll( void *data,
Expand All @@ -139,6 +174,15 @@ int mbedtls_platform_entropy_poll( void *data,
((void) ret);
#endif /* HAVE_GETRANDOM */

#if defined(HAVE_SYSCTL_ARND)
((void) file);
((void) read_len);
if( sysctl_wrapper( output, len ) == -1 )
return( MBEDTLS_ERR_ENTROPY_SOURCE_FAILED );
*olen = len;
return( 0 );
#else

*olen = 0;

file = fopen( "/dev/urandom", "rb" );
Expand All @@ -156,6 +200,7 @@ int mbedtls_platform_entropy_poll( void *data,
*olen = len;

return( 0 );
#endif /* HAVE_SYSCTL_ARND */
}
#endif /* _WIN32 && !EFIX64 && !EFI32 */
#endif /* !MBEDTLS_NO_PLATFORM_ENTROPY */
Expand Down