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.

Sign up for GitHub

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

Jump to bottom

Create rule S5344: Passwords should not be stored in plain-text or with a fast hashing algorithm (Part 1) #9278

Merged
merged 1 commit into from
May 16, 2024
Merged

Create rule S5344: Passwords should not be stored in plain-text or with a fast hashing algorithm (Part 1) #9278

merged 1 commit into from
May 16, 2024

Conversation

gregory-paidis-sonarsource
Copy link
Contributor

@gregory-paidis-sonarsource gregory-paidis-sonarsource commented May 15, 2024 •
edited
Loading

Part of ##8998

Whatever is checked, is implemented in this PR:

  • PasswordHasherOptions.IterationCount < 100K (Core)
  • PasswordHasherOptions.CompatibilityMode == IdentityV2 (Core)
  • KeyDerivation.Pbkdf2.iterationCount < 100K (Core)
  • Rfc2898DeriveBytes.Pbkdf2.iterations < 100K (Core)
  • PasswordHasher instantiated (FRM)
  • Rfc2898DeriveBytes.iterations < 100K (cross-platform)
  • Rfc2898DeriveBytes.hashAlgorithm does not exist (cross-platform)
  • (OpenBsdCrypt/BCrypt).Generate.cost < 12 (BouncyCastle)
  • PbeParametersGenerator.Init.iterationCount < 100K (BouncyCastle)
  • SCrypt.Generate N < 2^12, r < 8, or dklen < 32 (BouncyCastle)

@gregory-paidis-sonarsource gregory-paidis-sonarsource linked an issue May 15, 2024 that may be closed by this pull request
Create rule S5344: Passwords should not be stored in plain-text or with a fast hashing algorithm #8998
Closed
10 tasks
@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/implement-S5344 branch 2 times, most recently from e207cb3 to e98c967 Compare May 15, 2024 13:54
@gregory-paidis-sonarsource gregory-paidis-sonarsource marked this pull request as ready for review May 15, 2024 14:09
@gregory-paidis-sonarsource
Copy link
Contributor Author

@costin-zaharia-sonarsource I accepted 7 issues, which are as follows:

  • 6 of them are magic numbers, because I am tracking the position of arguments. IMO it is not worth it to extract them to variables.
  • 1 of them is the styling x => ... lambda rule, where I chose a concrete name instead of x for readability.

Let me know if you have any disagreements.

@gregory-paidis-sonarsource gregory-paidis-sonarsource mentioned this pull request May 15, 2024
Closed
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 16, 2024

Quality Gate Passed Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 16, 2024

Quality Gate Passed Quality Gate passed for 'SonarAnalyzer for .NET'

Issues
0 New issues
7 Accepted issues

Measures
0 Security Hotspots
98.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

costin-zaharia-sonarsource
costin-zaharia-sonarsource approved these changes May 16, 2024
View reviewed changes
Copy link
Member

@costin-zaharia-sonarsource costin-zaharia-sonarsource left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM! 🚀

analyzers/rspec/cs/S5344.html
<p>When bcrypt’s hashing function is used, it is important to select a round count that is high enough to make the function slow enough to prevent
brute force: More than 12 rounds.</p>
<p>For bcrypt’s key derivation function, the number of rounds should likewise be high enough to make the function slow enough to prevent brute force:
More than 4096 rounds <code>(2**12)</code>.<br> This number is not the same coefficient as the first one because it uses a different algorithm.</p>
Copy link
Member

Choose a reason for hiding this comment

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

"2**12" -> shouldn't this be 2^12 or 2<sup>12</sup>?

analyzers/tests/SonarAnalyzer.Test/Rules/PasswordsShouldBeStoredCorrectlyTest.cs
""")
.AddReferences(NuGetMetadataReference.BouncyCastle())
.Verify();
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should cover also:

        var x = new Rfc2898DeriveBytes(HardCodedKey, new byte[] { 1 }, 120_000);
        x.IterationCount = 1;                                       // Noncompliant

        var y = new Rfc2898DeriveBytes(HardCodedKey, new byte[] { 1 }, 120_000);
        y.IterationCount = 1;                                       // Noncompliant

        void MakeItUnsafe(Rfc2898DeriveBytes password)
        {
            password.IterationCount = 1;                            // Noncompliant
        }

analyzers/src/SonarAnalyzer.CSharp/Rules/PasswordsShouldBeStoredCorrectly.cs
Comment on lines +125 to +126
tracker.AssignedValue(context) is int iterationCount
&& iterationCount < IterationCountThreshold;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
tracker.AssignedValue(context) is int iterationCount
&& iterationCount < IterationCountThreshold;
tracker.AssignedValue(context) is < IterationCountThreshold;

analyzers/src/SonarAnalyzer.CSharp/Rules/PasswordsShouldBeStoredCorrectly.cs
"Identity v2 uses only 1000 iterations. Consider changing to identity V3.",
propertyTracker.MatchSetter(),
propertyTracker.MatchProperty(new MemberDescriptor(KnownType.Microsoft_AspNetCore_Identity_PasswordsHasherOptions, "CompatibilityMode")),
x => propertyTracker.AssignedValue(x) is int mode && mode == 0); // PasswordHasherCompatibilityMode.IdentityV2 results to 0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
x => propertyTracker.AssignedValue(x) is int mode && mode == 0); // PasswordHasherCompatibilityMode.IdentityV2 results to 0
x => propertyTracker.AssignedValue(x) is 0); // PasswordHasherCompatibilityMode.IdentityV2 results to 0

@gregory-paidis-sonarsource gregory-paidis-sonarsource merged commit ef727d3 into master May 16, 2024
28 checks passed
@gregory-paidis-sonarsource gregory-paidis-sonarsource deleted the greg/implement-S5344 branch May 16, 2024 15:47
@gregory-paidis-sonarsource
Copy link
Contributor Author

I will take care of the issues on part2, so that I can validate tomorrow the findings of this one :)

@gregory-paidis-sonarsource gregory-paidis-sonarsource removed a link to an issue May 24, 2024
Create rule S5344: Passwords should not be stored in plain-text or with a fast hashing algorithm #8998
Closed
10 tasks
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource added this to the 9.26 milestone May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin-zaharia-sonarsource costin-zaharia-sonarsource costin-zaharia-sonarsource approved these changes

Assignees

@gregory-paidis-sonarsource gregory-paidis-sonarsource

Labels
None yet
Projects
None yet
Milestone
9.26
Development

Successfully merging this pull request may close these issues.
3 participants
@gregory-paidis-sonarsource @costin-zaharia-sonarsource @zsolt-kolbay-sonarsource