RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

> On Apr 20, 2016, at 8:54 AM, Bradford Wetmore <bradford.wetmore at oracle.com> wrote:
> 
> > Webrev updated again at
> >
> > http://cr.openjdk.java.net/~weijun/8051408/webrev.10/
> > http://cr.openjdk.java.net/~weijun/8051408/webrev.10/spec
> > http://cr.openjdk.java.net/~weijun/8051408/webrev.10/specdiff
> 
> Some initial comments.
> 
> security/java.security
> ======================
> 123-133:  Would you consider changing the ordering of the algorithm
> names to be consistent between the two sections?  Say:
> 
>    NativePRNG, SHA1PRNG, and DRBG.

Sure.

> 
> 175:  Should we add DRBG:SUN as a backup for non-windows?

If NativePRNGBlocking:SUN is not always available, yes we can.

> 
> 181:
> # NIST SP 800-90Ar1 lists several DRBG mechanisms, each can be configured with
> ->
> # NIST SP 800-90Ar1 lists several DRBG mechanisms. Each can be configured with
> 
> 184:
> request of "DRBG" SecureRandom implemented in the SUN provider.
> ->
> request of "DRBG" SecureRandom implementations in the SUN provider.
> (Other DRBG implementations might also use this property.)

OK.

> 
> 188:  Can you adjust the line lengths so it looks polished, and not
> edited in place?  I use a vi macro (using fmt) for this.

OK.

> 
> 189:  Do you want to add "currently", or leave as is?
> names are not configurable
> ->
> names are not currently configurable

Yes.

> 
> 194:  Am I missing something?
> is a slash-separated list
> ->
> is a comma-separated list

Oops, will correct.

> 
> 198:  Should we add a short 1-liner description for the fields?  The
> variable meanings (esp pr/df) may not be obvious to a casual observer.
> For example, using these three fields as an example:
> 
>    mech_name: default "Hash_DRBG"
>        "Hash_DRBG" | "HMAC_DRBG" | "CTR_DRBG"
> 
> +        The DRBG mechanism to use.
> 
>    pr: default "none"
>        "pr_and_reseed" | "reseed_only" | "none"
> 
> +        Prediction resistance...
> 
>    df: default "use_df", only applicable to CTR_DRBG
>        "use_df" | "no_df"
> 
> +        Derivation Function...

Not sure about the format, how about

#   # The DRBG mechanism to use. default "Hash_DRBG"
#   mech_name: 
#     "Hash_DRBG" | "HMAC_DRBG" | "CTR_DRBG"

So this is double comment. Please note I also put the default value into the comment.

> 
> 210:  Instead of pointing to 800-90A here, you should instead point to
> the Sun Provider document.  That document will then reference
> 800-90A/Section 10, and will use the Standard Algorithm names that
> you have defined for these algorithms.  (I assume you'll be adding
> those to the standard algorithm name doc, right?  I don't recall seeing
> that part of the review yet, but maybe haven't gotten that far.)

Yes, I will.

The standard algorithm name is only "DRBG", but here it's the DRBG algorithm name (SHA-256 etc). Are we going to talk about this in Sun Provider doc?

> 
> 214: In the past, we've used spaces as a field separator for names in
> a provider, I'm wondering if we should make this name something like
> "3KeyTDEA" instead?

OK.

> 
> 229: I hadn't noticed this before, but the Security variable "drbg"
> doesn't match the style of the other variables, securerandom.* or
> otherwise. I think we should use something like either:
> 
>    securerandom.drbg
>    securerandom.drbg.config

Will choose "securerandom.drbg.config".

> 
> 229: Why an empty string here?  Why not actually specify the default here
> instead of burying the default somewhere where it might changed without
> a corresponding update to this file?

There is a small technical issue here. The problem is that the default value of algorithm_name depends on mech_name, and the default value of strength depends on algorithm_name. If we set "Hash_DRBG,SHA-256,128" here and we only change one of them, another one might be illegal.

For example, if I want to create a SecureRandom using MoreDrbgParameters (yes, this is an internal class) with

   SecureRandom.getInstance("drbg", new MoreDrbgParameters(
       null, "CTR_DRBG", "3KeyTDEA", null, false,
         DrbgParameters.instantiate(-1, NONE, null)))

it will fail, because with -1 in DrbgParameters.instantiate(), the new configuration will look like

   CTR_DRBG,3KeyTDEA,128

but CTR_DRBG,3KeyTDEA does not support 128 bit strength.

I will have to use DrbgParameters.instantiate(112, NONE, null) but that does not looks comfortable because I thought the system can give me a default working strength.

This won't be a problem if only public API is used, but I will have to redo some implementation codes.

Thanks
Max

> 
> Thanks,
> Brad
>