-
Notifications
You must be signed in to change notification settings - Fork 226
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create rule S4347: 'SecureRandom' seeds should not be predictable (part 2) #9315
Create rule S4347: 'SecureRandom' seeds should not be predictable (part 2) #9315
Conversation
1629ec9
to
7a2409f
Compare
sr.Next(); // Noncompliant | ||
var ss = new byte[3]; | ||
ss[0] = b; | ||
ss.Initialize(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think I should add more complex cases for this scenario?
There are three steps here:
- Call
new
on 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
aa58fd3
to
e67e5ae
Compare
""") | ||
.Verify(); | ||
|
||
#if NET |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to track SecureRandom.NextXXX
, now it ALSO tracks IRandomGenerator.NextBytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can extract this to a method "IsIRandomGenerator(invocation)" and reuse the method also in IsSecureRandomMethod
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hide whitespace
on github UI to protect your eyes here 😎
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
} | ||
|
||
private ProgramState ProcessSecureRandomNext(ProgramState state, IInvocationOperationWrapper invocation) | ||
private ProgramState ProcessNextMethods(ProgramState state, IInvocationOperationWrapper invocation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense. The issue appears when using the concrete types; it's not caused by the contract.
e67e5ae
to
ae04f3a
Compare
ae04f3a
to
608dd1c
Compare
Quality Gate passed for 'Sonar .NET Java Plugin'Issues Measures |
Quality Gate passed for 'SonarAnalyzer for .NET'Issues Measures |
Part of #8992
Implementing Scenario 2
Also, added support for
Array.Initialize
, which re-sets an array to predictable.