Create rule S4347: 'SecureRandom' seeds should not be predictable (part 2) #9315
Conversation
gregory-paidis-sonarsource
force-pushed
the
greg/implement-S4347-part-2
branch
from
1629ec9
to
7a2409f
Compare
| sr.Next(); // Noncompliant | ||
| var ss = new byte[3]; | ||
| ss[0] = b; | ||
| ss.Initialize(); |
There was a problem hiding this comment.
This sets each element of the array to the default value, so I consider it predictable.
| [DataRow("VmpcRandomGenerator()")] | ||
| [DataTestMethod] | ||
| public void SecureRandomSeedsShouldNotBePredictable_CS_IRandomGenerator(string generator) => | ||
| builder |
There was a problem hiding this comment.
Do you think I should add more complex cases for this scenario?
There are three steps here:
- Call
newon a specific random generator - Call (or not)
AddSeedMaterial - Call
NextBytes
AddSeedMaterial and NextByts are very similar to the SecureRandom implementation, SetSeed and Next respectively, and they are actually implemented inside the same methods.
I am not sure how much value there is to add complex cases for scenario 2 and 3.
| } | ||
|
|
||
| private static bool IsSecureRandom(IInvocationOperationWrapper invocation) => | ||
| invocation.TargetMethod.ContainingType.Is(KnownType.Org_BouncyCastle_Security_SecureRandom); |
There was a problem hiding this comment.
This used to be DerivesFrom, but I changed it to Is because SetSeed and ALL the NextXXX methods are virtual, so the rule might not apply in a user's specific implementation.
There was a problem hiding this comment.
It makes sense. The issue appears when using the concrete types; it's not caused by the contract.
| bool IsRandomGeneratorMethod() => | ||
| invocation.TargetMethod.Name == "NextBytes" | ||
| && invocation.Instance is { } instance | ||
| && instance.Type.Is(KnownType.Org_BouncyCastle_Crypto_Prng_IRandomGenerator); |
There was a problem hiding this comment.
If you have something like this:
interface Foo { Work(); }
class Bar: Foo { Work() {} }...and you call someBar.Work(), the instance.Type is always the interface, as this is where the method was originally defined.
This is fine, because we only "taint" as predictable the two classes we care about, Vmpc and Digest random generators. See ProcessObjectCreation.
So even if we have a custom implementation of IRandomGenerator, it will never be considered "predictable" when we reach this point.
...src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs
Show resolved
Hide resolved
gregory-paidis-sonarsource
force-pushed
the
greg/implement-S4347-part-2
branch
2 times, most recently
from
aa58fd3
to
e67e5ae
Compare
| """) | ||
| .Verify(); | ||
|
|
||
| #if NET |
There was a problem hiding this comment.
There is no Span and ReadonlySpan in .NET Framework.
| { | ||
| return invocation.ArgumentValue("value") is { ConstantValue.HasValue: true } | ||
| ? state | ||
| : state.SetSymbolConstraint(invocation.Instance.TrackedSymbol(state), CryptographicSeedConstraint.Unpredictable); | ||
| : state.SetSymbolConstraint(array, CryptographicSeedConstraint.Unpredictable); |
There was a problem hiding this comment.
This could result in NRE, since invocation might not have an instance, for example when called inside the class it is defined, so you don't even need a this necessarily.
You will notice this change a lot around this file, where I check the TrackedSymbol for nullity and then I use it in SetSymbolConstraint.
TrackedSymbol is safe to be called on a null Instance, but SetSymbolicConstraint is not.
| } | ||
| return null; | ||
| } | ||
|
|
||
| private static ProgramState ProcessArrayInitialize(ProgramState state, IInvocationOperationWrapper invocation) => |
There was a problem hiding this comment.
Tracks myArray.Initialize()
| @@ -128,13 +146,15 @@ private ProgramState ProcessStringToBytes(ProgramState state, IInvocationOperati | |||
| && state[value]?.HasConstraint(CryptographicSeedConstraint.Predictable) is true; | |||
| } | |||
|
|
|||
| private static ProgramState ProcessSecureRandomSetSeed(ProgramState state, IInvocationOperationWrapper invocation) | |||
| private static ProgramState ProcessSeedingMethods(ProgramState state, IInvocationOperationWrapper invocation) | |||
There was a problem hiding this comment.
This used to track SecureRandom.SetSeed, now it ALSO tracks IRandomGenerator.AddSeedMaterial
| } | ||
|
|
||
| private ProgramState ProcessSecureRandomNext(ProgramState state, IInvocationOperationWrapper invocation) | ||
| private ProgramState ProcessNextMethods(ProgramState state, IInvocationOperationWrapper invocation) |
There was a problem hiding this comment.
This used to track SecureRandom.NextXXX, now it ALSO tracks IRandomGenerator.NextBytes
There was a problem hiding this comment.
You can extract this to a method "IsIRandomGenerator(invocation)" and reuse the method also in IsSecureRandomMethod.
There was a problem hiding this comment.
Or, if the method identification is needed in other rules we can move the common ones to KnownMethods.
If needed, I might move some while working on the other rule.
| @@ -129,26 +129,109 @@ void StartWithPredictable() | |||
| [DataRow("bs[42] *= b")] | |||
| [DataTestMethod] | |||
| public void SecureRandomSeedsShouldNotBePredictable_CS_ArrayEdited(string messWithArray) => | |||
| builder | |||
There was a problem hiding this comment.
Hide whitespace on github UI to protect your eyes here 😎
github-actions
bot
assigned costin-zaharia-sonarsource and unassigned gregory-paidis-sonarsource
| } | ||
|
|
||
| private ProgramState ProcessSecureRandomNext(ProgramState state, IInvocationOperationWrapper invocation) | ||
| private ProgramState ProcessNextMethods(ProgramState state, IInvocationOperationWrapper invocation) |
There was a problem hiding this comment.
You can extract this to a method "IsIRandomGenerator(invocation)" and reuse the method also in IsSecureRandomMethod.
| } | ||
|
|
||
| private ProgramState ProcessSecureRandomNext(ProgramState state, IInvocationOperationWrapper invocation) | ||
| private ProgramState ProcessNextMethods(ProgramState state, IInvocationOperationWrapper invocation) |
There was a problem hiding this comment.
Or, if the method identification is needed in other rules we can move the common ones to KnownMethods.
If needed, I might move some while working on the other rule.
| } | ||
|
|
||
| private static bool IsSecureRandom(IInvocationOperationWrapper invocation) => | ||
| invocation.TargetMethod.ContainingType.Is(KnownType.Org_BouncyCastle_Security_SecureRandom); |
There was a problem hiding this comment.
It makes sense. The issue appears when using the concrete types; it's not caused by the contract.
github-actions
bot
assigned gregory-paidis-sonarsource and unassigned costin-zaharia-sonarsource
github-actions
bot
moved this from Review in progress
to Review approved
in Best Kanban
gregory-paidis-sonarsource
force-pushed
the
greg/implement-S4347-part-2
branch
from
e67e5ae
to
ae04f3a
Compare
gregory-paidis-sonarsource
force-pushed
the
greg/implement-S4347-part-2
branch
from
ae04f3a
to
608dd1c
Compare
|
|
Part of #8992
Implementing Scenario 2
Also, added support for
Array.Initialize, which re-sets an array to predictable.