Design review: JEP 273: DRBG-Based SecureRandom Implementations

[Wang Weijun]

> On Nov 21, 2015, at 6:46 AM, Anthony Scarpino <anthony.scarpino at oracle.com> wrote:
> 
> On 11/18/2015 05:32 AM, Sean Mullan wrote:
>> The getInstance methods can now take a SecureRandomParameterSpec object
>> (rather than an AlgorithmParameterSpec). They should throw
>> InvalidAlgorithmParameterException (not IllegalArgumentException) if the
>> parameters are null or not the right type to be consistent with other
>> Spi classes.
> 
> Sorry for backtracking a bit, but when I look at the original proposal by Max,
> 
>   SecureRandom sr = SecureRandom.getInstance("HashDRBG");
>   hd.configure(new DrbgSpec("SHA-512", -1, true, false,
>         nonce, person), new MyEntropyInput());
> 
> I seem something like Cipher, where getInstance is getting the transformation and configure is like Cipher.init().  The contents of DrbgSpec do look like AlgorithmParameters, but in the context during the init.  I'm not sure moving all of this into getInstance is consistent with something like Cipher.
> 
> I'm not sure such things like the algorithm, strength, nonce, etc are right to be set via getInstance(). If it's possible to have limitations in support of a particular DRBG, where it only supported SHA256 but not SHA512 (for example, I don't know the spec), then maybe it does make sense to be a getInstance.

DrbgSpec (or whatever it will be called) is different for different DRBG algorithms. For example, for HashDRBG you specify a hash algorithm like SHA-256, for CtrDRBG, you specific a cipher like AES-256.

> 
> In the original design with configure(), was it possible to reconfigure?  Or was one to just throw away the instance and get a new one or reset()?

A reconfigure is like a reset because all internal states need to be reconstructed.

> 
> > nextBytes should throw NPE if bytes is null. I also think it should
> > not allow additionalInput to be null (should throw NPE), because this
> > is an overloaded method. If the caller doesn't want to specify
> > additionalInput, then it should call nextBytes(byte[]).
> 
> I don't think there should be a nextBytes(byte[] bytes, byte[] additionalInput).  If the user wants to have additionalInput they should use reseed(byte[]) only like you specified below.  I think having nextBytes() also doing reseeding is confusing.

According to SP 800-90A, this additional input is not about entropy. People might not find this method extra useful but at least it is needed to test the correctness of implementation. CAVP tests include cases for it.

That said, I re-read SP 800-90A again and seems there is a feature I haven't supported yet. The full generate function is

   Generate_function (state_handle, requested_number_of_bits, requested_security_strength,
     prediction_resistance_request, additional_input):

So requested_security_strength and prediction_resistance_request can be specified for each generate call. I think this is useful if people want to use a single SecureRandom object to generate different purposes of random bits. CAVP does not include this case so I haven't noticed this at the beginning.

Looks like a

   nextBytes(bytes, DrbgSpec)

can fulfill this purpose. This will also cover the nextBytes(bytes, additionalInput) case.

> 
> > I would prefer 2 reseed methods, one that didn't take any arguments,
> > and one that takes an additionalInput (and throws NPE if it is null):
> >
> >  public void reseed()
> >  public void reseed(byte[] additionalInput)
> 
> I agreed.

Thanks
Max

> 
> Tony
> 
>