stripe-node icon indicating copy to clipboard operation
stripe-node copied to clipboard

Math.random() is not a cryptographically secure way to generate UUIDv4s

Open gen0cide opened this issue 4 years ago • 2 comments
trafficstars

https://github.com/stripe/stripe-node/blob/ec960ead66f9942d68804f1a91dce8988d8d9660/lib/utils.js#L409

The function uses an outdated method of generating UUIDv4s that does not follow current PRNG guidance for UUIDv4 generation.

The referenced comment above the function (https://stackoverflow.com/a/2117523) has an updated implemention:

function uuidv4() {
  return ([1e7]+-1e3+-4e3+-8e3+-1e11).replace(/[018]/g, c =>
    (c ^ crypto.getRandomValues(new Uint8Array(1))[0] & 15 >> c / 4).toString(16)
  );
}

Given that Stripe is used for payment processing, it seems like sound cryptographic PRNG usage should be the default.

gen0cide avatar Dec 29 '20 20:12 gen0cide

The new implementation would be an improvement, but just to be clear: idempotency keys are scoped entirely to single user accounts so the worst that could happen is that a user accidentally reuses one of their own idempotency keys, and even with a non-crypto random number generation, that's very unlikely to ever happen, especially since it would have to occur within a 24 hour window.

Exciting to see on the linked SO page that there's the UUID module is coming down the pipeline for inclusion in the core language.

brandur-stripe avatar Jan 04 '21 18:01 brandur-stripe

For tracking purposes, upstream is considering an implementation for inclusion as well https://github.com/nodejs/node/pull/36729

tjfontaine-stripe avatar Jan 04 '21 18:01 tjfontaine-stripe