RFR [14] JDK-8226374 Restrict signature algorithms and named groups

[Xuelei Fan]

New webrev:
http://cr.openjdk.java.net/~xuelei/8226374/webrev.03/

On 7/25/2019 11:36 AM, Sean Mullan wrote:
> On 7/25/19 11:45 AM, Xuelei Fan wrote:
>> I updated the CSR and webrev per the review comments accordingly.
>>
>> webrev: http://cr.openjdk.java.net/~xuelei/8226374/webrev.02/
> 
> Not done reviewing yet, but here are a couple of comments so far:
> 
> * test/jdk/sun/security/ssl/CipherSuite/RestrictNamedGroup.java
> 
>    96             } catch (Exception exp) {
>    97                 continue;
>    98             }
> 
> Can you check for an expected exception here? Same comment on lines 
> 111-113 of RestrictSignatureScheme.java
> 
As the test is a client-server structure, the exception may be different 
for each run (client or server which thread dies first).  I updated to 
use "SSLException | IllegalStateException".  SSLException is the general 
exception, but IllegalStateException is the exception thrown by client 
or server thread when it dies and failed to wrap the SSLException.  The 
normal case for the signature schemes and named groups should have been 
covered by other test cases, it may not worthy to analysis the exception 
stack for exactly exception, which is not reliable if the exception 
stack get changed in the future.


> * src/java.base/share/classes/sun/security/ssl/ECDHServerKeyExchange.java
> 
> 114             if ((namedGroup == null) || (!namedGroup.isAvailable)) {
> 
> You don't do this check for null and isAvailable in other places, for 
> example ECDHClientKeyExchange.ECDHEClientKeyExchangeConsumer.consume() - 
> should you?
> 
Good point!  Currently, the restriction is only checked for the 
supported group extension.  I should add more check points in other 
places where named groups are used, for example client key exchange and 
certificate.  Stay tune for the next webrev.


>> CSR:    https://bugs.openjdk.java.net/browse/JDK-8227445
> 
> In the CSR, the Notes 1-3 are very similar. I only think you need to 
> mention that the names will be standardized in a subsequent CSR - and 
> maybe you can point or add a link to the issue that has been filed for 
> that. I don't think you need the other 2 notes.
> 
Updated.

Thanks,
Xuelei

> --Sean
> 
>>
>> Thanks,
>> Xuelei
>>
>> On 7/12/2019 9:14 AM, Xuelei Fan wrote:
>>> On 7/12/2019 5:24 AM, Sean Mullan wrote:
>>>> On 7/11/19 11:56 AM, Xuelei Fan wrote:
>>>>
>>>>>> Also, in the CSR you list all the different signature algorithms 
>>>>>> that could be disabled, but you use the TLS names, and not the 
>>>>>> standard JCE names. I found this a bit confusing, since if you 
>>>>>> added those exact TLS names to jdk.tls.disabledAlgorithms, I don't 
>>>>>> think it will work, or if it does we need additional changes to 
>>>>>> the jdk.tls.disabledAlgorithms definition - and maybe that is what 
>>>>>> we should do?  Also, I don't think it is possible to disable 
>>>>>> individual RSASSA-PSS algorithms, I think you can just disable all 
>>>>>> or none of them because the parameters are specified separately 
>>>>>> and not part of the standard JCE name. Similar to other algorithms 
>>>>>> - how would I just disable ecdsa_secp256r1_sha256 and nothing 
>>>>>> else? Is that an issue?
>>>>>>
>>>>> Yes, it is an issue now.  The AlgorithmConstraints is able to 
>>>>> accept parameters, but the current jdk.tls.disabledAlgorithms 
>>>>> property cannot. That's also why I used the TLS names 
>>>>> (ecdsa_secp256r1_sha256, rsa_pss_rsae_sha256, etc) rather than 
>>>>> standard names (SHA256withECDSA, RSASSA-PSS).
>>>>>
>>>>> I agree with you that it is confusing.  The use of 
>>>>> rsa_pss_rsae_sha256 may be fine, but the name "dsa_sha256" rather 
>>>>> then "SHA256withDSA" could be confusing.
>>>>>
>>>>> I was planned to add TLS signature algorithms into the standard 
>>>>> names, as we will do for the named groups.  But it looks like a 
>>>>> duplicate of the crypto Signature algorithms.
>>>>
>>>> I would lean towards this option. We could extend the 
>>>> jdk.tls.disabledAlgorithms property to allow you to specify TLS 
>>>> signature schemes as defined in 
>>>> https://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-signaturescheme 
>>>>
>>>>
>>>> We would also need to add a section to the Standard Names 
>>>> specification listing these schemes.
>>>>
>>>> The reason I like this option is because it fits well with the TLS 
>>>> namespace, and it is more flexible than the JCE names and we can 
>>>> more easily restrict new TLS signature schemes that are defined later.
>>> I agreed that is is more straightforward.  Okay, let's go with option.
>>>
>>> Between file a new enhancement for Standard Names documentation or 
>>> update the doc this time, do you have a preference?  Maybe, we can 
>>> update the doc together with JDKJDK-8210755?
>>>     https://bugs.openjdk.java.net/browse/JDK-8210755
>>>
>>> Thanks,
>>> Xuelei