Use Chacha20 for userland arc4random() and friends

[delphij]

Summary

Update userland arc4random() with OpenBSD's Chacha20
based arc4random().

crypto/heimdal/lib/roken/rand.c:
crypto/openssh/config.h:

  Eliminate in-tree usage of arc4random_stir().

include/stdlib.h:

  Remove arc4random_stir() and arc4random_addrandom() prototypes
  provide temporary shims for transistion period.  The plan is
  to get rid of these before 12.0-RELEASE, after a exp-run is
  performed.

lib/libc/gen/arc4random.c:

  Adopt OpenBSD arc4random.c,v 1.54 with bare minimum changes.

lib/libc/gen/arc4random.h:

  Adopt OpenBSD arc4random.h,v 1.4 but provide _ARC4_LOCK of
  our own.

lib/libc/gen/arc4random.3:

  Adopt OpenBSD arc4random.3,v 1.35 but keep FreeBSD r118247
  and r114444.

lib/libc/gen/arc4random-compat.c:

  Compatibility shims for arc4random_stir and arc4random_addrandom
  functions to preserve ABI.  Log once when called but do nothing
  otherwise

lib/libc/gen/getentropy.c:

  Fold __arc4_sysctl into getentropy.c (renamed to arnd_sysctl)
  and remove from ib/libc/include/libc_private.h.

sys/crypto/chacha20/chacha20.c:
sys/crypto/chacha20/chacha20.h:

  Make it possible to include the kernel implementation in libc.  Checked with cpp(1).

PR: 182610

Note that the intention is to get our arc4random() updated to OpenBSD's
version first; we will revisit and explore other possible optimizations
at a later time.

Test Plan

Run existing binaries with new libc.so image, make tinderbox,
and other test cases.

[devnexen_gmail.com]

lib/libc/gen/arc4random-compat.c

58 ↗

(On Diff #46823)

WARNING level maybe since it s advertised only once.

[cem]

As discussed before I am very supportive of killing use of RC4. Some style quibbles, questions, etc below.

lib/libc/gen/arc4random-compat.c

58 ↗

(On Diff #46823)

It seems weird to warn about a specific function being invoked but share a single warned variable.

60 ↗

(On Diff #46823)

return is gratuitous

lib/libc/gen/arc4random.3

133 ↗

(On Diff #46823)

Might document history in FreeBSD as well.

134–135 ↗

(On Diff #46823)

This last sentence does not add anything and is just confusing and should be removed.

lib/libc/gen/arc4random.c

47 ↗

(On Diff #46823)

MIN()?

49–53 ↗

(On Diff #46823)

What is this trying to accomplish?

74 ↗

(On Diff #46823)

This is a very odd pattern. Why is any of the contents of arc4random.h in a header instead of in this file?

lib/libc/gen/arc4random.h

51 ↗

(On Diff #46823)

How does this compile? Where are these structures defined?

lib/libc/gen/arc4random_uniform.c

1 ↗

(On Diff #46823)

Splitting out arc4random_uniform is a straightforward change. It can be done in its own commit with little scrutiny.

lib/libc/gen/chacha_private.h

1 ↗

(On Diff #46823)

I am skeptical of adding another copy of chacha to the tree. We have this identical file, minus 4 lines, in sys/crypto/chacha20/chacha.c. It seems like we can add the keystream-only mode to that file and include it instead of duplicating it.

(An alternative to adding the keystream mode would be to just zero a page of memory and mmap it several times in a row to create a long zero msg for keystream generation. I don't have a strong preference.)

[delphij]

Will commit the arc4random_uniform portion of change (which contained some trivial changes to type to make the code C99 compliant) to reduce the size of this changeset and revise to address some comments with plan outlined inline.

lib/libc/gen/arc4random-compat.c

58 ↗

(On Diff #46823)

ACK. Will move it to a per function static.

58 ↗

(On Diff #46823)

@devnexen_gmail.com It's not very clear to me what would be the best level. The fact that an application is doing it doesn't necessarily mean something bad (and it's actually a good sign that they are not linking against libc statically). With that in mind, maybe this belongs to LOG_DEBUG?

For userland LOG_WARNING isn't really distinguishable to LOG_NOTICE (only kern.warning are being sent to console).

60 ↗

(On Diff #46823)

ACK, this is actually against style(9), I will also fix __arc4random_addrandom_fbsd11.

lib/libc/gen/arc4random.3

133 ↗

(On Diff #46823)

What do you think about this addendeum at the end of this section?

The
.Fn arc4random
random number generator is first introduced in
.Fx 2.2.6 .
The ChaCha20 based implementation was introduced in
.Fx 12.0 ,
with obsolete stir and addrandom interfaces removed at the same time.

134–135 ↗

(On Diff #46823)

I think this should stay as-is, mainly to reduce diff against OpenBSD. We can always revisit this at a later time if we decided to diverge from them.

lib/libc/gen/arc4random.c

47 ↗

(On Diff #46823)

The goal is to minimize changes to this file and not diverge from OpenBSD. Their reasoning was at https://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/crypt/arc4random.c?rev=1.53&content-type=text/x-cvsweb-markup .

lib/libc/gen/arc4random.h

51 ↗

(On Diff #46823)

In arc4random.c. Their intention is to put non-portable part of the code in arc4random.h so implementations keep minimal differences in arc4random.c.

lib/libc/gen/arc4random_uniform.c

1 ↗

(On Diff #46823)

Sounds reasonable. I'll commit this portion first and update this changeset to make it smaller.

lib/libc/gen/chacha_private.h

1 ↗

(On Diff #46823)

Yes that's good point. I'll see how we could reuse that code.

[devnexen_gmail.com]

lib/libc/gen/arc4random.c

60 ↗

(On Diff #46823)

INHERIT_ZERO

[cem]

lib/libc/gen/arc4random.3

133 ↗

(On Diff #46823)

"was first introduced" to match the tense of the openbsd sentence(s).

Otherwise, looks good to me!

lib/libc/gen/arc4random.c

47 ↗

(On Diff #46823)

MIN() is a different name than min(), and regardless we don't build FreeBSD with MSVC.

lib/libc/gen/arc4random_uniform.c

1 ↗

(On Diff #46823)

Great, thanks!

lib/libc/gen/chacha_private.h

1 ↗

(On Diff #46823)

It looks like the keystream define just prevents the algorithm from mixing in the "message." Since the Chacha20 cipher is just a keystream XOR message construction, this is identical to keystream (or keystream XOR zero).

[delphij]

Drop chacha_private.h and address various reviewer comments.

[markm]

LGTM

[markm]

My "LGTM" assumes requested changes are taken.

[delphij]

Remove manual page for arc4random_stir and arc4random_addrandom.

Remove arc4random_addrandom references in ntp.

[markm]

There were a couple of changes which are largely cosmetic to do with #if logic. Change if you want, otherwise LGTM.

contrib/ntp/lib/isc/random.c

70 ↗

(On Diff #46919)

redundant (but harmless) #else

crypto/heimdal/lib/roken/rand.c

39 ↗

(On Diff #46919)

Harmless #if could be removed and following #elif could be turned into if.

[cem]

crypto/heimdal/lib/roken/rand.c

39 ↗

(On Diff #46919)

Removing this one would fall through to the srand(time()) case instead — not what we want. This one should be left alone.

lib/libc/gen/arc4random.c

49–53 ↗

(On Diff #46823)

Hm, I'm still not sure what the inline ifdefs are for.

lib/libc/gen/arc4random.h

51 ↗

(On Diff #46823)

Hm. I am not really a fan of this pattern and it seems incorrect to name this a .h. It's not a header — it's just source that happens to be #included in the exactly one place it can be included.

I'm not sure it's good to diverge from OpenBSD over that, but can we at least add a comment documenting the oddity of this "header" near the top of the file? E.g., something like, "This "header" is included exactly once in arc4random.c after necessary structure definitions. It is intended to contain the non-portable implementation details of arc4random.c."

lib/libc/gen/getentropy.c

113–114 ↗

(On Diff #46919)

Nice catch

[delphij]

Remove #else in contrib/ntp/lib/isc/random.c.

[delphij]

Addressed contrib/ntp/lib/isc/random.c issue; heimdal part would be left as-is for now (I think we can use HAVE_RAND but let's do it in a follow up commit).