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.

[simlu]

We've since moved away from this library to crypto.randomUUID, since uuid wasn't working with our test library anymore after state aka caching of randomness was introduced.

So I don't really have a reason to persue this ticket at this point. If noone else has a problem with randomness caching 🤷‍♂️

...and if they do I just recommend moving from uuid to crypto 😉

[puzpuzpuz]

crypto.randomUUID uses the same approach with entropy caching. The only difference is that it allows disabling it via options.

[simlu]

@puzpuzpuz Oh interesting, I did not know that. Looked it up and this is how that would work: crypto.randomUUID({ disableEntropyCache: true }).

Not particularly practical for a test library. So the important part there is that it can easily be stubbed.

I'm ok closing this ticket. The only follow up task might be then to have an option to disable the entropy cache.

[ctavan]

I think that adding an option to disable the randomness cache, in line with what the standard now provides, makes a lot of sense!

Anyone interested in contributing a PR?

I think this should also allow to stub the library again, wdyt @simlu ?

I‘m also glad to hear that crypto.randomUUID() from the standard library works for you! Introducing this to the standard library to allow folks get rid of the uuid npm dependency was exactly our intention.

[LinusU]

Personally, I think that relying on what a library calls internally is always going to be very fragile. If we accommodate this use case, does that mean that we also commit to not changing the call to randomFillSync to getRandomValues, or anything similar without making it a breaking change? How would that be documented?

I'm actually really surprised that Node.js decided to add disableEntropyCache, especially since they don't explain anything about why one would want to do that. Surely that should be an implementation detail that the end user shouldn't notice? And if there is any security problems with using an entropy cache, then shouldn't that be documented? ref

I think that the correct way to handle this in your tests is to mock the uuid function. Is there a reason as to why one would like to mock randomFillSync instead of just the v4 uuid function?

[github-actions]

Marking as stale due to 90 days with no activity.

[github-actions]

Closing issue due to 30 days since being marked as stale.