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.

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

Conversation

alarixnia
Copy link
Contributor

@alarixnia alarixnia commented Jun 11, 2020

This is basically the same as reading from /dev/urandom on supported systems, only it has a limit of 256 bytes per call (just to stop programs from spending too much time in the kernel), and does not require an open file descriptor (so it can be used in chroots, when resource limits are in place, or are otherwise exhausted).

It's functionally equivalent to the comparable function getentropy, but has been around for longer. It's actually used to implement getentropy in FreeBSD's libc. Discussions about adding getrandom or getentropy to NetBSD are still ongoing.

Note: this is also much faster than reading from /dev/urandom on NetBSD-current (the performance increase may not be as significant on other platforms). time(1) reports unpatched mbedTLS gen_entropy as spending 2-3 seconds in the kernel. With this patch applied, that number is 0. The overhead of reopening /dev/urandom 768 times in the process of generating 48K bytes is removed, and the overhead of libc stdio buffering for fread is removed -- libc is not optimized for repeated short reads on short lived files, so reads far more than necessary and stores it in a buffer.

  • KERN_ARND is present and works as described in all supported versions of FreeBSD and NetBSD, going back 10 years.
  • It's not present in DragonFly or OpenBSD.
  • Documentation (NetBSD manual page)
  • Comparable code in OpenSSL

This is basically the same as reading from /dev/urandom on supported
systems, only it has a limit of 256 bytes per call, and does not require
an open file descriptor (so it can be used in chroots, when resource
limits are in place, or are otherwise exhausted).

It's functionally equivalent to the comparable function getentropy(),
but has been around for longer. It's actually used to implement
getentropy in FreeBSD's libc. Discussions about adding getrandom or
getentropy to NetBSD are still ongoing.

It's present in all supported versions of FreeBSD and NetBSD.
It's not present in DragonFly or OpenBSD.

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

Comparable code in OpenSSL:
https://github.com/openssl/openssl/blob/ddec332f329a432a45c0131d83f3bfb46114532b/crypto/rand/rand_unix.c#L208

Signed-off-by: nia <[email protected]>
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! This looks generally good to me, except two details, one of which really needs fixing as it breaks one of our testing scripts.

library/entropy_poll.c Outdated Show resolved Hide resolved
* open file descriptor, and provides up to 256 bytes per call (basically the
* same as getentropy(), but with a longer history).
*
* 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.

@mpg mpg added component-crypto Crypto primitives and low-level interfaces enhancement needs-ci Needs to pass CI tests needs-work labels Jun 19, 2020
@gilles-peskine-arm
Copy link
Contributor

Unfortunately this breaks the build on FreeBSD.

+ CC=clang cmake -D 'CMAKE_BUILD_TYPE:String=Check' .
-- The C compiler identification is Clang 4.0.0
-- Check for working C compiler: /usr/bin/clang
-- Check for working C compiler: /usr/bin/clang -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Found PythonInterp: /usr/local/bin/python3 (found suitable version "3.6.4", minimum required is "3") 
-- Looking for pthread.h
-- Looking for pthread.h - found
-- Looking for pthread_create
-- Looking for pthread_create - not found
-- Looking for pthread_create in pthreads
-- Looking for pthread_create in pthreads - not found
-- Looking for pthread_create in pthread
-- Looking for pthread_create in pthread - found
-- Found Threads: TRUE  
-- Configuring done
-- Generating done
-- Build files have been written to: /builds/ws/-3423-head-2AJASB72BDLZDJXZMJZ2G@2
+ make clean
+ make
Scanning dependencies of target mbedcrypto
[  0%] Building C object library/CMakeFiles/mbedcrypto.dir/aes.c.o
[  0%] Building C object library/CMakeFiles/mbedcrypto.dir/aesni.c.o
[  0%] Building C object library/CMakeFiles/mbedcrypto.dir/arc4.c.o
[  1%] Building C object library/CMakeFiles/mbedcrypto.dir/aria.c.o
[  1%] Building C object library/CMakeFiles/mbedcrypto.dir/asn1parse.c.o
[  1%] Building C object library/CMakeFiles/mbedcrypto.dir/asn1write.c.o
[  1%] Building C object library/CMakeFiles/mbedcrypto.dir/base64.c.o
[  1%] Building C object library/CMakeFiles/mbedcrypto.dir/bignum.c.o
[  2%] Building C object library/CMakeFiles/mbedcrypto.dir/blowfish.c.o
[  2%] Building C object library/CMakeFiles/mbedcrypto.dir/camellia.c.o
[  2%] Building C object library/CMakeFiles/mbedcrypto.dir/ccm.c.o
[  2%] Building C object library/CMakeFiles/mbedcrypto.dir/chacha20.c.o
[  2%] Building C object library/CMakeFiles/mbedcrypto.dir/chachapoly.c.o
[  3%] Building C object library/CMakeFiles/mbedcrypto.dir/cipher.c.o
[  3%] Building C object library/CMakeFiles/mbedcrypto.dir/cipher_wrap.c.o
[  3%] Building C object library/CMakeFiles/mbedcrypto.dir/cmac.c.o
[  3%] Building C object library/CMakeFiles/mbedcrypto.dir/ctr_drbg.c.o
[  4%] Building C object library/CMakeFiles/mbedcrypto.dir/des.c.o
[  4%] Building C object library/CMakeFiles/mbedcrypto.dir/dhm.c.o
[  4%] Building C object library/CMakeFiles/mbedcrypto.dir/ecdh.c.o
[  4%] Building C object library/CMakeFiles/mbedcrypto.dir/ecdsa.c.o
[  4%] Building C object library/CMakeFiles/mbedcrypto.dir/ecjpake.c.o
[  5%] Building C object library/CMakeFiles/mbedcrypto.dir/ecp.c.o
[  5%] Building C object library/CMakeFiles/mbedcrypto.dir/ecp_curves.c.o
[  5%] Building C object library/CMakeFiles/mbedcrypto.dir/entropy.c.o
[  5%] Building C object library/CMakeFiles/mbedcrypto.dir/entropy_poll.c.o
/builds/ws/-3423-head-2AJASB72BDLZDJXZMJZ2G@2/library/entropy_poll.c:146:13: error: arithmetic on a pointer to void is a GNU extension [-Werror,-Wpointer-arith]
        buf += len;
        ~~~ ^
1 error generated.
*** Error code 1

Stop.
make[2]: stopped in /builds/ws/-3423-head-2AJASB72BDLZDJXZMJZ2G@2
*** Error code 1

Stop.
make[1]: stopped in /builds/ws/-3423-head-2AJASB72BDLZDJXZMJZ2G@2
*** Error code 1

Stop.
make: stopped in /builds/ws/-3423-head-2AJASB72BDLZDJXZMJZ2G@2
script returned exit code 1

@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed needs-ci Needs to pass CI tests needs-work labels Jun 23, 2020
@alarixnia
Copy link
Contributor Author

It should build with clang now.

@gilles-peskine-arm gilles-peskine-arm removed the needs-ci Needs to pass CI tests label Jun 23, 2020
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

The code looks ok, but I don't understand why this is the right approach.

* 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

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

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

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.

Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the build issue, however I think the fix could be improved.

library/entropy_poll.c Outdated Show resolved Hide resolved
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm left a comment

Choose a reason for hiding this comment

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

Approved, with the caveat that it's not clear to me that KERN_ARND is preferable to the simpler arc4random_buf (except on FreeBSD <12 where we can continue to live with /dev/urandom) and I'm happy to use KERN_ARND rather than arc4random.

@gilles-peskine-arm gilles-peskine-arm requested a review from mpg June 24, 2020 16:54
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

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

Thanks for adjusting the function prototype as suggested. Looks all good to me now.

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Jun 25, 2020
@mpg mpg merged commit 1cb2beb into Mbed-TLS:development Jun 25, 2020
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Jun 30, 2020
Motivation: the default behaviour of reopening /dev/urandom repeatedly
for every 128 bytes of entropy required is _exceedingly_ slow on NetBSD.
Not helped is using fread(), which assumes a long-lived file and buffers
excessively. This change makes the standard gen_entropy tool run in
milliseconds instead of seconds when it generates 48K of randomness.

Not only that, but sysctl is a lot more robust in e.g. chroots, resource
limited processes, etc.

Risk: On NetBSD, the security properties of the previous and current
behaviour are identical.

Upstreamed: Mbed-TLS/mbedtls#3423

Bump PKGREVISION.
@krytarowski krytarowski mentioned this pull request Jul 1, 2020
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Design and code approved - may be waiting for CI or backports component-crypto Crypto primitives and low-level interfaces enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants