[PATCH] urandom-seed: use seedrng for seeding the random number generator

[Andre Heider]

On 04/04/2022 21:35, Etienne Champetier wrote:
> Hi Jason,
> 
> Le lun. 4 avr. 2022 à 14:39, Jason A. Donenfeld <Jason at zx2c4.com> a écrit :
>>
>> Hey Etienne,
>>
>> On Tue, Mar 29, 2022 at 7:21 AM Jason A. Donenfeld <Jason at zx2c4.com> wrote:
>>>
>>> Hi Etienne,
>>>
>>> On Tue, Mar 29, 2022 at 1:06 AM Etienne Champetier
>>> <champetier.etienne at gmail.com> wrote:
>>>>> Oh that's an interesting set of considerations and it's possible I
>>>>> didn't understand some aspect of this. Most OSes should call seedrng
>>>>> once at boot and once at shutdown.
>>>>
>>>> As routers are always on devices, it's rare to have clean shutdown.
>>>> Personally, my routers boot after an upgrade or after a power loss,
>>>> so they almost never shutdown properly.
>>>
>>> That's a good point indeed.
>>>
>>>>> 1) read seed into memory, delete seed from disk, write into rng &
>>>>> credit if good seed, write new seed to disk; repeat at shutdown/some
>>>>> other time
>>>>> 2) read seed into memory, write into rng w/o crediting, re-use the
>>>>> same seed next boot
>>>>
>>>> Before this patch we had 2 and users could opt-in to renew seed on
>>>> each boot, so closer to 1.
>>>
>>> I guess the issue is that the implementation of (1) was somewhat
>>> non-optimal, but not exactly catastrophic either.
>>>
>>>> Looking at random.c, I would love add_device_randomness() behavior.
>>>> Maybe it was already answered on LKML,
>>>> but why can't writes to /dev/urandom from a process with CAP_SYS_ADMIN
>>>> be mixed in right away a la add_device_randomness() without being credited ?
>>>> This would not init the RNG faster, but this would make early
>>>> /dev/urandom reads "safer".
>>>
>>> add_device_randomness() does not mix in immediately. It goes into the
>>> entropy pool, but that doesn't get extracted into a new key until the
>>> next reseeding. It does get mixed in directly for crng_init=0, but not
>>> for crng_init=1 or crng_init=2, which is a big gap. Making
>>> /dev/urandom writes behave like that for crng_init=0 doesn't address
>>> the crng_init=1 and crng_init=2 cases, unfortunately. The bigger
>>> problem, though, is that some users of /dev/urandom credit the entropy
>>> via the RNDADDTOENTCNT ioctl _afterwards_. If we mixed it directly in,
>>> then programs with the pattern of write 4 bytes, credit 32 bits,
>>> writes 4 bytes, credit 32 bits, etc could have those 4 written bytes
>>> brute forced each time in what's called a "premature next". For that
>>> reason the key is only modified when 256 bits have accumulated first.
>>>
>>>> I'm fine with writing on each boot, but as we can't rely on shutdown,
>>>> what we could do with the seeds:
>>>> 1) load seed.no-credit, leave it on disk
>>>> 2) mv seed.credit seed.no-credit && load seed.no-credit (and credit it)
>>>> 3) read from getrandom a new seed.credit
>>>>
>>>> This would allow to always keep a seed on disk, only use seed.credit once,
>>>> and actually write seed.credit.
>>>> I would get rid of the whole hashing part as all our seeds would come
>>>> from getrandom().
>>>
>>> If possible, it's better to not leave a seed on disk after using it,
>>> even if not credited. If that's the only entropy, it's better to
>>> "forget" it after use, so that you can't compromise past secrets. At
>>> the very least, if you have poor entropy, you can replace the seed
>>> with HASH(seed), so at least it ratchets forward. Another thing to
>>> consider is that if you _do_ credit it, that'll initialize the RNG, so
>>> getrandom() automatically works without blocking. These two
>>> observations have lead to seedrng's current scheme, where the sequence
>>> is:
>>>
>>> - load
>>> - delete
>>> - seed & credit, or seed & don't credit, depending
>>> - save new seed, which may be creditable or not, depending on whether
>>> previous things made the rng init
>>>
>>> It sounds like maybe a modification of your suggestion might be to make this:
>>>
>>> - load
>>> - delete
>>> - seed & credit, or seed & don't credit, depending
>>> - save new seed using getrandom(0), so that it's always creditable
>>>
>>> Would that satisfy your concerns? Or are you also trying to preserve a
>>> mode where the filesystem doesn't need to be written to on each boot?
>>>
>>>
>>>
>>>> /var is a symlink to /tmp
>>>
>>> Oh, then in these cleanups, we should change that /tmp/run to /var/run
>>> just to be more "correct".
>>>
>>>>
>>>>> Is there a different place for it that would be good?
>>>>
>>>> Maybe we can leave it in etc and just make sure to exclude it from backups
>>>
>>> That seems like a good course of action.
>>>
>>> If you have a firm idea of what you want this to look like, would you
>>> like to send a series and I'll take a look?
>>
>> I never heard back from you, but all the concerns you raised strike me
>> as kind of important. Did you intend to move forward with those? Or
>> should I just send a revert for this whole thing, so that you can
>> address it some other time?
> 
> I meant to work on it last week but had 0 time, hopefully this week
> will be less busy :(
> Master is not less secure than before, so let's wait a bit to revert

Any progress on this?

PS: Since the last busybox bump we even have BUSYBOX_CONFIG_SEEDRNG now.

Cheers,
Andre

> 
> Best
> Etienne
> 
>>
>> Jason
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel at lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel