Found the following code in our code base:
public static final int DEFAULT_LENGTH = 16;
private static SecureRandom SR;
static
{
try
{
SecureRandom sd0 = new SecureRandom();
SR = new SecureRandom(sd0.generateSeed(DEFAULT_LENGTH * 2));
}
catch (Exception e){}
}
Here a default SecureRandom is created, and then that is used to create a seed for another one which is the one that will be used later in the class. Is this really necessary? Is the second somehow better than the first because this is done?
When the seed is generated for the second, the number of bytes is given, is this important? Could a SecureRandom seeded with a different amount of bytes than another potentially be better or worse? Should the number of bytes used to seed it somehow correspond to what it will be used for?
If setSeed is not called, the first call to nextBytes will force the SecureRandom object to seed itself. This self-seeding will not occur if setSeed was previously called. - javadoc
Is the self-seeding not good enough? Does it depend on what it's going to be used for?
Note: For some context, it is used in class that creates random ids for stuff stored in a database.
I think this is completely unneccessary, because as the Javadoc you quote clearly states: Default-constructed SecureRandom instances seed themselves. The person who wrote this probably didn't know that.
They might also actually decrease security by forcing a fixed seed length that could be less-than-ideal for the RNG implementation.
Finally, assuming the snippet is posted unaltered, the silent exception swallowing isn't very good coding style either.
Avoid using the default algorithm which is the case when doing new SecureRandom();
Instead do:
SecureRandom.getInstance("SHA1PRNG", "SUN");
If someone changes the default algorithm (as stated by @Jules) you won't be impacted.
Edited for Android:
For android, take a look at :
- https://android-developers.googleblog.com/2016/06/security-crypto-provider-deprecated-in.html
- http://www.infosecisland.com/blogview/24773-Android-N-Deprecating-Crypto-Provider-and-SHA1PRNG-Algorithm.html
- https://security.stackexchange.com/questions/128144/android-n-security-crypto-provider-is-deprecated
- Security "Crypto" provider deprecated in Android N
On Android, we don’t recommend specifying the provider. In general,
any call to the Java Cryptography Extension (JCE) APIs specifying a
provider should only be done if the provider is included in the
application or if the application is able to deal with a possible
ProviderNotFoundException.
...
in Android N we are deprecating the implementation of the SHA1PRNG
algorithm and the Crypto provider altogether
This is not only completely unnecessary, it may actually increase the predictability of the numbers generated by the SecureRandom object.
A SecureRandom that does not have an explicit seed set will self-seed. It uses a highly random data source to perform this operation, and is quite secure. The first SecureRandom in your code sample will use such a seed.
The second is seeded from the first by producing 256 random bits. Assuming the system-default SHA1PRNG is used, this is good enough. It uses 160 bits of state, so 256 random bits will completely satisfy it's requirements. But assume now somebody decides this isn't enough, and switches the default to a SHA512PRNG (they can do this without even looking at your code by changing java's security properties). Now you're providing too few seed bits to it: only half as many as it needs.
Get rid of it, and just use the self-seeded object, unless you have a better source of seed data than the system has available.
Just an addendum to this answer. According to Google, if you are using this code in android you should definitely seed the SecureRandom using a high entropy source like /dev/urandom or /dev/random.
Even the post below is an year old now, perhaps this has already been corrected but I couldn't confirm if it was.
https://plus.google.com/+AndroidDevelopers/posts/YxWzeNQMJS2
EDIT:
It seems that the default behavior of the class is now the one specified in the post so seeding is again deemed unnecessary:
http://developer.android.com/reference/java/security/SecureRandom.html
