Skip to content

Commit

Permalink
Create rule S4347: 'SecureRandom' seeds should not be predictable (pa…
Browse files Browse the repository at this point in the history 
…rt 2)
  • Loading branch information
@gregory-paidis-sonarsource
gregory-paidis-sonarsource committed May 22, 2024
1 parent 453957b commit aa58fd3
Show file tree
Hide file tree
Showing 4 changed files with 171 additions and 35 deletions.
75 changes: 58 additions & 17 deletions .../SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,18 +52,28 @@ protected override ProgramState PreProcessSimple(SymbolicContext context)
{
return ProcessArrayElementReference(state, arrayElementReference);
}
else if (operation.AsObjectCreation() is { } objectCreation)
{
return ProcessObjectCreation(state, objectCreation);
}
else if (operation.AsInvocation() is { } invocation)
{
return ProcessArraySetValue(state, invocation)
?? ProcessArrayInitialize(state, invocation)
?? ProcessStringToBytes(state, invocation)
?? ProcessSecureRandomGetInstance(state, invocation)
?? ProcessSecureRandomSetSeed(state, invocation)
?? ProcessSecureRandomNext(state, invocation)
?? ProcessSeedingMethods(state, invocation)
?? ProcessNextMethods(state, invocation)
?? state;
}
return state;
}

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;

private static ProgramState ProcessArrayCreation(ProgramState state, IArrayCreationOperationWrapper arrayCreation)
{
if (arrayCreation.Type.IsAny(KnownType.System_Byte_Array, KnownType.System_Char_Array))
Expand All @@ -85,18 +95,26 @@ private static ProgramState ProcessArrayElementReference(ProgramState state, IAr
private static ProgramState ProcessArraySetValue(ProgramState state, IInvocationOperationWrapper invocation)
{
if (invocation.TargetMethod.Name == nameof(Array.SetValue)
&& invocation.TargetMethod.ContainingType.Is(KnownType.System_Array))
&& invocation.TargetMethod.ContainingType.Is(KnownType.System_Array)
&& invocation.Instance.TrackedSymbol(state) is { } array)
{
return invocation.ArgumentValue("value") is { ConstantValue.HasValue: true }
? state
: state.SetSymbolConstraint(invocation.Instance.TrackedSymbol(state), CryptographicSeedConstraint.Unpredictable);
: state.SetSymbolConstraint(array, CryptographicSeedConstraint.Unpredictable);
}
return null;
}

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;

private static ProgramState ProcessSecureRandomGetInstance(ProgramState state, IInvocationOperationWrapper invocation) =>
invocation.TargetMethod.Name == "GetInstance"
&& invocation.TargetMethod.ContainingType.DerivesFrom(KnownType.Org_BouncyCastle_Security_SecureRandom)
&& IsSecureRandom(invocation)
&& invocation.Arguments.Length == 2
&& invocation.ArgumentValue("autoSeed") is { ConstantValue: { HasValue: true, Value: false } }
? state.SetOperationConstraint(invocation, CryptographicSeedConstraint.Predictable)
Expand Down Expand Up @@ -128,13 +146,15 @@ bool ArgumentIsPredictable(string parameterName) =>
&& state[value]?.HasConstraint(CryptographicSeedConstraint.Predictable) is true;
}

private static ProgramState ProcessSecureRandomSetSeed(ProgramState state, IInvocationOperationWrapper invocation)
private static ProgramState ProcessSeedingMethods(ProgramState state, IInvocationOperationWrapper invocation)
{
if (invocation.TargetMethod.Name == "SetSeed"
&& invocation.TargetMethod.ContainingType.DerivesFrom(KnownType.Org_BouncyCastle_Security_SecureRandom)
// If it is already unpredictable, do nothing. SecureRandom.SetSeed() does not overwrite the state, but _mixes_ it with the new value.
&& state[invocation.Instance]?.HasConstraint(CryptographicSeedConstraint.Unpredictable) is false
&& invocation.ArgumentValue("seed") is { } argumentValue)
if ((IsSetSeed() || IsAddSeedMaterial())
&& invocation.Instance is { } instance
// If it is already unpredictable, do nothing.
// Seeding methods do not overwrite the state, but _mix_ it with the new value.
&& state[instance]?.HasConstraint(CryptographicSeedConstraint.Unpredictable) is false
&& invocation.ArgumentValue("seed") is { } argumentValue
&& invocation.Instance.TrackedSymbol(state) is { } instanceSymbol)
{
var constraint = CryptographicSeedConstraint.Unpredictable;
if (argumentValue.ConstantValue.HasValue)
Expand All @@ -145,19 +165,40 @@ private static ProgramState ProcessSecureRandomSetSeed(ProgramState state, IInvo
{
constraint = value;
}
return state.SetSymbolConstraint(invocation.Instance.TrackedSymbol(state), constraint);
return state.SetSymbolConstraint(instanceSymbol, constraint);
}
return null;

bool IsSetSeed() =>
invocation.TargetMethod.Name == "SetSeed"
&& IsSecureRandom(invocation);

bool IsAddSeedMaterial() =>
invocation.TargetMethod.Name == "AddSeedMaterial"
&& invocation.Instance is { } instance
&& instance.Type.Is(KnownType.Org_BouncyCastle_Crypto_Prng_IRandomGenerator);
}

private ProgramState ProcessSecureRandomNext(ProgramState state, IInvocationOperationWrapper invocation)
private ProgramState ProcessNextMethods(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)
if ((IsSecureRandomMethod() || IsRandomGeneratorMethod())
&& invocation.Instance is { } instance
&& state[instance]?.HasConstraint(CryptographicSeedConstraint.Predictable) is true)
{
ReportIssue(invocation.WrappedOperation);
}
return state;
return null;

bool IsSecureRandomMethod() =>
SecureRandomNextMethods.Contains(invocation.TargetMethod.Name)
&& IsSecureRandom(invocation);

bool IsRandomGeneratorMethod() =>
invocation.TargetMethod.Name == "NextBytes"
&& invocation.Instance is { } instance
&& instance.Type.Is(KnownType.Org_BouncyCastle_Crypto_Prng_IRandomGenerator);
}

private static bool IsSecureRandom(IInvocationOperationWrapper invocation) =>
invocation.TargetMethod.ContainingType.Is(KnownType.Org_BouncyCastle_Security_SecureRandom);
}
3 changes: 3 additions & 0 deletions analyzers/src/SonarAnalyzer.Common/Helpers/KnownType.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,9 @@ public sealed partial class KnownType
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 Org_BouncyCastle_Crypto_Prng_DigestRandomGenerator = new("Org.BouncyCastle.Crypto.Prng.DigestRandomGenerator");
public static readonly KnownType Org_BouncyCastle_Crypto_Prng_IRandomGenerator = new("Org.BouncyCastle.Crypto.Prng.IRandomGenerator");
public static readonly KnownType Org_BouncyCastle_Crypto_Prng_VmpcRandomGenerator = new("Org.BouncyCastle.Crypto.Prng.VmpcRandomGenerator");
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");
Expand Down
119 changes: 101 additions & 18 deletions analyzers/tests/SonarAnalyzer.Test/Rules/SecureRandomSeedsShouldNotBePredictableTest.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,26 +129,109 @@ void StartWithPredictable()
[DataRow("bs[42] *= b")]
[DataTestMethod]
public void SecureRandomSeedsShouldNotBePredictable_CS_ArrayEdited(string messWithArray) =>
builder
.AddSnippet($$$"""
using System;
using System.Text;
using Org.BouncyCastle.Security;
class Testcases
{
void Method(byte b)
builder
.AddSnippet($$$"""
using System;
using System.Text;
using Org.BouncyCastle.Security;
class Testcases
{
var bs = new byte[42];
void Method(byte b)
{
var bs = new byte[42];

var sr = SecureRandom.GetInstance("SHA256PRNG", false);
sr.SetSeed(bs);
sr.Next(); // Noncompliant
var sr = SecureRandom.GetInstance("SHA256PRNG", false);
sr.SetSeed(bs);
sr.Next(); // Noncompliant

{{{messWithArray}}};
sr.SetSeed(bs);
sr.Next(); // Compliant
{{{messWithArray}}};
sr.SetSeed(bs);
sr.Next(); // Compliant
}
}
}
""")
.Verify();
""")
.Verify();

[DataRow("DigestRandomGenerator(digest)")]
[DataRow("VmpcRandomGenerator()")]
[DataTestMethod]
public void SecureRandomSeedsShouldNotBePredictable_CS_IRandomGenerator(string generator) =>
builder
.AddSnippet($$$"""
using System.Text;
using Org.BouncyCastle.Security;
using Org.BouncyCastle.Crypto.Prng;
using Org.BouncyCastle.Crypto.Digests;

class Testcases
{
const int SEED = 42;

void Method(byte[] bs, Sha256Digest digest, long seed)
{
IRandomGenerator rng = new {{{generator}}};
rng.NextBytes(bs); // Noncompliant {{Set an unpredictable seed before generating random values.}}
// ^^^^^^^^^^^^^^^^^

rng.AddSeedMaterial(SEED);
rng.NextBytes(bs, 0, 42); // Noncompliant
// ^^^^^^^^^^^^^^^^^^^^^^^^

rng.AddSeedMaterial(new byte[3]);
rng.NextBytes(bs); // Noncompliant

rng.AddSeedMaterial(Encoding.UTF8.GetBytes("exploding whale"));
rng.NextBytes(bs); // Noncompliant

rng.AddSeedMaterial(42);
rng.NextBytes(bs); // Noncompliant

rng.AddSeedMaterial(bs);
rng.NextBytes(bs); // Compliant, bs is unknown

rng.AddSeedMaterial(42);
rng.NextBytes(bs); // Compliant, seed is already unpredictable

rng = new {{{generator}}};
rng.NextBytes(bs); // Noncompliant

rng.AddSeedMaterial(seed);
rng.NextBytes(bs); // Compliant
}
}
""")
.Verify();

#if NET
[TestMethod]
public void SecureRandomSeedsShouldNotBePredictable_CS_IRandomGenerator_CustomImplementation() =>
builder
.AddSnippet($$$"""
using System;
using Org.BouncyCastle.Security;
using Org.BouncyCastle.Crypto.Prng;
using Org.BouncyCastle.Crypto.Digests;

class Testcases
{
void IRandomGenerator(byte[] bs)
{
var notRelevant = new MyGen();
notRelevant.NextBytes(bs); // Compliant, we only track "Digest" and "Vmpc".
}
}

class MyGen : IRandomGenerator
{
public void AddSeedMaterial(byte[] seed) { }
public void AddSeedMaterial(ReadOnlySpan<byte> seed) { }
public void AddSeedMaterial(long seed) { }
public void NextBytes(byte[] bytes) { }
public void NextBytes(byte[] bytes, int start, int len) { }
public void NextBytes(Span<byte> bytes) { }
}

""")
.VerifyNoIssues();
#endif
}
9 changes: 9 additions & 0 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/SecureRandomSeedsShouldNotBePredictable.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using System.Linq;
using System.Text;
using Org.BouncyCastle.Security;
using Org.BouncyCastle.Crypto.Prng;

class Testcases
{
Expand Down Expand Up @@ -254,6 +255,14 @@ void SecureRandom_Arrays(byte b, char[] whoKnows)
sr.SetSeed(Convert.FromBase64CharArray(whoKnows, 0, 42));
sr.Next(); // Compliant

sr = SecureRandom.GetInstance("SHA256PRNG", false);
sr.Next(); // Noncompliant
var ss = new byte[3];
ss[0] = b;
ss.Initialize();
sr.SetSeed(ss);
sr.Next(); // Noncompliant, Initialize is predictable

sr = SecureRandom.GetInstance("SHA256PRNG", false);
sr.Next(); // Noncompliant
sr.SetSeed(xs.Cast<byte>().ToArray());
Expand Down

0 comments on commit aa58fd3

Please sign in to comment.