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

[Brad Wetmore]

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
>>>
>