RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations
weijun.wang at oracle.com
Wed Apr 20 04:00:55 UTC 2016
> On Apr 20, 2016, at 11:34 AM, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> On 4/19/2016 9:09 PM, Xuelei Fan wrote:
>> On 4/15/2016 9:
> line 66-68: My understanding is that ...
> I would suggest rewords or remove this sentence. "Not used much" does
> not mean needing no synchronization.
Precisely engineNextBytes() should synchronize on both states and configuration, and engineGenerateSeed() should synchronize only on configuration. But since engineGenerateSeed() is not used a lot, I don't think it's not worth coding it with a special synchronize(configuration) so both now just synchronize on "this".
> As you have add synchronized
> keyword for engineGenerateSeed, I may suggest you remove lines 63-68,
> and move 57-61 to class description.
I'll move 57-61 to class description, and would like to keep 63-68 there, and update the words "does not need to be synchronized" to "does not need to be synchronized on the internal states".
> I think comment are mainly used for code readers. You used a lot "my"
> and "I", and questions to yourself in the comments. Would you mind
> change to use neutral tones and remove unnecessary comments in the final
> 662 // 6/7. Get entropy. TODO: 1st arg security strength?
> Do you want to complete the TODO task. Similar comments to other TODO
Will remove TODO word and add a clarification.
> 670 nonce = longToByteArray(cc.incrementAndGet());
> 685 private static byte longToByteArray(long l)
> The nonce is leading with "sun.drbg", and following by a 8-bytes integer
> value. This scheme only provider 8-bytes randomness. Looks like the
> quality does not meet the nonce requirements (8.7.6 NIST SP-800-90Ar1)
> for 256-bit strength.
8.6.7 allows it to be a "monotonically increased sequence number".
> 598 protected final synchronized void engineConfigure(
> engineConfigure is not part of SPI. Would you mind change the name and
> remove the leading "engine" in case of miss-understanding?
> It would be nice grant as less accessibility as possible. For the
> "protected" keyword, should it be package accessibility at present?
> Similar comment for other protected methods other than the override
> ones, and some public classes in the package.
Will go through them.
I created AbstractDrbg so that it can be the base for any DRBG not necessarily in this package. For example, the AbstractDrbgSpec test also contains an implementation.
> Section 7.2 of NIST SP 800-90Ar1 says: "The personalization string
> should be unique for all instantiations of the same DRBG mechanism type".
> Please check the unique for the personalization string in the
"Should" is not "shall" (section 4, terms). Two other reasons:
1. Checking for uniqueness needs to save all strings in memory.
2. The default is null, and all nulls are the same. Why bother checking for those non-nulls for uniqueness?
> Looking forward to your next webrev.
More information about the security-dev