random(4): Restore availability tradeoff prior to r346250

[cem]

Summary

As discussed in that commit message, it is a dangerous default. But the
safe default causes enough pain on a variety of platforms that for now,
restore the prior default.

Some of this is self-induced pain we should/could do better about; for
example, programmatic CI systems and VM managers should introduce entropy
from the host for individual VM instances. This is considered a future work
item.

On modern x86 and Power9 systems, this may be wholly unnecessary after
D19928 lands (even in the non-ideal case where early /boot/entropy is
unavailable), because they have fast hardware random sources available early
in boot. But D19928 is not yet landed and we have a host of architectures
which do not provide fast random sources.

This change adds several tunables and diagnostic sysctls, documented
thoroughly in UPDATING and sys/dev/random/random_infra.c.

Reported by: adrian, jhb, imp, and probably others
Relnotes: yeah

[cem]

At suggestion of rpokala@ and with help from np@, incorporate MD cyclecounter
in fako arc4random seeding when devrandom is bypassed unseeded.

[cem]

sys/mips/conf/PB92

136 ↗

(On Diff #56318)

This is from tinderbox — libkern/arc4random.c is standard, but "device random" is optional, which seems sort of paradoxical. An alternative fix might be to creatively use #ifdef DEV_RANDOM in arc4random.c. I hope to make device random standard in the future, so that seems counter-productive.

If you have strong opinions on "PB92," please share them.

[rpokala]

sys/dev/random/random_infra.c

62 ↗

(On Diff #56318)

Perhaps "it is seeded by sufficient entropy."?

[cem]

sys/dev/random/random_infra.c

62 ↗

(On Diff #56318)

Sure, that's better. Will fix.

[adrian]

I'm ok with flipping random on by default nowdays. There are bigger fish to fry in the "wasted kernel space" game.

[delphij]

sys/dev/random/randomdev.c

274 ↗

(On Diff #56318)

Why is this needed? I think this would zero out the result buffer, which would give deterministic result (0's) and there is not any way to notify the caller?

sys/libkern/arc4random.c

100 ↗

(On Diff #56318)

I think 'cc' and 'ts_now' is quite closely related, is it useful to have both?

105 ↗

(On Diff #56318)

Maybe use existing hash like SHA512_256:

SHA512_256_Init(&ctx);
SHA512_256_Update(&ctx, key, sizeof(key));
SHA512_256_Update(&ctx, cc, sizeof(cc));
SHA512_256_Final(key, &ctx);

Instead of simple XOR's?

[cem]

Thanks, @adrian and @delphij!

sys/dev/random/randomdev.c

274 ↗

(On Diff #56318)

Otherwise, if the consumer assumes the output is random and leaks it, kernel stack contents may be leaked. It does zero out the result buffer, which is deterministic. That is the point of the big knob — it is for people who don't care that their random is deterministic and prefer availability. Callers can pre-verify that the device is seeded with the is_random_seeded API.

sys/libkern/arc4random.c

100 ↗

(On Diff #56318)

They are certainly correlated; I'm not sure whether it is useful to have both. Maybe we can just drop 'ts_now', since the low order bits of cc are most likely to be non-deterministic.

105 ↗

(On Diff #56318)

What beneficial property are you suggesting we add with the hash? I am not especially opposed to the idea (although SHA512_256 will be quite slow on e.g., MIPS) but I'm not sure if there is a specific benefit in mind?

[imp]

This looks good, and we can make good use of this in our automation. It's adequate for netflix to decide what to do at boot when the aberrant conditions happen, as well as accommodating our diverse deployment model which is currently biased towards servers, but may not always be.

sys/dev/random/random_infra.c

72 ↗

(On Diff #56318)

non-zero is a good description for an int sysctl, but should this be 'true' since it's a bool?

85 ↗

(On Diff #56318)

same here.

sys/mips/conf/PB92

136 ↗

(On Diff #56318)

you might ask Adrian, because these files are kinda his baby.

[markm]

LGTM.

[cem]

Thanks markm and imp.

Before commit, I think I owe Ravi a small text change; I may owe Warner some text changes (open question); and there are some open questions with Xin about the actual fallback behavior for seeding arc4random that I would like to come to consensus on.

sys/dev/random/random_infra.c

72 ↗

(On Diff #56318)

Unfortunately, none of the tooling actually groks assigning true/false yet.

Internally, sysctl/tune bools are just U8 type with a sysctl_handle_bool handler.
sysctl(8) doesn't understand "true" and "false" literals for uint8_t, although that would be fairly easy to add.

I think loader just shoves the raw value into the kernel environment, and sysctl_register_oid -> sysctl_load_tunable_by_oid_locked eventually invokes sysctl_handle_bool on whatever U8 value came from loader; any non-zero value is coerced to true. That one is harder to fix, unless we hack up loader to translate true/false to 1/0, or actually add a distinct type for BOOL.

In other words, I think a numeric value is still required; we could specify "one" in particular, but any non-zero U8-sized value will be coerced to true.

Let me know if you feel strongly about "one" instead of "non-zero;" I'm happy to change the text.

sys/mips/conf/PB92

136 ↗

(On Diff #56318)

He gave mild blessing in a non-line comment earlier on this review.

[delphij]

(I'm not against with the overall plan, but see my comments about use of hash function inline).

sys/libkern/arc4random.c

105 ↗

(On Diff #56318)

It's mostly to get more bits flipped (avalanche effect found in a cryptographic hash algorithm), and also create a slightly more computation burden for those who wants to attempt a brute-force of the possible values at boot. Timestamps have more entropy in their lower bits, but less with their higher bits; when you are stacking up some related values, it's less susceptible to cancel each other out compared to plain XOR's (note that the tv_nsec and cc sizes have a GCD of sizeof(long), by the way). You may also want to consider mixing in additional data, like __FreeBSD_version here.

I don't think performance is a big concern here as the data being hashed are fairly small, and this is not a "hot" codepath.

[cem]

sys/libkern/arc4random.c

105 ↗

(On Diff #56318)

Ok, I'll make that change. One caveat with SHA512/256 in particular is that it is an optional module (optional crypto | geom_bde | ipsec | ipsec_support | zfs). On the other hand, vanilla SHA256 is already required by random. Do you have a strong preference for 512/256 over plain SHA2 256?

I'll go ahead and make the change (planning on SHA256 for now).

[delphij]

sys/libkern/arc4random.c

105 ↗

(On Diff #56318)

Do you have a strong preference for 512/256 over plain SHA2 256?

No, I just picked up a random one which I thought is available; since it's not part of standard kernel I think we should go with SHA256 instead (and thanks for pointing it out).

[imp]

sys/dev/random/random_infra.c

72 ↗

(On Diff #56318)

OK. Then the original wording is fine. I thought we did export that way, but I see now that I'm mistaken.

[cem]

• Incorporate Ravi's suggested description wording
• Incorporate a SHA256 version of Xin's suggestion for fallback arc4random seeding for reasons mentioned in differential (open to discussion, just presenting a prototype)

[cem]

Thanks Xin and Warner!

Last maybe open issue:

sys/dev/random/randomdev.c

274 ↗

(On Diff #56318)

Are you ok with this @delphij or is this still an open issue?

[delphij]

sys/dev/random/randomdev.c

274 ↗

(On Diff #56318)

Yes I'm convinced with your reasoning for zero'ing out.