Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Provide Random via injection and use SecureRandom for padding (DEV) #3676

Merged
merged 9 commits into from
Jul 14, 2021

Conversation

d4rken
Copy link
Member

@d4rken d4rken commented Jul 9, 2021

Fixes internal sonar check.

  • Replace use of Random in PaddingTool
  • Provide kotlin.Random via injection to abstract away picking the right source (api level dependent)
  • Provide two random sources
    • @RandomStrong for key generation
    • @RandomFast for padding (as RandomStrong can starve and block for entropy)

@d4rken d4rken added the maintainers Tag pull requests created by maintainers label Jul 9, 2021
@d4rken d4rken added this to the 2.6.0 milestone Jul 9, 2021
@d4rken d4rken requested a review from a team July 9, 2021 10:33
Copy link
Contributor

@mtwalli mtwalli left a comment

Choose a reason for hiding this comment

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

Lgtm

@mtwalli mtwalli self-assigned this Jul 12, 2021
Copy link
Contributor

@chiljamgossow chiljamgossow left a comment

Choose a reason for hiding this comment

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

I don't know what this is doing, but code looks fine

@chiljamgossow chiljamgossow self-assigned this Jul 13, 2021
@d4rken
Copy link
Member Author

d4rken commented Jul 13, 2021

I don't know what this is doing, but code looks fine

The internal build pipeline raised a security warning due to our use of the default Random() which has some flaws that make it unsuitable for use with security related operations.

A suitable alternative is SecureRandom which we already inject into other classes we have.
There are different ways to instantiate SecureRandom though which offer different performance.
We already used SecureRandom.getInstanceStrong() which is nice for keys (e.g. trace locations) and anything else that will "exist" for longer durations and benefits from stronger crypthographic properties.

But getInstanceStrong and just SecureRandom() (depending on the ROM), will return an algorithm/implementation that can "starve on entropy", meaning that a call to get a new number can block/hang until enough "random stuff" has happened (time passed, user input etc.) such that SecureRandom can provide a good enough new random number.

So for the padding calculation i added a second SecureRandom with a different algorithm that is still better than Random(), worse than SecureRandom.getStrongInstance(), but can provide more random numbers, faster and without hanging.

This may be more rare on an actual device, but happens very quickly in unit-tests (especially on CIs) where the system is artificial, e.g. a VM with no good randomness source.

For testing you could run the Padding Test but inject the "Strong" instance, it has a high chance of just hanging.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

76.7% 76.7% Coverage
0.0% 0.0% Duplication

@harambasicluka harambasicluka merged commit 36508fe into release/2.6.x Jul 14, 2021
@harambasicluka harambasicluka deleted the dev/fix-random-source branch July 14, 2021 07:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
maintainers Tag pull requests created by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants