Review from part 1 + Review 1

SonarSource · May 24, 2024 · ae04f3a · ae04f3a
1 parent eff860e
commit ae04f3a
Show file tree

Hide file tree

Showing 2 changed files with 23 additions and 6 deletions.
diff --git a/.../SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs b/.../SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs
@@ -69,11 +69,15 @@ protected override ProgramState PreProcessSimple(SymbolicContext context)
         return state;
     }
 
+    // new VmpcRandomGenerator()
+    // new DigestRandomGenerator(digest)
     private static ProgramState ProcessObjectCreation(ProgramState state, IObjectCreationOperationWrapper objectCreation) =>
         objectCreation.Type.IsAny(KnownType.Org_BouncyCastle_Crypto_Prng_DigestRandomGenerator, KnownType.Org_BouncyCastle_Crypto_Prng_VmpcRandomGenerator)
             ? state.SetOperationConstraint(objectCreation, CryptographicSeedConstraint.Predictable)
             : state;
 
+    // new byte/char[] { ... }
+    // new byte/char[42]
     private static ProgramState ProcessArrayCreation(ProgramState state, IArrayCreationOperationWrapper arrayCreation)
     {
         if (arrayCreation.Type.IsAny(KnownType.System_Byte_Array, KnownType.System_Char_Array))
@@ -86,12 +90,14 @@ private static ProgramState ProcessArrayCreation(ProgramState state, IArrayCreat
         return state;
     }
 
+    // array[42] = ...
     private static ProgramState ProcessArrayElementReference(ProgramState state, IArrayElementReferenceOperationWrapper arrayElementReference) =>
         (arrayElementReference.IsAssignmentTarget() || arrayElementReference.IsCompoundAssignmentTarget())
         && arrayElementReference.ArrayReference.TrackedSymbol(state) is { } array
             ? state.SetSymbolConstraint(array, CryptographicSeedConstraint.Unpredictable)
             : state;
 
+    // array.SetValue(value, index)
     private static ProgramState ProcessArraySetValue(ProgramState state, IInvocationOperationWrapper invocation)
     {
         if (invocation.TargetMethod.Name == nameof(Array.SetValue)
@@ -105,13 +111,15 @@ private static ProgramState ProcessArraySetValue(ProgramState state, IInvocation
         return null;
     }
 
+    // array.Initialize()
     private static ProgramState ProcessArrayInitialize(ProgramState state, IInvocationOperationWrapper invocation) =>
         invocation.TargetMethod.Name == nameof(Array.Initialize)
         && invocation.TargetMethod.ContainingType.Is(KnownType.System_Array)
         && invocation.Instance.TrackedSymbol(state) is { } array
             ? state.SetSymbolConstraint(array, CryptographicSeedConstraint.Predictable)
             : null;
 
+    // SecureRandom.GetInstance("algorithm", false)
     private static ProgramState ProcessSecureRandomGetInstance(ProgramState state, IInvocationOperationWrapper invocation) =>
         invocation.TargetMethod.Name == "GetInstance"
         && IsSecureRandom(invocation)
@@ -120,7 +128,10 @@ private static ProgramState ProcessSecureRandomGetInstance(ProgramState state, I
             ? state.SetOperationConstraint(invocation, CryptographicSeedConstraint.Predictable)
             : null;
 
-    private ProgramState ProcessStringToBytes(ProgramState state, IInvocationOperationWrapper invocation)
+    // Encoding.UTF8.GetBytes(s)
+    // Convert.FromBase64CharArray(chars, ...)
+    // Convert.FromBase64String(s)
+    private static ProgramState ProcessStringToBytes(ProgramState state, IInvocationOperationWrapper invocation)
     {
         return (IsEncodingGetBytes() || IsConvertFromBase64String() || IsConvertFromBase64CharArray())
             ? state.SetOperationConstraint(invocation, CryptographicSeedConstraint.Predictable)
@@ -146,6 +157,8 @@ bool ArgumentIsPredictable(string parameterName) =>
             && state[value]?.HasConstraint(CryptographicSeedConstraint.Predictable) is true;
     }
 
+    // secureRandom.SetSeed(bytes/number)
+    // randomGenerator.AddSeedMaterial(bytes/number)
     private static ProgramState ProcessSeedingMethods(ProgramState state, IInvocationOperationWrapper invocation)
     {
         if ((IsSetSeed() || IsAddSeedMaterial())
@@ -175,10 +188,11 @@ bool IsSetSeed() =>
 
         bool IsAddSeedMaterial() =>
             invocation.TargetMethod.Name == "AddSeedMaterial"
-            && invocation.Instance is { } instance
-            && instance.Type.Is(KnownType.Org_BouncyCastle_Crypto_Prng_IRandomGenerator);
+            && IsRandomGenerator(invocation);
     }
 
+    // secureRandom.NextXXX()
+    // randomGenerator.NextBytes()
     private ProgramState ProcessNextMethods(ProgramState state, IInvocationOperationWrapper invocation)
     {
         if ((IsSecureRandomMethod() || IsRandomGeneratorMethod())
@@ -195,10 +209,13 @@ bool IsSecureRandomMethod() =>
 
         bool IsRandomGeneratorMethod() =>
             invocation.TargetMethod.Name == "NextBytes"
-            && invocation.Instance is { } instance
-            && instance.Type.Is(KnownType.Org_BouncyCastle_Crypto_Prng_IRandomGenerator);
+            && IsRandomGenerator(invocation);
     }
 
     private static bool IsSecureRandom(IInvocationOperationWrapper invocation) =>
         invocation.TargetMethod.ContainingType.Is(KnownType.Org_BouncyCastle_Security_SecureRandom);
+
+    private static bool IsRandomGenerator(IInvocationOperationWrapper invocation) =>
+        invocation.Instance is { } instance
+        && instance.Type.Is(KnownType.Org_BouncyCastle_Crypto_Prng_IRandomGenerator);
 }
diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/SecureRandomSeedsShouldNotBePredictableTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/SecureRandomSeedsShouldNotBePredictableTest.cs
@@ -204,7 +204,7 @@ void Method(byte[] bs, Sha256Digest digest, long seed)
 
 #if NET
     [TestMethod]
-    public void SecureRandomSeedsShouldNotBePredictable_CS_IRandomGenerator_CustomImplementation() =>
+    public void SecureRandomSeedsShouldNotBePredictable_CS_IRandomGenerator_Inheritance() =>
         builder
             .AddSnippet($$$"""
                 using System;