RFR 8066397 Remove network-related seed initialization code in ThreadLocal/SplittableRandom

[Bradford Wetmore]

Various comments for this thread from June/July/November/December.

Some of the comments I'm responding to may already be better understood 
than when they were originally written.

Peter wrote in response to a suggestion to use /dev/random:

>> Although the approach would cause some more classes to load, no
>> arbitrary providers should be initialized.
>
> I think this is waht you get when you set
> "java.util.secureRandomSeed" system property to "true". TLR uses
> java.security.SecureRandom.getSeed(8) in this case.

For the "no arbitrary provider" part, that may not be quite correct. 
getSeed() creates/pulls from the default SecureRandom impl (i.e. new 
SecureRandom().generateSeed()), so it pulls in the Security Provider 
mechanism to determine the most preferred implementation, which could 
initialize additional higher-priority providers until an instance of 
SecureRandom is found.  For example, ucrypto on Solaris doesn't have a 
SecureRandom impl, so it would then fall back to PKCS11.

As has been pointed out, the various Oracle SecureRandom implementations 
and their preference order are a twisty maze of passages, somewhat but 
not exactly alike.  (With apologies to the "Colossal Cave.")  The 
default preference order is:

Solaris (sparc/sparcv9/x86/x64)
     "PKCS11" - "SunPKCS11"
     "NativePRNG" - "Sun"
     "SHA1PRNG" - "Sun"
     "NativePRNGBlocking" - "Sun"
     "NativePRNGNonBlocking" - "Sun"

Linux (x86/x64)/MacOS
     "NativePRNG" - "Sun"
     "SHA1PRNG" - "Sun"
     "NativePRNGBlocking" - "Sun"
     "NativePRNGNonBlocking" - "Sun"

Windows (x86/x64)
     "SHA1PRNG" - "Sun"
     "Windows-PRNG" - "SunMSCAPI"

Here's a few impl details for seeding calls.

PKCS11:
-------
generateSeed() routes to engineNextBytes(), which goes to the underlying 
PKCS11.

NativePRNG:  (Unix-only)
-----------
generateSeed() by default routes to /dev/random, unless the System 
Entropy Gathering Device (EGD) (set via a Security/System property) 
routes to something else.  (FYI: nextBytes() uses /dev/urandom.)

NativePRNG$BLOCKING:  (Unix-only)
--------------------
generateSeed() always routes to /dev/random.  (FYI: nextBytes() uses 
/dev/random.)

NativePRNG$NONBLOCKING:  (Unix-only)
-----------------------
generateSeed() always routes to /dev/urandom.  (FYI: nextBytes() uses 
/dev/urandom.)


SHA1PRNG:
---------
generateSeed() depends on the value of the EGD:

default EGD is:  "/dev/random"

     Note: if string "/dev/urandom" is set, urandom is used instead.

     Unix:  generateSeed() routes to /dev/random
            (NativeSeedGenerator: pure java)

     Win:   generateSeed() routes to CryptGenRandom
            (NativeSeedGenerator + libjava native)

non-default (not "/dev/random"/"/dev/urandom"):  "URL" is specified
     UNIX/Win:  routes to URL

If both above fail:
     falls back to ThreadedSeedGenerator (Pure Java)


Windows-PRNG
------------
generateSeed() routes to mscapi.PRNG/CryptGenRandom
     Note uses libmscapi, not libjava.


Peter wrote:
> The most problematic one is the default on Windows platform (the
> platform that does not have the "/dev/urandom" special file and would
> be used as a fall-back by your proposal) -
> sun.security.provider.SecureRandom. This one seeds itself by
> constructing an instance of itself with the result returned from
> SeedGenerator.getSystemEntropy() method. This method, among other
> things, uses networking code to gather system entropy:

SeedGenerator.getSystemEntropy() that includes the Network interfaces is 
only called when you need to seed the SHA1PRNG internal seeder in order 
to generate nextBytes().  generateSeed() doesn't trigger that internal 
seeder initialization.

So as you can see above, SecureRandom.generateSeed(int) is really 
variable in what you might end up with, and how much cruft comes with 
it.  For sure, the provider mechanism will be dragged in which is fairly 
substantial.


Peter wrote:
------------
> So by default yes, plain NativePRNG (the default on
> UNIX-es) is using /dev/urandom for nextBytes(), but this can be
> changed by defining java.security.egd or securerandom.source system
> property.

EGD really only affects where Seed byes are obtained from, IIRC, 
nextBytes() is not generally affected by this value.  It does tweak 
which implementation is most preferred within the Sun provider, but the 
majority of the effect is in Native/SHA1PRNG choice of generateSeed.

The original suggestion back in June:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2014-June/027389.html
http://cr.openjdk.java.net/~plevart/jdk9-dev/TLR_SR_SeedGenerator/webrev.01/

for directly calling into NativeSeedGenerator makes more sense if you 
want to avoid duplicating existing code and creating a new native 
libraries as in the current proposal (webrev.03).  Your data shows that 
this approach pulls in a much smaller subset of classes than using the 
full SecureRandom.getInstance().generateSeed() API.  I've gone through 
the threads a couple times now:  somehow I've missed the rationale for 
why you're moving away from this (.01) for webrev.03.

To the actual proposal:

     http://cr.openjdk.java.net/~plevart/jdk9-dev/SystemRandom/webrev.03/

Overall, I'm ok with what's proposed.  This is more straightforward to 
parse/understand than trying to adjust NativeSeedGenerator to 
create/call directly (e.g. UNIX:  new 
NativeSeedGenerator("/dev/urandom") or Windows:  new 
NativeSeedGenerator()).  But I'd still like to understand why you moved 
away from this.

One concern is that you're duplicating native libraries in java.base, 
and it would be the third JDK library overall with this type of call. 
There's one in libjava (for java.base/WinCAPISeedGenerator for 
sun.security.provider.NativeSeedGenerator) and sunmscapi (for 
jdk.crypto.mscapi/SunMSCAPI/sun.security.mscapi).  Would it work to 
tweak the WinCAPISeedGenerator so you don't have to create a new dll for 
java.base?

What are the fallbacks for SystemRandomImpl if /dev/urandom or the 
rtlGenRandomFN/CryptGenRandom aren't available?  Is that something 
you'll bake into TLR or will you do it here?

Having TLR seed the other clients is ok with me, the APIs make it clear 
that this isn't a strongly secure mechanism.

>> (Also, at some point we might reconsider our cowardice about not
>> improving the internal java.util.Random algorithm. j.u.Random is
>> much more commonly used, and does not fare well on quality tests.
>> On the other hand, the more that users instead choose to use
>> SR or TLR, the better.)
>
> The main problem is code (not just JDK test code) that hardwires
> expected Random.next* output under given seeds. Which might be
> enough reason to leave it alone.
> Do any CCC members have an opinion?

I'm *NOT* a CCC member (IANACCCM?).  However, the current javadocs are 
very specific on several points.  The big ones for me:  "If two 
instances of Random are created with the same seed...<deleted>...they 
will generate and return identical sequences of numbers".  It doesn't 
specify whether these are two instances are in the same VM or are across 
VMs/vendors, but the wording:  "Java implementations must use all the 
algorithms shown here for the class Random, for the sake of absolute 
portability of Java code" which makes me think it's the latter.  That 
is, you should not change the algorithm.  That's my $0.02.

The following is just one last thing to keep in mind.  If you call 
generateSeed() on Linux (e.g. in the current code for 
TLR/SplittableRandom: java.util.secureRandomSeed calls to getSeed()), 
you could block.  We still receive "hang" reports because apps/libraries 
insist on SHA1PRNG which uses 20 bytes of /dev/random to seed the 
seeder.  We especially see this on systems that simultaneously start 
multiple VMs and drain the /dev/random pool quickly.  Another 8 bytes of 
TLR/SplittableRandom could have further impact.

Martin wrote:

> https://bugs.openjdk.java.net/browse/JDK-8047769

If you've been following this bug, I've figured why the NativePRNG$* 
classes are initing and thus opening the /dev/random,urandom.  This 
definitely needs some adjustment.

Brad