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

src: use V8 entropy source if RAND_bytes() != 1 #44493

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Sep 2, 2022

RAND_bytes() may return 0 to indicate an error, in which case the buffer might not have been filled with random data at all. Instead of ignoring this case, let V8 use its own entropy source. Historically, this used to be a weak source of entropy, but V8 now implements a proper source even on Windows.

And even if V8's own entropy source turns out to be weak, it does not matter much: V8's PRNG itself is not cryptographically secure, so even if it is seeded from a cryptographically secure entropy source, it does not produce cryptographically secure random numbers.

RAND_bytes() may return 0 to indicate an error, in which case the buffer
might not have been filled with random data at all. Instead of ignoring
this case, let V8 use its own entropy source. Historically, this used to
be a weak source of entropy, but V8 now implements a proper source even
on Windows. And even if V8's own entropy source turns out to be weak, it
does not matter much: V8's PRNG itself is not cryptographically secure,
so even if it is seeded from a cryptographically secure entropy source,
it does not produce cryptographically secure random numbers.
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/crypto
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Sep 2, 2022
@tniessen tniessen added openssl Issues and PRs related to the OpenSSL dependency. v8 engine Issues and PRs related to the V8 dependency. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 2, 2022
@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2022
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot
Copy link
Collaborator

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 4, 2022
@nodejs-github-bot nodejs-github-bot merged commit 7371d33 into nodejs:main Sep 4, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 7371d33

@RafaelGSS
Copy link
Member

Hi @tniessen, this PR didn't land clearly on v18.x-staging. Would you mind manually backporting it? I know that's little changes, but the cherry-pick was applying other changes when I tried to fix the conflict (https://gist.github.com/RafaelGSS/bde210dddc7a3019e867ba52db037ade) making the make test fail.

@RafaelGSS RafaelGSS added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 5, 2022
Fyko pushed a commit to Fyko/node that referenced this pull request Sep 15, 2022
RAND_bytes() may return 0 to indicate an error, in which case the buffer
might not have been filled with random data at all. Instead of ignoring
this case, let V8 use its own entropy source. Historically, this used to
be a weak source of entropy, but V8 now implements a proper source even
on Windows. And even if V8's own entropy source turns out to be weak, it
does not matter much: V8's PRNG itself is not cryptographically secure,
so even if it is seeded from a cryptographically secure entropy source,
it does not produce cryptographically secure random numbers.

PR-URL: nodejs#44493
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@juanarbol
Copy link
Member

This seems to depend on a crypto (native) method introduced in #35093 and marked as "semver-major"; I will proceed to mark this as a "dont-land-on-v16.x"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants