Create rule S4357: 'SecureRandom' seeds should not be predictable

analyzers/rspec/cs/S4347.html

@@ -0,0 +1,135 @@
+<p>When using <code>SecureRandom</code>, it is important not to use predictable seeds. This class is used to generate cryptographically strong random
+numbers. Using a predictable seed will make its output predictable as well, which counteracts the use case of <code>SecureRandom</code>.</p>
+<h2>Why is this an issue?</h2>
+<p><code>java.security.SecureRandom</code> is often used to generate random values for cryptographic algorithms. When a random number generator is
+used for cryptographic purposes, the generated numbers must be as random and unpredictable as possible. When <code>SecureRandom</code> is improperly
+seeded with a constant or a predictable value, its output will also be predictable.</p>
+<p>This can have severe security implications for cryptographic operations that rely on the randomness of the generated numbers. By using a
+predictable seed, an attacker can potentially guess or deduce the generated numbers, compromising the security of whatever cryptographic algorithm
+relies on <code>SecureRandom</code>.</p>
+<h3>What is the potential impact?</h3>
+<p>It is crucial to understand that the strength of cryptographic algorithms heavily relies on the quality of the random numbers used. By improperly
+seeding the <code>SecureRandom</code> class, we introduce a significant weakness that can be exploited by attackers.</p>
+<h4>Insecure cryptographic keys</h4>
+<p>One of the primary use cases for the <code>SecureRandom</code> class is generating cryptographic keys. If an attacker can predict the seed used to
+initialize the SecureRandom instance, they may be able to derive the same keys. Depending on the use case, this can lead to multiple severe outcomes,
+such as:</p>
+<ul>
+  <li> Being able to decrypt sensitive documents, leading to privacy breaches or identity theft. </li>
+  <li> Gaining access to a private key used for signing, allowing an attacker to forge digital signatures and impersonate legitimate entities. </li>
+  <li> Bypassing authentication mechanisms that rely on public-key infrastructure (PKI), which can be abused to gain unauthorized access to systems or
+  networks. </li>
+</ul>
+<h4>Session hijacking and man-in-the-middle attack</h4>
+<p>Another scenario where this vulnerability can be exploited is in the generation of session tokens or nonces for secure communication protocols. If
+an attacker can predict the seed used to generate these tokens, they can impersonate legitimate users or intercept sensitive information.</p>
+<h2>How to fix it in BouncyCastle</h2>
+<p>BouncyCastle provides several random number generators implementations. Most of these will automatically create unpredictable output.</p>
+<p>The remaining random number generators require seeding with an unpredictable value before they will produce unpredictable outputs. These should be
+seeded with at least 16 bytes of random data to ensure unpredictable output and that the random seed cannot be guessed using a brute-force attack.</p>
+<h3>Code examples</h3>
+<h4>Noncompliant code example</h4>
+<p><code>SecureRandom</code> instances created with <code>GetInstance()</code> are seeded by default. Disabling seeding results in predictable
+output.</p>
+<pre data-diff-id="101" data-diff-type="noncompliant">
+using Org.BouncyCastle.Security;
+
+byte[] random = new byte[8];
+
+SecureRandom sr = SecureRandom.GetInstance("SHA256PRNG", false);
+sr.NextBytes(random); // Noncompliant
+</pre>
+<p><code>DigestRandomGenerator</code> and <code>VmpcRandomGenerator</code> instances require seeding before use. Predictable seed values will result
+in predictable outputs.</p>
+<pre data-diff-id="102" data-diff-type="noncompliant">
+using Org.BouncyCastle.Crypto.Digest;
+using Org.BouncyCastle.Crypto.Prng;
+
+byte[] random = new byte[8];
+
+IRandomGenerator digest = new DigestRandomGenerator(new Sha256Digest());
+digest.AddSeedMaterial(Encoding.UTF8.GetBytes("predictable seed value"));
+digest.NextBytes(random); // Noncompliant
+
+IRandomGenerator vmpc = new VmpcRandomGenerator();
+vmpc.AddSeedMaterial(Convert.FromBase64String("2hq9pkyqLQJkrYTrEv1eNw=="));
+vmpc.NextBytes(random); // Noncompliant
+</pre>
+<p>When a <code>SecureRandom</code> is created using an unseeded <code>DigestRandomGenerator</code> and <code>VmpcRandomGenerator</code> instance, the
+<code>SecureRandom</code> will create predictable outputs.</p>
+<pre data-diff-id="103" data-diff-type="noncompliant">
+using Org.BouncyCastle.Crypto.Digest;
+using Org.BouncyCastle.Crypto.Prng;
+using Org.BouncyCastle.Security;
+
+byte[] random = new byte[8];
+
+IRandomGenerator digest = new DigestRandomGenerator(new Sha256Digest());
+SecureRandom sr = new SecureRandom(digest);
+sr.NextBytes(random); // Noncompliant
+</pre>
+<h4>Compliant solution</h4>
+<p>Allow <code>SecureRandom.GetInstance()</code> to automatically seed new <code>SecureRandom</code> instances.</p>
+<pre data-diff-id="101" data-diff-type="compliant">
+using Org.BouncyCastle.Security;
+
+byte[] random = new byte[8];
+
+SecureRandom sr = SecureRandom.GetInstance("SHA256PRNG");
+sr.NextBytes(random);
+</pre>
+<p>Use unpredictable values to seed <code>DigestRandomGenerator</code> and <code>VmpcRandomGenerator</code> instances. The
+<code>SecureRandom.GenerateSeed()</code> method is designed for this purpose.</p>
+<pre data-diff-id="102" data-diff-type="compliant">
+using Org.BouncyCastle.Crypto.Digest;
+using Org.BouncyCastle.Crypto.Prng;
+using Org.BouncyCastle.Security;
+
+byte[] random = new byte[8];
+
+IRandomGenerator digest = new DigestRandomGenerator(new Sha256Digest());
+digest.AddSeedMaterial(SecureRandom.GenerateSeed(16));
+digest.NextBytes(random);
+
+IRandomGenerator vmpc = new VmpcRandomGenerator();
+vmpc.AddSeedMaterial(SecureRandom.GenerateSeed(16));
+vmpc.NextBytes(random);
+</pre>
+<p>An overload of the <code>SecureRandom</code> constructor will automatically seed the underlying <code>IRandomGenerator</code> with an unpredictable
+value.</p>
+<pre data-diff-id="103" data-diff-type="compliant">
+using Org.BouncyCastle.Crypto.Digest;
+using Org.BouncyCastle.Crypto.Prng;
+using Org.BouncyCastle.Security;
+
+byte[] random = new byte[8];
+
+IRandomGenerator digest = new DigestRandomGenerator(new Sha256Digest());
+SecureRandom sr = new SecureRandom(digest, 16);
+sr.NextBytes(random);
+</pre>
+<h2>Resources</h2>
+<h3>Documentation</h3>
+<ul>
+  <li> Java Documentation - <a href="https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/security/SecureRandom.html">Class
+  <code>java.security.SecureRandom</code></a> </li>
+</ul>
+<h3>Standards</h3>
+<ul>
+  <li> OWASP - <a href="https://owasp.org/Top10/A02_2021-Cryptographic_Failures/">Top 10 2021 Category A2 - Cryptographic Failures</a> </li>
+  <li> OWASP - <a href="https://owasp.org/www-project-top-ten/2017/A6_2017-Security_Misconfiguration">Top 10 2017 Category A6 - Security
+  Misconfiguration</a> </li>
+  <li> CWE - <a href="https://cwe.mitre.org/data/definitions/330">CWE-330 - Use of Insufficiently Random Values</a> </li>
+  <li> CWE - <a href="https://cwe.mitre.org/data/definitions/332">CWE-332 - Insufficient Entropy in PRNG</a> </li>
+  <li> CWE - <a href="https://cwe.mitre.org/data/definitions/336">CWE-336 - Same Seed in Pseudo-Random Number Generator (PRNG)</a> </li>
+  <li> CWE - <a href="https://cwe.mitre.org/data/definitions/337">CWE-337 - Predictable Seed in Pseudo-Random Number Generator (PRNG)</a> </li>
+  <li> <a href="https://wiki.sei.cmu.edu/confluence/display/java/MSC63-J.+Ensure+that+SecureRandom+is+properly+seeded">CERT, MSC63J.</a> - Ensure that
+  SecureRandom is properly seeded </li>
+</ul>
+<h2>Implementation Specification</h2>
+<p>(visible only on this page)</p>
+<h3>Message</h3>
+<p>Change this seed value to something unpredictable, or remove the seed.</p>
+<h3>Highlighting</h3>
+<p>The call to SecureRandom.setSeed() or the SecureRandom(byte[]) constructor call</p>
+

analyzers/rspec/cs/S4347.json

@@ -0,0 +1,43 @@
+{
+  "title": "\"SecureRandom\" seeds should not be predictable",
+  "type": "VULNERABILITY",
+  "code": {
+    "impacts": {
+      "SECURITY": "HIGH"
+    },
+    "attribute": "LOGICAL"
+  },
+  "status": "ready",
+  "remediation": {
+    "func": "Constant\/Issue",
+    "constantCost": "2min"
+  },
+  "tags": [
+    "cwe",
+    "pitfall"
+  ],
+  "defaultSeverity": "Critical",
+  "ruleSpecification": "RSPEC-4347",
+  "sqKey": "S4347",
+  "scope": "Main",
+  "securityStandards": {
+    "CWE": [
+      330,
+      332,
+      336,
+      337
+    ],
+    "OWASP": [
+      "A6"
+    ],
+    "OWASP Top 10 2021": [
+      "A2"
+    ],
+    "ASVS 4.0": [
+      "2.3.1",
+      "2.6.2",
+      "2.9.2"
+    ]
+  },
+  "quickfix": "unknown"
+}

analyzers/rspec/cs/Sonar_way_profile.json

@@ -246,6 +246,7 @@
     "S4260",
     "S4275",
     "S4277",
+    "S4347",
     "S4423",
     "S4426",
     "S4428",

analyzers/src/SonarAnalyzer.CSharp/Rules/SymbolicExecution/SymbolicExecutionRunner.cs

@@ -34,10 +34,6 @@ namespace SonarAnalyzer.Rules.CSharp;
 [DiagnosticAnalyzer(LanguageNames.CSharp)]
 public class SymbolicExecutionRunner : SymbolicExecutionRunnerBase
 {
-    public SymbolicExecutionRunner() : this(AnalyzerConfiguration.AlwaysEnabled) { }
-
-    internal /* for testing */ SymbolicExecutionRunner(IAnalyzerConfiguration configuration) : base(configuration) { }
-
     protected override ImmutableDictionary<DiagnosticDescriptor, RuleFactory> AllRules { get; } = ImmutableDictionary<DiagnosticDescriptor, RuleFactory>.Empty
         .Add(InvalidCastToInterface.S1944, CreateFactory<EmptyRuleCheck, SonarRules.InvalidCastToInterfaceSymbolicExecution>()) // This old SE rule is part of S3655.
         .Add(HashesShouldHaveUnpredictableSalt.S2053, CreateFactory<HashesShouldHaveUnpredictableSalt, SonarRules.HashesShouldHaveUnpredictableSalt>())
@@ -51,11 +47,14 @@ public SymbolicExecutionRunner() : this(AnalyzerConfiguration.AlwaysEnabled) { }
         .Add(CalculationsShouldNotOverflow.S3949, CreateFactory<CalculationsShouldNotOverflow>())
         .Add(ObjectsShouldNotBeDisposedMoreThanOnce.S3966, CreateFactory<ObjectsShouldNotBeDisposedMoreThanOnce, SonarRules.ObjectsShouldNotBeDisposedMoreThanOnce>())
         .Add(EmptyCollectionsShouldNotBeEnumerated.S4158, CreateFactory<EmptyCollectionsShouldNotBeEnumerated, SonarRules.EmptyCollectionsShouldNotBeEnumerated>())
-        .Add(RestrictDeserializedTypes.S5773, CreateFactory<RestrictDeserializedTypes, SonarRules.RestrictDeserializedTypes>());
+        .Add(RestrictDeserializedTypes.S5773, CreateFactory<RestrictDeserializedTypes, SonarRules.RestrictDeserializedTypes>())
+        .Add(SecureRandomSeedsShouldNotBePredictable.S4347, CreateFactory<SecureRandomSeedsShouldNotBePredictable>());
 
+    protected override SyntaxClassifierBase SyntaxClassifier => CSharpSyntaxClassifier.Instance;
     public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => base.SupportedDiagnostics.ToImmutableArray();
 
-    protected override SyntaxClassifierBase SyntaxClassifier => CSharpSyntaxClassifier.Instance;
+    public SymbolicExecutionRunner() : this(AnalyzerConfiguration.AlwaysEnabled) { }
+    internal /* for testing */ SymbolicExecutionRunner(IAnalyzerConfiguration configuration) : base(configuration) { }
 
     protected override void Initialize(SonarAnalysisContext context)
     {

analyzers/src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs

@@ -0,0 +1,156 @@
+/*
+ * 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 System.Text;
+using SonarAnalyzer.Common.Walkers;
+using SonarAnalyzer.SymbolicExecution.Constraints;
+
+namespace SonarAnalyzer.SymbolicExecution.Roslyn.RuleChecks.CSharp;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class SecureRandomSeedsShouldNotBePredictable : SymbolicRuleCheck
+{
+    private const string DiagnosticId = "S4347";
+    private const string MessageFormat = "Set an unpredictable seed before generating random values.";
+    private static readonly ImmutableHashSet<string> SecureRandomNextMethods = ImmutableHashSet.Create(
+               "Next",
+               "NextInt",
+               "NextLong",
+               "NextDouble",
+               "NextBytes");
+
+    public static readonly DiagnosticDescriptor S4347 = DescriptorFactory.Create(DiagnosticId, MessageFormat);
+    protected override DiagnosticDescriptor Rule => S4347;
+
+    public override bool ShouldExecute()
+    {
+        var walker = new Walker();
+        walker.SafeVisit(Node);
+        return walker.Result;
+    }
+
+    protected override ProgramState PreProcessSimple(SymbolicContext context)
+    {
+        var state = base.PreProcessSimple(context);
+        var operation = context.Operation.Instance;
+
+        if (operation.AsArrayCreation() is { } arrayCreation)
+        {
+            return ProcessArrayCreation(state, arrayCreation);
+        }
+        else if (operation.AsInvocation() is { } invocation)
+        {
+            return ProcessSecureRandomGetInstance(state, invocation)
+                ?? ProcessStringToBytes(state, invocation)
+                ?? ProcessSecureRandomSetSeed(state, invocation)
+                ?? ProcessSecureRandomNext(state, invocation);
+        }
+        return state;
+    }
+
+    // TODO: Handle array access or simple assignment when someone messes with the byte array
+    // TODO: Handle Guids
+    // TODO: Handle char array overloads of Encoding+Convert (Optional)
+    private static ProgramState ProcessArrayCreation(ProgramState state, IArrayCreationOperationWrapper arrayCreation)
+    {
+        if (arrayCreation.Type.IsAny(KnownType.System_Byte_Array, KnownType.System_Char_Array))
+        {
+            var isConstant = arrayCreation.Initializer.WrappedOperation is null || arrayCreation.Initializer.ElementValues.All(x => x.ConstantValue.HasValue);
+            return state.SetOperationConstraint(
+                arrayCreation,
+                isConstant ? CryptographicSeedConstraint.Predictable : CryptographicSeedConstraint.Unpredictable);
+        }
+        return state;
+    }
+
+    private static ProgramState ProcessSecureRandomGetInstance(ProgramState state, IInvocationOperationWrapper invocation) =>
+        invocation.TargetMethod.Name == "GetInstance"
+        && invocation.TargetMethod.ContainingType.DerivesFrom(KnownType.Org_BouncyCastle_Security_SecureRandom)
+        && invocation.Arguments.Length == 2
+        && invocation.ArgumentValue("autoSeed") is { ConstantValue: { HasValue: true, Value: false } }
+            ? state.SetOperationConstraint(invocation, CryptographicSeedConstraint.Predictable)
+            : null;
+
+    private ProgramState ProcessStringToBytes(ProgramState state, IInvocationOperationWrapper invocation)
+    {
+        return (IsEncodingGetBytes() || IsConvertFromBase64String())
+        && invocation.ArgumentValue("s") is { ConstantValue.HasValue: true }
+            ? state.SetOperationConstraint(invocation, CryptographicSeedConstraint.Predictable)
+            : null;
+
+        bool IsEncodingGetBytes() =>
+            invocation.TargetMethod.Name == nameof(Encoding.UTF8.GetBytes)
+            && invocation.TargetMethod.ContainingType.DerivesFrom(KnownType.System_Text_Encoding);
+
+        bool IsConvertFromBase64String() =>
+            invocation.TargetMethod.Name == nameof(Convert.FromBase64String)
+            && invocation.TargetMethod.ContainingType.Is(KnownType.System_Convert);
+    }
+
+    private static ProgramState ProcessSecureRandomSetSeed(ProgramState state, IInvocationOperationWrapper invocation)
+    {
+        if (invocation.TargetMethod.Name == "SetSeed"
+        && invocation.TargetMethod.ContainingType.DerivesFrom(KnownType.Org_BouncyCastle_Security_SecureRandom)
+        && state[invocation.Instance]?.HasConstraint(CryptographicSeedConstraint.Unpredictable) is false // If it is already unpredictable, do nothing
+        && invocation.ArgumentValue("seed") is { } argumentValue)
+        {
+            var constraint = CryptographicSeedConstraint.Unpredictable;
+
+            if (argumentValue.ConstantValue.HasValue)
+            {
+                constraint = CryptographicSeedConstraint.Predictable;
+            }
+            else if (state[argumentValue]?.Constraint<CryptographicSeedConstraint>() is { } value)
+            {
+                constraint = value;
+            }
+            return state.SetSymbolConstraint(invocation.Instance.TrackedSymbol(state), constraint);
+        }
+        return null;
+    }
+
+    private ProgramState ProcessSecureRandomNext(ProgramState state, IInvocationOperationWrapper invocation)
+    {
+        if (SecureRandomNextMethods.Contains(invocation.TargetMethod.Name)
+            && invocation.TargetMethod.ContainingType.DerivesFrom(KnownType.Org_BouncyCastle_Security_SecureRandom)
+            && state[invocation.Instance]?.HasConstraint(CryptographicSeedConstraint.Predictable) is true)
+        {
+            ReportIssue(invocation.WrappedOperation);
+        }
+        return state;
+    }
+
+    private sealed class Walker : SafeCSharpSyntaxWalker
+    {
+        public bool Result { get; private set; }
+
+        public override void Visit(SyntaxNode node)
+        {
+            if (!Result)
+            {
+                base.Visit(node);
+            }
+        }
+
+        // TODO: Check if we can make this short-circuit more efficiently
+        public override void VisitInvocationExpression(InvocationExpressionSyntax node) =>
+            Result = node.NameIs("GetInstance");
+    }
+}

analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs

@@ -211,6 +211,7 @@ public sealed partial class KnownType
         public static readonly KnownType Org_BouncyCastle_Crypto_Generators_DHParametersGenerator = new("Org.BouncyCastle.Crypto.Generators.DHParametersGenerator");
         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_Security_SecureRandom = new("Org.BouncyCastle.Security.SecureRandom");
         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");
@@ -235,6 +236,8 @@ public sealed partial class KnownType
         public static readonly KnownType System_Byte = new("System.Byte");
         public static readonly KnownType System_Byte_Array = new("System.Byte") { IsArray = true };
         public static readonly KnownType System_Char = new("System.Char");
+        public static readonly KnownType System_Char_Array = new("System.Char") { IsArray = true };
+        public static readonly KnownType System_Convert = new("System.Convert");
         public static readonly KnownType System_CLSCompliantAttribute = new("System.CLSCompliantAttribute");
         public static readonly KnownType System_CodeDom_Compiler_GeneratedCodeAttribute = new("System.CodeDom.Compiler.GeneratedCodeAttribute");
         public static readonly KnownType System_Collections_CollectionBase = new("System.Collections.CollectionBase");

analyzers/src/SonarAnalyzer.Common/SymbolicExecution/Constraints/ConstraintKind.cs

@@ -32,6 +32,8 @@ public enum ConstraintKind
     CollectionNotEmpty,
     CryptographicallyStrong,
     CryptographicallyWeak,
+    CryptographicallyPredictable,
+    CryptographicallyUnpredictable,
     DisposableDisposed,
     InitializationVectorInitialized,
     InitializationVectorNotInitialized,