RFR 8051408: JEP 273: DRBG-Based SecureRandom Implementations

[Seán Coffey]

A few comments from supportability side of the table.

=============
sun/security/provider/AbstractDrbg.java

> +            if (dp.getStrength() > strength) {
> +                throw new IllegalArgumentException("strength too high");
> +            }
> +            if (result.length > maxNbLength) {
> +                throw new IllegalArgumentException("result too long");
> +            }
Please print these bad strengths / results in the exception.

Similar corrections are needed in the engineConfigure method :

>   608                 if (config.getStrength() > highestSecurity) {
>   609                     throw new IllegalArgumentException("strength too big");
>   610                 }
>   611                 if (config.getPersonalizationString() != null && config.getPersonalizationString().length > maxPsLength) {
>   612                     throw new IllegalArgumentException("ps too long");
>   613                 }

> throw new IllegalArgumentException("unknown params type");
can you print the type of params that was passed in ? (X 2 calls)


> +            if (dp.getAdditionalInput() != null && dp.getAdditionalInput().length > maxAiLength) {
> +                throw new IllegalArgumentException("ai too long");
Please print ai value.


> +                    // This SEI does not support pr
> +                    throw new IllegalArgumentException();
Cou you put your comment in the body of the IllegalArgumentException ?

=============
sun/security/provider/CtrDrbg.java

+        try {
+            aesLimit = Cipher.getMaxAllowedKeyLength("AES");
+        } catch (Exception e) {
+            // should not happen
+            throw new AssertionError("Cannot detect AES");
+        }

Just to be safe, can you add e as Throwable variable for AssertionError ?


> +        if (input.length != seedLen) {
> +            // Should not happen
> +            throw new IllegalArgumentException("input must be of seedlen bytes");

can you print the lengths expected ?
=============
sun/security/provider/DRBG.java

> +                            if (strength < 0) {
> +                                throw new IllegalArgumentException(
> +                                        "strength in drbg cannot be negative");
> +                            }
Let's print the value of the 'part' string in this exception.

+            } else {
+                throw new IllegalArgumentException("Unsupported params");
+            }

can you print the type of params that were passed in ?

+            default:
+                throw new IllegalArgumentException("Unsupported mech");
+        }

can yuo print the mech value encoutered ?
=============
sun/security/provider/HashDrbg.java

> +            } catch (DigestException e) {
> +                throw new AssertionError("will not happen");
> +            }

Famous last words ;)
Can you add e as Throwable cause to AssertionError  ? (happens in two areas)

Regards,
Sean.

On 05/04/2016 03:34, Wang Weijun wrote:
> Updated webrev again at
>
>   http://cr.openjdk.java.net/~weijun/8051408/webrev.09/
>   http://cr.openjdk.java.net/~weijun/8051408/webrev.09/spec
>   http://cr.openjdk.java.net/~weijun/8051408/webrev.09/specdiff
>
> The only change is that SecureRandomInstantiateParameters, SecureRandomNextBytesParameters and SecureRandomReseedParameters are removed and only a single SecureRandomParameters is added. There seems no reason to introduce 3 marker interfaces.
>
> Thanks
> Max
>
>
>> On Apr 1, 2016, at 7:34 PM, Wang Weijun <weijun.wang at oracle.com> wrote:
>>
>> Hi All
>>
>> Updated webrev at
>>
>>   http://cr.openjdk.java.net/~weijun/8051408/webrev.08/
>>   http://cr.openjdk.java.net/~weijun/8051408/webrev.08/spec
>>   http://cr.openjdk.java.net/~weijun/8051408/webrev.08/specdiff
>>
>> Spec changes:
>>
>>   - More text in @implNote of DrbgParameters.java, which somehow matches the Minimal Documentation Requirements described in 11.1 of NIST SP 800-90Ar1.
>>
>>   - DrbgParameters.instantiate(strength,cap,ps) throws NPE if cap is null
>>
>>   - SecureRandom.java: no more @implSpec for new methods since impl is in SecureRandomSpi. Also, make the following word changes in all UOE cases:
>>
>>     - * @throws UnsupportedOperationException if the implementation
>>     - *         has not overridden this method.
>>     + * @throws UnsupportedOperationException if the underlying provider
>>     + *         implementation has not overridden this method.
>>
>> "drbg" security property changes:
>>
>>   - delimiter is now ",". Otherwise, "SHA-512/256" is ambiguous.
>>
>>   - AbstractDrbg#toString and DrbgParameters$Instantiate#toString also use "," now.
>>
>>   - default value is "", thus each aspect uses its own default as described in the comment.
>>
>>   - examples
>>
>> Code changes:
>>
>>   - DRBG.java: more check for the "drbg" security property, one aspect cannot be set twice,
>>     and strength must be positive
>>
>>   - HashDrbg.java optimization
>>
>>     * Use MessageDigest#digest(output,offset,length) instead of digest()+arraycopy.
>>       (BTW, why is DigestException a checked exception?) --  a little useful
>>
>>     * addBytes() now updates its first argument, therefore less round of adding -- very useful
>>
>>     * store "new byte[1]" and "new byte[]{1}" as constants -- a little useful
>>
>>   - MoreDrbgParameters: now including mech, so it can fully cover the "drbg" security property.  It still includes non-publicly configurable items like entropy source and nonce, that the DRBG (known-answer) Test Vectors require.
>>
>> Thanks
>> Max
>>