From e207cb3ff72290b3e43b4d94a02384bad6144ae7 Mon Sep 17 00:00:00 2001 From: Gregory Paidis Date: Mon, 13 May 2024 17:38:38 +0200 Subject: [PATCH] Create rule S5344: Passwords should not be stored in plain-text or with a fast hashing algorithm --- analyzers/rspec/cs/S5344.html | 317 ++++++++++++++++++ analyzers/rspec/cs/S5344.json | 54 +++ analyzers/rspec/cs/Sonar_way_profile.json | 1 + .../Rules/PasswordsShouldBeStoredCorrectly.cs | 137 ++++++++ .../SonarAnalyzer.Common/Helpers/KnownType.cs | 5 + .../PackagingTests/RuleTypeMappingCS.cs | 2 +- .../PasswordsShouldBeStoredCorrectlyTest.cs | 149 ++++++++ .../PasswordsShouldBeStoredCorrectly.Core.cs | 144 ++++++++ .../AspNetCoreMetadataReference.cs | 2 + 9 files changed, 810 insertions(+), 1 deletion(-) create mode 100644 analyzers/rspec/cs/S5344.html create mode 100644 analyzers/rspec/cs/S5344.json create mode 100644 analyzers/src/SonarAnalyzer.CSharp/Rules/PasswordsShouldBeStoredCorrectly.cs create mode 100644 analyzers/tests/SonarAnalyzer.Test/Rules/PasswordsShouldBeStoredCorrectlyTest.cs create mode 100644 analyzers/tests/SonarAnalyzer.Test/TestCases/PasswordsShouldBeStoredCorrectly.Core.cs diff --git a/analyzers/rspec/cs/S5344.html b/analyzers/rspec/cs/S5344.html new file mode 100644 index 00000000000..4a93cd61b37 --- /dev/null +++ b/analyzers/rspec/cs/S5344.html @@ -0,0 +1,317 @@ +

The improper storage of passwords poses a significant security risk to software applications. This vulnerability arises when passwords are stored +in plaintext or with a fast hashing algorithm. To exploit this vulnerability, an attacker typically requires access to the stored passwords.

+

Why is this an issue?

+

Attackers who would get access to the stored passwords could reuse them without further attacks or with little additional effort.
Obtaining the +plaintext passwords, they could then gain unauthorized access to user accounts, potentially leading to various malicious activities.

+

What is the potential impact?

+

Plaintext or weakly hashed password storage poses a significant security risk to software applications.

+

Unauthorized Access

+

When passwords are stored in plaintext or with weak hashing algorithms, an attacker who gains access to the password database can easily retrieve +and use the passwords to gain unauthorized access to user accounts. This can lead to various malicious activities, such as unauthorized data access, +identity theft, or even financial fraud.

+

Credential Reuse

+

Many users tend to reuse passwords across multiple platforms. If an attacker obtains plaintext or weakly hashed passwords, they can potentially use +these credentials to gain unauthorized access to other accounts held by the same user. This can have far-reaching consequences, as sensitive personal +information or critical systems may be compromised.

+

Regulatory Compliance

+

Many industries and jurisdictions have specific regulations and standards to protect user data and ensure its confidentiality. Storing passwords in +plaintext or with weak hashing algorithms can lead to non-compliance with these regulations, potentially resulting in legal consequences, financial +penalties, and damage to the reputation of the software application and its developers.

+

How to fix it in ASP.NET Core

+

Code examples

+

Noncompliant code example

+

Using Microsoft.AspNetCore.Cryptography.KeyDerivation:

+
+using Microsoft.AspNetCore.Cryptography.KeyDerivation;
+using System.Security.Cryptography;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8);
+
+string hashed = Convert.ToBase64String(KeyDerivation.Pbkdf2(
+    password: password!,
+    salt: salt,
+    prf: KeyDerivationPrf.HMACSHA256,
+    iterationCount: 1, // Noncompliant
+    numBytesRequested: 256 / 8));
+
+

Using System.Security.Cryptography:

+
+using System.Security.Cryptography;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8);
+Rfc2898DeriveBytes kdf = new Rfc2898DeriveBytes(password, 128/8); // Noncompliant
+string hashed = Convert.ToBase64String(kdf.GetBytes(256 / 8));
+
+

Using Microsoft.AspNetCore.Identity:

+
+using Microsoft.AspNetCore.Identity;
+using Microsoft.Extensions.Options;
+
+string password = Request.Query["password"];
+IOptions<PasswordHasherOptions> options = Options.Create(new PasswordHasherOptions() {
+    IterationCount = 1 // Noncompliant
+});
+PasswordHasher<User> hasher = new PasswordHasher<User>(options);
+string hash = hasher.HashPassword(new User("test"), password);
+
+

Compliant solution

+

Using Microsoft.AspNetCore.Cryptography.KeyDerivation:

+
+using Microsoft.AspNetCore.Cryptography.KeyDerivation;
+using System.Security.Cryptography;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8);
+
+string hashed = Convert.ToBase64String(KeyDerivation.Pbkdf2(
+    password: password!,
+    salt: salt,
+    prf: KeyDerivationPrf.HMACSHA256,
+    iterationCount: 100_000,
+    numBytesRequested: 256 / 8));
+
+

Using System.Security.Cryptography

+
+using System.Security.Cryptography;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8);
+Rfc2898DeriveBytes kdf = new Rfc2898DeriveBytes(password, salt, 100_000, HashAlgorithmName.SHA256);
+string hashed = Convert.ToBase64String(kdf.GetBytes(256 / 8));
+
+

Using Microsoft.AspNetCore.Identity:

+
+using Microsoft.AspNetCore.Identity;
+using Microsoft.Extensions.Options;
+
+string password = Request.Query["password"];
+PasswordHasher<User> hasher = new PasswordHasher<User>();
+string hash = hasher.HashPassword(new User("test"), password);
+
+

How does this work?

+

Select the correct PBKDF2 parameters

+

If PBKDF2 must be used, be aware that default values might not be considered secure.
Depending on the algorithm used, the number of iterations +should be adjusted to ensure that the derived key is secure. The following are the recommended number of iterations for PBKDF2:

+ +

Note that PBKDF2-HMAC-SHA256 is recommended by NIST.
Iterations are also called "rounds" depending on the library used.

+

When recommended cost factors are too high in the context of the application or if the performance cost is unacceptable, a cost factor reduction +might be considered. In that case, it should not be chosen under 100,000.

+

Going the extra mile

+

Pepper

+

In a defense-in-depth security approach, peppering can also be used. This is a security technique where an external secret value +is added to a password before it is hashed.
This makes it more difficult for an attacker to crack the hashed passwords, as they would need to know +the secret value to generate the correct hash.
Learn more here.

+

How to fix it in ASP.NET

+

Code examples

+

Noncompliant code example

+
+using System.Security.Cryptography;
+
+RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider();
+byte[] salt = new byte[32];
+rngCsp.GetBytes(salt);
+Rfc2898DeriveBytes kdf = new Rfc2898DeriveBytes(password, salt, 100, HashAlgorithmName.SHA256); // Noncompliant
+string hashed = Convert.ToBase64String(kdf.GetBytes(256 / 8));
+
+

Using using Microsoft.AspNet.Identity:

+
+using Microsoft.AspNet.Identity;
+
+string password = "NotSoSecure";
+PasswordHasher hasher = new PasswordHasher();
+string hash = hasher.HashPassword(password); // Noncompliant
+
+

Compliant solution

+
+using System.Security.Cryptography;
+
+RNGCryptoServiceProvider rngCsp = new RNGCryptoServiceProvider();
+byte[] salt = new byte[32];
+rngCsp.GetBytes(salt);
+Rfc2898DeriveBytes kdf = new Rfc2898DeriveBytes(password, salt, 100_000, HashAlgorithmName.SHA256); // Noncompliant
+string hashed = Convert.ToBase64String(kdf.GetBytes(256 / 8));
+
+

How does this work?

+

Select the correct PBKDF2 parameters

+

If PBKDF2 must be used, be aware that default values might not be considered secure.
Depending on the algorithm used, the number of iterations +should be adjusted to ensure that the derived key is secure. The following are the recommended number of iterations for PBKDF2:

+ +

Note that PBKDF2-HMAC-SHA256 is recommended by NIST.
Iterations are also called "rounds" depending on the library used.

+

When recommended cost factors are too high in the context of the application or if the performance cost is unacceptable, a cost factor reduction +might be considered. In that case, it should not be chosen under 100,000.

+

Going the extra mile

+

Pepper

+

In a defense-in-depth security approach, peppering can also be used. This is a security technique where an external secret value +is added to a password before it is hashed.
This makes it more difficult for an attacker to crack the hashed passwords, as they would need to know +the secret value to generate the correct hash.
Learn more here.

+

How to fix it in BouncyCastle

+

Code examples

+

Noncompliant code example

+

Using SCrypt:

+
+using Org.BouncyCastle.Crypto.Generators;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8); // divide by 8 to convert bits to bytes
+
+string hashed = Convert.ToBase64String(SCrypt.Generate(Encoding.Unicode.GetBytes(password), salt, 4, 2, 1, 32));  // Noncompliant
+
+

Using BCrypt:

+
+using Org.BouncyCastle.Crypto.Generators;
+using Org.BouncyCastle.Crypto.Parameters;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8);
+
+string hashed = OpenBsdBCrypt.Generate(password.ToCharArray(), salt, 4); // Noncompliant
+
+

Using PBKDF2:

+
+using Org.BouncyCastle.Crypto.Generators;
+using Org.BouncyCastle.Crypto.Parameters;
+using System.Security.Cryptography;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8);
+Pkcs5S2ParametersGenerator gen = new Pkcs5S2ParametersGenerator();
+gen.Init(Encoding.Unicode.GetBytes(password), salt, 100);  // Noncompliant
+KeyParameter keyParam = (KeyParameter) gen.GenerateDerivedMacParameters(256);
+string hashed = Convert.ToBase64String(keyParam.GetKey());
+
+

Compliant solution

+

Using SCrypt:

+
+using Org.BouncyCastle.Crypto.Generators;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8); // divide by 8 to convert bits to bytes
+
+string hashed = Convert.ToBase64String(SCrypt.Generate(Encoding.Unicode.GetBytes(password), salt, 1<<12, 8, 1, 32));  // Noncompliant
+
+

Using BCrypt:

+
+using Org.BouncyCastle.Crypto.Generators;
+using Org.BouncyCastle.Crypto.Parameters;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8);
+
+string hashed = OpenBsdBCrypt.Generate(password.ToCharArray(), salt, 14); // Noncompliant
+
+

Using PBKDF2:

+
+using Org.BouncyCastle.Crypto.Generators;
+using Org.BouncyCastle.Crypto.Parameters;
+using System.Security.Cryptography;
+
+string password = Request.Query["password"];
+byte[] salt = RandomNumberGenerator.GetBytes(128 / 8);
+Pkcs5S2ParametersGenerator gen = new Pkcs5S2ParametersGenerator();
+gen.Init(Encoding.Unicode.GetBytes(password), salt, 100_000);  // Noncompliant
+KeyParameter keyParam = (KeyParameter) gen.GenerateDerivedMacParameters(256);
+string hashed = Convert.ToBase64String(keyParam.GetKey());
+
+

How does this work?

+

Select the correct Bcrypt parameters

+

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.

+

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 (2**12).
This number is not the same coefficient as the first one because it uses a different algorithm.

+

Select the correct Scrypt parameters

+

If scrypt must be used, the default values of scrypt are considered secure.

+

Like Argon2id, scrypt has three different parameters that can be configured. N is the CPU/memory cost parameter and must be a power of two. r is +the block size and p is the parallelization factor.

+

All three parameters affect the memory and CPU usage of the algorithm. Higher values of N, r and p result in safer hashes, but come at the cost of +higher resource usage.

+

For scrypt, OWASP recommends to have a hash length of at least 64 bytes, and to set N, p and r to the values of one of the following rows:

+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
N (cost parameter)p (parallelization factor)r (block size)

217 (1 << 17)

1

8

216 (1 << 16)

2

8

215 (1 << 15)

3

8

214 (1 << 14)

5

8

213 (1 << 13)

10

8

+

Every row provides the same level of defense. They only differ in the amount of CPU and RAM used: the top row has low CPU usage and high memory +usage, while the bottom row has high CPU usage and low memory usage.

+

Select the correct PBKDF2 parameters

+

If PBKDF2 must be used, be aware that default values might not be considered secure.
Depending on the algorithm used, the number of iterations +should be adjusted to ensure that the derived key is secure. The following are the recommended number of iterations for PBKDF2:

+ +

Note that PBKDF2-HMAC-SHA256 is recommended by NIST.
Iterations are also called "rounds" depending on the library used.

+

When recommended cost factors are too high in the context of the application or if the performance cost is unacceptable, a cost factor reduction +might be considered. In that case, it should not be chosen under 100,000.

+

Going the extra mile

+

Pepper

+

In a defense-in-depth security approach, peppering can also be used. This is a security technique where an external secret value +is added to a password before it is hashed.
This makes it more difficult for an attacker to crack the hashed passwords, as they would need to know +the secret value to generate the correct hash.
Learn more here.

+

Resources

+

Documentation

+ +

Standards

+ + diff --git a/analyzers/rspec/cs/S5344.json b/analyzers/rspec/cs/S5344.json new file mode 100644 index 00000000000..982ecf7c343 --- /dev/null +++ b/analyzers/rspec/cs/S5344.json @@ -0,0 +1,54 @@ +{ + "title": "Passwords should not be stored in plaintext or with a fast hashing algorithm", + "type": "VULNERABILITY", + "code": { + "impacts": { + "SECURITY": "HIGH" + }, + "attribute": "TRUSTWORTHY" + }, + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "30min" + }, + "tags": [ + "cwe", + "spring" + ], + "defaultSeverity": "Critical", + "ruleSpecification": "RSPEC-5344", + "sqKey": "S5344", + "scope": "Main", + "securityStandards": { + "CWE": [ + 256, + 916 + ], + "OWASP": [ + "A3" + ], + "OWASP Top 10 2021": [ + "A2", + "A4" + ], + "PCI DSS 3.2": [ + "6.5.3" + ], + "PCI DSS 4.0": [ + "6.2.4" + ], + "ASVS 4.0": [ + "2.10.3", + "2.4.1", + "2.4.2", + "2.4.3", + "2.4.4", + "2.4.5" + ], + "STIG ASD 2023-06-08": [ + "V-222542" + ] + }, + "quickfix": "unknown" +} diff --git a/analyzers/rspec/cs/Sonar_way_profile.json b/analyzers/rspec/cs/Sonar_way_profile.json index b4ff4ab92e1..e4c89497302 100644 --- a/analyzers/rspec/cs/Sonar_way_profile.json +++ b/analyzers/rspec/cs/Sonar_way_profile.json @@ -267,6 +267,7 @@ "S5042", "S5122", "S5332", + "S5344", "S5443", "S5445", "S5542", diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/PasswordsShouldBeStoredCorrectly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/PasswordsShouldBeStoredCorrectly.cs new file mode 100644 index 00000000000..8d5b2ce4250 --- /dev/null +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/PasswordsShouldBeStoredCorrectly.cs @@ -0,0 +1,137 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +using SonarAnalyzer.Helpers.Trackers; + +namespace SonarAnalyzer.Rules.CSharp; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class PasswordsShouldBeStoredCorrectly : SonarDiagnosticAnalyzer +{ + private const string DiagnosticId = "S5344"; + private const string MessageFormat = "{0}"; + private const string UseMoreIterationsMessageFormat = "Use at least 100 000 iterations here."; + private const int IterationCountThreshold = 100_000; + + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + protected override void Initialize(SonarAnalysisContext context) + { + NetCore(context); + Rfc2898DeriveBytes(context); + BouncyCastle(context); + } + + private static void NetCore(SonarAnalysisContext context) + { + var propertyTracker = CSharpFacade.Instance.Tracker.PropertyAccess; + Track( + propertyTracker, + context, + UseMoreIterationsMessageFormat, + propertyTracker.MatchSetter(), + propertyTracker.MatchProperty(new MemberDescriptor(KnownType.Microsoft_AspNetCore_Identity_PasswordsHasherOptions, "IterationCount")), + 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 + + var argumentTracker = CSharpFacade.Instance.Tracker.Argument; + Track( + argumentTracker, + context, + UseMoreIterationsMessageFormat, + argumentTracker.MatchArgument(ArgumentDescriptor.MethodInvocation(KnownType.Microsoft_AspNetCore_Cryptography_KeyDerivation_KeyDerivation, "Pbkdf2", "iterationCount", 3)), + x => ArgumentLessThan(x, IterationCountThreshold)); + } + + private static void Rfc2898DeriveBytes(SonarAnalysisContext context) + { + // Raise when hashAlgorithm is present + var argumentTracker = CSharpFacade.Instance.Tracker.Argument; + // Exclude the constructors that have a hashAlgorithm parameter + var constructorArgument = ArgumentDescriptor.ConstructorInvocation( + ctor => ctor.ContainingType.Is(KnownType.System_Security_Cryptography_Rfc2898DeriveBytes) && ctor.Parameters.Any(x => x.Name == "hashAlgorithm"), + (methodName, comparison) => string.Compare(methodName, "Rfc2898DeriveBytes", comparison) == 0, + null, + x => x.Name == "iterations", + null, + null); + var invocationArgument = ArgumentDescriptor.MethodInvocation(KnownType.System_Security_Cryptography_Rfc2898DeriveBytes, "Pbkdf2", "iterations", x => x is 2 or 3); + Track( + argumentTracker, + context, + UseMoreIterationsMessageFormat, + argumentTracker.Or( + argumentTracker.MatchArgument(constructorArgument), + argumentTracker.MatchArgument(invocationArgument)), + x => ArgumentLessThan(x, IterationCountThreshold)); + + // Raise when hashAlgorithm is NOT present + var objectCreationTracker = CSharpFacade.Instance.Tracker.ObjectCreation; + Track( + objectCreationTracker, + 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")); + } + + private static void BouncyCastle(SonarAnalysisContext context) + { + var tracker = CSharpFacade.Instance.Tracker.Argument; + + Track( + tracker, + 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))), + x => ArgumentLessThan(x, 12)); + + Track( + tracker, + context, + UseMoreIterationsMessageFormat, + tracker.MatchArgument(ArgumentDescriptor.MethodInvocation(KnownType.Org_BouncyCastle_Crypto_PbeParametersGenerator, "Init", "iterationCount", 2)), + x => ArgumentLessThan(x, IterationCountThreshold)); + } + + private static bool HasFewIterations(PropertyAccessContext context, PropertyAccessTracker tracker) => + tracker.AssignedValue(context) is int iterationCount + && iterationCount < IterationCountThreshold; + + private static bool ArgumentLessThan(ArgumentContext context, int threshold) => + context.SemanticModel.GetConstantValue(((ArgumentSyntax)context.Node).Expression) is { HasValue: true, Value: int value } + && value < threshold; + + private static void Track(SyntaxTrackerBase tracker, + SonarAnalysisContext context, + string message, + params SyntaxTrackerBase.Condition[] conditions) where TContext : SyntaxBaseContext => + tracker.Track(new(context, AnalyzerConfiguration.AlwaysEnabled, Rule), [message], conditions); +} diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs index 4e0e9834b1a..1816525ab37 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs @@ -61,6 +61,7 @@ public sealed partial class KnownType public static readonly KnownType Microsoft_AspNetCore_Components_RouteAttribute = new("Microsoft.AspNetCore.Components.RouteAttribute"); public static readonly KnownType Microsoft_AspNetCore_Components_SupplyParameterFromQueryAttribute = new("Microsoft.AspNetCore.Components.SupplyParameterFromQueryAttribute"); public static readonly KnownType Microsoft_AspNetCore_Cors_Infrastructure_CorsPolicyBuilder = new("Microsoft.AspNetCore.Cors.Infrastructure.CorsPolicyBuilder"); + public static readonly KnownType Microsoft_AspNetCore_Cryptography_KeyDerivation_KeyDerivation = new("Microsoft.AspNetCore.Cryptography.KeyDerivation.KeyDerivation"); public static readonly KnownType Microsoft_AspNetCore_Hosting_HostingEnvironmentExtensions = new("Microsoft.AspNetCore.Hosting.HostingEnvironmentExtensions"); public static readonly KnownType Microsoft_AspNetCore_Hosting_WebHostBuilderExtensions = new("Microsoft.AspNetCore.Hosting.WebHostBuilderExtensions"); public static readonly KnownType Microsoft_AspNetCore_Http_CookieOptions = new("Microsoft.AspNetCore.Http.CookieOptions"); @@ -73,6 +74,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_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"); @@ -208,9 +210,12 @@ public sealed partial class KnownType public static readonly KnownType Org_BouncyCastle_Asn1_X9_ECNamedCurveTable = new("Org.BouncyCastle.Asn1.X9.ECNamedCurveTable"); 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_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"); public static readonly KnownType Org_BouncyCastle_Crypto_Parameters_RsaKeyGenerationParameters = new("Org.BouncyCastle.Crypto.Parameters.RsaKeyGenerationParameters"); + public static readonly KnownType Org_BouncyCastle_Crypto_PbeParametersGenerator = new("Org.BouncyCastle.Crypto.PbeParametersGenerator"); public static readonly KnownType Serilog_Events_LogEventLevel = new("Serilog.Events.LogEventLevel"); public static readonly KnownType Serilog_ILogger = new("Serilog.ILogger"); public static readonly KnownType Serilog_LoggerConfiguration = new("Serilog.LoggerConfiguration"); diff --git a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs index ba7bd4005c3..eed58fe5f6a 100644 --- a/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.Test/PackagingTests/RuleTypeMappingCS.cs @@ -5268,7 +5268,7 @@ internal static class RuleTypeMappingCS // ["S5341"], // ["S5342"], // ["S5343"], - // ["S5344"], + ["S5344"] = "VULNERABILITY", // ["S5345"], // ["S5346"], // ["S5347"], diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/PasswordsShouldBeStoredCorrectlyTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/PasswordsShouldBeStoredCorrectlyTest.cs new file mode 100644 index 00000000000..389e6c6594f --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/PasswordsShouldBeStoredCorrectlyTest.cs @@ -0,0 +1,149 @@ +/* + * SonarAnalyzer for .NET + * Copyright (C) 2015-2024 SonarSource SA + * mailto: contact AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +using SonarAnalyzer.Rules.CSharp; + +namespace SonarAnalyzer.Test.Rules; + +[TestClass] +public class PasswordsShouldBeStoredCorrectlyTest +{ + private static readonly VerifierBuilder Builder = new VerifierBuilder(); + +#if NET + [TestMethod] + public void PasswordsShouldBeStoredCorrectly_CS_Core() => + Builder + .WithOptions(ParseOptionsHelper.FromCSharp8) + .AddPaths("PasswordsShouldBeStoredCorrectly.Core.cs") + .AddReferences([ + AspNetCoreMetadataReference.MicrosoftExtensionsIdentityCore, + AspNetCoreMetadataReference.MicrosoftAspNetCoreCryptographyKeyDerivation, + CoreMetadataReference.SystemSecurityCryptography, + ]) + .Verify(); +#endif + + [TestMethod] + public void PasswordsShouldBeStoredCorrectly_CS_Rfc2898DeriveBytes() => + Builder + .AddSnippet(""" + using System.Security.Cryptography; + class Testcases + { + const int ITERATIONS = 100_000; + + void Method(int iterations, byte[] bs) + { + new Rfc2898DeriveBytes("password", bs); // Noncompliant + new Rfc2898DeriveBytes("password", 42); // Noncompliant + new Rfc2898DeriveBytes(bs, bs, 42); // Noncompliant + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + new Rfc2898DeriveBytes(iterations: 42, salt: bs, password: bs); // Noncompliant {{Use at least 100 000 iterations and a state-of-the-art digest algorithm here.}} + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + 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("password", bs, 42, HashAlgorithmName.MD5); // Noncompliant + new Rfc2898DeriveBytes("password", 42, 42, HashAlgorithmName.MD5); // 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 + } + } + """) + .AddReferences(MetadataReferenceFacade.SystemSecurityCryptography) + .Verify(); + + [TestMethod] + public void PasswordsShouldBeStoredCorrectly_CS_BouncyCastle_Generate() => + Builder + .AddSnippet(""" + using Org.BouncyCastle.Crypto.Generators; + + class Testcases + { + const int COST = 12; + + void Method(char[] cs, byte[] bs) + { + OpenBsdBCrypt.Generate(cs, bs, 4); // Noncompliant {{Use a cost factor of at least 12 here.}} + OpenBsdBCrypt.Generate(cost: 4, password: cs, salt: bs); // Noncompliant + OpenBsdBCrypt.Generate("", cs, bs, 4); // Noncompliant + // ^ + OpenBsdBCrypt.Generate( + cost: 4, // Noncompliant + // ^^^^^^^ + version: "", + password: cs, + salt: bs); + + OpenBsdBCrypt.Generate(cs, bs, COST); // Compliant + OpenBsdBCrypt.Generate(cs, bs, 42); // Compliant + OpenBsdBCrypt.Generate("", cs, bs, COST); // Compliant + OpenBsdBCrypt.Generate("", cs, bs, 42); // Compliant + OpenBsdBCrypt.Generate( + cost: 42, // Compliant + version: "", + password: cs, + salt: bs); + + BCrypt.Generate(bs, bs, 4); // Noncompliant + // ^ + BCrypt.Generate(bs, bs, COST); // Compliant + } + } + """) + .AddReferences(NuGetMetadataReference.BouncyCastle()) + .Verify(); + + [TestMethod] + public void PasswordsShouldBeStoredCorrectly_CS_BouncyCastle_Init() => + Builder + .AddSnippet(""" + using Org.BouncyCastle.Crypto; + using Org.BouncyCastle.Crypto.Generators; + + class Testcases + { + const int ITERATIONS = 100_000; + + void Method(byte[] bs, PbeParametersGenerator baseGen, Pkcs5S2ParametersGenerator gen, int iterations) + { + baseGen.Init(bs, bs, 42); // Noncompliant {{Use at least 100 000 iterations here.}} + // ^^ + gen.Init(iterationCount: 42, password: bs, salt: bs); // Noncompliant + // ^^^^^^^^^^^^^^^^^^ + + baseGen.Init(bs, bs, ITERATIONS); // Compliant + gen.Init(iterationCount: iterations, password: bs, salt: bs); // Compliant + } + } + """) + .AddReferences(NuGetMetadataReference.BouncyCastle()) + .Verify(); +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/PasswordsShouldBeStoredCorrectly.Core.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/PasswordsShouldBeStoredCorrectly.Core.cs new file mode 100644 index 00000000000..cd62c017cd6 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/PasswordsShouldBeStoredCorrectly.Core.cs @@ -0,0 +1,144 @@ +using System; +using System.Text; +using System.Security.Cryptography; +using Microsoft.AspNetCore.Cryptography.KeyDerivation; +using Microsoft.AspNetCore.Identity; + +class Testcases +{ + const int ITERATIONS = 100_000; + const PasswordHasherCompatibilityMode MODE = PasswordHasherCompatibilityMode.IdentityV3; + + void PasswordHasherOptions_IterationCount(int iterations) + { + var sut = new PasswordHasherOptions() + { + IterationCount = 100_000 // Compliant + }; + + sut = new PasswordHasherOptions() + { + IterationCount = 1 // Noncompliant {{Use at least 100 000 iterations here.}} + // ^^^^^^^^^^^^^^ + }; + + sut.IterationCount = -1; // Noncompliant {{Use at least 100 000 iterations here.}} + // ^^^^^^^^^^^^^^^^^^ + sut.IterationCount = 42; // Noncompliant + sut.IterationCount = 100_000 - 1; // Noncompliant + sut.IterationCount = ITERATIONS - 42; // Noncompliant + + sut.IterationCount = ITERATIONS; // Compliant + sut.IterationCount = 100_000; // Compliant + sut.IterationCount = 1_000_000; // Compliant + + if (iterations >= 100_000) + { + sut.IterationCount = iterations; // Compliant + } + else + { + sut.IterationCount = iterations; // Compliant FN + } + + SetIterationCount(42); // Compliant FN + + void SetIterationCount(int value) + { + sut.IterationCount = value; + } + } + + void PasswordHasherOptions_CompatibilityMode(PasswordHasherCompatibilityMode mode) + { + var sut = new PasswordHasherOptions + { + CompatibilityMode = PasswordHasherCompatibilityMode.IdentityV3 // Compliant + }; + + sut = new PasswordHasherOptions() + { + CompatibilityMode = PasswordHasherCompatibilityMode.IdentityV2 // Noncompliant {{Identity v2 uses only 1000 iterations. Consider changing to identity V3.}} + // ^^^^^^^^^^^^^^^^^ + }; + + sut.CompatibilityMode = MODE; // Compliant + sut.CompatibilityMode = (PasswordHasherCompatibilityMode)1; // Compliant + sut.CompatibilityMode = PasswordHasherCompatibilityMode.IdentityV3; // Compliant + + sut.CompatibilityMode = 0; // Noncompliant {{Identity v2 uses only 1000 iterations. Consider changing to identity V3.}} + // ^^^^^^^^^^^^^^^^^^^^^ + sut.CompatibilityMode = (PasswordHasherCompatibilityMode)0; // Noncompliant + sut.CompatibilityMode = PasswordHasherCompatibilityMode.IdentityV2; // Noncompliant + + if (mode == PasswordHasherCompatibilityMode.IdentityV3) + { + sut.CompatibilityMode = mode; // Compliant + } + else + { + sut.CompatibilityMode = mode; // Compliant FN + } + + SetCompatibility(PasswordHasherCompatibilityMode.IdentityV2); // Compliant FN + + void SetCompatibility(PasswordHasherCompatibilityMode value) + { + sut.CompatibilityMode = value; + } + } + + void KeyDerivation_Pbkdf2(int iterations) + { + KeyDerivation.Pbkdf2(null, null, 0, 42, 0); // Noncompliant {{Use at least 100 000 iterations here.}} + // ^^ + KeyDerivation.Pbkdf2(null, null, 0, iterationCount: 100_000 - 42, 0); // Noncompliant + KeyDerivation.Pbkdf2(null, null, 0, iterationCount: ITERATIONS - 42, 0); // Noncompliant + + KeyDerivation.Pbkdf2( + iterationCount: 42, // Noncompliant {{Use at least 100 000 iterations here.}} + // ^^^^^^^^^^^^^^^^^^ + password: "exploding", + salt: Encoding.UTF8.GetBytes("whale"), + numBytesRequested: 42, + prf: 0); + + KeyDerivation.Pbkdf2(null, null, 0, 100_000, 0); // Compliant + KeyDerivation.Pbkdf2(null, null, 0, iterationCount: 100_000 - 42 + 43, 0); // Compliant + KeyDerivation.Pbkdf2(null, null, 0, iterationCount: ITERATIONS, 0); // Compliant + KeyDerivation.Pbkdf2(null, null, 0, iterationCount: iterations, 0); // Compliant + + KeyDerivation.Pbkdf2( + iterationCount: 100_000, // Compliant + password: "exploding", + salt: Encoding.UTF8.GetBytes("whale"), + numBytesRequested: 42, + prf: 0); + } + + void Rfc2898DeriveBytes_Pbkdf2(int iterations, byte[] bs, HashAlgorithmName han, ReadOnlySpan ss, ReadOnlySpan cs, Span dest) + { + Rfc2898DeriveBytes.Pbkdf2(bs, bs, 42, han, 42); // Noncompliant {{Use at least 100 000 iterations here.}} + Rfc2898DeriveBytes.Pbkdf2("", bs, 42, han, 42); // Noncompliant + Rfc2898DeriveBytes.Pbkdf2(ss, ss, 42, han, 42); // Noncompliant + Rfc2898DeriveBytes.Pbkdf2(ss, ss, dest, 42, han); // Noncompliant + Rfc2898DeriveBytes.Pbkdf2(cs, ss, 42, han, 42); // Noncompliant + Rfc2898DeriveBytes.Pbkdf2(cs, ss, dest, 42, han); // Noncompliant + // ^^ + + Rfc2898DeriveBytes.Pbkdf2( + iterations: 42, // Noncompliant + // ^^^^^^^^^^^^^^ + password: cs, + salt: ss, + hashAlgorithm: han, + outputLength: 0); + + Rfc2898DeriveBytes.Pbkdf2(bs, bs, ITERATIONS, han, 42); // Compliant + Rfc2898DeriveBytes.Pbkdf2("", bs, ITERATIONS, han, 42); // Compliant + Rfc2898DeriveBytes.Pbkdf2(ss, ss, ITERATIONS, han, 42); // Compliant + Rfc2898DeriveBytes.Pbkdf2(ss, ss, dest, ITERATIONS, han); // Compliant + Rfc2898DeriveBytes.Pbkdf2(cs, ss, ITERATIONS, han, 42); // Compliant + Rfc2898DeriveBytes.Pbkdf2(cs, ss, dest, ITERATIONS, han); // Compliant + } +} diff --git a/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/AspNetCoreMetadataReference.cs b/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/AspNetCoreMetadataReference.cs index 47e3866773d..dd235b42b80 100644 --- a/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/AspNetCoreMetadataReference.cs +++ b/analyzers/tests/SonarAnalyzer.TestFramework/MetadataReferences/AspNetCoreMetadataReference.cs @@ -41,6 +41,8 @@ public static class AspNetCoreMetadataReference public static MetadataReference MicrosoftAspNetCoreMvcViewFeatures { get; } = CreateReference("Microsoft.AspNetCore.Mvc.ViewFeatures.dll", Sdk.AspNetCore); public static MetadataReference MicrosoftAspNetCoreRouting { get; } = CreateReference("Microsoft.AspNetCore.Routing.dll", Sdk.AspNetCore); public static MetadataReference MicrosoftExtensionsHostingAbstractions { get; } = CreateReference("Microsoft.Extensions.Hosting.Abstractions.dll", Sdk.AspNetCore); + public static MetadataReference MicrosoftExtensionsIdentityCore { get; } = CreateReference("Microsoft.Extensions.Identity.Core.dll", Sdk.AspNetCore); + public static MetadataReference MicrosoftAspNetCoreCryptographyKeyDerivation { get; } = CreateReference("Microsoft.AspNetCore.Cryptography.KeyDerivation.dll", Sdk.AspNetCore); public static MetadataReference MicrosoftExtensionsDependencyInjectionAbstractions { get; } = CreateReference("Microsoft.Extensions.DependencyInjection.Abstractions.dll", Sdk.AspNetCore); public static MetadataReference MicrosoftExtensionsLoggingEventSource { get; } = CreateReference("Microsoft.Extensions.Logging.EventSource.dll", Sdk.AspNetCore); public static MetadataReference MicrosoftExtensionsPrimitives { get; } = CreateReference("Microsoft.Extensions.Primitives.dll", Sdk.AspNetCore);