Regression: Stub pseudo randomness broken

[simlu]

Describe the bug

Stubbing of pseudo randomness no longer feasible / working with this change

The below example works reliably with 8.3.0 but breaks with 8.3.1

How to reproduce

const crypto = require('crypto');
const { v4: uuid } = require('uuid');

crypto.randomBytes = (size, cb) => {
  let result = crypto.createHash('sha256').update('some-seed').digest();
  while (result.length < size) {
    result = Buffer.concat([result, crypto.createHash('sha256').update(result).digest()]);
  }
  result = result.slice(0, size);
  return cb ? cb(null, result) : result;
};
crypto.randomFillSync = (buffer, offset, size) => {
  const o = offset || 0;
  const s = size || buffer.byteLength;
  crypto.randomBytes(s, (err, res) => {
    res.copy(buffer, o, 0, s);
  });
  return buffer;
};

console.log(uuid(), uuid());
// => 0ad40e47-01e4-4acb-afa0-ad8c30a6aa42, 0ad40e47-01e4-4acb-afa0-ad8c30a6aa42

Above can probably be made even more minimal. The example was distilled from here.

Expected behavior

It should be feasible to stub the uuid generation with a PRNG at any given point in time. With the optimization introduces in 8.3.1 this is no longer possible without re-requiring uuid before every randomness "reset".

Runtime

• OS: Linux
• Runtime: Node.js
• Runtime Version: v10.22.1

Additional information

Background: I'm the maintainer of a test framework that supports pseudo randomness injection. The use case of making uuid generation consistent between test executions is very important to us to e.g. make requests fingerprints consistent since we are using nock.

I'm sure the discussion above is going to go towards "why is above example even necessary", ie it should be enough to seed once? The problem is that state was introduced with uuid v8.3.1. So randomness can no longer be "reset" when necessary (i.e. between running different tests). That means that to reset the randomness state one needs to re-require the uuid import. This is very ugly and leads to memory leaks.

I am very happy to explain this further and give more background if needed!

Maybe there is a way that we can do both: keep the ability for stubbing as well as the optimization that was introduced. But I'm not sure how just yet. Very much looking forward to having a good discussion here. Cheers!

[ctavan]

Good catch. I agree that randomness should somehow be mockable.

However, OTOH I don't immediately see a simple solution. Will require some more thinking I guess.

[puzpuzpuz]

A primitive solution might be to use the seed to generate only one octet and then duplicate it in the buffer returned from randomBytes():

const crypto = require('crypto');
const { v4: uuid } = require('./dist/');

crypto.randomBytes = (size, cb) => {
  // we use only first octet
  let result = crypto.createHash('sha256').update('some-seed').digest().slice(0, 8);
  while (result.length < size) {
    // the rest is a simple copy of the octet
    result = Buffer.concat([result, result]);
  }
  result = result.slice(0, size);
  return cb ? cb(null, result) : result;
};
crypto.randomFillSync = (buffer, offset, size) => {
  const o = offset || 0;
  const s = size || buffer.byteLength;
  crypto.randomBytes(s, (err, res) => {
    res.copy(buffer, o, 0, s);
  });
  return buffer;
};

console.log(uuid(), uuid());
// Prints: 0ad40e47-01e4-4acb-8ad4-0e4701e40acb 0ad40e47-01e4-4acb-8ad4-0e4701e40acb

However, this approach is fragile (just like any other approach to mocking core modules, especially crypto) and may not fit into the requirements for the mocking library (not sure here).

I can also think of another approach: uuid could allow users to change the size of the RNG pool, so the mocking library could set the pool size to 16 to have the same behavior as in 8.3.0. However, here the mocking library would need to detect presence of uuid dependency.

And, yes, both approaches are far from being ideal, which is expected when someone tries to change the internal behavior of a library by mocking core APIs.

[simlu]

@puzpuzpuz

(1) Yeah, I was thinking about doing that approach, however it prevents one from having a predictable sequence of uuids. So that's not a possible solution unfortunately

(2) Setting a random pool size is not great, but possible. One could also try mocking v4 from uuid, but mostly the function is directly imported, so mocking is not possible. And doing a resetPool() function seems really hacky. Not a big fan...

❗ Apart from the whole mocking library issue, I'm also questioning if there are security concern here with using a cache / pool / state. I can't think of any of hand, but if feels like the sort of thing that might open an attack vector?

[puzpuzpuz]

Apart from the whole mocking library issue, I'm also questioning if there are security concern here with using a cache / pool / state. I can't think of any of hand, but if feels like the sort of thing that might open an attack vector?

The concerns are basically the same as with vanilla crypto.randomFillSync()/crypto.randomBytes() usage. Internally, it boils down to OpenSSL's RAND_bytes call which uses a CSPRNG. So, personally I don't see any new attack vectors being introduced with the discussed optimization. If you see anything in particular on your mind, I'm really curious to hear your thoughts.

[broofa]

We could check for something like crypto.fillRandomBytes.isMock = true to disable the rnds8Pool caching but... can't say I'm a big fan of that idea. Adding special case code to work around issues in downstream dependencies is never ideal.

Or just revert #513. It's not like perf is a critical issue.

[simlu]

[...] If you see anything in particular on your mind, I'm really curious to hear your thoughts.

Somewhere along the lines of: If you have memory access you can reliably know the next uuid generated. Before that wasn't the case.

[puzpuzpuz]

Somewhere along the lines of: If you have memory access you can reliably know the next uuid generated. Before that wasn't the case.

Previously the allocated buffer could be intercepted as well, with a smaller time frame, but that's arguable. Obviously, if someone has the memory access, you're already screwed.

[simlu]

Somewhere along the lines of: If you have memory access you can reliably know the next uuid generated. Before that wasn't the case.

Previously the allocated buffer could be intercepted as well, with a smaller time frame, but that's arguable. Obviously, if someone has the memory access, you're already screwed.

I've worked a lot with security relevant systems and libraries in the past and this just really gives me a bad vibe. Let me give you an example (somewhat constructed, but not unrealistic).

(1) Using this library in a program to generate a secret password. After copying the password and deleted it from the program, it can still be found in memory. This is the case even if the user "logs out" of the program (as long as the program doesn't shut down). Yes, this could be "fixed" by overwriting the used pool values.

(2) Similar to the first scenario, the user logged into a program and copied the pool content. Now they log out and someone else logs in and uses the program to generate a secret password. Since the first user copied the pool, they now know the secret password.

Considering how widespread this library is used, I'd recommend an immediate rollback and potentially an npm audit report for this version...

[puzpuzpuz]

@simlu it's very hard to argue against a bad vibe, but I'll try.

Scenario 1 was exactly the same before #513, as the buffer was reused since #435. And even before that the buffer could be still available on heap until it's eventually overwritten during GC compaction or new objects.

As for the scenario 2, I don't quite understand what's meant by "user logged into a program". Is it a OS-level user log in event or something else? How both users are supposed to copy the pool? A heap dump, a core dump, or something else? If one of the two versions, the previous version of the library was also vulnerable to this kind of attack.

Considering how widespread this library is used, I'd recommend an immediate rollback and potentially an npm audit report for this version...

I thought that this issue is dedicated to a broken mocking library. Probably, it makes sense to open another security-related issue and provide some concrete examples of new attack vectors introduced by the change. But that's just an advice and maintainers of this library may think otherwise.

[simlu]

Agreed, this should be separate issue. I'll open one

[github-actions]

Marking as stale due to 90 days with no activity.