32

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.

Svish
  • 152,914
  • 173
  • 462
  • 620

5 Answers5

28

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.

ddso
  • 1,731
  • 12
  • 15
  • 1
    `SecureRandom` only appends custom seeds, it never replaces. – Serdalis Sep 30 '11 at 09:35
  • Yeah, I was wondering about that swallowing is well. Can those lines even throw an exception? Perhaps if there are no security providers at all? But can that even happen? – Svish Sep 30 '11 at 09:52
  • @Serdalis What do you mean by that? Does it always generate its own seed, and then when you send in a seed in the constructor or the `setSeed` method, it is only appended to the one it already generated itself? – Svish Sep 30 '11 at 09:53
  • 1
    Yes It will never replace its seed, you can only augment it with what you enter. – Serdalis Sep 30 '11 at 10:51
  • 5
    Actually that's not entirely true. It will never replace seed once it has been set, but the self-seeding happens only on the first call to generate a random number if no explicit seed has been set (this is necessary, otherwise you couldn't set up a SecureRandom to produce a specific set of output numbers). – Jules Mar 03 '12 at 12:37
6

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 :

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

Community
  • 1
  • 1
alexbt
  • 16,415
  • 6
  • 78
  • 87
  • Thanks Nate, that's more accurate indeed! "SHAPRNG" =­> "SHA1PRNG" – alexbt Aug 05 '12 at 03:04
  • 1
    But that is what Jules warned against! If someone changes the default algorithm to more secure, the Java application with such hardcoded algorithm will not adapt. – krzychu Nov 09 '16 at 10:12
  • 1
    True! But at the time, this was my first or second answer ever, so I didn't have enough reputation to add a comment. Also, no other answer provided the exact line of code to "not use the default algorithm", hence my answer ;) – alexbt Nov 09 '16 at 11:32
  • 1
    The "SHA1PRNG"should _not_ be used anymore! See here for details: https://android-developers.googleblog.com/2016/06/security-crypto-provider-deprecated-in.html – phw Mar 15 '17 at 11:20
  • @phw Thanks for the link. My answer is about *not using the default implementation*, not about the specific "provider". I used SUN as an example. Your link specifically talks about deprecating the SHA1PRNG with the crypto provider: `getInstance("SHA1PRNG", "crypto")` – alexbt Mar 15 '17 at 16:52
  • 2
    Actually, it is recommended to use the default implementation, so this answer is contrary to advice. And I don't understand the justification for it. You want the default algorithm to continually improve for better security in the future. – President James K. Polk Apr 29 '18 at 14:07
  • @JamesKPolk I won't defend my answer to the death, I'm not a security expert. However, when I wrote this answer my code was being "reviewed" by security experts and this was one of their comments. The reason (if I recall) was because the default implementation can be overriden by attacker. I tried to find a source that would explain better than me: https://lists.owasp.org/pipermail/esapi-dev/2014-July/002425.html => "you could pull in a rogue 3rd party library that [...] dynamically insert their own provider as the very first so that it would be selected ahead of the SUN..." do you agree ? – alexbt Apr 29 '18 at 21:29
  • No, I don't really agree. The scenario of "pulling in" a malicious 3rd party library is fairly severe and can't really be mitigated by explicitly specifying a certain secure random implementation. I didn't downvote, I just left a comment for future visitors. – President James K. Polk Apr 30 '18 at 01:58
5

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.

Jules
  • 14,841
  • 9
  • 83
  • 130
  • 5
    Even worse - the code is completely absurd. Either the self-seeding is sufficiently secure - then there is no need to seed. Or it is not - then the code does not improve matters either, since it relies on the self-seeding done by sd0 to provide the seed for SR. 8-() – Dr. Hans-Peter Störr Feb 28 '13 at 12:07
2

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

HenriqueMS
  • 3,864
  • 2
  • 30
  • 39
0

Sad, that javadoc does not say, what the minimum seed size "DEFAULT_LENGTH" is to reach the intended security level of algorithm design, not even for some default implementation. :(

Essentially security depends on true random; there is nothing like "an algorithm seeding itself without exterior data". Unless the inputs of a seed generator are revealed, it is not possible to certify any security level.

Providers of true random are 1.) https://www.random.org/ 2.) Tools as VeraCrypt derive white noise from mouse motion.

If your goal is real security, you will combine numbers from anonymous random generators with self-cerified white noise.

Sam Ginrich
  • 661
  • 6
  • 7