Code review request, 7188658 Add possibility to disable client initiated renegotiation

[Xuelei Fan]

I agree that the property name is pretty confusing now.  We need to
consolidate the property naming styles, at least in JSSE component. I
will go back to this topic later.

Xuelei

On 6/28/2013 6:36 AM, Brad Wetmore wrote:
> Going back to this...
> 
> On 6/18/2013 9:04 PM, Xuelei Fan wrote:
>> On 6/19/2013 10:50 AM, Brad Wetmore wrote:
>>> Sorry for the delay.
>>>
>>> Two comments about the property name.
>>>
>>> I don't see the jdk.tls *System* Property prefix used anywhere else,
>>> except as a *Security* properties:
>>>
>>>      jdk.tls.rejectClientInitializedRenego
>>>
>>> Otherwise, I think we should stick to one of the current namespace.
>>>
>>>      jsse.               (my preference since this is expected across
>>>                           other JDKs)
>>>      sun.security.ssl.
>>>
>> At the beginning, I use the prefix "jsse.*". But it is rejected (CCC)
>> because the system property has no effect on other providers.
> 
> ...and...
> 
>> "jsse" prefix
>> should not be used in provider level libraries (for example, Oracle
>> JSSE provider).
> 
> I'm not following the logic at all here.  I think I'm hearing that:
> 
>     jsse.*:    used by the javax.net.ssl.* code
>     jdk.tls.*: used by the sun.security.ssl.* code
> 
> But that doesn't quite make sense since we already have the following
> SunJSSE-specific properties:  jsse.enableSNIExtension,
> jsse.SSLEngine.acceptLargeFragments, jsse.enableCBCProtection.
> 
> For the javax.net.ssl.* code, we already have been using since very
> early on:
> 
>     ssl.*:  used by the javax.net.ssl.* code.  e.g.
>             KeyManagerFactory, TrustManagerFactory,
>             ServerSocketFactory.provider, SocketFactory.provider
> 
> And then there are the other sun.security.ssl.* and javax.net.ssl.* for
> SunJSSE providers that we expect other providers will use:
> 
>     sun.security.ssl.*
>             allowUnsafeRenegotiation, allowLegacyHelloMessages
> 
>     javax.net.ssl.*
>             keyStore, keyStorePassword, etc.
>             trustStore, trustStorePassword, etc.
> 
> And now we're adding yet another naming scheme?  If it weren't for:
> 
> 
> http://docs.oracle.com/javase/7/docs/technotes/guides/security/jsse/JSSERefGuide.html#Customization
> 
> 
> I'd be completely lost.
> 
> Brad
> 
> 
>> Now, two
>> styles are acceptable. "jdk.*" is for JDK properties, and "jsse.*" is
>> used when other parties also should follow it. "sun.security.ssl" is
>> deprecated, I think.
>>
>>> Regarding rejectClientInitializedRenego, I think the word "Initiated"
>>> more succinctly captures the intent of this property, rather than
>>> "Initalized."  And as long as the word "Renegotiation" is, it should
>>> probably be spelled out completely.  AFAIK, we rarely abbreviate
>>> external names like this.
>>>
>>> Same "initiated" comment about the variable name in your codereview, but
>>> I don't care much about Renogo.
>>>
>> I prefer "Initiated" to "Initalized".  Let's make the update in another
>> update.
> 
> Thanks for considering.
> 
>>> ServerHandshaker.java
>>> =====================
>>> 283:  My initial thought was a no_renegotiation(100) warning, but that
>>> allows the client to decide what to do, rather than the server
>>> terminating.
>>>
>> No, we cannot.  First of all, warning message is not very useful because
>> in general the sending party cannot know how the receiving party behave.
>>   Secondly, it is the expected behavior to *reject" client initiated
>> renegotiation. It is the server who should make the decision, but not
>> the client.
>>
>>> This TLS logic decision is not straightforward, so this needs
>>> commenting.
>>>
>> I think "reject client initialized renegotiation" has say all. ;-) I
>> will add words about "state != HandshakeMessage.ht_hello_request".
>>
>>
>>> 379:  since you're making changes here.
>>>
>>>    response->respond
>>>
>> OK.
>>
>>> Since you already have CCC approval and are ready to putback, I would
>>> suggest putting back now, and let's file another CCC to change the name.
>>>   That should be a no-brainer.
>>>
>> OK.
>>
>> Thanks,
>> Xuelei
>>
>>> Thanks!
>>>
>>> Brad
>>>
>>>
>>>
>>>
>>>
>>> On 5/29/2013 7:05 PM, Xuelei Fan wrote:
>>>> Got it. Yes, this fix is addressing a different issue from you
>>>> mentioned
>>>> below.
>>>>
>>>> Thanks,
>>>> Xuelei
>>>>
>>>> On 5/30/2013 9:53 AM, Bernd Eckenfels wrote:
>>>>> Am 30.05.2013, 02:18 Uhr, schrieb Xuelei Fan <xuelei.fan at oracle.com>:
>>>>>>> 2381456
>>>>>> Would you mind send me the link of the bug, or the code review
>>>>>> request
>>>>>> mail?  I may miss some mails about this direction.
>>>>>
>>>>> I am afraid I cant sent the link, the Bug is in review state and
>>>>> therefore not visible for me. It was acknowledged 2012-11-12, see
>>>>> attached. I guess the link would be
>>>>> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=2381456 (not
>>>>> sure if
>>>>> the numbers are the same in the new bug tool).
>>>>>
>>>>>> Good suggestion.  Oracle provider of JSSE had addressed the TLS
>>>>>> renegotiation issue in JDK 1.4.2 update 26, JDK 1.5.0 update 24
>>>>>> and JDK
>>>>>> 6u 19 around the end of 2009 and the beginning of 2010.  Here is the
>>>>>> readme of the fix:
>>>>>> http://www.oracle.com/technetwork/java/javase/documentation/tlsreadme2-176330.html.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Thats a different problem, I was thinking about preventing execessive
>>>>> client initiated renegotiations. This is for example CVE-2011-1473
>>>>> from
>>>>> THC.
>>>>>
>>>>>>> You mentioned industry will move to a secure handshake - are you
>>>>>>> aware of any initiative in that direction?
>>>>>>>
>>>>>> See http://www.rfc.org/rfc/rfc5746.txt.  As far as I know, nearly all
>>>>>> major vendors of SSL protocols has support RFC5746.
>>>>>
>>>>> Ok, but thats a different issue. I was expecting 7188658 to address
>>>>> another point, but I might be wrong.
>>>>>
>>>>> I understand that as of Oracle policy we cannot discuss it. Even if
>>>>> this
>>>>> is a very well known issue. :-/
>>>>>
>>>>> Greetings
>>>>> Bernd
>>>>
>>