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

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

2013-06-30 Thread Brad Wetmore


Bernd,

I also think this is an excellent suggestion.  Glad that you made it and 
Xuelei is considering it.  Sorry I didn't get a chance to respond before 
I left for the weekend.  There will be a bit of work to do in the 
current code to support abandoning a handshake, but shouldn't be too bad.


Brad


On 6/27/2013 5:51 PM, Xuelei Fan wrote:

On 6/28/2013 8:05 AM, Bernd Eckenfels wrote:

Am 28.06.2013, 01:51 Uhr, schrieb Xuelei Fan xuelei@oracle.com:

Please don't send a no_renegotiation warning alert. Warning message is
not very useful because in general the sending party cannot know how the
receiving party behave.  The server side need to reject client initiated
renegotiation proactively.


Just for the record, I totally disagree. I would make the option a multi
value like accept(default)|ignore|reject. Because you never can know
how the other side reacts. Ignoring renego requests is totally safe in
the spec and in a situation where you chose to turn off renogotiation by
clients you can have only two things:

a) clients continue to work when you ignore them
b) clients break

If you always terminate the connection there is no chance for some
clients to keep working.


Great suggestion.  I will consider to add the new ignore option.


Today you can already achieve the termination of connection (by
disabling all ciphersuites after initial handshake). You dont need to
add code if you dont offer more (i.e. ignore) options.


You're right here.  This new system property is just to make the work a
little easier that developers won't need to touch the source code in
applications.


Greetings
Bernd

PS: and regarding the naming a question, is JSSE the name of the Sun
implementaion or of the Specification?

I intend to use it for specification. But the names in JSSE is a little
confusing now (See Brad's response). We need to consolidate the naming
style.

Xuelei

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

2013-06-27 Thread 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@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

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

2013-06-27 Thread Brad Wetmore


continued, I forgot this next part.


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.


Exactly.


This TLS logic decision is not straightforward, so this needs commenting.


And the above is what I wanted to see in the comments.  That is, why we 
don't send a no_renegotiation warning alert.  It's a subtle but 
important enough point that should be documented.  I think we should 
open a separate bug to handle this.  Just a couple of lines are needed.



I think reject client initialized renegotiation has say all. ;-) I
will add words about state != HandshakeMessage.ht_hello_request.


Different comment.

Brad

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

2013-06-27 Thread Xuelei Fan

On 6/28/2013 6:44 AM, Brad Wetmore wrote:
 continued, I forgot this next part.
 
 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.
 
 Exactly.
 
 This TLS logic decision is not straightforward, so this needs
 commenting.
 
 And the above is what I wanted to see in the comments.  That is, why we
 don't send a no_renegotiation warning alert.  It's a subtle but
 important enough point that should be documented.  I think we should
 open a separate bug to handle this.  Just a couple of lines are needed.
 
What do you think these words:

Please don't send a no_renegotiation warning alert. Warning message is
not very useful because in general the sending party cannot know how the
receiving party behave.  The server side need to reject client initiated
renegotiation proactively.

Thanks,
Xuelei

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

2013-06-27 Thread 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.