Code Review Request 8072452 Support DHE sizes up to 8192-bits

Xuelei Fan xuelei.fan at oracle.com
Fri Apr 15 01:41:28 UTC 2016 

Good points!  I made the update accordingly.

   http://cr.openjdk.java.net/~xuelei/8072452/webrev.04/

Thanks,
Xuelei

On 4/15/2016 1:33 AM, Seán Coffey wrote:
> without sounding like a broken record(!), can I ask that new exceptions
> provide better information (where possible) ?
> 
> com/sun/crypto/provider/DHKeyPairGenerator.java
> 
>> +            throw new InvalidParameterException(
>> +                    "Keysize must be multiple of 64, and can only
>> range " +
>> +                    "from 512 to 8192 (inclusive)");
> This is a new checkKeySize method - we should print what keysize was
> attempted in the exception message! Granted - the code was borrowed from
> old code but hopefully you can improve while here.
> 
> likewise in com/sun/crypto/provider/DHParameterGenerator.java (print the
> keysize in InvalidParameterException)
> 
> Can we improve this exception message while here also  ?
>> +        if ((exponentSize <= 0) || (exponentSize >= primeSize)) {
>> +            throw new InvalidAlgorithmParameterException(
>> +                "Exponent size must be positive and less than modulus
>> size");
> 
>> +        } catch (Exception ex) {
>> +            throw new ProviderException("Unexpected exception: " + ex);
> 
> Could we use the two arg constructor for ProviderException above and
> pass ex as Throwable ?
> 
> ===
> 
> sun/security/provider/DSAParameterGenerator.java
>> +                ("Prime size should be 512 - 1024, or 2048, 3072");
> 
> again, could we print the strength variable in above exception ?
> 
> ===
> 
> sun/security/ssl/ServerHandshaker.java
> 
>>                       throw new IllegalArgumentException(
>> -                        "Customized DH key size should be positive
>> integer " +
>> -                        "between 1024 and 2048 bits, inclusive");
>> +                        "Customized DH key size should not be less " +
>> +                        "than 1024 bits");
> 
> Can we print customizedDHKeySize value ?
> 
> ===
> 
> sun/security/pkcs11/P11KeyPairGenerator.java
> 
> Let's get the keySize printed in all InvalidAlgorithmParameterException
> calls that you're editing.
> 
> Thanks,
> Sean.
> 
> On 14/04/2016 12:12, Xuelei Fan wrote:
>> Thanks for the comments.  Here is the new webrev:
>>      http://cr.openjdk.java.net/~xuelei/8072452/webrev.03/
>>
>> On 4/14/2016 7:25 AM, Valerie Peng wrote:
>>> Here are some comments for the test changes:
>>>
>>> <test/sun/security/pkcs11/KeyPairGenerator/TestDH2048.java>
>>> - Not testing key size 768?
>>> - line 82, would be useful to indicate version here, e.g. NSS (as of
>>> version xxx) has a hard coded ...
>>> - line 27, 4096 should be 8192?
>>>
>> Updated.
>>
>>> <test/sun/security/provider/DSA/TestAlgParameterGenerator.java>
>>> - format changes only? No test for (3072, 256)?
>>>
>> I removed the update of this file.
>>
>>> <test/com/sun/crypto/provider/KeyAgreement/UnsupportedDHKeys.java>
>>> - line 50, dhp8192(8191) looks like a typo?
>>> - line 66, do we really need to generate the key pair here?
>>> - line 71, seem redundant and potentially confusing as it's aligned
>>> differently comparing to line70?
>>>
>> Updated.
>>
>>> <test/sun/security/pkcs11/KeyAgreement/SupportedDHKeys.java>
>>> - line 63, why test for KeyAgreement here instead of KeyPairGenerator?
>>>
>> Better to use KeyPairGenerator.  Updated.
>>
>>> <test/sun/security/pkcs11/KeyAgreement/UnsupportedDHKeys.java>
>>> - line 53, dhp8192(8191) looks like a typo?
>>> - line 64, why test for KeyAgreement here instead of KeyPairGenerator?
>>> - line 75, do we really need to generate the key pair here?
>>> - line 80, seem redundant and potentially confusing as it's aligned
>>> differently comparing to line79?
>>>
>> Updated.
>>
>>> <test/sun/security/provider/DSA/SupportedDSAParaGen.java>
>>> - Use "Param" instead of "Para" in the test name "SupportedDSAParaGen"?
>> Hm, nice name.
>>
>>> - Just curious, have u tried the specified timeout value across
>>> different platforms?
>> Not really, I only tested on JPRT and the solaris-sparc desktop.  This
>> test may be intermittent timeout failure in some circumstances.  I added
>> the intermittent key tag.
>>
>>> - The file is new but the copyright year starts from 2012?
>>>
>> Updated.
>>
>>> As for the updated sources, all changes look fine. But while I was
>>> looking at
>>> src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java, I
>>> have some more questions:
>>> - line 145, do we need to check for upper limit here? It used to have
>>> 2048, should it be 8192 now? If yes, then line 1526 need to be enhanced
>>> too.
>> I was wondering to remove the limit in case third party's implementation
>> support more size.  Not necessary.  Updated and limited to 8192 bits.
>>
>>> - line 1551, now that more DH sizes are supported, shouldn't we make
>>> some changes here? Now it's either 1024 or 2048. The older impl on line
>>> 1550 uses 1024 as the lower limit and 2048 as the upper limit and will
>>> keep the value as is if it's in-between the limits. If there is a reason
>>> to keep 2048 as the upper limit, it'd be useful to add some comment here
>>> for clarification.
>>>
>> This is mainly for compatibility and interoperability impact.  Some old
>> deployed application may not support DH key size bigger than 2048.  I
>> added back the comments.
>>
>> Thanks,
>> Xuelei
>>
>>> Thanks,
>>> Valerie
>>>
>>> On 4/13/2016 7:23 AM, Xuelei Fan wrote:
>>>> Hi Valerie,
>>>>
>>>> All comments are good to me and accepted in the new webrev:
>>>>
>>>>     http://cr.openjdk.java.net/~xuelei/8072452/webrev.02/
>>>>
>>>> On 4/13/2016 8:26 AM, Valerie Peng wrote:
>>>>> Hi Xuelei,
>>>>>
>>>>> Mostly look good, just some comments in line...
>>>>>
>>>>> <src/java.base/share/classes/com/sun/crypto/provider/DHKeyPairGenerator.java>
>>>>>
>>>>>
>>>>>
>>>>> line 95-100, so essentially, we will use the built-in parameters as
>>>>> much
>>>>> as possible and if none available, we will try to generate the
>>>>> parameters for key sizes<= 1024, right? The current comment is a
>>>>> little
>>>>> hard to parse. Maybe we can just explain it as below?
>>>>>        // Use the built-in parameters (ranging from 512 to 8192) when
>>>>> available
>>>>>        this.params = ParameterCache.getCachedDHParameterSpec(keysize);
>>>>>       // Due to performance issue, only support DH parameters
>>>>> generation
>>>>>       // up to 1024 bits
>>>>>       if ((this.params == null)&&  (keysize>  1024)) {
>>>>>
>>>> Updated.
>>>>
>>>>> <src/java.base/share/classes/sun/security/provider/DSAParameterGenerator.java>
>>>>>
>>>>>
>>>>>
>>>>> - line 212, redundant?
>>>> Yes.  Updated.
>>>>
>>>>> <src/jdk.crypto.pkcs11/share/classes/sun/security/pkcs11/P11KeyPairGenerator.java>
>>>>>
>>>>>
>>>>>
>>>>> - line 277-303, the "params"argument for this particular method is
>>>>> always null for non-RSA algorithms based on the usage. It is commented
>>>>> on line 232 and several other places as indicated by the "// XXX
>>>>> sanity
>>>>> check params" comments. So, more code changes will be needed here.
>>>>> Either you can add another boolean hasParams and rely on that
>>>>> argument,
>>>>> or change the specified argument type from RSAKeyGenParameterSpec to a
>>>>> generic one and then cast it later when it is needed.
>>>>>
>>>> Good catch!  Updated per the latter proposal.
>>>>
>>>>> <src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java>
>>>>> - line 141 - 146 and line 1548 - 1554 are identical. Seems to me that
>>>>> they can be customized a little as they are for different variable
>>>>> assignment. The "customizedDHKeySize" variable on line 147 is from
>>>>> user,
>>>>> right? Maybe what you really mean is something like the following?
>>>>>       // DH parameter generation can be extremely slow, best to use
>>>>> one of
>>>>> the
>>>>>       // supported pre-computed DH parameters (see DHCrypt class)
>>>>> As for line 1548 - 1554, it can be as simple as:
>>>>>       // DH parameter generation can be extremely slow, make sure
>>>>> to use
>>>>> one of the
>>>>>       // supported pre-computed DH parameters (see DHCrypt class)
>>>> Updated.
>>>>
>>>>> - line 148, can customizedDHKeySize be 1024? The old code accepts
>>>>> it but
>>>>> the new code will error out. Is there a reason for rejecting 1024? The
>>>>> check only rejects "<1024" but yet the exception message says the
>>>>> value
>>>>> should be larger than 1024.
>>>>> Lastly, "should larger" =>  "should be larger" (line 150).
>>>>>
>>>> 1024 bits should not be rejected.  Updated the exception message.
>>>>
>>>> Thanks for the comments!
>>>>
>>>> Xuelei
>>>>
>>>>> I will look at the tests next and send u comments separately.
>>>>> Thanks,
>>>>> Valerie
>>>>> On 4/1/2016 8:53 AM, Xuelei Fan wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Please review this improvement update to support DHE sizes up to
>>>>>> 8192-bits and DSA sizes up to 3072-bits:
>>>>>>
>>>>>>      http://cr.openjdk.java.net/~xuelei/8072452/webrev.00
>>>>>>
>>>>>> This updated extends to support 3072-bits DH and DSA parameters
>>>>>> generation, and pre-computed DH parameters up to 8192 bits and
>>>>>> pre-computed DSA parameters up to 3072-bits.
>>>>>>
>>>>>> Thanks,
>>>>>> Xuelei
>

More information about the security-dev mailing list