crypto: freeze crypto module to prevent malicious code from controlling key generation

[chjj]

Currently, a malicious module can do:

const crypto = require('crypto');

crypto.randomBytes = function randomBytes(size) {
  const bytes = Buffer.alloc(size);

  bytes.write('hello, i just stole your private key!');

  return bytes;
};

A more sophisticated version of this attack would seed a PRNG with a predictable key to make the resulting bytes look random.

I feel like randomBytes is particularly vulnerable here, but this also applies to any key generation method as well.

If I'm supposed to be generating unpredictable secret data, I want to be 100% certain that crypto.randomBytes is calling out to the OpenSSL RNG, not user code.

The solution would be a one line fix of Object.freeze(crypto).

I realize this is a breaking change, and existing code may be using this "feature" for non-malicious reasons, but the change here is probably worth the breakage.

[addaleax]

What kind of malicious modules are you worried about here? I don’t want to sound dismissive, but if you’re running untrusted JS code, it can do anything like this already, whether it makes changes to builtin modules or not. (For example: Any module can observe what the current JS code does through the inspector API.)

And in particular crypto.randomBytes might seem like something that has reasonable use cases for overriding – for example, somebody might want to implement fuzz testing by providing their own seeded RNG, and use that for their Node.js applications, in order to achieve reproducible results.

[addaleax]

I guess @nodejs/security-triage would be the team to ping here?

[mscdex]

I think the same thing could be said for monkey-patching any part of node core, not just crypto. Running untrusted code in a non-locked down environment (especially with direct access to node APIs) is a bad idea in general.

[chjj]

@addaleax @mscdex

I figured this might be the response, so I'll try to make a good argument for this...

The security implications here are different from an attacker monkey patching another part of node core. Altering any other part of the core API is short-lived. For example, say someone monkey-patched the crypto.verify function. The application will now accept an invalid signature: that's bad but it is confined to the life-span of the process. The RNG is unique in that keys are persistent in many cases. A user may unknowingly pass a tainted key to another application. I can't think of any other part of node core where this kind of attack persists beyond the process state (beyond the machine's state, depending on what is done with the key).

In the NPM ecosystem there have already been a couple of attacks trying to steal bitcoin keys by monkey patching wallet modules. crypto.randomBytes is often used to generate these keys and seems like a major target for this kind of attack. It's probably actually an easier target since it doesn't require any knowledge of the node_modules directory structure (require('crypto') works everywhere). It also doesn't require any phoning home like other theft attempts since the PRNG key would be known ahead of time -- less exposure for the attacker.

Running untrusted code in a non-locked down environment (especially with direct access to node APIs) is a bad idea in general.

I don't think anyone is advocating running untrusted code. I'm specifically referring to the context of an attack -- a situation in which untrusted code is run against the user's wishes.

edit because I want to emphasize the severity of this issue:

The attack I described works without any network connection at all, meaning that even if someone were being safe and doing signing/generation on an air-gapped machine, their keys can still be stolen. There is no other situation in node.js where data can be stolen from a user without a network connection.

[mscdex]

I can't think of any other part of node core where this kind of attack persists beyond the process state (beyond the machine's state, depending on what is done with the key).

Stealing data sent/received over http(s) comes to mind.

I'm specifically referring to the context of an attack -- a situation in which untrusted code is run against the user's wishes.

I don't understand this. Against the user's wishes? It sounds like you're running untrusted code from the get-go then?

I also don't understand the use case being described. If someone is stealing keys, at some point they would need to be transferred to the attacker no?

[sam-github]

Instead of monkey-patching a frozen crypto.randomBytes, monkey-patch fs, and whenever a PEM key is written.. replace it with a fixed one. Or, monkey patch http, and replace the output. Or, monkey-patch the function that is calling crypto.randomBytes, and replace it.

This strikes me as more giving the appearance of security without substance. On the other hand, anything that makes attacks even slightly more difficult could be described as "defence in depth". But there's a fine line between DiD and snake-oil in depth.

[chjj]

Stealing data sent/received over http(s) comes to mind.

See my edit as I clarified this. This attack works without a network connection. There is no other high-stake data theft possibility that works without a network connection.

I don't understand this. Against the user's wishes? It sounds like you're running untrusted code from the get-go then?

There have already been attacks like this in the NPM ecosystem. I'm referring to situations like this: https://github.com/bitpay/copay/issues/9346

I also don't understand the use case being described. If someone is stealing keys, at some point they would need to be transferred to the attacker no?

No. If the attacker seeds their malicious RNG with a predictable key, they know every key the user will generate.

[bnoordhuis]

While I understand the sentiment, security measures like what you're proposing are very easy to defeat with require('inspector') or by loading a native module. Debuggers and add-ons can do everything.

It doesn't make attacks materially harder and would only give a false sense of security.

[chjj]

I suppose I see it differently: I understand node is not meant to be a secure under these conditions, but I want attacks to be as high-effort as possible. I know there is no perfect solution for this issue, but I see solutions as a red herring. I never look for solutions, only mitigations. This is a one-line mitigation to get rid of some low-hanging fruit.

Anyway, it seems like people are not on board with this. I guess I'll be figuring out different mitigations for my own code. As an aside: I was previously directly calling out to RAND_bytes via a binding, but that's no longer possible due to some misbehavior in node-gyp (very long story).

Feel free to close.