Created attachment 186932 [details] patche that enable "pure" entropy sources such as RDRND to actually be mixed At vBSDCon, JMG and I co-presented a talk on an entropy analysis and audit on /dev/random that we conducted out of mutual interest. In the course of our work, we found the following: * so-called "PURE" sources of entropy, such as RDRND on Intel chips, are harvested however the results of the harvest are never mixed in due to the harvest mask bit never being set, with no way to set it. * Conducting an SP800-90B entropy analysis on the non-IID track for non-whitened entropy (the data fed into randomdev_hash_iterate, essentially), min-entropy is rather low because of a) the trng sources weren't being mixed, and b) there is a lot of repeat and predictable garbage that is of no value in the harvest_event structure, especially for events with only 4 bytes worth of data from their source in the he_entropy field. Attached are patches which correct these two issues. They are from work done downstream with the HardenedBSD team and have been tested.
Created attachment 186933 [details] patch to increase the min-entropy Makes sure that we only feed the first 8-12 bytes of the harvest_event hash since otherwise we are including predictable/repeat bytes which decrease min-entropy when measured per guidance from US NIST in SP800-90B
(In reply to W. Dean Freeman from comment #1) For this 2nd patch, I don't see any value in making it optional. Does that seem reasonable to you or can you make a case for the HBSD_RANDOM_HIGH_ENTROPY option? Also re: 2nd patch, this code seems totally broken: + fortuna_state.fs_pool[pl].fsp_length = MIN(RANDOM_FORTUNA_MAXPOOLSIZE, + fortuna_state.fs_pool[pl].fsp_length + sizeof(event->he_somecounter) + + event->he_size); Note that fsp_length is of type u_int and RANDOM_FORTUNA_MAXPOOLSIZE is UINT_MAX. You aren't doing saturating arithmetic, but instead just overflowing.
(In reply to W. Dean Freeman from comment #0) Re this patch, wouldn't this much smaller change fix the immediate issue? --- a/sys/sys/random.h +++ b/sys/sys/random.h @@ -94,7 +94,7 @@ enum random_entropy_source { ENTROPYSOURCE }; -#define RANDOM_HARVEST_EVERYTHING_MASK ((1 << (RANDOM_ENVIRONMENTAL_END + 1)) - 1) +#define RANDOM_HARVEST_EVERYTHING_MASK ((1 << ENTROPYSOURCE) - 1) Changing the sysctl behaviors seems orthogonal to that change. Sure, the sysctls will not accurately reflect Pure sources. But they will be harvested correctly, I think. Please let me know if I misunderstand. Thanks!
(In reply to Conrad Meyer from comment #2) Conrad, We made it optional so that we could test it down stream but still have a switch to flip back to up-stream behavior if we wanted to. Frankly, I don't see value in leaving it optional long-term as I think it is the correct behavior. Re: saturating add, I probably should have removed the original comment. It was MarkM's, not mine/Oliver's. The code change with the MIN() was based on a suggestion from jmg@. Looking back, it does look like I missed where he said to change RANDOM_FORTUNA_MAXPOOLSIZE to just be a u_int instead of UINT_MAX.
(In reply to Conrad Meyer from comment #3) The core of the problem with the pure sources was, in random_harvest_queue and random_harvest_direct, the value hc_source_mask is checked to see whether the source bit is set. If it isn't, the functions bail out before they actually either add an entry to the harvest ring buffer (in the case of random_harvest_queue) or feed into ra_process_event (ie, random_fortuna_process_event). Thus, entropy is being harvested, by virtue of the fact that sources push, rather than having /dev/random pull from them. However, the results of the push were not being mixed in. Additionally, there was no way via sysctl or any other means to set the bit for the source. We have added code which will ensure the bit gets set on register for sources which do register as entropy sources (which is really just RDRND and VIA Padlock), however many pure sources, such as RANDOM_PURE_BROADCOM, which is the source for the trng in Raspberry Pi devices, don't register. They use random_harvest_queue. The point of the patch is to correct that. Additionally, it allows showing the value in sysctl. For sources other than RDRND and VIA, there isn't an additional way beyond the mask to see whether the source is actually turned on or not, since they don't register. The patch corrects the behavior and allows the operator to ensure that it is set.
So hc_source_mask is initialized to RANDOM_HARVEST_EVERYTHING_MASK. Is expanding RANDOM_HARVEST_EVERYTHING_MASK to cover the Pure devices sufficient to harvest from RDRAND and Via? Or would that break anything (e.g., due to the devices that did not register)? Yes, the rest of the 1st patch is a nice enhancement to expose those bits to userspace that were not exposed before. I'm just not clear on if the bits must only be set on registration, or if it's ok to just have them all set from the beginning. Thanks! Would you mind throwing the patches up on Phabricator so comments can be more clearly associated with specific lines of code?
(In reply to Conrad Meyer from comment #6) I shall have to learn to use Phabricator then. I'll figure it out as soon as possible. For RDRND and Via, if they don't register they'll never get called anyway. random_sources_feed won't call their associated functions and push into random_harvest_direct. But if the bit isn't set, then you're harvesting entropy and throwing it away. If you have a more elegant solution, I am open. A couple of us noodled on this and we are running it in various places in development and production at this point and it works. That doesn't mean it can't be better though.
Created attachment 186937 [details] changes maxpoolsize from UINT_MAX to INT_MAX per jmg comment
Ok, I think I'm happy with your set of patches as-is now. Thanks all involved for doing the investigation, diagnosing the issue, coming up with a fix, and reporting it back to FreeBSD.
A commit references this bug: Author: cem Date: Fri Oct 6 18:27:56 UTC 2017 New revision: 324372 URL: https://svnweb.freebsd.org/changeset/base/324372 Log: random(4): Discard low entropy inputs The later fields of the harvest_event structure are predictable and provide little value to the entropy pool. Only feed in the relatively high entropy counter and explicit entropy buffer to increase measured input entropy. See also: https://people.freebsd.org/~jmg/vbsdcon_2017_ddfreebsdrng_slides.pdf PR: 222807 Submitted by: W. Dean Freeman <badfilemagic AT gmail.com> Reviewed by: jmg (earlier version), delphij Approved by: secteam (delphij) Obtained from: HBSD 8d809124d563937edd84c9c9d5494406e359c55c Security: no -- low entropy marginal input has no known negative affect on pool quality Differential Revision: https://reviews.freebsd.org/D12610 Changes: head/sys/dev/random/fortuna.c
FYI I've got the main patch up on phab here: https://reviews.freebsd.org/D12611 , awaiting blessing from secteam@.
A commit references this bug: Author: cem Date: Sat Oct 7 19:02:03 UTC 2017 New revision: 324394 URL: https://svnweb.freebsd.org/changeset/base/324394 Log: random(4): Gather entropy from Pure sources At initialization, hc_source_mask only includes non-Pure sources. The patch changes source registration to enable the registered source in the hc_source_mask bitmask. This mask governs which sources are harvested. This patch also disallows userspace from disabling such sources. PR: 222807 Submitted by: W. Dean Freeman <badfilemagic AT gmail.com> Reviewed by: jmg (earlier version), delphij Approved by: secteam (delphij) Obtained from: HBSD 0054e3e170e083811acc9f3b637f8be8a86c03e7 Security: yes Differential Revision: https://reviews.freebsd.org/D12611 Changes: head/sys/dev/random/random_harvestq.c head/sys/dev/random/randomdev.c head/sys/sys/random.h