Showing posts with label bug. Show all posts
Showing posts with label bug. Show all posts

Thursday, August 6, 2026

The Coldcard incident

A vulnerability in the Coldcard hardware wallet's Bitcoin key generation allowed attackers to steal more than 2,000 BTC (about $120 million). The vulnerability had been introduced by an update on March 2021 and was only exploited a few days ago. The immediate root cause was the code using a less safe/lower entropy random number generator, which allowed the attackers to find wallet addresses and their keys by brute force.

For processes that require high security (like creating bitcoin wallet keys), you have to use cryptographically secure random number generators. On a PC, you might use python's secret module. On small embedded devices, you often don't have access to high-level libraries, so the firmware must implement a secure entropy source itself.

The relevant code depends on a fork of MicroPython maintained by Coldcard (a heavily stripped-down Python implementation for microcontrollers) and on the libngu library, reportedly maintained by Coinkite's CTO. The code expected a function named rng_get(), but Coldcard's implementation exposed differently named functions, so the linker silently selected MicroPython's software PRNG. The problem can be summarized as:

[ Intended Architecture ]
  Entropy Source (STM32 Hardware TRNG) ──► Cryptographically Secure Seed (128-bit)

[ Actual Vulnerable Execution ]
  Silent Check Failure (MICROPY_HW_ENABLE_RNG == 0)
    │
    ▼
  Fallback: MicroPython Software PRNG (Yasmarang)
    │
    ├─► Initialized by: Device Unique ID (UID) + Timer Registers
    └─► Entropy drop: 128-bit (Standard) ──► ~40 bits (Mk2/Mk3)

Because the flawed RNG only had ~40 bits of true entropy, the actual number of possible seeds an attacker had to search was around 2⁴⁰ ≈ 1.1 trillion, large in absolute terms, but small enough to be tractable with modern parallel computing, versus 2¹²⁸ or 2²⁵⁶ for a properly random seed, which is astronomically infeasible.

The general shape of the attack (all 4 steps done offline):

  1. Once the attackers understood the bug (the RNG fell back to a weak software PRNG instead of hardware entropy), they could reconstruct the actual algorithm and its reduced state/seed space rather than treating it as a black box.
  2. Instead of guessing random 256-bit values, the attacker only needs to iterate through the much smaller set of values (1.1 trillion) the flawed generator could actually produce.
  3. For each value, they derive the corresponding Bitcoin address. This uses the standard, public BIP32/BIP39 derivation path (seed → master key → address), completely deterministic and fast to compute.
  4. They compare candidate addresses against the full set of funded/known Bitcoin addresses (a public, downloadable dataset). A match means you've found a real private key controlling real funds.
While iterating through 1.1 trillion values on a single high CPU core could take a year (at 50,000 elliptic-curve (EC) point multiplications per second), a high-end, optimized GPU might do it in about 10 days (at 1 million EC point multiplications per second). Once the search space was precomputed or was being iterated systematically, matching against known funded addresses is comparatively cheap, and the attacker likely had already built the candidate list (private-key-to-address mapping) in advance, which is why the sweep itself (emptying 1,196 addresses in 41 minutes) was so fast and automated.

Details: The weak-entropy path (the software Yasmarang PRNG) only happened because MICROPY_HW_ENABLE_RNG was defined as 0. In libngu (ngu/random.c):

#ifndef MICROPY_HW_ENABLE_RNG
# error "get a HW TRNG plz"
#endif
...
extern uint32_t rng_get(void);      // "I need a function called rng_get()"
#define CHIP_TRNG_32()   rng_get()  // then calls it

This only checks whether the macro exists, not its value. Because it was defined (even as 0), the #error never fired and the build continued. It declares rng_get but does not define (implement) it. It expects whoever builds the final firmware to supply a real function with that exact name. In MicroPython (ports/stm32/rng.c):

#if MICROPY_HW_ENABLE_RNG
// real STM32 hardware TRNG path for rng_get()
#else
// Yasmarang software PRNG fallback for rng_get()
#endif

When the value is 0, the #else branch is taken → the weak software PRNG is compiled in and linked as rng_get(). In Coldcard’s own board RNG files (e.g. stm32/COLDCARD*/rng.c):

#if MICROPY_HW_ENABLE_RNG
#error "this code replaces normal RNG module"
#endif

They intentionally set the macro to 0 so their custom hardware RNG code would be used instead of MicroPython’s. But that custom code never exported the exact rng_get() symbol that libngu expected. Coldcard’s custom RNG code (in files like stm32/COLDCARD/rng.c or stm32/COLDCARD_MK4/rng.c) implements its own, more paranoid hardware TRNG logic. However, it only provided functions under different names, such as:

  • rng_get_or_fault()
  • random_buffer()
  • pyb_rng_get() (the Python-visible version)

It never defined a global function literally named rng_get(). MicroPython’s own ports/stm32/rng.c does define a function named rng_get(). Because MICROPY_HW_ENABLE_RNG was set to 0, the linker picks this weaker version:

uint32_t rng_get(void) {
  return pyb_rng_yasmarang(); // software PRNG
}

When the final firmware binary is built, the linker has to resolve every extern reference.

libngu says: “I need a symbol called rng_get.”

Coldcard’s custom code does not supply a symbol with that exact name.

MicroPython’s code does supply a symbol with that exact name (the software version).

So the linker binds libngu’s call to MicroPython’s software rng_get().

That’s why the custom hardware TRNG code was present in the source tree and even reviewed, yet was never actually used for seed generation. The call path never reached it because the symbol name didn’t match.

If the board-specific RNG file (e.g. stm32/COLDCARD/rng.c or the Mk4 equivalent) had contained a global function literally named rng_get, then the linker would have seen two definitions of the same symbol, would produce a duplicate symbol / multiple definition error and the build would fail.

The solution is to make the ownership of the entropy source explicit and fail-closed, instead of relying on a fragile combination of preprocessor flags and linker symbol resolution. Stop going through libngu / ngu.random for seed generation. Keep the critical path (wallet seed creation) on Coldcard’s own known-good function that already talks directly to the hardware TRNG. Only use ngu.random for non-critical things (if at all).

Another good practice would be to write unit tests to verify the of the pass/key generation. Those tests could look like the following (of course, they should be much more sophisticated than this):

# 1. Generate many seeds using the exact production call path
seeds = set()
n = 50000;
for _ in range(n):
  raw = rng.generate_bytes(32)
  seeds.add(raw)
print(f"len(seeds): {len(seeds)}")
# 2. Basic sanity: should almost never collide
assert len(seeds) == n, "Collisions detected – entropy is far too low"

People are reporting that using widely available AI without access to current events on the internet did not reveal the bug, so this incident is not an obvious coding mistake.

Music: Orhan Gencebay - Hatasız Kul Olmaz

Monday, June 16, 2025

C/C++ header mismatch bug

I encountered a problem where a field in a global struct (myStruct) held a valid value before entering a function foo, but turned into garbage after entering it. When I consulted AI tools, they suggested that foo might be allocating very large local arrays, causing a stack overflow that could corrupt the global structure. Another possibility was an out-of-bounds write elsewhere in the code.

After a week of debugging and trying various solutions—such as increasing the thread's stack size—I discovered the root cause: The function foo was defined in a C library with multiple versions. Each version resided in a different folder but had the same file names. Which folder was used depended on a #define. I was including the header from one version of the library, but linking against the implementation from another. If the struct definitions had matched, this wouldn’t have caused an issue, but they differed—evident from the differing sizeof(myStruct). As a result, myStruct was interpreted using the wrong layout, leading to corrupted values from an incorrect memory region.

Sunday, June 15, 2025

C++ pointer bug

This C++ code has a significant bug that will cause undefined behavior:
#include <iostream>
class A {
  public:
    int val;
};
void reset(A *p_a) {
  if (p_a != NULL) {
      delete p_a;
  }
  p_a = new A();
}
int main() {
  A *p_a = new A();
  p_a->val = 5;
  std::cout << "Before reset, p_a->val:" << p_a->val << "\n";
  reset(p_a);
  std::cout << "After reset, p_a->val:" << p_a->val << "\n";
  return 0;
}

The reset function receives a copy of the pointer p_a, not a reference to it. When you modify p_a inside the function (with p_a = new A()), you're only changing the local copy - the original pointer in main() remains unchanged. What actually happens:

  1. p_a in main() points to an A object with val = 5 
  2. reset() receives a copy of this pointer 
  3. reset() deletes the original object (memory is freed) 
  4. reset() creates a new object, but assigns it only to the local copy 
  5. The original p_a in main() still points to the deleted memory 
  6. Accessing p_a->val after reset() is undefined behavior (accessing freed memory) 
The Fix: Pass the pointer by reference using a pointer-to-pointer or reference-to-pointer:
//Reference to pointer
void reset(A *&p_a) {
  if (p_a != nullptr) {
    delete p_a;
  }
  p_a = new A();
  // Call with: reset(p_a);

An even better fix is to use smart pointers, which removes the necessity for the reset function:

auto p_a = std::make_unique<A>();

You can detect such problems by enabling AddressSanitizer (ASAN) in Visual Studio:

  1. Right-click your project → Properties
  2. Go to Configuration Properties → C/C++ → General
  3. Set Enable Address Sanitizer to Yes (/fsanitize=address)
  4. Go to Configuration Properties → C/C++ → Optimization
  5. Set Optimization to Disabled (/Od) for better debugging
  6. Set Whole Program Optimization to No
  7. Go to Configuration Properties → C/C++ → Debug Information Format
  8. Set to Program Database (/Zi) or Program Database for Edit & Continue (/ZI)
In Eclipse CDT:

  1. Open your C/C++ project in Eclipse CDT
  2. Right-click project → Properties
  3. Navigate to C/C++ Build → Settings
  4. Under Tool Settings:
    1. GCC C++ Compiler → Miscellaneous
    2. GCC C Compiler → Miscellaneous
    3. Add to "Other flags": -fsanitize=address -g -O1
  5. Project Properties → C/C++ Build → Settings
  6. GCC C++ Linker → Miscellaneous
  7. Add to "Other objects": -fsanitize=address

Wednesday, February 12, 2025

Longitude convention and octal values

Recently, I encountered a bug in code that handled longitude values, where 33 degrees longitude is typically written as 033. Upon investigation, I discovered that Java was calculating 033 + 1 as 28 instead of 34. This happened because, when we write longitude values like "033" in geographic notation, we mean decimal 33 degrees. However, if we write it directly in Java, C++, or similar languages, it is interpreted as octal 33 (which equals decimal 27) due to the leading zero!

This is a classic example of why it's important to be careful when working with domain-specific number formats - what makes sense in geographic notation can have unexpected behavior when directly translated to programming language syntax!

Thursday, December 28, 2023

The unbearable lightness of C

We have a Simulink project from which I generate C code to use in a Visual Studio C++ project. The Simulink project works fine, I can build the C code without any errors, but when I ran the C executable, I got an access violation error due to trying to write to address 0x0. The C project was working fine for previous versions. 

I initially identified the revision where this error first appeared. I reviewed the changed code and couldn't find anything wrong.

After a week of debugging I found out that it was due to an off-by-one error; An array was defined with size 47 using Simulink function ssSetNumDiscState(S, 47) in mdlInitializeSizes(...), but later in function mdlInitializeConditions(...), a for loop with upper bound of 48 was executed which resulted in writing to the memory adjacent to the allocated section for that array. 
static void mdlInitializeConditions(SimStruct *S) {
    real_T *states = ssGetRealDiscStates(S);
    for (int i=0; i < 48; i++) {
    	*(states + i) = 0;
    }
}
It has nothing to do with the latest change in the sense that it was not related to the logic of the change. Instead, that change altered the memory mapping of the build, putting another array (TUBufferPtr) after the first and the overflow caused the value at TUBufferPtr[0] to become 0x0 (NULL). When the program tried to write to the address represented by TUBufferPtr[0], it naturally caused an access violation because writing to address 0x0 is not allowed.
When I looked at the repository history, I saw that this error was introduced 3 years ago and for all these years, it did not become visible! The access violation only occurred when other unrelated code updates caused the compiler to arrange memory slightly differently.

This is also one of the reasons why sometimes C programs behave differently between debug and release builds or on different versions (service packs) of the same operating system. That discrepancy is an indication of an error hiding somewhere in your program. Another way such an error can become visible is when you have it in your C++ DLL that you call from your Java program. One day you update your JDK and your program crashes because DLL and JVM share the same memory space. Naturally, your first inclination is to blame the JDK update but in reality there is a buffer overflow in your DLL code.

You can never say for sure that modifying a code in module A won't have an effect on an unrelated module B. As long as these modules are in the same process, i.e. use the same memory, the compiler might put them side by side and an error in A can overflow to B. Functionally distant modules can become "close relatives" in memory address space.

In a way, I was fortunate that the overwrite contained zero values. If it had used some value that was a valid address for the application to write to, it would cause seemingly random behavior and would have been a lot more fun (!)

Note that a typical static code analyzer would not able to catch this problem because we are defining the data structure with Simulink specific ssSetNumDiscState and getting it with ssGetRealDiscStates functions.

I solved the problem by adding #define NUMBER_OF_DISCRETE_STATES (47) and using that define in both ssSetNumDiscState() and mdlInitializeConditions(). This case study also serves as a cautionary tale illustrating why you should use defined constants rather than magic numbers.

In my nightly automated tests I was only checking if exe was generated which only proved that it compiled. I added running the exe and checking if it finished successfully because an access violation can only occur at runtime.