Page MenuHomeFreeBSD

random(4): deduplicate explicit_bzero() in harvest
ClosedPublic

Authored by cem on May 19 2019, 8:49 PM.

Details

Summary

I noticed that Fortuna was explicitly zeroing the provided event in addition
to most callers. Pull the responsibility for zeroing events, which is
general to any conceivable algorithm, out of the algorithm-specific code and
into the callers. Most callers indirect through
random_fortuna_process_event(), so add the logic there.

Add one missing bzero in randomdev_accumulate(). Also, remove a redundant
bzero in the same function -- randomdev_hash_finish() is obliged to bzero
the hash state.

Test Plan

This is mostly just an easy cleanup with no change in function. The only real
behavioral change is the missing bzero added in randomdev_accumulate() (used by
write(2) on /dev/random).

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

cem created this revision.May 19 2019, 8:49 PM
delphij accepted this revision.May 21 2019, 2:16 AM

Looks good to me in principal.

It do beg the question: why wasn't the bzero done in ra_event_processor instead? (i.e. an event should be considered as "consumed" upon return).

sys/dev/random/randomdev.c
356 ↗(On Diff #57569)

Nice finding!

This revision is now accepted and ready to land.May 21 2019, 2:16 AM
cem added a comment.May 21 2019, 2:42 AM

Thanks for taking a look, Xin!

It do beg the question: why wasn't the bzero done in ra_event_processor instead? (i.e. an event should be considered as "consumed" upon return).

That might be reasonable. I put it here mostly because (1) it is generic to any event processor; i.e., I don't think it makes sense to duplicate in Fortuna, AlgorithmX, etc. This point is mostly hypothetical since we only support Fortuna, though. And (2), it can be done outside the CONFIG_S_LOCK for RANDOM_LOADABLE (which again is moot in the GENERIC kernel). I guess a weak (3) would be that randomdev_accumulate reuses the same event structure repeatedly and only needs to explicit_bzero it once; pushing it inside ra_event_processor duplicates work.

It would be totally acceptable to me to move it in ra_event_processor instead, if that is your preference.

One other minor thing I noticed is that our Fortuna pool accumulators (fsp_hash) do not actually use SHA_d-256(), i.e., SHA-256(SHA-256(0^512 || ·)), but instead SHA-256²(), i.e., SHA-256(SHA-256(·)). FS&K §9.5.5 RandomData() suggests accumulating pool entropy P_i with the SHA_d-256 construction. However, in this particular case, I don't think there is any security benefit to SHA-d-256 over SHA-256², nor SHA-256² over plain SHA-256 (I don't think length extension attacks are relevant to this use).

sys/dev/random/randomdev.c
356 ↗(On Diff #57569)

Thanks :)

This revision was automatically updated to reflect the committed changes.