specs-compliance-issue: crypto.subtle.deriveKey implicit key leak on global Promise pollution

[WebReflection]

Version

v24.0.1

Platform

Linux lunar-lake 6.6.87.2-microsoft-standard-WSL2

Subsystem

No response

What steps will reproduce the bug?

The issue.js file contains defensive code so that if this is imported before anything else it should never allow malicious code to intercept any asynchronous operation around encryption (or decryption in the real world):

const {
  Function: { prototype: { apply, call } },
  Promise: { prototype: { then } },
  String: { fromCharCode },
  Uint8Array,
  crypto: { subtle },
  btoa,
} = globalThis;

const bound = (_, $) => _[$].bind(_);
const applier = call.bind(apply);
const caller = call.bind(call);

const encode = buffer => btoa(applier(fromCharCode, null, new Uint8Array(buffer)));
const encoder = bound(new TextEncoder, 'encode');

const withResolvers = bound(Promise, 'withResolvers');
const randomUUID = bound(crypto, 'randomUUID');
const importKey = bound(subtle, 'importKey');
const deriveKey = bound(subtle, 'deriveKey');
const encrypt = bound(subtle, 'encrypt');

const name = 'PBKDF2';
const method = 'AES-CBC';
const iterations = 8192;
const SHA = 256;

export default (
  password = randomUUID(),
  iv = new Uint8Array(16),
) => {
  const salt = encoder(password);
  const { resolve, promise } = withResolvers();
  let key;

  console.log('A');
  caller(
    then,
    importKey(
      'raw',
      salt,
      { name },
      false,
      ['deriveBits', 'deriveKey']
    ),
    _ => {
      console.log('B');
      // the leak!
      caller(
        then,
        deriveKey(
          {
            name,
            salt,
            iterations,
            hash: `SHA-${SHA}`
          },
          _,
          { name: method, length: SHA },
          true,
          ['encrypt', 'decrypt']
        ),
        _ => {
          console.log('C');
          key = _;
          resolve();
        },
      );
    },
  );

  // return a function that will encrypt through that key
  return async value => {
    // no reason to be defensive, this is `undefined`
    await promise;
    // now use the key to encrypt without leaking it
    return caller(
      then,
      encrypt(
        { name: method, iv },
        key,
        encoder(value),
      ),
      encode,
    )
  };
};

Now the test that reveals BUSTED while Bun or any browser would never reach that point because that point is never possible to reach (accordingly with the written developer intent):

import password from './issue.js';

const encrypt = password('1234567890');

// enforce environment pollution
if (true) {
  const { then } = Promise.prototype;
  Promise.prototype.then = function (after, ...rest) {
    return then.call(this, value => {
      if (value instanceof CryptoKey) console.log('⚠️ BUSTED ⚠️', value);
      return after(value);
    }, ...rest);
  };
}

console.log(await encrypt('Hello, world!'));
// 'zaIZGmACZxAh/zBwyYm7CA=='

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior? Why is that the expected behavior?

The expected behavior is A B C zaIZGmACZxAh/zBwyYm7CA== as the only things outputted in shell/console, which is the case for both Web browsers and Bun but in NodeJS any malicious code that overrides the global Promise.prototype.then could retrieve a reference to the key used to encrypt and, eventually, decrypt that value, diverging from web standards specs compliance:

What do you see instead?

A
B
⚠️ BUSTED ⚠️ CryptoKey {
  type: 'secret',
  extractable: true,
  algorithm: { name: 'AES-CBC', length: 256 },
  usages: [ 'encrypt', 'decrypt' ]
}
C

The key leaks through basic Promise.prototype.then pollution but apparently only in the deriveKey case, although that's good enough to retrieve something that no foreign code should ever be able to retrieve because:

• the key is not held or referenced or attached to anything ever
• the key does not pass through await or any explicit/implicit then invocation
• the leak is not visible/possible in browsers or other runtimes so NodeJS here is inconsistent (and leaky)

Additional information

When code is carefully written to avoid lazy poisoned environments it's very hard to test that no leaks actually happened because internally NodeJS seems to return a Promise without enforcing a non-leaky trap for that derived key so that evil patch on top of Promise.prototype.then becomes effective.

[WebReflection]

P.S. if you drop if (value instanceof CryptoKey) from the evil code you'll notice that while browsers and Bun leak only the await that receives the already encrypted "secret", in NodeJS there are 4 busted values that include the key and the ArrayBuffer used here and there.

[WebReflection]

Last from me, just to not look foolish ... the issue is broader than just Promise.prototype.then ... apparently all natives can be poisoned with ease and leak details that shouldn't ... another thing that fails in NodeJS only (not Bun, not browsers) is this, added to the pollution part:

  const buffer = Object.getOwnPropertyDescriptor(Uint8Array.__proto__.prototype, 'buffer');
  Object.defineProperty(Uint8Array.__proto__.prototype, 'buffer', {
    get() {
      console.log('⚠️ BUFFER LEAK ⚠️');
      return buffer.get.call(this);
    }
  });

I wonder if there should be a "safer" or "defensive" flag somewhere so that "random evil code" couldn't interfere with literally the entirety of the project if properly instrumented ... but leaking promises are, at least, a bigger concern, as these have easily recognizable context, buffers not so (or other things ... yet NodeJS should use internals and not polluted/patched prototypes, still imho).

---

edit I guess the bigger question is why browsers, hence v8 in browser, does never leak while NodeJS leaks all over

[bnoordhuis]

I'm going to close this as invalid because the thing you're describing falls outside node's threat model. It clearly states all bets are off if you run untrusted code, it's not a sandbox like a browser is.

[ljharb]

In this case, though, this is a web standard feature - and web standards do not allow modification of globals to affect the behavior of builtins. In other words, I think that it's a spec violation and must be fixed, regardless of node's typical threat model.

[bnoordhuis]

@WebReflection you may want to reword your report because all that talk about security obscures that it's a spec compliance issue.

[ljharb]

fwiw, security (in the sense of, robustness against mutation of globals) is a spec compliance issue. The only things node can get away with not locking down against that are things that aren't following an external speck, since every non-node standard considers that part of its threat model.

[WebReflection]

@bnoordhuis

It clearly states all bets are off if you run untrusted code, it's not a sandbox like a browser is.

ever heard of npm and stuff like left-pad and the like? if anyone wants to get slightly serious about security it can and those rare modules, unless these get also compromised on npm, could be trusted against all scenarios.

In this case, I was witnessing a vital key leak for a module imported before anything else and the leak at distance wasn't even malicious code, it was NodeJS internals.

Imagine the horror when I've realized I could not code cover with certainty my code was indeed safe by specs via any NodeJS based CI.

you may want to reword your report because all that talk about security obscures that it's a spec compliance issue

my report is that NodeJS leaks where specs don't and that's a security concern to me. Anyone in here though could feel free to adjust the text I wrote to describe the issue the best they can, I won't be offended by any edit as long as stuff like this doesn't just get closed carelessly ... so thanks @ljharb for chiming in and re-opening this issue, appreciated.

[jasnell]

FWIW, I agree with @bnoordhuis ... this really is not a security issue. If code runs in Node.js it is trusted, that's the way it's been from the beginning.

That said, @ljharb is correct about spec compliance. This is tricky because so much of Node.js is implemented in JavaScript. We make significant effort to use safe primordials captured at startup before any user code has a chance to pollute the global namespace but it's imperfect and raises a number of issues of its own. It's going to be difficult to fix without significant effort and significant risk of breaking changes. To be clear this is not a quick fix that we just go make really quick.

@WebReflection ... given that everyone who works on Node.js does so on a volunteer basis, and given that we're always looking for more contributions from the ecosystem, if this is an area that you feel strongly about, then I would encourage you to get involved to help make the necessary fixes. Also... no one is going to edit your words in your comments but I do want to point out: if you believe this is a legitimate security vulnerability in the runtime, posting a public issue to talk about it is not the correct process and would potentially put users at risk. We have a responsible disclosure policy and process that should be followed instead.