When random.bytes() runs but doesn't work

OpEdWhen random.bytes() runs but doesn't workWhat a commit message tells us about the recent COLDCARD bugDusty DaemonAug 01, 20263234ShareThis is a guest post from noted Core-Lightning developer, ddustin, who dug into the Coldcard firmware commit history to uncover what happened and why the code failed.IntroI began investigating the Coldcard hack and was immediately shocked. I need to explain why.When we developers work on code, we organize or code changes into changesets we call “commits.” The purpose of doing so is to show a clear history of what code was changed including why and how.This is done precisely for instances like this where it appears Bitcoiner’s funds are being stolen en masse, so we can investigate and understand exactly how it could happen.Good developers write clear commit messages, written notes that go along with the code changes that explain what the specific change is accomplishing.To make a clear commit message, you typically want to the commit to represent a smaller change of code, so there’s less to comment on.A good goal as a developer is a high commit message to change ratio. The more lines of code that you change, the more comments explaining why you’re changing the code. More message and less code changes per commit is generally a good idea.Here is an example chosen randomly from some of my own work.The commit message is 235 characters, and the commit changes 15 lines of code. That’s a ratio of 235/15 = ~16.In the cold card, the commit that introduced the low entropy bug is here.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.003This is an atrociously bad comment to code change ratio.There are some rare instances where a low comment ratio is justifiable -- but changing the most important part of the code is not one of those cases!Code that touches functions critical to the security of the project need to have a higher ratio of comments to changes and more stringent review.The second commit contributing to the weak entropy issue on Coldcards is here.The commit message is 1 character: simply the character “x.” The commit changes ~1000 lines of code making the ratio 1/1000 = ~0.001The IssueIn the commit titled “runs” (ratio: ~0.003), it appears they are importing and configuring C code to make custom micropython code work on the STM32, the board that all Coldcards run on.STM32 is the most common CPU for small devices like this, and configurations like what this commit introduces are common.In the ‘runs’ commit, the hardware RNG (Random Number Generation) was disabled with the following line of code. #define MICROPY_HW_ENABLE_RNG (0)This is what caused the bug. Setting this value to zero tells the default micropython rng code to not use the hardware RNG device and that it should use the Yasmarang RNG instead. The developer added an inline comment “explaining” this change// We have our own version of this code.The COLDCARD version of this code appears to be in reference to the functions added in rng.h and rng.crng.hMP_DECLARE_CONST_FUN_OBJ_0(pyb_rng_get_obj);MP_DECLARE_CONST_FUN_OBJ_1(pyb_rng_get_bytes_obj);These appear to be an attempt to override the stm32 rng library’s `pyb_rng_getobj` function. This approach ran into trouble. The stm32 rng.c file already defines the pyb_rng_et_obj variable and sets the value to `pyb_mg_get`. You can’t have two definitions of the same variable and have it compile.rng.cMP_DEFINE_CONST_FUN_OBJ_0(pyb_rng_get_obj, pyb_rng_get);Expanding the macro and thinking of it logically is just this pseudocode:var pyb_rng_get_obj = pyb_rng_getCommit `37e4af5` adds a custom rng.c file, where he copy pasted the same macro definitionMP_DEFINE_CONST_FUN_OBJ_0(pyb_rng_get_obj, pyb_rng_get);There is now no way for this code to compile. It appears that the developer is naively trying to override the `pyb_rng_get_obj` variable by creating a duplicate version of the variable. C does not work this wa