Comment by nullc
15 hours ago
This writeup isn't very good and misses/misunderstands the programming error that leads to the flaw.
I'm commenting because I think it's important to understand the issue.
The article would have you think that the change in question was a tiny change to a flag to make it compile, but in reality the commit in question is a 1533 line addition of the entire RNG infrastructure.
The fundamental cause is a mixup between a value test and a definedness test.
Coldcard attempted to replace the micropython wrapper on the hardware TRNG, apparently in order to provide a more aggressive handling of fault/error conditions.
The micropython hwrng code is gated by an #if check, the replacement HWRNG code is gated by an ifndef. So "#define MICROPY_HW_ENABLE_RNG (0)" deactivated the micropython implementation but failed to activate the internal one (which was #ifndef MICROPY_HW_ENABLE_RNG ... which didn't fire because MICROPY_HW_ENABLE_RNG was _defined_).
This was easier to miss because the usages weren't only in different files-- they were in different repositories.
There is a more abstract point to make that in cryptographic software the absence of a secure randomness source (the STM32 TRNG) should never fall back to an insecure source (a trivial PRNG which might have only had on the order of 20-bits of uncertainty in its input). But the code that had the fallback was micropython which was not authored by the coldcard creators and is presumably not intended for cryptographic applications...
In later code (for MK4+ devices) the issue was further masked without being corrected by xor-ing in another insecure PRNG seeded by 32-bits from another TRNG. ... itself acting like an additional insecure fallback. (Why it first hashes 64-bits of TRNG output then throws away half the entropy is a mystery...)
RNG failures can be difficult to detect because the real randomness and a PRNG are indistinguishable by any simple tests of the output. I understand the coldcard developers ran extensive tests on the randomness generated by these devices-- they may well have been just testing the PRNG. It's something of a "color of your bits" issue ( https://ansuz.sooke.bc.ca/entry/23 ).
The same sort of issue happens at multiple levels of the stack, e.g. IIRC the STM32 TRNG itself does some kind of whitening that could have the same effect of concealing an RNG failure.
The existence of insecure fake randomness in the code at all was a red flag that had been noted previously -- though the really bad one was hidden away in the micropython code and not even obviously at play. I think this is a rare case of a bug that would be more easily found from binary analysis than review of the source code (e.g. no access to the STM32 TRNG at all).
Without these fallbacks the failure to use the TRNG would have been immediately detected by the developers (e.g. when every attempt generated the same seed), and a review could be validated by fault injection (NOP out the hwrng and verify that the test fails).
Although I might disagree with the style of the write up, I think it is right that there are weak engineering practices applied:
- not enough information provided in commit message. - this change ought to have been split into smaller chunks (e.g. introduce indirection whilst code continues to use hardware) - limited (automated) testing - potential confusion by the author over C concepts - additional complexity which could have been hidden behind a cleaner interface rather than using defines
I agree about your points about:
- defines over multiple repo's made this more complicated that was necessary. - fault injections would have been the appropriate way to test this (or maybe mocks) - testing of randomness needs careful design, e.g. RNG seeding can be a good way
Taking a step back, we as a community of excellence need to emphasise that this isn't a criticism about the person/author. There needs to be a clear analysis of what went wrong whilst being kind to the person/people involved.
Well, if the developer read libngu's random.c he would have noticed that it uses rng_get() and that it's asking for MICROPY_HW_ENABLE_RNG (albeit the failsafe is defective, reading it should have at least triggered "wait, what?").
So I'd say the root cause that the developer didn't check what random.bytes() does under the hood or how libngu expects random to be generated.
At no point did I ever think that setting "enable HW RNG" to 0 "provided a more aggressive handling of fault/error conditions". I always assumed it disabled the HW RNG.
https://github.com/switck/libngu/blob/537519a829259622ea6b03...
vs
https://github.com/Coldcard/micropython/blob/4107246f8a08080...
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.
3 replies →
The article clearly indicates the size of the changes: "The commit message is 5 characters and is simply the word “runs.” The commit changes 1534 lines of code making the ratio 5/1534 = ~0.003"
Have you found a write up you’d recommend on how the threat actors actually exploited the vulnerabilities?
Hopefully people are being a little circumspect right now with explotation instructions because there are still vulnerable coins out there (particularly from mk4+ wallets, as well ones with passwords that are not strong enough) which have not been taken or rescued yet and may still be rescued by owners.
Aside, I'm somewhat surprised that MISRA (as of 2012 at least) doesn't have a directive related to definedness vs value errors.
Always use -Wundef. For CCAN all config vars use #if and we lint for ifdefs of them, because you have to pick a side here, and sometimes using HAVE_X in C code is useful, so I prefer always-defined.
I wonder if some sort of "taint" analysis could be used - you generate a byte of randomness and trace it back through code until you see all the sources of entropy which were used as input.
Hard problem in general, but with LLMs surely this is possible now, either with them inspecting or by them providing some sort of "formal" proof - this function calls this function which reads this buffer which gets input from here...