RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations
Bradford Wetmore
bradford.wetmore at oracle.com
Sun Apr 24 23:40:35 UTC 2016
I haven't followed much of the discussion between you/Xuelei, I will
hopefully look at this tomorrow. I just started with webrev.11 and ran
heads down with it for the last few days.
Comments attached.
Brad
On 4/21/2016 1:52 AM, Wang Weijun wrote:
> Hi All
>
> Webrev updated again at
>
> 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
>
> Changes since webrev.10:
>
> - DRBG's generateSeed() will directly read from securerandom.source and does not have the synchronized modifier anymore. This is the same as SHA1PRNG.
>
> - new Capability methods boolean supportsReseeding() and boolean supportsPredictionResistance()
>
> - The "drbg" security property is renamed to "securerandom.drbg.config".
>
> - SecureRandom#nextBytes(byte[]) will no longer throw an NPE. It's up to a provider to decide what to do. Currently, SunPKCS11 ignores it and all others throw an NPE.
>
> - Move spec in Instantiate#getCapability to Capability.
>
> - Internal nonce provider upgraded from a long to 16 bytes.
>
> - "DRBG:SUN" added as a fallback for "securerandom.strongAlgorithms" security property on *nix
>
> - Rename engineConfigure() to configure() in AbstractDrbg, and merge configureInternal() into it.
>
> - Provider#newInstanceUtil will throw NoSuchMethodException (instead of IAE) if a SecureRandom implementation has not provided a constructor with a params parameter and user is calling getInstance(alg,params). This is just an internal change since the exception thrown by getInstance() is always be NoSuchAlgorithmException.
>
> - More information in exceptions
>
> - Other refactoring
>
> Thanks
> Max
>
-------------- next part --------------
Overall:
========
I apologize if some of the comments ended up on the wrong line, or my
wording is unclear. I ran out of time to double check them before I
had to leave today.
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.
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.
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.
---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 didn't see any health tests. What is your plan for that?
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.
181:
mechanisms, Each
->
mechanisms. Each
186:
Applications can request for different instantiation parameters
->
Applications can request different instantiation parameters
212-213: Still talks about 800-90 instead of the Sun provider document.
I notice the particular value of "3KeyTDEA" is mentioned later in this
file, but not the other string values.
java/security/Provider.java
===========================
Copyright date.
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.
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.
74:
The effective instantiate parameters used in the instantiation must match
->
The implementation's effective instantiated parameters must match
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.
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.
110:
Please note that {@code reseed} is not always supported.
->
Please note that {@code reseed} may not be supported by all SecureRandom
implementations.
230/653: Just curious why you made this change. Seems like it just adds an
extra stack frame.
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.
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.
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.
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.
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?
XXX: what do you do bout large size requests or things that can't be
reseeded?
752: I don't think this addition made it into your CCC.
java/security/SecureRandomSpi.java
==================================
See comment in SecureRandom:592.
java/security/DrbgParameters.java
=================================
The current document is 800-90Ar1 but you later mention the older 800-90A in
various places? Should probably be consisent?
47:
A DRBG implementation has a configuration, including but not limited to,
->
A DRBG implementation has a configuration, including but not limited to:
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
56:
* <li> highest security strength
->
* <li> highest security strength.
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.
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
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...
81:
contain a constructor that has a
->
contain the 1-arg SecureRandomParameter constructor that takes a
141:
DrbgParameters.instantiate(112, NONE, null);
->
DrbgParameters.instantiate(112, NONE, null));
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.
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")
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.
In java.security, you call it "3KeyTDEA", but here the string is
"DESede". Which is it?
Reseeding is supported, and prediction resistance can be turned on or
off.
->
Both reseeding and prediction resistance are supported.
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.
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.
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);
472:
@throws NullPointerException if {@code capacity} is {@code null}.
->
@throws NullPointerException if {@code capability} is {@code null}.
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? 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.
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.
64: Is this serialVersionUID a placeholder, or the final value?
94: A final constant string should be cap'd. DEFAULTSTRENGTH
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.
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?
235:
The prediction resistance flag used by this instance.
->
The entropySource used by this instance.
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.
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?
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?
460: would you want to call security "min_entropy" to match the spec?
483: Can't this be final? Netbeans is complaining about a non-final
defaultES at 503. Also private?
551: What is register() used for?
605: It would be nice if you could provide a mapping to the steps in
800-90Ar1.
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'?
674: can be final. Do you want to use the ES to generate an initial
value for the monotonically increasing sequence number?
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.
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.
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?
51: transient here?
54: Can this be private?
88: Add @overrides?
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.
164: TODO?
355: Couldn't addBytes() here and in HashDrbg be refactored into a
common routine in AbstractDrbg?
384: Isn't additional_input supposed to be 0^seedlen if AI == null?
422: What is this for?
sun/security/provider/DRBG.java
===============================
60: A final constant string should be cap'd. propName -> PROPNAME
62: Is this serialVersionUID a placeholder, or the final value? I've
noticed it in other places too.
93: space missing "part:"
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
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.
170: To avoid a double call here, might suggest:
tmp = dp.getStrength();
if (tmp != -1) {
strength = tmp;
}
sun/security/provider/EntropySource.java
========================================
37/43: Do you want to call this parameter "security_strength" to be
consistent with 800-90A?
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.
136: I really don't think you should be outputing V/C here. See comment
at top.
154: space style.
197: A quick comment here would be nice.
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.
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...
49/51: Same comment.
48: if null, does a default get selected?
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.
------------
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?
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
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
More information about the security-dev
mailing list