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

fix(nms): Fix insecure randomness #12417

Merged
merged 1 commit into from May 3, 2022
Merged

fix(nms): Fix insecure randomness #12417

merged 1 commit into from May 3, 2022

Conversation

spikey979
Copy link
Contributor

Signed-off-by: Kristijan spikey979@gmail.com

fix(nms): Fix insecure randomness

Summary

Using a cryptographically weak pseudo-random number generator to generate a security-sensitive value, such as a password, makes it easier for an attacker to predict the value. A cryptographically secure pseudo-random number generator should be used instead.

Additional Information

This is for code scanning alerts:
https://github.com/magma/magma/security/code-scanning/62
https://github.com/magma/magma/security/code-scanning/63
https://github.com/magma/magma/security/code-scanning/64
https://github.com/magma/magma/security/code-scanning/65

@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines. label Apr 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2022

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: nms NMS-related issue label Apr 7, 2022
@spikey979 spikey979 self-assigned this Apr 7, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2022

nms-workflow

0 files  0 suites   0s ⏱️
0 tests 0 ✔️ 0 💤 0

Results for commit d6544fd.

♻️ This comment has been updated with latest results.

@spikey979 spikey979 marked this pull request as ready for review April 7, 2022 09:10
@spikey979 spikey979 requested review from a team and andreilee April 7, 2022 09:10
@thmsschmitt
Copy link
Contributor

Screenshot 2022-04-08 at 13 45 47
The server runs in node. You need to use https://nodejs.org/api/crypto.html instead.

@spikey979
Copy link
Contributor Author

Screenshot 2022-04-08 at 13 45 47 The server runs in node. You need to use https://nodejs.org/api/crypto.html instead.

If I use crypto.getRandomValues() without "winodw", in CI tests I get error: Cannot resolve name crypto. [cannot-resolve-name]

@thmsschmitt
Copy link
Contributor

You need to import it:

import crypto from 'crypto';

also note that it has a different API then window.crypto. See https://nodejs.org/api/crypto.html

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines. and removed size/XS Denotes a PR that changes 0-9 lines. labels Apr 11, 2022
@voisey voisey requested a review from thmsschmitt April 26, 2022 08:39
Copy link
Contributor

@thmsschmitt thmsschmitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry that this takes so long.

crypto.getRandomValues is only available since node v17.4.0 and we are using v16.14. Also only using 4 bytes isn't great.

I suggest we use something like
crypto.randomBytes(16).toString('hex')

@spikey979
Copy link
Contributor Author

@thmsschmitt thanks for helping

@thmsschmitt
Copy link
Contributor

@spikey979 I'm waiting for the ready2merge label. It would also be good to rebase this once more.

Signed-off-by: Kristijan <spikey979@gmail.com>
@spikey979 spikey979 added the ready2merge This PR is ready to be merged (is approved and passes all required checks) label May 3, 2022
@thmsschmitt thmsschmitt merged commit c2fa6c3 into magma:master May 3, 2022
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
Signed-off-by: Kristijan <spikey979@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: nms NMS-related issue ready2merge This PR is ready to be merged (is approved and passes all required checks) size/S Denotes a PR that changes 10-29 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants