Update #5: JEP 123: SecureRandom Draft and Implementation.
Xuelei Fan
xuelei.fan at oracle.com
Wed Apr 10 13:21:44 UTC 2013
Looks fine to me. Thanks for take the comments.
Xuelei
On 4/10/2013 3:17 PM, Brad Wetmore wrote:
> Hi Xuelei/Weijun,
>
> Thanks for the comments.
>
> The version I plan to putback is:
>
> http://cr.openjdk.java.net/~wetmore/6425477/webrev.05/
>
> I probably won't have time to take any further comments before vacation,
> but can address things when I'm back. I have a few more from Bernd that
> I'd like to look at.
>
> On 1/17/2013 7:44 AM, Xuelei Fan wrote:
>> Hi Brad,
>>
>> Please note the priorities for each comment. As the M6 is coming, you
>> can only take P1 comments.
>>
>> P1: need to update
>> P2: suggested update, you can take it after M6.
>> P3: minor comments, it's OK to leave it unchanged.
>> P4: personal preference for your consideration, or my question.
>
> I think I've addressed all.
>
>> java/security/SecureRandom.java
>> -------------------------------------
>> 1-1. the performance of Pattern.compile [P2]
>> Pattern.compile() is expensive. I would suggest to use private static
>> lazy-initialized class attribute the patterns.
>>
>> public class SecureRandom extends java.util.Random {
>> private static final String regex = "...";
>> private static final Pattern pattern;
>>
>> public static SecureRandom getStrongSecureRandom() {
>> ...
>> if (pattern == null) {
>> pattern = Pattern.compile(regex);
>> }
>> ...
>> }
>> }
>
> Good point. Other people mentioned this privately.
>
> BTW, as you have above, this Pattern can't be final (not a constructor),
> and needs to be volatile. To avoid multiple initializations, I went
> with the Effective Java 2nd Edition Item 71 (FileHolder) pattern instead.
>
>> 1-2. spaces are allowed between algorithm and provider [P4]
>> According to the regex ("\\s*(\\S+)\\s*\\:\\s*(\\S+)\\s*"), spaces
>> are allowed around the tokens. For example, " NativePRNGBlocking : SUN
>> " is valid. I would like to use a stricter syntax at the beginning in
>> case of any special requirement comes in the future.
>
> Changed. Weijun also mentioned this. I now allow:
>
> * Entries are alg:prov separated by ,
> * Allow for prepended/appended whitespace between entries.
>
>> 1-3. may only need one regex for both "algorithm" and
>> "algorithm:provider" [P3]
>> I think one regex is OK for both:
> "([\\S&&[^:]]+)(\\:([\\S&&[^:]]+))?".
>>
>> NativePRNGBlocking:Sun
>> group 1: NativePRNGBlocking
>> group 2: :Sun
>> group 3: Sun
>>
>> NativePRNGBlocking
>> group 1: NativePRNGBlocking
>> group 2: null
>> group 3: null
>>
>> If group 2 is non-null, it is of the "algorithm:provider" style.
>
> I modified this quite a bit and now the entire parsing logic is in one
> regexp. I iterate over it until group(5) is null.
>
> * Capture groups:
> * 1 - alg
> * 2 - :prov (optional)
> * 3 - prov (optional)
> * 4 - ,nextEntry (optional)
> * 5 - nextEntry (optional)
>
> \\s*([\\S&&[^:,]]*)(\\:([\\S&&[^,]]*))?\\s*(\\,(.*))?
>
>> 1-4. a typo at line 614 [P1]:
>> - 614 // Pattern for "algorithm.provider"
>> + 614 // Pattern for "algorithm:provider"
>
> Done.
>
>> java.security-windows
>> -------------------------------------
>> 2-1. Is it possible to enable "NativePRNGBlocking:SUN" in Windows? [P2]
>
> To my knowledge, there is no concept of Blocking/NonBlocking or
> quality-of-service levels in the SunMSCAPI CryptGenRandom API. There is
> just one implementation (Vista SP1+: AES/NIST SP 800-90, earlier uses
> FIPS 186-2.)
>
>> + securerandom.strongAlgorithms=Windows-PRNG:SunMSCAPI
>>
>> I was wondering to enable "NativePRNGBlocking:SUN" here before I know
>> that the "NativePRNGBlocking:SUN" is not available on Windows:
>> + securerandom.strongAlgorithms=Windows-PRNG:SunMSCAPI, \
>> + NativePRNGBlocking:SUN
>>
>> The availability of "securerandom.strongAlgorithms" property depends on
>> the enabled security providers, and the platform.
>
> Yes.
>
>> If "SunMSCAPI"
>> provider is not enabled, the "Windows-PRNG:SunMSCAPI" will not work. I
>> think "NativePRNGBlocking:SUN" is not available on Windows system.
>
> Correct.
>
>> It is
>> not as obviously as that the "Windows-PRNG:SunMSCAPI" is not available
>> on Unix/Linux/Mac OS systems. We need to documentation this behaviors
>> clear somewhere else.
>
> I'm planning to have a new section in the Sun Providers document which
> describes the various implementations available on which platforms, and
> we'll use my internal SecureRandom wiki page as the base.
>
> I'm thinking seriously about adding a NativePRNG in the Sun Provider to
> point to similar functionality that is in Windows-PRNG:SunMSCAPI. That
> is, SHA1PRNG and NativePRNG would exist on all Oracle platforms.
>
> JDK-8011737: Consider adding a NativePRNG equivalent,
> probably based on MSCAPI code.
>
> Of course, I'd also like to add
>
> JDK-8003584: Consider adding a more modern SecureRandom
> implementation
>
>> P11SecureRandom.java
>> -------------------------------------
>> 3-1: to support strong algorithm in PKCS11 [P4]
>> Is SHA1PRNG:SunPKCS11 a strong algorithm?
>
> There is no SHA1PRNG in PKCS11. The PKCS11 SecureRandom impl reads
> directly from the PKCS11 library. If setSeed was ever called, then use
> a "mixer" SHA1PRNG to combine the seed with bytes from the PKCS11 impl.
>
>> I think it would be nice to
>> add it as a backup in the strong algorithm property.
>
> One question which I'll answer but you didn't actually ask. :)
>
> AFAIK, SHA1PRNG:SUN has never undergone a formal evaluation for its
> strength, so I hesitated including it in the list. All indications,
> anecdotal reports, and some graduate level research have shown it is
> quite strong, but it was a homebrew algorithm developed back in the days
> when export control restricted what we could include in the JDK.
>
> For UNIXn, I'm thinking of sticking with the NonBlocking OS
> implementations which return values only when "sufficient entropy has
> been collected," and the MSCAPI which uses NIS 800-90.
>
>> SeedGenerator.java
>> -------------------------------------
>> 4-1: downgrade normative reference to java security property file [P3]
>> [line 57-60] As you have already there, I would suggest to use the new
>> style of security property. See
>> http://hg.openjdk.java.net/jdk8/tl/jdk/rev/346c0af4af41 and the
>> description from SeanM,
>>
> http://mail.openjdk.java.net/pipermail/security-dev/2012-December/006144.html.
>
>>
>> line 57-60:
>> - * accomplished by setting the value of the "securerandom.source"
>> - * Security property (in the Java security properties file) to a URL
>> - * specifying the location of the entropy gathering device, or by
> setting
>> - * the "java.security.egd" System property.
>>
>> + * accomplished by setting the value of the
>> + * {@code securerandom.source} security property to a URL
>> + * specifying the location of the entropy gathering device, or by
> setting
>> + * the {@code java.security.egd} System property.
>
> I also took out the following paragraph, which also talked specifically
> about the file and its location.
>
>> SunEntries.java
>> -------------------------------------
>> 5-1: what's the usage of "NativePRNGNonBlocking"? [P2]
>>
>> + if (NativePRNG.NonBlocking.isAvailable()) {
>> + map.put("SecureRandom.NativePRNGNonBlocking",
>> + "sun.security.provider.NativePRNG$NonBlocking");
>> + }
>>
>> I did not find the description of this algorithm in the specification
>> (CCC) or other export documentation. Do you want to add it to Oracle
>> provider names doc?
>
> Yes, it will be added.
>
>> Otherwise, I would suggest to comment out this
>> algorithm. The above would set a external SecureRandom algorithm, I
> think.
>
> Yes, it does.
>
> I talked around this in the CCC, but I didn't actually spell out the new
> names. That's an oversight that probably needs to be corrected. I've
> sent Joe Darcy an email. Here's the current CCC description:
>
> NativePRNG reads seeds from /dev/random and nextBytes from
> /dev/urandom. I added two new NativePRNG implementations which are
> completely blocking or nonblocking. The
> "securerandom.strongAlgorithms" property points to the blocking
> variant.
>
> NativePRNG reads seeds from securerandom.source/java.security.egd
> (default: /dev/random), and nextBytes from /dev/urandom
>
> NativePRNGBlocking reads both seed and nextBytes from /dev/random
>
> NativePRNGNonBlocking reads both seed and nextBytes from /dev/urandom.
>
>> sun/security/provider/NativePRNG.java
>> -------------------------------------
>> 6-1: line 36-42, wordsmithing. [P3]
>>
>> "It obtains seed and random numbers by reading system files such as
>> the special device files /dev/random and /dev/urandom. This
>> implementation respects the {@code securerandom.source} security
>> property and {@code java.security.egd} system property
>> for obtaining seed material. If the file
>> specified by the properties does not exist, /dev/random is the
>> default seed source, and /dev/urandom is the default source of
>> random numbers."
>
> Thanks.
>
>> 6-2: Do you want to put something here? [P4]
>>
>> 321 // XXX change the urandom/random to seed/next
>
> Yes. XXX was my marker to not forget that. I have finished the
> variable renaming and updated the comments.
>
>> src/windows/classes/sun/security/provider/NativeSeedGenerator.java
>> -------------------------------------
>> 7-1: Not about this fix, but the code looks strange to me. [P4]
>> The constructor calls:
>> 44 super();
>
> This super() call could be left out, since there is just the
> SeedGenerator default constructor. In the Solaris/UNIX equivalent, it
> extends URLSeedGenerator, which does have a 1-arg constructor.
>
>> The SeedGenerator static block will be called and SeedGenerator.instance
>> will be initialized.
>>
>> According to the code in SeedGenerator.java:
>> 145 static public void generateSeed(byte[] result) {
>> 146 instance.getSeedBytes(result);
>> 147 }
>>
>> The getSeedBytes() method of the initialized instance will be used.
>> However, in Windows platform, I think the
>> NativeSeedGenerator.getSeedBytes() should be called, I think.
>>
>> I think the NativeSeedGenerator class should override the generateSeed()
>> method. About the super() call in NativeSeedGenerator, I think the
>> initialization of instance (line 92-142, SeedGenerator.java) may be not
>> necessary. The initialized instance in SeedGenerator is useless in
>> Windows from my understand. Am I missing something?
>
> Let's see if I can explain sufficiently.
>
> When SeedGenerator.generateSeed() is called from
> sun.security.SecureRandom, a single class instance of the SeedGenerator
> is created via the static initializer. There are three possible
> variants, Native/URL/Threaded. Depending on the values of the
> egdSource, we'll select one of the three, and set the global "instance"
> variable accordingly. Calls to SeedGenerator.generateSeed() are then
> rerouted to the underlying instance.getSeedBytes(). getSeedBytes is an
> abstract method in SeedGenerator, but concrete implementations are
> available in all three variants.
>
> Hope that helps.
>
> Brad
>
>
>
>> Otherwise, looks fine to me.
>>
>> Xuelei
>>
>>
>> On 1/11/2013 8:24 AM, Brad Wetmore wrote:
>>> Minor tweak. It occurred to me that people might use "." as separators
>>> (for example using some OIDs scheme), so I changed the syntax slightly
>>> of the system property to use ":" instead.
>>>
>>> For example:
>>>
>>> # This is a comma-separated list of algorithm and/or algorithm:provider
>>> # entries.
>>> #
>>> securerandom.strongAlgorithms=NativePRNGBlocking:SUN,
>>> SP800-90A/AES/CTR:IBMJDK
>>>
>>> Latest is now webrev.04.
>>>
>>> http://cr.openjdk.java.net/~wetmore/6425477/
>>>
>>> Brad
>>>
>>>
>>>
>>
More information about the security-dev
mailing list