RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

Bradford Wetmore bradford.wetmore at oracle.com
Wed Apr 27 00:05:44 UTC 2016


Deleting the cruft that doesn't need further discussion.

On 4/24/2016 9:13 PM, Wang Weijun wrote:
>>> http://cr.openjdk.java.net/~weijun/8051408/webrev.11/
>>> http://cr.openjdk.java.net/~weijun/8051408/webrev.11/spec
>>> http://cr.openjdk.java.net/~weijun/8051408/webrev.11/specdiff
>
> Thanks for the review. I copied your DRBG.txt below with my comments.
>
> Most are accepted, some I mentioned my previous thoughts.

Sorry, I've not been as up to date on previous discussions.

> Some needs clarification or discussion (does not mean I don't accept them), tagged with *** so you can search for them.
>
> BTW, the CAVP test is on test/closed. I'll send you a private mail.

We may need to include part/all of it in the open, depending on how 
strict we want to be following the health testing requirement.

>> Overall:
>> ========
>>
>> IMPORTANT:  By 800-90Ar1, sections 8.3/8.5, I don't think outputting the
>> internal state is allowed, even for debugging.  Please recheck this and
>> let's discuss.
>
> *** Not very sure. Someone can always use reflection to look at the internal states.
>
> Well the main reason I printed out the internal state is to compare it with the CAVP text file when there is something wrong. Surely I can remove them now.

Hm...we should talk to the person I mentioned in my other email, he 
might have an opinion on this.

>> Also by the 800-90Ar1 spec, and given that SecureRandom is supposedly
>> serializable, I don't think
>> serialization to recover the exact state is (or should be) allowed:
>>
>>     The internal state for one DRBG instantiation shall not be used as
>>     the internal state for a different instantiation.
>>
>> I'll/we'll need to think about this some more.
>
> *** Since after s11n/des11n it's *another* instance, seems we should not remember the states. A consequence is that it has to be reinitialized.

I think we're on the same page here.

>> I didn't see any checks for repeated input parameters.  IIRC, if you get
>> entropy repeating, that's a failure case.
>
> *** I thought that would be health check for an entropy source. How do you expect me to check it? A cache for recent inputs? Not very practical.

Section 6.5 of 800-90B.  Or maybe we should just assume the entropy data 
to be valid and not worry about this?

>> I didn't see any health tests.  What is your plan for that?
>
> *** If by health test your means the CAVP known-output tests, I am going to put it into test/closed since it's reading a huge (13MB) external file and should be stored on an artifact server.
>
> That said, I'll be happy to extract a subset of it and make it a public test.

TBD in another mail.

>> 123-128: Ordering still the same.
>>      NativePRNG, SHA1PRNG, and DRBG.
>
> I didn't realize your were asking me to be consistent. I only updated the first appearance. Will fix.

Yes.  Thanks.

>> Applications can request for different instantiation parameters
>> ->
>> Applications can request different instantiation parameters
>
> I thought "request for" is a correct phrase. Will fix.

It's a little awkward.

>> 212-213: Still talks about 800-90 instead of the Sun provider document.
>
> *** How just list all names here? I already listed all mech names.

Ok.  It should probably also go into the Sun provider doc, where we talk 
about what algorithms/modes/etc are available.

>> I notice the particular value of "3KeyTDEA" is mentioned later in this
>> file, but not the other string values.
>
> CtrDrbg.java.

Not sure what you were saying here.

>>
>> java/security/Provider.java
>> ===========================
>> Copyright date.
>
> Already updated. You reading webrev.10?

I might have started on this!  Sorry!

>> I'm really tempted to file a bug to clean up the really long lines added
>> by the lambda folks.  Ugh...This make reviewing changes in frame-based
>> webrevs hard.
>
> There aren't many in this file. I'll break them.

Thanks!

>> java/security/SecureRandom.java
>> ===============================
>> 54,78,406,439,513,etc:  > 80 chars.  I won't list any more, but expect it
>> might occur in other files as well.
>
> You expect me to be 100% strict with 80 chars. Will do.

<soapbox>
Yes please!  Sorry, it's a pet peeve of mine and some others.  I do hear 
the usual "Get a more modern terminal emulator", but that's not the 
issue.  The problem is real estate.  Even with a 24" monitor 
(1900x1200), doing a side-by-side webrev (i.e. frames) comparison on a 
160 character line means I've got to scroll both sides.  It's even worse 
on a laptop with less real estate.  A 3 column merge is nigh impossible. 
  And what if you need to print a hard copy?

I realize some breaks are impossible, but if you're regularly indenting 
a large amount, you probably will want to refactor your code.
</soapbox>

>> 72:  I don't know for sure about this, but I don't think you need a <p>
>> following a <blockquote>.  It gives extra vertical space in the output.
>> I noticed this several places in the resulting javadoc.
>
> I don't see any visual difference between the 2 styles.
>
> Turns out the extra space is a empty line in pre (try Select All and you'll see). I'll put </pre> at the end of the previous line.

Ah yes!  Thanks.

>> 592:  I've not be happy with the wording both here and in SecureRandomSpi.
>> The value returned here may not be the same as the one used to instantiate
>> it.
>>
>> I'm wondering if maybe something like this would be clearer:
>>
>>     Returns the effective {@link SecureRandomParameters}
>>     that is actually used to instantiate this {@code SecureRandom}.
>> ->
>>     Returns the effective SRP for this SecureRandom instance.
>
> OK.
>
> In fact, I was deliberately trying to express the meaning that the returned value here could be different from the one passed into getInstance(). But maybe as your suggested, not mentioning instantiate at all is clearer.
>
> Anyway I think I've emphasized this many times in other places.

You have, but this seems to be contradictory to that.  My point is that 
some people might see "used to instantiate" as the parameter that was 
actually passed in, rather than the derived value.

>> 660:  Reading the next method, I wonder if we should we also add a
>> @throws NPE here for then bytes is null?  It's undocumented currently.
>
> *** Wow. It was there and Xuelei suggested me removing it. The reason is that SunPKCS11's nextBytes happily accepted null (and ignore it).  Although I see no benefits doing that (SunPKCS11 has not use this chance to do any instantiate work etc), maybe it's safe to not require non-null here.

Instead we should probably file a spec bug instead and get the SunPKCS11 
fixed.

>> 673-677: You probably know this, but according to:
>>
>>     http://www.oracle.com/technetwork/articles/java/index-137868.html
>>
>> a @param ends with a period if another sentence follows it.  But
>> it's done throughout this file, so suggest you not change all the other
>> places in this push, but you may want to keep it consistent within
>> your new method.
>
> *** Please confirm if I understand correctly: No period if it's only a clause or an expression.
>
> So I should remove the period at the end of 674.

See the section starting:

     When writing a phrase, do not capitalize and do not end with a
     period:

There's several rules there.

>>
>> 676:  You mention unrecognizable/unsupported, does that include
>> illegal?  For example, if prediction_resistance_flag was not set at
>> instantiate, but Prediction_reistance_request was set in this method?
>
> I'll add "illegal", although "unsupported" kinds implies it. Otherwise, what is the difference of "unrecognizable" and "unsupported"? We know this parameter defined for DRBG but we haven't yet supported it?

unsupported/unrecognizable are likely the same:  I would expect that to 
mean they sent us an object that our implementation just doesn't know 
what to do with.  e.g. they extended and sent us a new subclass of 
SecureRandomParameters that isn't a NextBytes.  whereas illegal is that 
they sent us a value that can't be allowed at runtime.  e.g. pr was set 
in this object, but pr wasn't requsted in the initiate.

>> XXX: what do you do bout large size requests or things that can't be
>> reseeded?
>
> This won't happen in our implementation so I haven't mentioned them.
>
> For large size nextBytes() it's an implementation's duty to generate again and again until all is obtained.

At one time, we had talked about modifying nextBytes(byte[]) internally 
keep regenerating until there was enough to return to avoid imposing 
this assumption on new code.  For example, if someone had a 90K 
Hash_DRBG/SHA* request (64K per call), the framework would call generate 
twice before turning the concatenated buffer array.

But this assumption is fine as well.  It needs to be documented in both 
nextBytes().  Something along those lines.  "If the underlying 
implementation is prohibited from supplying a full arrays worth of data, 
the application must repeatedly call..."

> *** I am not sure what you are asking of reseeding. UnsupportedOperationException not enough?

My comment was that if I specify PR here and the impl doesn't support 
reseeding, is that an IllegalArgument exception?

>> 752:  I don't think this addition made it into your CCC.
>
> *** The 3rd part of spec in CCC only mentions changes related to DRBG. I think this one is trivial and should not be included there.

Likely needs to be a separate CCC then.

>> 171: I think we should leave this part out, or at least warn that it
>> may not be supported in all distributions.
>>
>>     A provider is free to add other DRBG implementations with specific
>>     configurations using different SecureRandom algorithm names (for
>>     example, "Hash_DRBG/SHA-512")
>
> Leave this out. I couldn't resist making suggestions on these old names that are removed now.

Ok thanks.  Since we didn't have, encouraging others to add them seem 
strange.

>> I really think all of the Implementations Notes here should be placed
>> in the Sun Provider document.  If you want to leave the section saying
>> "see the DRBG section of the Sun Providers", that's fine.  Some
>> additional notes about this section whereever it ends up.
>
> Correct. The @implSpec was written to comply with 800-90Ar1's 11.1 and will be places in the Sun Provider doc. I was planning to add a reference here after the target is complete.

Ok.

>> In java.security, you call it "3KeyTDEA", but here the string is
>> "DESede".  Which is it?
>
> I'll use "3KeyTDEA (known as DEDede in JCE)".

Assuming a typo?  DEDede->DESede

You're still leaving the DESede alias, right?

>> 472:
>> @throws NullPointerException if {@code capacity} is {@code null}.
>> ->
>> @throws NullPointerException if {@code capability} is {@code null}.
>
> Oops.

No problem, I've done that once or twice (or...).  Funny how you can 
look at something a million times, but you just believe what you want it 
to say instead of what it actually says.  ;)

>> java/security/SecureRandomParameters.java
>> =========================================
>> 31:
>>     A specific type of {@code SecureRandom} can implement this interface.
>>
>> Don't you mean that different types of *SecureRandomParameter* can be
>> created and passed to SR's?
>
> Yes, different for DRBG and NRBG, for example.

My point is that you'll be updating SecureRandom with 
SecureRandomParameter here.

>> sun/security/provider/AbstractDrbg.java
>> =======================================
>> 98,368,369, etc.  Lines > 80.  This is throughout your code.  Hard to
>> read on a laptop with narrow display.
>
> I'll shorted the comments, but sometimes for codes I am OK with a little more than 80 chars. I can make it strict.

There are some places that it's difficult to make clean break, but 
fortunately they are pretty few.

>> 64:  Is this serialVersionUID a placeholder, or the final value?
>
> *** My understanding is that serialVersionUID calculated with serialno is to be compatible with an earlier version that has no this field. In a new file with serialVersionUID, it can anything and I prefer one that is easy to understand.

I wasn't sure if you were going to take the final version of the file 
and run it through serialver to get the actual value.

>> 107:  In 800-90Ar1, many of the variables have specific names.  They
>> don't generally follow our conventions, but would you consider using a
>> close derivative?  For example, highestSupportedSecurityStrength and
>> maxPersonalizationStringLength?  That would make a little more clear
>> how the DRBG variables map to the implementation.  I notice you've used
>> those names in other places, but wasn't sure if that was by design or
>> not.
>
> I use the name in 800-90Ar1 in comments but prefer a more-Java-like variable name.
 > If you like I can be more close.

It makes it a little easier to read.

>> 145:  Should you mention that MAX_INT is actually smaller than the
>> amount allowed by the various algorithms?  Or should the implementation
>> be long-based throughout?
>
> *** Since a Java array can only holds MAX_INT of elements, we won't be able to support long.

Yes, of course!  :)

 > I can add an explanation.

Sound good.

>> Also, there are several places here and in other files (DRBG.java)
>> where you do multiple function calls where the return value won't be
>> changing: first to check legality, then to actually capture/use the value.
>> See lines 348/365/366.  Can't these calls be combined?
>
> Yes I can.
>
> As you see, when the return value is a cloned array (line 359) I only call once. When it's a primitive I think an extra call is not too bad.

Just seems like a simple enhancement.

>> 355:  Are you somewhere calling this DRBG.nextBytes() multiple times if
>> the output size is too big to be handled by a single DRBG call?
>
> No. It should but since here maxNbLength is MAX_INT I omit the call.

Ok.

> *** I created these maxNbLength/maxAiLength/maxPsLength variables to be more aligned with pseudo codes in 800-90Ar1, but honestly they have no real use in the impl and I sometimes ignore it.

Ah.

>> 460:  would you want to call security "min_entropy" to match the spec?
>
> Sure, and I will change min/max there to minLength/maxLength.

Thanks.

>> 483:  Can't this be final?  Netbeans is complaining about a non-final
>> defaultES at 503.  Also private?
>
> Yes and yes.
>
>>
>> 551:  What is register() used for?
>
> Useless. I was doing an experiment with lines in SunEntries
>
>    new HashDRBG(null).register(map)
>
> and those attributes can be use by an app to detect capabilities.

We usually put those into the SunEntries directly so it can be used by 
the JCE selection mechanism.

>> 642:  We have this problem in JSSE: in various places, you have debug
>> strings like "instantiate", but in a multi-SR environment, you'll have
>> no idea which one is in play.  Do you want to use '"instantiate" + this'?
>
> *** Is "this" enough? this.toString() shows the configuration. Do you want System.identityHashCode()? I can add it right into toString().

Something that makes things quickly understandable which object is being 
used would be helpful.  SeanC would be a good one to ask, as he's more 
involved in the supportability side.  I'm just remembering trying to 
debug some JSSE debug output, and we had multiple thread/sockets debug 
output all interleaved together.  It was impossible to parse.

>> 674:  can be final.  Do you want to use the ES to generate an initial
>> value for the monotonically increasing sequence number?
>
> *** I don't think it's necessary, as long as it changes with duplication.

Ok.

>> 692:  Isn't there some preexisting code somewhere we can use?  This seems
>> like a common need, and I recall having written something similar a
>> while ago.
>
> No colon/space/newline? I don't know.

I just found an RFE, and added my comment to it:

     https://bugs.openjdk.java.net/browse/JDK-8143863

>> sun/security/provider/CtrDrbg.java
>> ==================================
>>
>> 51:  transient here?
>
> *** Already transient.

Should it be transient?  It's not in the other files.


>> 141:  AES-192/256 is indeed supported, it's just that the unlimited
>> policy files aren't installed.  I would suggest a better error message
>> here, like "algorithm not available (policy)" or something similar.
>
> OK.
>
> "algorithm not available (because policy)". I recently read a article about modern English do not use "because of" anymore.

Hm...ok.

>> 422:  What is this for?
>
> SecretKeySpec for DESede use 24 bytes but k in CTR_DRBG is 21 bytes.

Ah, parity bits.

>> sun/security/provider/DRBG.java
>> ===============================
>> 60: A final constant string should be cap'd.  propName -> PROPANE
>
> OK.

er...PROP_NAME or PROPNAME, not PROPANE!  ;)

>> 93: space missing "part:"
>
> Do we need a space here?

     for (String s : names)

> *** BTW, where is a modern coding style guide?

I don't think there is anything recent.  I took this style from:

     8/technotes/guides/language/foreach.html

which follows from the ? operator.

>> sun/security/provider/HmacDrbg.java
>> ===================================
>> By 8.3/8.5, I don't think outputting V/C is allowed, even in debug.
>> Please recheck this.
>>
>> 76:  Don't you need to re-init() this again?  It's supposed to be the
>> current value of K.
>
> *** In Step 2, v is updated but not k.

But k was updated in step 1 of HMAC_DRBG, or are K/V only updated at the 
exit of the method?  I'm guessing if you got it to pass, you must be right.

>> sun/security/provider/MoreDrbgParameters.java
>> =============================================

>> 48:  if null, does a default get selected?
>
> Yes, the one in the security property. I should also mentioned when passed into HashDrbg/HmacDrbg/CtrDrbg it is ignored.

In the others, you say "a default XXX will be used", but it's missing here.

>> com/sun/crypto/provider/SunJCE.java
>> ===================================
>> I wasn't able to find any HmacSHA512/* OIDs at oid-info.com.
>>
>>     http://oid-info.com/get/1.2.840.113549.2
>>
>> Just the ones for MessageDigest, which you have in SunEntries.
>
> Neither can I find in other places. I even asked on Stack Overflow
>
> http://stackoverflow.com/questions/33605411/oids-for-hmacsha512-224-and-hmacsha512-256

I saw your question when I was looking, but didn't realize that was you!  ;)

I hope this helps some more.

Brad



More information about the security-dev mailing list