Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cryptographically secure random integer in range #29707

Closed
olalonde opened this issue Sep 25, 2019 · 20 comments
Closed

Cryptographically secure random integer in range #29707

olalonde opened this issue Sep 25, 2019 · 20 comments
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@olalonde
Copy link
Contributor

olalonde commented Sep 25, 2019

Is your feature request related to a problem? Please describe.

There is no API to obtain a random integer within a range. It's a common enough problem to deal with (e.g. to implement other algorithms like Fisher–Yates shuffle) and easy to get wrong. Most other runtimes/languages implement this in their standard library.

Describe the solution you'd like

// something like:
const randomInt = require('crypto').randomInt

const min = 0
const max = 10
randomInt(min, max, cb)

Describe alternatives you've considered

I'm not sure about the internal implementation but I gave it a stab here: https://github.com/olalonde/crypto-range. http://www.pcg-random.org/posts/bounded-rands.html has a nice overview of different techniques (my implementation is probably not the most efficient).

@olalonde olalonde changed the title Cryptographically secure random number in range Cryptographically secure random integer in range Sep 25, 2019
@sam-github sam-github added the feature request Issues that request new features to be added to Node.js. label Sep 25, 2019
@sam-github
Copy link
Contributor

@nodejs/tooling

@bnoordhuis bnoordhuis added the crypto Issues and PRs related to the crypto subsystem. label Sep 25, 2019
@bnoordhuis
Copy link
Member

I think this would make a good addition. But.

  1. There should be support for BigInts
  2. Floating-point support would be nice too
  3. A step parameter à la python's random.randrange() might be nice (but not essential)

@olalonde
Copy link
Contributor Author

olalonde commented Sep 26, 2019

There should be support for BigInts

randomBigInt(min, max, cb)?

Floating-point support would be nice too

In a generic "random" library I'd agree but since this is "crypto", I'm not entirely sure. At least, I personally am not sure what it would mean to implement it correctly nor feel very confident to do it (some relevant discussion here).

A step parameter à la python's random.randrange() might be nice (but not essential)

Shouldn't be too hard to add this argument.

@bnoordhuis
Copy link
Member

randomBigInt(min, max, cb)?

Yes, that could work. Another option is to make randomInt() return a BigInt if its arguments are BigInts, i.e.:

typeof randomInt(0, 42)    // 'number'
typeof randomInt(0n, 42n)  // 'bigint'
typeof randomInt(0n, 42)   // TBD: 'bigint' or TypeError?

Pro: lets callers support numbers and bigints transparently

Con: potential for type confusion?

Floating-point support

The problems around generating good, uniformly distributed random floating-point numbers is precisely why I think it would be good to have in core because I expect most people won't be aware of the subtleties with 1.0 / randomInt(0, 42).

If nothing else, it lets us warn about distribution caveats in the documentation.


An alternative approach is to:

  1. implement crypto.randomBits(n) that generates a buffer with exactly n random bits (not bytes) with uniform distribution, and

  2. add a way to turn that buffer into a BigInt efficiently (possibly with support for big and little endianness?)

Higher-level APIs can then be derived from the above primitives and wouldn't necessarily need to live in core.

Technically even randomBits() doesn't need to live in core because it can be implemented in terms of randomBytes():

function randomBits(n) {
  const t = n >>> 3
  const u = n & 7
  const b = randomBytes(t + !!u)
  if (u > 0) b[t] &= (1 << u) - 1
  return b
}

I'm ignoring callbacks here for the sake of brevity.

@olalonde
Copy link
Contributor Author

Pro: lets callers support numbers and bigints transparently
Con: potential for type confusion?

There was some related discussion on the BigInt proposal. I believe it was ultimately decided not to overload existing math functions to support BigInt. In the spirit of consistency, I would vote for an explicit randomBigInt. That being said... the same document warns against using BigInt for crypto purposes, so maybe it would be better to err on the side of caution and leave it out for now?

If there's a chance it can get merged, I'd be happy to send a PR for randomInt(min, max, [step,], cb) but tbh, I don't really feel confident about my ability to implement other APIs.

@bnoordhuis
Copy link
Member

@nodejs/crypto This needs your input.

@olalonde
Copy link
Contributor Author

olalonde commented Oct 1, 2019

Bump

@jasnell
Copy link
Member

jasnell commented Oct 1, 2019

@bnoordhuis:

Con: potential for type confusion?

Really not a fan of polymorphic signatures, not only because of type confusion but due to performance costs. I'd much rather just keep the variants separate.

I can definitely get behind having these APIs in core. I'm less convinced on the floating point variant but I buy into @bnoordhuis' argument there.

@sam-github
Copy link
Contributor

Feature seems reasonable to me. @orlonde, it sounds like a PR would be accepted, go for it.

@tniessen
Copy link
Member

tniessen commented Oct 1, 2019

If we are only considering safe integers (that is, 53 bits or less), would the usual floor(range * random()) with a cryptographically secure version of Math.random() be enough? I am not sure, I feel like it might have security implications, but if it does not, we could simply provide the floating point variant for now.

Another option is to make randomInt() return a BigInt if its arguments are BigInts, i.e.:

This doesn't sound too bad to me. I am not a huge fan of bloating our APIs with functions that only differ in types, but I understand @jasnell's arguments.

@devsnek
Copy link
Member

devsnek commented Oct 1, 2019

i think "random floats" are diverse enough of a problem that we should leave it to userland. we have int in range, float in range, int to float in range (e.g. 0x2440 for 10.0), list goes on.

@bnoordhuis
Copy link
Member

@jasnell @tniessen If most uses end up looking like y = (typeof x === 'bigint' ? randomBigInt : randomInt)(0, x) then we don't gain much from having separate functions.

That said, I don't really feel strongly either way.

@devsnek Can you elaborate? What diversity are you referring to?

@jasnell
Copy link
Member

jasnell commented Oct 2, 2019

Likewise if uses end doing the type check after the function call. Code will already need to be aware that bigints are being used so a dedicated function is not going to make things more difficult there.

@adamreisnz
Copy link

Has there been any movement or progress on this feature?
A cryptographically secure Math.random() would be great.

@olalonde
Copy link
Contributor Author

olalonde commented Aug 2, 2020

Attempting a pull request: #34600

@niceque
Copy link

niceque commented Sep 7, 2020

That's could be a great functionality!

@tniessen
Copy link
Member

tniessen commented Sep 8, 2020

A basic variant of this feature has landed in #34600, and will be available in future releases of Node.js. For further feature requests (e.g., use cases for random big integers), please open a new issue.

@tniessen tniessen closed this as completed Sep 8, 2020
@RodolfoSilva
Copy link

RodolfoSilva commented Sep 10, 2020

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/random

function getRandomInt(min, max) {
  min = Math.ceil(min);
  max = Math.floor(max);
  //The maximum is exclusive and the minimum is inclusive
  return Math.floor(Math.random() * (max - min) + min);
}

@madarche
Copy link
Contributor

@RodolfoSilva Math.random() does not provide cryptographically secure random numbers. It shouldn't be used for anything related to security, cf. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/random. On the opposite, the Node.js crypto module provides cryptographically secure methods.

@RodolfoSilva
Copy link

Thanks @madarche 😁.

I hadn't paid attention to this note. My mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests

10 participants