Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Create rule S4347: 'SecureRandom' seeds should not be predictable (Part 1) #9300

Merged
merged 1 commit into from
May 24, 2024
Merged

Create rule S4347: 'SecureRandom' seeds should not be predictable (Part 1) #9300

merged 1 commit into from
May 24, 2024

Conversation

gregory-paidis-sonarsource
Copy link
Contributor

Part of #8992

Implementing Scenario 1

@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/implement-S4347 branch 2 times, most recently from 96ddd49 to d4ef712 Compare May 17, 2024 16:42
@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/implement-S4347 branch 6 times, most recently from 3068250 to c856b6f Compare May 21, 2024 14:16
gregory-paidis-sonarsource
gregory-paidis-sonarsource commented May 21, 2024
View reviewed changes
...src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs Outdated

private static ProgramState ProcessArrayElementReference(ProgramState state, IArrayElementReferenceOperationWrapper arrayElementReference) =>
arrayElementReference.IsAssignmentTarget() || arrayElementReference.IsCompoundAssignmentTarget()
? state.SetSymbolConstraint(arrayElementReference.ArrayReference.TrackedSymbol(state), CryptographicSeedConstraint.Unpredictable)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, if the array access is on the left side of the assignment, I set it to unpredictable.
I could look into the right side of the assignment and see if it has constant value, but I am not sure if there is much value in this case, so I opted for the FN.

gregory-paidis-sonarsource
gregory-paidis-sonarsource commented May 21, 2024
View reviewed changes
...src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs
&& state[value]?.HasConstraint(CryptographicSeedConstraint.Predictable) is true;
}

private static ProgramState ProcessSecureRandomSetSeed(ProgramState state, IInvocationOperationWrapper invocation)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This processes SecureRandom.SetSeed(long/byte[])

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments you left on the PR helped a lot. Thanks for that.

What do you think about including them in the code?

@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/implement-S4347 branch 2 times, most recently from 2a3576c to ebe9b96 Compare May 21, 2024 14:40
@gregory-paidis-sonarsource gregory-paidis-sonarsource force-pushed the greg/implement-S4347 branch 2 times, most recently from 486700e to 453957b Compare May 21, 2024 16:23
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 21, 2024

Quality Gate Passed Quality Gate passed for 'Sonar .NET Java Plugin'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@sonarcloud SonarCloud
Copy link

sonarcloud bot commented May 21, 2024

Quality Gate Passed Quality Gate passed for 'SonarAnalyzer for .NET'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
95.3% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@gregory-paidis-sonarsource gregory-paidis-sonarsource marked this pull request as ready for review May 21, 2024 16:37
@gregory-paidis-sonarsource
Copy link
Contributor Author

@costin-zaharia-sonarsource if you look at the spec:

image

...it also mentions GUID handling.
I did not do this on this PR, and I am not sure if it is worth doing. We already have a rule for safe/secure GUID creation, which will make our users know the correct way to instantiate one.

We could also implement only the following:

  • Guid.Empty is BAD
  • new Guid() is BAD
  • Guid.NewGuid() is GOOD

What do you think?
In any case, I will do it in the second or third PR, because there is a lot of overlap between all the "scenarios" we need to cover, and this PR is already big.

costin-zaharia-sonarsource
costin-zaharia-sonarsource reviewed May 22, 2024
View reviewed changes
analyzers/rspec/cs/S4347.html
<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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, java.security.SecureRandom?

It was probably meant to be Org.BouncyCastle.Security.SecureRandom but you can also check with appsec squad.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see also the documentation link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent catch, thanks!
Left a comment on RSPEC PR.

@gregory-paidis-sonarsource gregory-paidis-sonarsource changed the title Create rule S4357: 'SecureRandom' seeds should not be predictable (Part 1) Create rule S4347: 'SecureRandom' seeds should not be predictable (Part 1) May 22, 2024
costin-zaharia-sonarsource
costin-zaharia-sonarsource approved these changes May 22, 2024
View reviewed changes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Please check though since I've added a few minor comments.

...src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs
? state.SetOperationConstraint(invocation, CryptographicSeedConstraint.Predictable)
: null;

private ProgramState ProcessStringToBytes(ProgramState state, IInvocationOperationWrapper invocation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can be static

...src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs
&& state[value]?.HasConstraint(CryptographicSeedConstraint.Predictable) is true;
}

private static ProgramState ProcessSecureRandomSetSeed(ProgramState state, IInvocationOperationWrapper invocation)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments you left on the PR helped a lot. Thanks for that.

What do you think about including them in the code?

...src/SonarAnalyzer.CSharp/SymbolicExecution/Roslyn/SecureRandomSeedsShouldNotBePredictable.cs
private static ProgramState ProcessSecureRandomSetSeed(ProgramState state, IInvocationOperationWrapper invocation)
{
if (invocation.TargetMethod.Name == "SetSeed"
&& invocation.TargetMethod.ContainingType.DerivesFrom(KnownType.Org_BouncyCastle_Security_SecureRandom)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the indentation is off here.

analyzers/tests/SonarAnalyzer.Test/Rules/SecureRandomSeedsShouldNotBePredictableTest.cs Show resolved Hide resolved
analyzers/tests/SonarAnalyzer.Test/TestCases/SecureRandomSeedsShouldNotBePredictable.cs
@@ -0,0 +1,262 @@
using System;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is very well tested, maybe too well since the symbolic execution engine has it's own tests.

I did not see any tests with inheritance. Would it be worth adding some? SecureRandom is not sealed, although most of the methods we check are static so I'm not sure if it's actually useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will add a test on part 3, as in part1+2 I do not call new on SecureRandom, only GetInstance, which is a static method that always returns SecureRandom.

@gregory-paidis-sonarsource
Copy link
Contributor Author

Since most of the suggestions are not breaking, I will merge this and apply the suggestions on the part#2 PR.

@gregory-paidis-sonarsource gregory-paidis-sonarsource merged commit 896cf49 into master May 24, 2024
42 checks passed
@gregory-paidis-sonarsource gregory-paidis-sonarsource deleted the greg/implement-S4347 branch May 24, 2024 08:35
@zsolt-kolbay-sonarsource zsolt-kolbay-sonarsource added this to the 9.26 milestone May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@costin-zaharia-sonarsource costin-zaharia-sonarsource costin-zaharia-sonarsource approved these changes

Assignees

@gregory-paidis-sonarsource gregory-paidis-sonarsource

Labels
None yet
Projects
None yet
Milestone
9.26
Development

Successfully merging this pull request may close these issues.
3 participants
@gregory-paidis-sonarsource @costin-zaharia-sonarsource @zsolt-kolbay-sonarsource