Comment by dale_glass
7 hours ago
The my_random_bytes implementation doesn't look good at all at first glance. Let's see:
1. I believe it errors out if the HWRNG returns the same value twice. That's actually a thing that can legitimately happen. "0" is also a legitimate output.
2. "here" is a terrible name for a length
3. It does a memcpy of a minimum of 4 bytes to the destination, even if count is lower. It'll also overflow longer buffers with a length not divisible by 4.
I have absolutely no interest in defending this code-- and already considered it untrusthworthy before any of this. But in the interest of accuracy:
> It does a memcpy of a minimum of 4 bytes
This is a common misreading of MIN(). MIN(4,x) is a number that is a MAXIMUM of 4, not a minimum.
Count is the number of bytes remaining in the buffer. The input to the copy is a 4-byte word. min(4,count) will produce a number 0-4 which is always equal to or less than count. The copy will not overflow the buffer or overrun the input: If count is 3, for example, then here will be 3 and it will copy 3 bytes.
> believe it errors out if the HWRNG returns the same value twice. That's actually a thing that can legitimately happen. "0" is also a legitimate output.
It's been a while since I looked but I believe the STM32 manual advises you to throw away data when this happens, because the rng is updated async with the processor and reads that are too fast will produce 0s or duplicate values. Entropy loss from doing so is generally negligible. Were it me I'd read enough into a cryptographic hash to render the output cryptographically close to uniform and not have to worry about it further. (particularly since some uses of cryptographic numbers are extremely sensitive to even small biases)
I do wonder how fatal MP_EFAULT actually is...-- on a device like this being jumpy at failing the RNG is reasonable, but if it bricks the device (for example) that would be too much for a condition that (IIRC) the datasheet says can happen. If that error is worse than causing a reboot then it might be the case that their emergency fix deployment might have the effect of causing problems by deploying never-actually-tested code into the wild. When I initially looked at this code before finding the flaw I was somewhat surprised that this test didn't produce spurious failures.
[Maybe someone who isn't traveling and on vacation might want to trace out that error condition, -- and check my vague recollection of the STM32 datasheets, as I could be remembering some other part]
There have been cases where a small statistical bias like avoiding duplicate bytes can be amplified to a full break. I don't think this problem is applicable to a bitcoin private key though.
Right, that's particularly a concern for DSA nonces. Coldcard uses RFC 6979 however, and duplicates at the 32-bit word level is a different matter than at the byte level.