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 2) #9287

Merged
merged 3 commits into from
May 22, 2024
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ce44f7
Create rule S5344: Part 2
gregory-paidis-sonarsource May 16, 2024
65b559b
Review from downtown
gregory-paidis-sonarsource May 17, 2024
0a0ec88
Review 1
gregory-paidis-sonarsource May 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
55 changes: 45 additions & 10 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/PasswordsShouldBeStoredCorrectly.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,28 +36,29 @@ public sealed class PasswordsShouldBeStoredCorrectly : SonarDiagnosticAnalyzer

protected override void Initialize(SonarAnalysisContext context)
{
NetCore(context);
AspNetCore(context);
AspNetFramework(context);
Rfc2898DeriveBytes(context);
BouncyCastle(context);
}

private static void NetCore(SonarAnalysisContext context)
private static void AspNetCore(SonarAnalysisContext context)
{
var propertyTracker = CSharpFacade.Instance.Tracker.PropertyAccess;
Track(
propertyTracker,
context,
UseMoreIterationsMessageFormat,
propertyTracker.MatchSetter(),
propertyTracker.MatchProperty(new MemberDescriptor(KnownType.Microsoft_AspNetCore_Identity_PasswordsHasherOptions, "IterationCount")),
propertyTracker.MatchProperty(new MemberDescriptor(KnownType.Microsoft_AspNetCore_Identity_PasswordHasherOptions, "IterationCount")),
costin-zaharia-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
x => HasFewIterations(x, propertyTracker));
Track(
propertyTracker,
context,
"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
propertyTracker.MatchProperty(new MemberDescriptor(KnownType.Microsoft_AspNetCore_Identity_PasswordHasherOptions, "CompatibilityMode")),
x => propertyTracker.AssignedValue(x) is 0); // PasswordHasherCompatibilityMode.IdentityV2 results to 0
costin-zaharia-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

var argumentTracker = CSharpFacade.Instance.Tracker.Argument;
Track(
Expand All @@ -68,6 +69,16 @@ private static void NetCore(SonarAnalysisContext context)
x => ArgumentLessThan(x, IterationCountThreshold));
}

private static void AspNetFramework(SonarAnalysisContext context)
{
var tracker = CSharpFacade.Instance.Tracker.ObjectCreation;
Track(
tracker,
context,
"PasswordHasher does not support state-of-the-art parameters. Use Rfc2898DeriveBytes instead.",
tracker.WhenDerivesFrom(KnownType.Microsoft_AspNet_Identity_PasswordHasherOptions));
costin-zaharia-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
}

private static void Rfc2898DeriveBytes(SonarAnalysisContext context)
{
// Raise when hashAlgorithm is present
Expand Down Expand Up @@ -98,6 +109,15 @@ private static void Rfc2898DeriveBytes(SonarAnalysisContext context)
"Use at least 100,000 iterations and a state-of-the-art digest algorithm here.",
objectCreationTracker.MatchConstructor(KnownType.System_Security_Cryptography_Rfc2898DeriveBytes),
x => x.InvokedConstructorSymbol.Value.Parameters.All(x => x.Name != "hashAlgorithm"));

var propertyTracker = CSharpFacade.Instance.Tracker.PropertyAccess;
Track(
propertyTracker,
context,
UseMoreIterationsMessageFormat,
propertyTracker.MatchSetter(),
propertyTracker.MatchProperty(new MemberDescriptor(KnownType.System_Security_Cryptography_Rfc2898DeriveBytes, "IterationCount")),
x => HasFewIterations(x, propertyTracker));
costin-zaharia-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
}

private static void BouncyCastle(SonarAnalysisContext context)
Expand All @@ -109,21 +129,36 @@ private static void BouncyCastle(SonarAnalysisContext context)
context,
"Use a cost factor of at least 12 here.",
tracker.Or(
tracker.MatchArgument(ArgumentDescriptor.MethodInvocation(KnownType.Org_BouncyCastle_Crypto_Generators_OpenBsdBCrypt, "Generate", "cost", x => x is 2 or 3)),
tracker.MatchArgument(ArgumentDescriptor.MethodInvocation(KnownType.Org_BouncyCastle_Crypto_Generators_BCrypt, "Generate", "cost", 2))),
tracker.MatchArgument(
ArgumentDescriptor.MethodInvocation(KnownType.Org_BouncyCastle_Crypto_Generators_OpenBsdBCrypt, "Generate", "cost", x => x is 2 or 3)),
tracker.MatchArgument(
ArgumentDescriptor.MethodInvocation(KnownType.Org_BouncyCastle_Crypto_Generators_BCrypt, "Generate", "cost", 2))),
x => ArgumentLessThan(x, 12));

Track(
tracker,
context,
UseMoreIterationsMessageFormat,
tracker.MatchArgument(ArgumentDescriptor.MethodInvocation(KnownType.Org_BouncyCastle_Crypto_PbeParametersGenerator, "Init", "iterationCount", 2)),
tracker.MatchArgument(
ArgumentDescriptor.MethodInvocation(KnownType.Org_BouncyCastle_Crypto_PbeParametersGenerator, "Init", "iterationCount", 2)),
x => ArgumentLessThan(x, IterationCountThreshold));

TrackSCrypt("N", 2, "Use a cost factor of at least 2 ^ 12 for N here.", 1 << 12);
TrackSCrypt("r", 3, "Use a memory factor of at least 8 for r here.", 8);
TrackSCrypt("dkLen", 5, "Use an output length of at least 32 for dkLen here.", 32);

void TrackSCrypt(string argumentName, int argumentPosition, string diagnosticMessage, int threshold) =>
Track(
tracker,
context,
diagnosticMessage,
tracker.MatchArgument(ArgumentDescriptor.MethodInvocation(
KnownType.Org_BouncyCastle_Crypto_Generators_SCrypt, "Generate", argumentName, argumentPosition)),
x => ArgumentLessThan(x, threshold));
costin-zaharia-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
}

private static bool HasFewIterations(PropertyAccessContext context, PropertyAccessTracker<SyntaxKind> tracker) =>
tracker.AssignedValue(context) is int iterationCount
&& iterationCount < IterationCountThreshold;
tracker.AssignedValue(context) is < IterationCountThreshold;
costin-zaharia-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

private static bool ArgumentLessThan(ArgumentContext context, int threshold) =>
context.SemanticModel.GetConstantValue(((ArgumentSyntax)context.Node).Expression) is { HasValue: true, Value: int value }
Expand Down
4 changes: 3 additions & 1 deletion analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public sealed partial class KnownType
public static readonly KnownType log4net_ILog = new("log4net.ILog");
public static readonly KnownType log4net_LogManager = new("log4net.LogManager");
public static readonly KnownType log4net_Util_ILogExtensions = new("log4net.Util.ILogExtensions");
public static readonly KnownType Microsoft_AspNet_Identity_PasswordHasherOptions = new("Microsoft.AspNet.Identity.PasswordHasherOptions");
public static readonly KnownType Microsoft_AspNet_SignalR_Hub = new("Microsoft.AspNet.SignalR.Hub");
public static readonly KnownType Microsoft_AspNetCore_Builder_DeveloperExceptionPageExtensions = new("Microsoft.AspNetCore.Builder.DeveloperExceptionPageExtensions");
public static readonly KnownType Microsoft_AspNetCore_Builder_DatabaseErrorPageExtensions = new("Microsoft.AspNetCore.Builder.DatabaseErrorPageExtensions");
Expand All @@ -74,7 +75,7 @@ public sealed partial class KnownType
public static readonly KnownType Microsoft_AspNetCore_Http_IResponseCookies = new("Microsoft.AspNetCore.Http.IResponseCookies");
public static readonly KnownType Microsoft_AspNetCore_Http_IResult = new("Microsoft.AspNetCore.Http.IResult");
public static readonly KnownType Microsoft_AspNetCore_Http_Results = new("Microsoft.AspNetCore.Http.Results");
public static readonly KnownType Microsoft_AspNetCore_Identity_PasswordsHasherOptions = new("Microsoft.AspNetCore.Identity.PasswordHasherOptions");
public static readonly KnownType Microsoft_AspNetCore_Identity_PasswordHasherOptions = new("Microsoft.AspNetCore.Identity.PasswordHasherOptions");
public static readonly KnownType Microsoft_AspNetCore_Mvc_AcceptVerbsAttribute = new("Microsoft.AspNetCore.Mvc.AcceptVerbsAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_ApiControllerAttribute = new("Microsoft.AspNetCore.Mvc.ApiControllerAttribute");
public static readonly KnownType Microsoft_AspNetCore_Mvc_ApiConventionMethodAttribute = new("Microsoft.AspNetCore.Mvc.ApiConventionMethodAttribute");
Expand Down Expand Up @@ -211,6 +212,7 @@ public sealed partial class KnownType
public static readonly KnownType Org_BouncyCastle_Asn1_X9_X962NamedCurves = new("Org.BouncyCastle.Asn1.X9.X962NamedCurves");
public static readonly KnownType Org_BouncyCastle_Crypto_Engines_AesFastEngine = new("Org.BouncyCastle.Crypto.Engines.AesFastEngine");
public static readonly KnownType Org_BouncyCastle_Crypto_Generators_BCrypt = new("Org.BouncyCastle.Crypto.Generators.BCrypt");
public static readonly KnownType Org_BouncyCastle_Crypto_Generators_SCrypt = new("Org.BouncyCastle.Crypto.Generators.SCrypt");
public static readonly KnownType Org_BouncyCastle_Crypto_Generators_DHParametersGenerator = new("Org.BouncyCastle.Crypto.Generators.DHParametersGenerator");
public static readonly KnownType Org_BouncyCastle_Crypto_Generators_OpenBsdBCrypt = new("Org.BouncyCastle.Crypto.Generators.OpenBsdBCrypt");
public static readonly KnownType Org_BouncyCastle_Crypto_Generators_DsaParametersGenerator = new("Org.BouncyCastle.Crypto.Generators.DsaParametersGenerator");
Expand Down
96 changes: 85 additions & 11 deletions analyzers/tests/SonarAnalyzer.Test/Rules/PasswordsShouldBeStoredCorrectlyTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,21 +36,51 @@ public class PasswordsShouldBeStoredCorrectlyTest
.AddReferences([
AspNetCoreMetadataReference.MicrosoftExtensionsIdentityCore,
AspNetCoreMetadataReference.MicrosoftAspNetCoreCryptographyKeyDerivation,
CoreMetadataReference.SystemSecurityCryptography,
..MetadataReferenceFacade.SystemSecurityCryptography,
costin-zaharia-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
])
.Verify();
#endif

[TestMethod]
public void PasswordsShouldBeStoredCorrectly_CS_PasswordHasherOptions() =>
Builder
.WithOptions(ParseOptionsHelper.FromCSharp9)
.AddReferences(NuGetMetadataReference.MicrosoftAspNetIdentity())
.AddSnippet("""
using Microsoft.AspNet.Identity;

class Testcases
{
void Method()
{
var _ = new PasswordHasherOptions(); // Noncompliant {{PasswordHasher does not support state-of-the-art parameters. Use Rfc2898DeriveBytes instead.}}
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^
PasswordHasherOptions x = new(); // Noncompliant
// ^^^^^
_ = new PasswordHasherOptions() {}; // Noncompliant
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_ = new PasswordHasherOptions { IterationCount = 42 }; // Noncompliant
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_ = new Derived(); // Noncompliant
// ^^^^^^^^^^^^^
}
}

public class Derived: PasswordHasherOptions { }
""")
.Verify();

[TestMethod]
public void PasswordsShouldBeStoredCorrectly_CS_Rfc2898DeriveBytes() =>
Builder
.AddReferences(MetadataReferenceFacade.SystemSecurityCryptography)
.AddSnippet("""
using System.Security.Cryptography;
class Testcases
{
const int ITERATIONS = 100_000;

void Method(int iterations, byte[] bs)
void Method(int iterations, byte[] bs, HashAlgorithmName han)
{
new Rfc2898DeriveBytes("password", bs); // Noncompliant
new Rfc2898DeriveBytes("password", 42); // Noncompliant
Expand All @@ -60,28 +90,44 @@ void Method(int iterations, byte[] bs)
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
new Rfc2898DeriveBytes("password", bs, 42); // Noncompliant
new Rfc2898DeriveBytes("password", 42, 42); // Noncompliant
new Rfc2898DeriveBytes(bs, bs, 42, HashAlgorithmName.MD5); // Noncompliant {{Use at least 100,000 iterations here.}}
new Rfc2898DeriveBytes(bs, bs, 42, han); // Noncompliant {{Use at least 100,000 iterations here.}}
// ^^
new Rfc2898DeriveBytes("password", bs, 42, HashAlgorithmName.MD5); // Noncompliant
new Rfc2898DeriveBytes("password", 42, 42, HashAlgorithmName.MD5); // Noncompliant
new Rfc2898DeriveBytes("password", bs, 42, han); // Noncompliant
new Rfc2898DeriveBytes("password", 42, 42, han); // Noncompliant
new Rfc2898DeriveBytes(bs, bs, ITERATIONS); // Noncompliant
new Rfc2898DeriveBytes("", bs, ITERATIONS); // Noncompliant
new Rfc2898DeriveBytes("", 42, ITERATIONS); // Noncompliant
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

new Rfc2898DeriveBytes(bs, bs, iterations, HashAlgorithmName.SHA1); // Compliant
new Rfc2898DeriveBytes(bs, bs, ITERATIONS, HashAlgorithmName.SHA1); // Compliant
new Rfc2898DeriveBytes("", bs, ITERATIONS, HashAlgorithmName.SHA1); // Compliant
new Rfc2898DeriveBytes("", 42, ITERATIONS, HashAlgorithmName.SHA1); // Compliant
new Rfc2898DeriveBytes(bs, bs, iterations, han); // Compliant
new Rfc2898DeriveBytes(bs, bs, ITERATIONS, han); // Compliant
new Rfc2898DeriveBytes("", bs, ITERATIONS, han); // Compliant
new Rfc2898DeriveBytes("", 42, ITERATIONS, han); // Compliant

var x = new Rfc2898DeriveBytes(bs, bs, ITERATIONS, han);
x.IterationCount = 1; // Noncompliant {{Use at least 100,000 iterations here.}}
// ^^^^^^^^^^^^^^^^
Comment on lines +108 to +109
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 a compliant example is missing with setting the IterationCount property to a safe value.


new Rfc2898DeriveBytes(bs, bs, ITERATIONS, han)
{
IterationCount = 42 // Noncompliant {{Use at least 100,000 iterations here.}}
// ^^^^^^^^^^^^^^
costin-zaharia-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
};
}

void MakeItUnsafe(Rfc2898DeriveBytes password)
{
password.IterationCount = 1; // Noncompliant {{Use at least 100,000 iterations here.}}
// ^^^^^^^^^^^^^^^^^^^^^^^
}
}
""")
.AddReferences(MetadataReferenceFacade.SystemSecurityCryptography)
.Verify();

[TestMethod]
public void PasswordsShouldBeStoredCorrectly_CS_BouncyCastle_Generate() =>
Builder
.AddReferences(NuGetMetadataReference.BouncyCastle())
.AddSnippet("""
using Org.BouncyCastle.Crypto.Generators;

Expand Down Expand Up @@ -118,12 +164,12 @@ void Method(char[] cs, byte[] bs)
}
}
""")
.AddReferences(NuGetMetadataReference.BouncyCastle())
.Verify();

[TestMethod]
public void PasswordsShouldBeStoredCorrectly_CS_BouncyCastle_Init() =>
Builder
.AddReferences(NuGetMetadataReference.BouncyCastle())
.AddSnippet("""
using Org.BouncyCastle.Crypto;
using Org.BouncyCastle.Crypto.Generators;
Expand All @@ -144,6 +190,34 @@ void Method(byte[] bs, PbeParametersGenerator baseGen, Pkcs5S2ParametersGenerato
}
}
""")
.Verify();

[TestMethod]
public void PasswordsShouldBeStoredCorrectly_CS_BouncyCastle_Generate_SCrypt() =>
Builder
.AddReferences(NuGetMetadataReference.BouncyCastle())
.AddSnippet("""
using Org.BouncyCastle.Crypto.Generators;

class Testcases
{
void Method(byte[] bs, int p)
{
SCrypt.Generate(bs, bs, 1 << 12, 8, p, 32); // Compliant

SCrypt.Generate(bs, bs, 1 << 11, 42, p, 42); // Noncompliant {{Use a cost factor of at least 2 ^ 12 for N here.}}
// ^^^^^^^
SCrypt.Generate(bs, bs, 1 << 12, 7, p, 42); // Noncompliant {{Use a memory factor of at least 8 for r here.}}
// ^
SCrypt.Generate(bs, bs, 1 << 12, 42, p, 31); // Noncompliant {{Use an output length of at least 32 for dkLen here.}}
// ^^

SCrypt.Generate(bs, bs, 1 << 11, 7, p, 31);
// ^^^^^^^ {{Use a cost factor of at least 2 ^ 12 for N here.}}
// ^ @-1 {{Use a memory factor of at least 8 for r here.}}
// ^^ @-2 {{Use an output length of at least 32 for dkLen here.}}
}
}
""")
.Verify();
}
1 change: 1 addition & 0 deletions analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/NuGetMetadataReference.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public static class NuGetMetadataReference
public static References MicrosoftAspNetCoreRouting(string packageVersion) => Create("Microsoft.AspNetCore.Routing", packageVersion);
public static References MicrosoftAspNetCoreMvcRazorRuntime(string packageVersion = "2.2.0") => Create("Microsoft.AspNetCore.Razor.Runtime", packageVersion);
public static References MicrosoftAspNetCoreRoutingAbstractions(string packageVersion) => Create("Microsoft.AspNetCore.Routing.Abstractions", packageVersion);
public static References MicrosoftAspNetIdentity(string packageVersion = "3.0.0-rc1-final") => Create("Microsoft.AspNet.Identity", packageVersion);
gregory-paidis-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
public static References MicrosoftAspNetMvc(string packageVersion) => Create("Microsoft.AspNet.Mvc", packageVersion);
public static References MicrosoftAspNetCoreAppRef(string packageVersion) => Create("Microsoft.AspNetCore.App.Ref", packageVersion);
public static References MicrosoftAspNetSignalRCore(string packageVersion = "2.4.1") => Create("Microsoft.AspNet.SignalR.Core", packageVersion);
Expand Down