>> 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. 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.
>
> 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.
>
> 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.
>
> Now that we have a standardized/supported/documented/evaluated algorithm
> in the form of DRBG, I think the time has come to add our first required
> SecureRandom algorithm. You'll want to add some verbiage like this to
> SecureRandome. See MessageDigest for an example.
Good.
>
> ---begin---
> <p> Every implementation of the Java platform is required to support
> the following standard {@code SecureRandom} algorithm:
> <ul>
> <li>{@code DRBG}</li>
> </ul>
> This algorithms are described in the <a href=
> "{@docRoot}/../technotes/guides/security/StandardNames.html#SecureRandom">
> SecureRandom section</a> of the
> Java Cryptography Architecture Standard Algorithm Name Documentation.
> Consult the release documentation for your implementation to see if any
> other algorithms are supported.
> ---end---
>
> 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.
>
> 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.
>
>
> security/java.security
> ======================
> A couple of my previous comments were not yet addressed. Please see my
> previous email.
>
> 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.
>
> 181:
> mechanisms, Each
> ->
> mechanisms. Each
Ha. I changed ", e" to ", E". Will fix.
>
> 186:
> Applications can request for different instantiation parameters
> ->
> Applications can request different instantiation parameters
I thought "request for" is a correct phrase. Will fix.
>
> 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.
> I notice the particular value of "3KeyTDEA" is mentioned later in this
> file, but not the other string values.
CtrDrbg.java.
>
>
> java/security/Provider.java
> ===========================
> Copyright date.
Already updated. You reading webrev.10?
>
> 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.
>
>
> 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.
>
> 74:
> The effective instantiate parameters used in the instantiation must match
> ->
> The implementation's effective instantiated parameters must match
OK.
>
> 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.
>
> 79: Not quite sure what "must be determined at the beginning..."
> Did you mean something like:
>
> The underlying SecureRandom impl may lazily configure itself,
> but the effective parameters must be determined on the first
> operation.
I meant although the DRBG is only instantiated at the 1st nextBytes(), the
effective instantiate parameters are determined when getInstance() returns one.
Therefore whenever you call getParameters() you get the same result.
Your "the first operation" is quite vague, I'll use "after it's created". Each
DRBG is created with its constructor with the SRP parameter, so this is precise
and correct.
>
> 110:
> Please note that {@code reseed} is not always supported.
> ->
> Please note that {@code reseed} may not be supported by all SecureRandom
> implementations.
OK.
>
> 230/653: Just curious why you made this change. Seems like it just adds an
> extra stack frame.
I will revert the changes. Cannot remember why for 230. For 653, I wanted to
express that one method is equivalent to calling another method with certain
argument.
>
> 325: I just noticed that all of the getInstance() variants have the exact
> same opening sentence, even when the provider parameters are different.
> Probably not worth changing at this point.
Yes, I was following the existing wording.
>
> 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.
> 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.
>
> 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.
>
> 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?
>
> 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.
*** I am not sure what you are asking of reseeding.
UnsupportedOperationException not enough?
>
> 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.
>
>
> java/security/SecureRandomSpi.java
> ==================================
> See comment in SecureRandom:592.
Got it.
>
>
> java/security/DrbgParameters.java
> =================================
> The current document is 800-90Ar1 but you later mention the older 800-90A in
> various places? Should probably be consistent?
Will use Ar1. Sometimes I think Ar1 is just a version name, and A is still the
standard.
>
> 47:
> A DRBG implementation has a configuration, including but not limited to,
> ->
> A DRBG implementation has a configuration, including but not limited to:
OK.
>
> Also in this section, you mention that a DRBG has a "configuration,"
> but to me that immediately implies there might be an API to configure it,
> but that implication isn't denied until after the list appears. Maybe:
>
> The 800-90Ar1 specification allows for a variety of DRBG
> implementation choices, such as:
>
> entropy source
> DRBG mechanism
> ....
>
> These choices are set in each implementation and are not directly
> managed by the SecureRandom API. Check your DRBG provider's
> documentation to find an appropriate implementation for the situation.
>
> The 800-90Ar1 API does have some configurable options, such as:
>
> required security strength
> if prediction resistance is required
> Personalization and Additional Input Strings
OK.
>
> 56:
> * <li> highest security strength
> ->
> * <li> highest security strength.
OK.
>
> 67: Do you have these 800-90A function names backwards?
>
> Calling SecureRandom.nextBytes(byte[], SecureRandomParameters) and
> SecureRandom.reseed(SecureRandomParameters) map to the Reseed_function
> and Generate_function defined in NIST SP 800-90A, separately.
Ah.
>
> 62: The word {@code Instantiate} isn't obvious that it's a nested class.
> Suggest a full expansion. i.e.
>
> A DRBG instance is instantiated with parameters from an
> Instantiate object
> ->
> A DRBG instance is instantiated with parameters from an
> DRBGParameters.Instantiate object
OK.
>
> For this section from 62-78, it would really help the flow if you can
> make the wording similar for each case:
>
> A DRBG instance can be instantiated with parameters from a
> DRBGParameters.Instance object...maps to the Instantiate_function
>
> A DRBG instance can be reseeded with parameters from a
> DRBGParameters.Reseed object...maps to the Reseed_function...calling
> with no parameters is equivalent to...
>
> A DRBG instance generates data with additional parameters using a
> DRBGParameters.NextBytes...maps to the Generate_function...calling
> with no parameters is equivalent to...
OK.
>
> 81:
> contain a constructor that has a
> ->
> contain the 1-arg SecureRandomParameter constructor that takes a
OK.
>
> 141:
> DrbgParameters.instantiate(112, NONE, null);
> ->
> DrbgParameters.instantiate(112, NONE, null));
Oops.
>
> 152:
> It is guaranteed to support 256 bits of security strength with
> prediction resistance turned on.
> ->
> If it successfully returns an instance, that instance is guaranteed
> to support 256 bits of security strength with prediction resistance
> available.
OK.
>
> 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.
>
> 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.
>
> 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)".
>
> Reseeding is supported, and prediction resistance can be turned on or
> off.
> ->
> Both reseeding and prediction resistance are supported.
OK.
>
> 226-228:
> When this object is used in a SecureRandom.getInstance() call, it means
> the requested minimum capability. When it's used in
> SecureRandom.getParameters(), it means the effective capability.
> ->
> When this object is passed to a SecureRandom.getInstance() call, it is
> the requested minimum capability. When it's returned from
> SecureRandom.getParameters(), it is the effective capability.
Good.
>
> 242: What would you think of renaming the variables "requested" and
> "effective" here and just getting rid of "(req)" and "(eff)" in the table
> header completely.
Sure.
>
> 354/355: I've noticed this in several files. There's a
> lot of spaces missing in your ? operator. For example,
> between "null?" and "null:". Another example:
> (isPr?"pr_and_reseed":(supportReseed?"reseed_only":"none")
>
> Should be:
> return (cond ? ret1 : ret2);
OK.
>
> 472:
> @throws NullPointerException if {@code capacity} is {@code null}.
> ->
> @throws NullPointerException if {@code capability} is {@code null}.
Oops.
>
>
> 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.
> I'm not crazy about the following verbiage,
> but something like:
>
> Some SecureRandom implementations might require additional
> configurational/operational parameters. Objects of classes which
> implement this interface can be passed to those implementations that
> support them.
Good.
>
>
> 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.
>
> 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.
>
> 94: A final constant string should be cap'd. DEFAULT STRENGTH
Good.
>
> 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.
>
> 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. I can add an explanation.
>
> 235:
> The prediction resistance flag used by this instance.
> ->
> The entropySource used by this instance.
Oops.
>
> 348: It would be nice if you could provide a mapping comment to the steps in
> 800-90Ar1. e.g.
>
> // 3. If requested_security_strength > the security_strength
> // indicated in the internal state, then return
> if (dp.getStrength() > strength) {
> throw new IllegalArgumentException("strength too high: "
> + dp.getStrength());
> }
>
> // 4. If the length of the additional_input > max_additional...
> // ...
> byte[] ai = dp.getAdditionalInput();
> if (ai != null && ai.length > maxAiLength) {
>
> It looks like you do that around line 650 with the Instantiate procedure
> and in other parts of the code, but not here and some other place IIRC.
Sure.
>
> 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.
>
> 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.
*** 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.
>
> 460: would you want to call security "min_entropy" to match the spec?
Sure, and I will change min/max there to minLength/maxLength.
>
> 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.
>
> 605: It would be nice if you could provide a mapping to the steps in
> 800-90Ar1.
Good.
>
> 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().
>
> 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.
>
> 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.
>
> sun/security/provider/AbstractHashDrbg.java
> ===========================================
> 129: Can you make this into a single debug output line? Otherwise you
> run the risk of interspersed data. We have this problem in JSSE all the
> time.
>
I can.
>
> sun/security/provider/ByteArrayAccess.java
> ==========================================
> Ok.
>
>
> sun/security/provider/CtrDrbg.java
> ==================================
> By 8.3/8.5, I don't think outputting V/C is allowed, even in debug.
> Please recheck this.
>
> 40: Constants should be cap'd. Also, can this be private?
Yes and yes.
>
> 51: transient here?
*** Already transient.
>
> 54: Can this be private?
Yes.
>
> 88: Add @overrides?
Yes.
>
> 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.
>
> 164: TODO?
*** In Table 3 of 800-90Ar1, ctrLen can be between 4 and blocklen. I was not
sure if I can hard code it.
>
> 355: Couldn't addBytes() here and in HashDrbg be refactored into a
> common routine in AbstractDrbg?
I think so.
I spent some time optimizing HashDrbg and has not touched CtrDrbg.
>
> 384: Isn't additional_input supposed to be 0^seedlen if AI == null?
Yes, see 414.
Maybe I didn't want to touch the input argument. But since I already changed
them on 379/381 I can do it.
>
> 422: What is this for?
SecretKeySpec for DESede use 24 bytes but k in CTR_DRBG is 21 bytes.
>
>
> sun/security/provider/DRBG.java
> ===============================
> 60: A final constant string should be cap'd. propName -> PROPANE
OK.
>
> 62: Is this serialVersionUID a placeholder, or the final value? I've
> noticed it in other places too.
See previous explanation.
>
> 93: space missing "part:"
Do we need a space here?
*** BTW, where is a modern coding style guide?
>
> 129: I mentioned this in a private email, but will mention it here
> for the record. There's a bug here in the parsing code. You go right
> to checking strength, instead of determining whether this part is an
> algorithm or a strength. Note if you reverse the order of algorithm
> and strength:
>
> Testing hash_drbg,128,sha-512,Pr_and_Reseed with null...
> Result NSAE
> Exception in thread "main" java.security.NoSuchAlgorithmException: Error
> constructing implementation (algorithm: DRBG, provider: SUN, class:
> sun.security.provider.DRBG)
> ...deleted...
> Caused by: java.lang.IllegalArgumentException: strength cannot be provided
> more than once in securerandom.drbg.config
>
> Caused by: java.lang.IllegalArgumentException: strength cannot be
> provided more than once in securerandom.drbg.config
Yes.
>
> 147: Under what conditions would you get a MoreDrbgParameters passed
> here? I'm guessing this is for testing? Please add a comment to
> indicate this.
Yes.
>
> 170: To avoid a double call here, might suggest:
>
> tmp = dp.getStrength();
> if (tmp != -1) {
> strength = tmp;
> }
Yes. I've explained why I did that in a previous comment.
>
>
> sun/security/provider/EntropySource.java
> ========================================
> 37/43: Do you want to call this parameter "security_strength" to be
> consistent with 800-90A?
OK.
>
>
> sun/security/provider/HashDrbg.java
> ===================================
> By 8.3/8.5, I don't think outputting V/C is allowed, even in debug.
> Please recheck this.
>
> 59: Should this check even be here, or maybe a RuntimeException? By this
> point, we should have a valid value here. Otherwise, instantiateAlgorithm
> is going to choke later on.
Yes you are right, no need to check.
Before the getInstance(alg,params) change I allow a separate configure() call,
and before this method or nextBytes is called, algorithm is null. If a DRBG
instance is serialized and deserialized initEngine() should not try to init an
engine. This is no more true now.
>
> 136: I really don't think you should be outputing V/C here. See comment
> at top.
>
> 154: space style.
Sometimes I think too many spaces do not look good. I'll be strict from now on.
I promise.
>
> 197: A quick comment here would be nice.
Is it necessary?
*** In fact, I am wondering if I should use long, otherwise reseedCounter could
be negative.
>
>
> 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.
>
>
> sun/security/provider/MoreDrbgParameters.java
> =============================================
> 46:
> the {@link EntropySource} to use, set to {@code null}
> and a default entropy...
>
> Did you mean:
>
> the {@link EntropySource} to use. If set to null, a default...
Yes.
>
> 49/51: Same comment.
OK.
>
> 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.
>
>
> sun/security/provider/SHA5.java
> ===============================
> Ok.
>
> I verified the initial values were copied correctly.
>
>
> sun/security/provider/SunEntries.java
> =====================================
> OK. OID values are correct.
>
>
> sun/security/util/Debug.java
> ============================
> Ok
>
>
> com/sun/crypto/provider/HmacCore.java
> =====================================
> OK.
>
>
> 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 didn't get too much into the tests, but I didn't see much exercising
> of the actual DRBG's new functions. nextBytes(..., SRP), reseed(SRP),
> etc. There were some that exercised the interface, but not the actual
> new code Did I miss them?
In test/closed.
>
>
> test/java/security/SecureRandom/Serialize.java
> ==============================================
>
>
> test/com/sun/crypto/provider/Mac/HmacSHA512.java
> ================================================
>
>
> test/sun/security/provider/MessageDigest/SHA512.java
> ====================================================
>
>
> test/sun/security/provider/SecureRandom/AbstractDrbgSpec.java
> =============================================================
> 255:
> maximum strength is 192
> ->
> Maximum strength is 128
Yes.
>
>
> test/sun/security/provider/SecureRandom/AutoReseed.java
> =======================================================
>
>
> test/sun/security/provider/SecureRandom/CommonSeeder.java
> =========================================================
>
>
> test/sun/security/provider/SecureRandom/DRBGAlg.java
> ====================================================
>
>
> test/sun/security/provider/SecureRandom/SelfSeed.java
> =====================================================
>
>
> test/sun/security/provider/SecureRandom/StrongSeedReader.java
> =============================================================
> OK
>
Thanks again.
--Max
>
