5 ms·

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

94 points by Funes- 2mo ago

anonymousiam 2mo ago
This post assumed that the author of the "runs" commit was not acting maliciously. Has that been established? It's not mentioned in this article.
- Sharlin 2mo ago
  Hanlon’s razor.
- eru 2mo ago
  Also: writing this stuff in C just begs for bugs. LLM agents are cheap and good enough that you can write in eg Lean or whatever. Or at least write it in Rust.
  - thin_carapace 2mo ago
    I get the feeling that the bugs found in this instance can be more directly attributed to the author having zero experience operating a microcontroller
    - scott_w 2mo ago
      Honestly, I'm going to be a bit harsher: I think they're either: 1. A novice who doesn't know how to debug issues. 2. Totally incompetent and copying code from Stack Overflow. I don't think it's specific to microcontrollers, either, the error from the C compiler is something that a competent programmer would be able to interpret, or at least see it as a signal to get someone more experienced in the domain to learn about.
      - nullc 2mo ago
        > the error from the C compiler is something that a competent programmer would be able to interpret, contrary to the post, this was almost certainly not an issue of compilers throwing errors. Reduced familarity with C could have played a role, but given the number of C experts that looked at this knowing there was an error and still misidentified the cause I don't think we need to reach for that powerful an explanation. Confusion of definedness vs value check would make for a fine underhanded C entry. The flaw was not particularly clear from the source... and most common QA procedures could not distinguish a PRNG from TRNG once the error happened.
        scott_w 2mo ago
        Fair point: I was using the information in the article to form my opinion. I still think that, if the developer in question ignored a compiler error as described, that’s a serious competence issue. It’s harder to spot after the fact because any reviewers wouldn’t have had a chance to see the compiler output. The article makes the fair point of the size of the change and the complete lack of information in the commit message, which should have set off alarm bells…
      - eru 2mo ago
        Of course, being a total novice and claiming to produce a super secure gadget are not to things you want to go together.
      - Scoundreller 1mo ago
        Hey now, I'm totally incompetent, so my code writes entries in a log file for virtually everything because otherwise I have no idea if it's doing what I expect. Would've caught some critical function not being called at all.
  - jgilias 2mo ago
    The bug has been up since before the LLM shift.
  - jki275 2mo ago
    C isn't the problem here. one might argue that the combination of micro python and C and the person writing it having no understanding of how either one works is the problem. Also running python on a microcontroller to do cryptography is fucking insane.
    - nullc 2mo ago
      > Also running python on a microcontroller to do cryptography is fucking insane. I feel like I should confess to snubbing this (and some other) hardware wallet projects for the fact it used micropython. I think it's hard to draw the line between systems programmer snobbery and good advice... and I feel a little like a cop guilty of stop-and-frisk on the basis of skin color. Because in general the ideas necessary to produce reliable software aren't well understood or agreed on there is a risk of letting style preferences which are only correlated with good engineering but aren't causative of good engineering get mistaken-- and this can cause errors in both directions, both mistaking stuff as good because it uses the "right" tools, or mistaking something as bad because it doesn't.
      - jki275 1mo ago
        I think some things telegraph competence and some the opposite -- and our experiences over the years tend to shape what we see that way.
- karamanolev 2mo ago
  Per the article, not my opinion. It has been established that "I didn't do it on purpose" is not enough in some cases. You have to _not do it_ _on purpose_. That "fake it till you make it", in this case, understanding what your code does, is perfectly fine in some contexts, not in others, like this one.
- silvestrov 2mo ago
  When looking through all the other commits the user has made: they all lack a good commit message. This is systematic for that user. Either the user needs to improve or let go. The organization needs to learn that good commit messages are a requirement for anything to do with crypto and payment systems.
  - inigyou 2mo ago
    The commit message is just a surface level manifestation of a deeper issue. Fixing the commit message does not fix the issue.
- anonymars 2mo ago
  Ironic: > NEW POLICY If I don't know you, I don't merge you! Thanks XZ! https://github.com/switck/libngu https://github.com/switck/libngu
fenestella 2mo ago
[flagged]
ChimpWithHat 2mo ago
Very good writeup and an astonishing level of negligence for an extremely security critical piece of code. The losses are going to surpass 100 million, these guys should be thrown in jail.
- jonathanlydall 2mo ago
  This does not seem to be a good write up at all, see nullc’s explanation in this discussion of (what seems to me) the actual root cause whereas this article spent a lot of words making the argument that bad commit messages must mean bad code, later using the assertion to try “prove” what the technical problem was. I agree that a bad commit message combined with a big commit is a huge smell, but as it’s just metadata for the code and not the code itself, it is not in itself evidence in any form. That being said, probably okay after explaining the technical code issues for the author add a couple of a sentences about how all this was was part of 1000s of a lines of code changed in commits with useless messages which demonstrates generally bad code hygiene.
  - smithcoin 2mo ago
    I think what is the author is getting at is that if you are cavalier in your commit messages when the stakes are low, chances are you aren’t taking writing code that handles money seriously. Made me think of some merges I could have taken more seriously.
nullc 2mo 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 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).
dmitrygr 2mo ago
>Micropython creates the illusion embedded developers do not need to understand C, their CPU, or other advanced concepts to do embedded programming. Amen.
londons_explore 2mo ago
I am getting suspicious of "random number generator not actually returning random numbers" bugs.... They are the perfect bug for someone trying to "accidentally" make a secure system insecure. I wonder if bribe money was involved or three letter agencies...
- dist-epoch 2mo ago
  And they waited 5 years to exploit it? And do you think the NSA desperately needed $90 mil (the amount stolen so far), so they created this elaborate backdoor? Or were they waiting until Satoshi or Binance decided to deposit a billion dollars into this wallet? All the employees which touched the code will be on watch lists now anyway. And sudden new house or yacht will be immediately flagged.
  - londons_explore 2mo ago
    Need not be the NSA. Might just be some regular thief. The longer they wait, the more they get to steal after all.
  - josephg 2mo ago
    > And do you think the NSA desperately needed $90 mil (the amount stolen so far) If this was done as part of an operation, we would have no idea what it was actually used for. It’s very unlikely that the crypto wallet thefts we know about were the intended target.
nonfamous 2mo ago
From the Twitter advisory on the issue being referenced here [1]: >>> To every other developer: we believe this is a sober reality of the new AI paradigm. AI-assisted code review can now find latent bugs at a speed that is outpacing even the industry’s most seasoned experts. If your firmware is open-source or has ever been public, assume it's already being read by attackers and defenders alike. Kinda turns the “many eyes” principle of OSS on its head, eh? [1] https://x.com/nvk/status/2083216713693151552?s=61 https://x.com/nvk/status/2083216713693151552?s=61
- dist-epoch 2mo ago
  If the industry most seasoned experts are not using LLMs right now to front-run the discovery of vulnerabilities in the code they are responsible for, they are not experts, but clowns.
  - catlifeonmars 2mo ago
    A lot of the vulnerabilities being discovered are low hanging fruit. An LLM could discover them, but so could a human. Let me amend your statement: If the industry most seasoned experts are not engaging in regular security reviews of their firmware, they are not experts, but clowns. LLMs are useful for this but not absolutely necessary to address the core issue.
- abecedarius 2mo ago
  If your source is not open that's barely a speedbump, given a binary. At least that's my impression of the current state.
  - nullc 2mo ago
    This flaw would probably be easier to find from the binary: the source itself was misleading, but if you analyized the binary you'd find that there was no access to the hwrng at all (assuming the dead code got eliminated by the compiler) or that the only function that accessed it was never called. One lesson out of this is that now that AI has made a certain grade of review cheap is that it would be useful to perform security review both against the source code and against the resulting binary.
PunchyHamster 2mo ago
> The commit message is 235 characters, and the commit changes 15 lines of code. That’s a ratio of 235/15 = ~16. the commit message is not 235 character long, the commit message is this > splice-script: Test for msat chan balances > splice-script: Round channel balances down author mistakes commit message for PR which doesn't get persisted into code's history and don't even contain the ID of bug being fixed. So yeah it is better but nothing to actually brag about
hypeatei 2mo ago
> You, as a developer working on Bitcoin, need to take your time to understand your changes, document them clearly, and verify they do what you think. Or, you could realize that using a financial technology which relies on no software bugs ever is probably flawed at its core. "Not your keys, not your coins" was (is?) the zeitgeist but even people who did the "right thing" and used a cold wallet still got screwed. I don't know why anyone would keep their wealth in something that requires so much technical excellence.
- inigyou 2mo ago
  I take it you get your paycheck in cash?
  - anonymars 2mo ago
    Banking-related errors have recourse
  - hypeatei 2mo ago
    No, via direct deposit. I had a co-worker once who miskeyed his routing+account numbers and guess what? He still got his money a few days later and it wasn't lost to the void. There is no blockchain, seed phrases, nor cold wallets required for this amazing feat of money transfer/remediation to take place.
    - inigyou 2mo ago
      Great! Maybe the government should stop over regulating the banking system so the same protection will also be available to someone who buys drugs.
      - anonymars 2mo ago
        Wait, is the argument now that cryptocurrency only exists and is useful for buying drugs? It's tough to keep track of what point you're trying to make
      - 2mo ago
koolba 2mo ago
> 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. No! Blanket statements like this is how you end up with 40 pages of slop AI comments in PRs that nobody reads. Comments should be terse and meaningful. They should document surprising behavior or choices. The less comments you have, the more meaningful each one becomes because your time and eyeballs are limited as well.
- coldbrewed 2mo ago
  We shouldn't tolerate slop in any form, especially not in commit comments. What we should care about is communicating design decisions, intent, and especially we should explain concerns that can't be intuited from reading the code. A longer, more detailed comment that explains some significant detail can be especially valuable but it should earn the weight that it carries. I think we're all in agreement that a one word "runs" commit and a 6 paragraph slop commit both fail to communicate effectively.
deleted 2mo ago
[deleted]
mangudai 2mo ago
[flagged]

94 points by Funes- 2mo ago

This post assumed that the author of the "runs" commit was not acting maliciously. Has that been established? It's not mentioned in this article.

Sharlin 2mo ago
Hanlon’s razor.
eru 2mo ago
Also: writing this stuff in C just begs for bugs. LLM agents are cheap and good enough that you can write in eg Lean or whatever. Or at least write it in Rust.
- thin_carapace 2mo ago
  I get the feeling that the bugs found in this instance can be more directly attributed to the author having zero experience operating a microcontroller
  - scott_w 2mo ago
    Honestly, I'm going to be a bit harsher: I think they're either: 1. A novice who doesn't know how to debug issues. 2. Totally incompetent and copying code from Stack Overflow. I don't think it's specific to microcontrollers, either, the error from the C compiler is something that a competent programmer would be able to interpret, or at least see it as a signal to get someone more experienced in the domain to learn about.
    - nullc 2mo ago
      > the error from the C compiler is something that a competent programmer would be able to interpret, contrary to the post, this was almost certainly not an issue of compilers throwing errors. Reduced familarity with C could have played a role, but given the number of C experts that looked at this knowing there was an error and still misidentified the cause I don't think we need to reach for that powerful an explanation. Confusion of definedness vs value check would make for a fine underhanded C entry. The flaw was not particularly clear from the source... and most common QA procedures could not distinguish a PRNG from TRNG once the error happened.
      - scott_w 2mo ago
        Fair point: I was using the information in the article to form my opinion. I still think that, if the developer in question ignored a compiler error as described, that’s a serious competence issue. It’s harder to spot after the fact because any reviewers wouldn’t have had a chance to see the compiler output. The article makes the fair point of the size of the change and the complete lack of information in the commit message, which should have set off alarm bells…
    - eru 2mo ago
      Of course, being a total novice and claiming to produce a super secure gadget are not to things you want to go together.
    - Scoundreller 1mo ago
      Hey now, I'm totally incompetent, so my code writes entries in a log file for virtually everything because otherwise I have no idea if it's doing what I expect. Would've caught some critical function not being called at all.
- jgilias 2mo ago
  The bug has been up since before the LLM shift.
- jki275 2mo ago
  C isn't the problem here. one might argue that the combination of micro python and C and the person writing it having no understanding of how either one works is the problem. Also running python on a microcontroller to do cryptography is fucking insane.
  - nullc 2mo ago
    > Also running python on a microcontroller to do cryptography is fucking insane. I feel like I should confess to snubbing this (and some other) hardware wallet projects for the fact it used micropython. I think it's hard to draw the line between systems programmer snobbery and good advice... and I feel a little like a cop guilty of stop-and-frisk on the basis of skin color. Because in general the ideas necessary to produce reliable software aren't well understood or agreed on there is a risk of letting style preferences which are only correlated with good engineering but aren't causative of good engineering get mistaken-- and this can cause errors in both directions, both mistaking stuff as good because it uses the "right" tools, or mistaking something as bad because it doesn't.
    - jki275 1mo ago
      I think some things telegraph competence and some the opposite -- and our experiences over the years tend to shape what we see that way.
karamanolev 2mo ago
Per the article, not my opinion. It has been established that "I didn't do it on purpose" is not enough in some cases. You have to _not do it_ _on purpose_. That "fake it till you make it", in this case, understanding what your code does, is perfectly fine in some contexts, not in others, like this one.
silvestrov 2mo ago
When looking through all the other commits the user has made: they all lack a good commit message. This is systematic for that user. Either the user needs to improve or let go. The organization needs to learn that good commit messages are a requirement for anything to do with crypto and payment systems.
- inigyou 2mo ago
  The commit message is just a surface level manifestation of a deeper issue. Fixing the commit message does not fix the issue.
anonymars 2mo ago
Ironic: > NEW POLICY If I don't know you, I don't merge you! Thanks XZ! https://github.com/switck/libngu https://github.com/switck/libngu

fenestella 2mo ago

[flagged]

ChimpWithHat 2mo ago

Very good writeup and an astonishing level of negligence for an extremely security critical piece of code. The losses are going to surpass 100 million, these guys should be thrown in jail.

jonathanlydall 2mo ago
This does not seem to be a good write up at all, see nullc’s explanation in this discussion of (what seems to me) the actual root cause whereas this article spent a lot of words making the argument that bad commit messages must mean bad code, later using the assertion to try “prove” what the technical problem was. I agree that a bad commit message combined with a big commit is a huge smell, but as it’s just metadata for the code and not the code itself, it is not in itself evidence in any form. That being said, probably okay after explaining the technical code issues for the author add a couple of a sentences about how all this was was part of 1000s of a lines of code changed in commits with useless messages which demonstrates generally bad code hygiene.
- smithcoin 2mo ago
  I think what is the author is getting at is that if you are cavalier in your commit messages when the stakes are low, chances are you aren’t taking writing code that handles money seriously. Made me think of some merges I could have taken more seriously.

nullc 2mo 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 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).

dmitrygr 2mo ago

>Micropython creates the illusion embedded developers do not need to understand C, their CPU, or other advanced concepts to do embedded programming. Amen.

londons_explore 2mo ago

I am getting suspicious of "random number generator not actually returning random numbers" bugs.... They are the perfect bug for someone trying to "accidentally" make a secure system insecure. I wonder if bribe money was involved or three letter agencies...

dist-epoch 2mo ago
And they waited 5 years to exploit it? And do you think the NSA desperately needed $90 mil (the amount stolen so far), so they created this elaborate backdoor? Or were they waiting until Satoshi or Binance decided to deposit a billion dollars into this wallet? All the employees which touched the code will be on watch lists now anyway. And sudden new house or yacht will be immediately flagged.
- londons_explore 2mo ago
  Need not be the NSA. Might just be some regular thief. The longer they wait, the more they get to steal after all.
- josephg 2mo ago
  > And do you think the NSA desperately needed $90 mil (the amount stolen so far) If this was done as part of an operation, we would have no idea what it was actually used for. It’s very unlikely that the crypto wallet thefts we know about were the intended target.

nonfamous 2mo ago

From the Twitter advisory on the issue being referenced here [1]: >>> To every other developer: we believe this is a sober reality of the new AI paradigm. AI-assisted code review can now find latent bugs at a speed that is outpacing even the industry’s most seasoned experts. If your firmware is open-source or has ever been public, assume it's already being read by attackers and defenders alike. Kinda turns the “many eyes” principle of OSS on its head, eh? [1] https://x.com/nvk/status/2083216713693151552?s=61 https://x.com/nvk/status/2083216713693151552?s=61

dist-epoch 2mo ago
If the industry most seasoned experts are not using LLMs right now to front-run the discovery of vulnerabilities in the code they are responsible for, they are not experts, but clowns.
- catlifeonmars 2mo ago
  A lot of the vulnerabilities being discovered are low hanging fruit. An LLM could discover them, but so could a human. Let me amend your statement: If the industry most seasoned experts are not engaging in regular security reviews of their firmware, they are not experts, but clowns. LLMs are useful for this but not absolutely necessary to address the core issue.
abecedarius 2mo ago
If your source is not open that's barely a speedbump, given a binary. At least that's my impression of the current state.
- nullc 2mo ago
  This flaw would probably be easier to find from the binary: the source itself was misleading, but if you analyized the binary you'd find that there was no access to the hwrng at all (assuming the dead code got eliminated by the compiler) or that the only function that accessed it was never called. One lesson out of this is that now that AI has made a certain grade of review cheap is that it would be useful to perform security review both against the source code and against the resulting binary.

PunchyHamster 2mo ago

> The commit message is 235 characters, and the commit changes 15 lines of code. That’s a ratio of 235/15 = ~16. the commit message is not 235 character long, the commit message is this > splice-script: Test for msat chan balances > splice-script: Round channel balances down author mistakes commit message for PR which doesn't get persisted into code's history and don't even contain the ID of bug being fixed. So yeah it is better but nothing to actually brag about

hypeatei 2mo ago

> You, as a developer working on Bitcoin, need to take your time to understand your changes, document them clearly, and verify they do what you think. Or, you could realize that using a financial technology which relies on no software bugs ever is probably flawed at its core. "Not your keys, not your coins" was (is?) the zeitgeist but even people who did the "right thing" and used a cold wallet still got screwed. I don't know why anyone would keep their wealth in something that requires so much technical excellence.

inigyou 2mo ago
I take it you get your paycheck in cash?
- anonymars 2mo ago
  Banking-related errors have recourse
- hypeatei 2mo ago
  No, via direct deposit. I had a co-worker once who miskeyed his routing+account numbers and guess what? He still got his money a few days later and it wasn't lost to the void. There is no blockchain, seed phrases, nor cold wallets required for this amazing feat of money transfer/remediation to take place.
  - inigyou 2mo ago
    Great! Maybe the government should stop over regulating the banking system so the same protection will also be available to someone who buys drugs.
    - anonymars 2mo ago
      Wait, is the argument now that cryptocurrency only exists and is useful for buying drugs? It's tough to keep track of what point you're trying to make
    - 2mo ago

koolba 2mo ago

> 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. No! Blanket statements like this is how you end up with 40 pages of slop AI comments in PRs that nobody reads. Comments should be terse and meaningful. They should document surprising behavior or choices. The less comments you have, the more meaningful each one becomes because your time and eyeballs are limited as well.

coldbrewed 2mo ago
We shouldn't tolerate slop in any form, especially not in commit comments. What we should care about is communicating design decisions, intent, and especially we should explain concerns that can't be intuited from reading the code. A longer, more detailed comment that explains some significant detail can be especially valuable but it should earn the weight that it carries. I think we're all in agreement that a one word "runs" commit and a 6 paragraph slop commit both fail to communicate effectively.

deleted 2mo ago

[deleted]

mangudai 2mo ago

[flagged]