stepansnigirev
commented
1 year ago One thing that worries me about this PR:
random32() is used not only when new mnemonic is generated, but also during signing.
Random numbers are not critical for signing - nonces are generated deterministically anyways, but random numbers can add extra protection against side-channel attacks.
The problem is that this PR will break projects that don't use generateMnemonic() function directly, but provide entropy received differently. Because if you are signing but your unallocated heap and stack is zeroed the program will hang.
I am not sure what to do about this. Maybe do not hang if stack and heap are filled with zeroes? Would pragma message be enough?
Also plz take a look at #24
The purpose of this commit is: 1) warn users with pragma message to avoid using those functions in production 2) add uninitialized stack (together as the previous uninitialized heap) as extra entropy source (because while testing some embedded devices the heap allocations are often zero-filled and I was often getting the same private keys, so the current implementation is EXTREMELY unsafe) 3) double check if heap and stack are truly uninitialized (zero-fill and pattern checks), some compilers might initialized stack variables automatically (e.g. Clang with -ftrivial-auto-var-init), so fail seed initialization in that case. 4) is seeding fails, loop forever, do not generate any random
Seed init is still garbage and a TRNG should be used, but at least it's a (tiny) bit less unsafe.
About the old line: memcpy(arr, hash, 32); // to maintain previous entropy, kinda This doesn't make any sense, since static uint8_t hash[32]; will be always inizialized to zero, so this memcpy only zero-fills the first 32 bytes which obviously reduces the entropy and it's pretty much useless and I removed it.