Skip to content

Conversation

@github-actions
Copy link
Contributor

@github-actions github-actions bot commented Mar 28, 2024

You can preview this rule here (updated a few minutes after each push).

The shared files referenced by this PR are due to be changed by #3836. The shared content will no longer reference SecureRandom and will instead generically reference secure random number generators.

Review

A dedicated reviewer checked the rule description successfully for:

  • logical errors and incorrect information
  • information gaps and missing content
  • text style and tone
  • PR summary and labels follow the guidelines

@jamie-anderson-sonarsource jamie-anderson-sonarsource changed the title Create rule S4347 Create rule S4347: Secure random number generators must not output predictable values Mar 28, 2024
Copy link
Contributor

@egon-okerman-sonarsource egon-okerman-sonarsource left a comment

Choose a reason for hiding this comment

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

Mostly okay, just one compliant example should be changed I think.

digest.NextBytes(random);

IRandomGenerator vmpc = new VmpcRandomGenerator();
vmpc.AddSeedMaterial(Guid.NewGuid().ToByteArray());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is preferable to use something else (e.g. turning SecureRandom.GenerateSeed into a byte array) here.

I'm not an expert on the matter, but reading the Guid.NewGuid docs:

On non-Windows platforms, starting with .NET 6, this function calls the OS's underlying cryptographically secure pseudo-random number generator (CSPRNG) to generate 122 bits of strong entropy. In previous versions of .NET, the entropy is not guaranteed to be generated by a CSPRNG.

It is recommended that applications not use the NewGuid method for cryptographic purposes.

@sonarqube-next
Copy link

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@sonarqube-next
Copy link

Quality Gate passed Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

Copy link
Contributor

@egon-okerman-sonarsource egon-okerman-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@egon-okerman-sonarsource egon-okerman-sonarsource left a comment

Choose a reason for hiding this comment

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

LGTM

@gregory-paidis-sonarsource
Copy link
Contributor

Hey @jamie-anderson-sonarsource , there are two instances in the CSHARP code that refer to java-specific methods:

  1. On Why is this an issue? section

image

  1. On the Documentation section

image

You can see it if you click the preview on the top of the PR.
Would you kindly take a look and fix it?

@egon-okerman-sonarsource
Copy link
Contributor

@gregory-paidis-sonarsource I resolved the problems, should be okay now.

@gregory-paidis-sonarsource gregory-paidis-sonarsource marked this pull request as ready for review May 29, 2024 15:21
@sonarqube-next
Copy link

Quality Gate passed Quality Gate passed for 'rspec-tools'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@sonarqube-next
Copy link

Quality Gate passed Quality Gate passed for 'rspec-frontend'

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@gregory-paidis-sonarsource gregory-paidis-sonarsource merged commit 9e7f366 into master May 29, 2024
@gregory-paidis-sonarsource gregory-paidis-sonarsource deleted the rule/S4347-add-csharp branch May 29, 2024 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants