Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-03-02 Thread Mark Thomas 
On 01/03/2011 16:50, Filip Hanik - Dev Lists wrote:
 On 2/28/2011 5:27 PM, Mark Thomas wrote:
 On 01/03/2011 00:16, Filip Hanik - Dev Lists wrote:
snip/
 4. SSLAuthenticator should have a flag to fail directly without trying
 to reneg if the connector is misconfigured to avoid reneg for clients
 vulnerable to the man in the middle reneg attack
snip/
 I don't understand what you mean in point 4. Could you try and expand on
 that.
 Sure, a renegotiation with a non updated client, IIRC would bring
 CVE-2009-3555 SSL Man-In-The-Middle attack.
 Hence, some sysadmins should have the configuration option to only allow
 the initial handshake.
 Add in a flag that would say disableRenegotiation=true (or similar).
 Meaning, the only time the valve would work, is if the clientAuth=true
 in the connector.

Ah, got it. That should be doable. Need to be careful that the
SSLAuthenticator can correctly distinguish between the two cases but
there should be enough information around already without the need to
add another configuration option.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-03-01 Thread Filip Hanik - Dev Lists 

On 2/28/2011 5:27 PM, Mark Thomas wrote:

On 01/03/2011 00:16, Filip Hanik - Dev Lists wrote:

On 2/28/2011 4:49 PM, Mark Thomas wrote:

It isn't clear to me if you are voting -1

on the above commit, and the following commits. r1074675

Understood and agree those commits are broken. I'll get those backed out
shortly.


If you wish to do this, it should at least include:
1. input filters need to check if they retrieved the entire body
if only partial, why even attempt a reneg and make your thread hang for
soTimeout while it fails. this is another DoS scenario. the system knows
if it read the entire body or not. it's part of the protocol itself, no
need to rely on timeouts for a reneg to fail.

2. don't change the names of all the flags, since it makes the diffs so
much harder to review. just change the lines pertinent to the change.

3. implement rehandshake as simple as possible, by using the
handshake(...) and using its return code

4. SSLAuthenticator should have a flag to fail directly without trying
to reneg if the connector is misconfigured to avoid reneg for clients
vulnerable to the man in the middle reneg attack

5. SSLAuthenticator should be able to find out if the cert truly was
client-auth or if it came from another source. otherwise, putting
httpd/mod_jk in front of it, and I can bypass client-auth as the
document states is required

6. And if you want the most performant solution, instead of opening a
selector on the same thread, just call sslEngine.beginHandshake, add the
connection to the poller, and return from the call all together. this
way, the worker thread is not in use during a handshake, and it's done
in the poller just like the initial hand shake. this protects you from
slow clients using up threads. this is of course more complicated, so I
would not expect it in the first iteration.

I would say the other connectors would benefit from improvements in
1,4,5 as well.

I agree on all of those points (with a few questions - see below). My
current thinking is approaching it in this order.

Do 2 in a separate commit. The flag needs to be renamed to ease
confusion but a separate commit that does just that should be easy to
review.

Yes, that would be much better.

Address 3 for the NIO connector. That will bring it in line with BIO and
APR.

Fix 1 for all connectors.

I don't understand what you mean in point 4. Could you try and expand on
that.

Sure, a renegotiation with a non updated client, IIRC would bring
CVE-2009-3555 SSL Man-In-The-Middle attack.
Hence, some sysadmins should have the configuration option to only allow the 
initial handshake.
Add in a flag that would say disableRenegotiation=true (or similar).
Meaning, the only time the valve would work, is if the clientAuth=true in the 
connector.


Fix 5. I may re-word the Javadoc again. Doing the client cert validation
in httpd is valid.
But how do you know it took place in httpd? Sounds like adding httpd/mod_jk in the mixture, Tomcat makes an assumption that client-auth took 
place.

6 is definitely more complicated. I did try this before but gave up.
That was before I had anything working. It may well be easier to get
there from a working solution.

I can help you here. But I'd like the simple solution first.
The reason the NIO connector doesn't use individual selectors, is that on some systems with high concurrency, having too many selectors made 
the system puke.


Filip


Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



-
No virus found in this message.
Checked by AVG - www.avg.com
Version: 10.0.1204 / Virus Database: 1435/3473 - Release Date: 02/28/11





-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-28 Thread Filip Hanik - Dev Lists 

On 2/27/2011 4:30 AM, Mark Thomas wrote:

On 25/02/2011 20:16, Filip Hanik - Dev Lists wrote:

The simplest solution is, would be to use an individual selector.
Register the socket and issue a select() on the thread you are running on.
If you want to use a shared selector (like NIO does for reads and
writes) it requires a bit more logic.

I have implemented the simple solution and based on a quick test with
the Eclipse debugger the handshake now blocks while waiting for client data.

A review would be good since my understanding of NIO is not as good as
yours.
My initial recommendation is to pull out this change, and as default behavior, throw an exception if the SSLAuthenticator is trying to 
authenticate and the need-client-auth is not configured.


There is much complexity in implementing the renegotiation without a unit test case, as there are both application buffers and network 
buffers in the NIO implementation that will need to be tested more carefully.


So for the sake of not holding up releases, implement the exception case first, where you force the user to configure client authentication, 
until there is a configuration that we are more comfortable with.


best
Filip


Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



-
No virus found in this message.
Checked by AVG - www.avg.com
Version: 10.0.1204 / Virus Database: 1435/3473 - Release Date: 02/28/11





-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-28 Thread Filip Hanik - Dev Lists 

Just a little bit more on this.
I'm not seeing where SSLAuthenticator.java validates that the request came in 
on a SSL connection, and what if the SSL cert came from mod_jk.
I'm not sure what the requirements for CERT authentication is, but if it is that the cert MUST be validated against a trust store, then this 
valve, must make sure that the validation actually has taken place.


Filip


On 2/28/2011 11:06 AM, Filip Hanik - Dev Lists wrote:

On 2/27/2011 4:30 AM, Mark Thomas wrote:

On 25/02/2011 20:16, Filip Hanik - Dev Lists wrote:

The simplest solution is, would be to use an individual selector.
Register the socket and issue a select() on the thread you are running on.
If you want to use a shared selector (like NIO does for reads and
writes) it requires a bit more logic.

I have implemented the simple solution and based on a quick test with
the Eclipse debugger the handshake now blocks while waiting for client data.

A review would be good since my understanding of NIO is not as good as
yours.
My initial recommendation is to pull out this change, and as default behavior, throw an exception if the SSLAuthenticator is trying to 
authenticate and the need-client-auth is not configured.


There is much complexity in implementing the renegotiation without a unit test case, as there are both application buffers and network 
buffers in the NIO implementation that will need to be tested more carefully.


So for the sake of not holding up releases, implement the exception case first, where you force the user to configure client 
authentication, until there is a configuration that we are more comfortable with.


best
Filip


Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



-
No virus found in this message.
Checked by AVG - www.avg.com
Version: 10.0.1204 / Virus Database: 1435/3473 - Release Date: 02/28/11





-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org



-
No virus found in this message.
Checked by AVG - www.avg.com
Version: 10.0.1204 / Virus Database: 1435/3473 - Release Date: 02/28/11





-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-28 Thread Filip Hanik - Dev Lists 

Ok, I've done some research. My conclusion is that I'm -1 for this change.
My suggestion on the other thread, that we should simply throw an exception if the server is not configured to handle client-auth in the 
connector still stands.


Let me explain why:

1. The idea behind this code was that if the client is using SSL, the SSL request must have been authenticated against the trust store, 
meaning that the initial handshake did this.

2. If the server was not configured for client auth [1], the current 
implementation wants to do a renegotiation.
3. The current implementation tries to swallow up as much body content as 
possible, until maxPostSize has been filled up.

This breaks in the following use case:
1. Client Opens TCP Session
2. Client Completes Initial Handshake
3. Client Sends HTTP Headers and Body of Size N, where NmaxPostSize

When the server in this case has read the HTTP headers, it's already too late to renegotiate, since the body is already underway. The only 
time we could renegotiate, would be after the request had been read in its entirety. (There is a short window where this could happen, if 
the client had sent a 100-Continue header before the body was even started, but I'm excluding this use case for now.)


Since we can't read the entire body without discarding it, we can't implement 
this as the way it has been. Hence the -1.

Here is the proposed solution:
1. When SSLAuthenticator starts up, it should inspect configuration of the connector to make sure there is at least one connector with 
clientAuth=true in it

 Note: You need to decide what to do if there is only an AJP connector, do you 
trust that the webserver did client-auth on your behalf?

2. If a request comes in, and the system is not configured for client-auth, all a server can do is send a 400 Bad Request (or Not 
Authorized) back.
Note: It should also send a Connection:close so that we can close the connection without having to read the body. (See Rainer's email about 
draining implications)


3. We should not implement server initiated renegotiation at this point in time, since we can't do it in a way it would allow for the system 
to handle the above use case.


4. We should not implement renegotiation period on the NIO connector at this time, since it would still risk a vulnerability with clients 
that are not updated.


In short, the NIO code can safely be backed out. The solution is quite simple, 
and it does not involve modifying the connectors.


[1]
setNeedClientAuth(true);
http://download.oracle.com/javase/6/docs/api/javax/net/ssl/SSLEngine.html#setNeedClientAuth(boolean)


best
Filip

On 2/25/2011 1:16 PM, Filip Hanik - Dev Lists wrote:

This looks like a CPU spinning handshake to me.
The operation handshake(true, true); returns an IO interest to be registered 
with a selector.
If the client is slow here or misbehaving, you could end up in a end less loop, and hence we can have introduced a very simple DoS 
vulnerability here.


The simplest solution is, would be to use an individual selector. Register the 
socket and issue a select() on the thread you are running on.
If you want to use a shared selector (like NIO does for reads and writes) it 
requires a bit more logic.

I understand where you are going with this solution, and I can probably fix 
this today as I sit on the airplane on my way home.

best
Filip



On 02/25/2011 12:19 PM, ma...@apache.org wrote:

Author: markt
Date: Fri Feb 25 19:19:13 2011
New Revision: 1074675

URL: http://svn.apache.org/viewvc?rev=1074675view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=49284
Support SSL re-negotiation in the HTTP NIO connector
There is a fair amount of renaming in this patch. The real work is in the new 
rehandshake() method in the SecureNioChannel.

Modified:
 tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
 tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
 tomcat/trunk/java/org/apache/tomcat/util/net/NioChannel.java
 tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
 tomcat/trunk/webapps/docs/changelog.xml

Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java?rev=1074675r1=1074674r2=1074675view=diff

==
--- tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java 
(original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java Fri Feb 
25 19:19:13 2011
@@ -23,6 +23,8 @@ import java.nio.channels.SelectionKey;
  import java.util.Locale;
  import java.util.concurrent.Executor;

+import javax.net.ssl.SSLEngine;
+
  import org.apache.coyote.ActionCode;
  import org.apache.coyote.Request;
  import org.apache.coyote.RequestInfo;
@@ -42,7 +44,9 @@ import org.apache.tomcat.util.net.NioCha
  import org.apache.tomcat.util.net.NioEndpoint;

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-28 Thread Mark Thomas 
On 28/02/2011 22:50, Filip Hanik - Dev Lists wrote:
 Ok, I've done some research. My conclusion is that I'm -1 for this change.
 My suggestion on the other thread, that we should simply throw an
 exception if the server is not configured to handle client-auth in the
 connector still stands.
 
 Let me explain why:
 
 1. The idea behind this code was that if the client is using SSL, the
 SSL request must have been authenticated against the trust store,
 meaning that the initial handshake did this.
 2. If the server was not configured for client auth [1], the current
 implementation wants to do a renegotiation.
 3. The current implementation tries to swallow up as much body content
 as possible, until maxPostSize has been filled up.
 
 This breaks in the following use case:
 1. Client Opens TCP Session
 2. Client Completes Initial Handshake
 3. Client Sends HTTP Headers and Body of Size N, where NmaxPostSize
 
 When the server in this case has read the HTTP headers, it's already too
 late to renegotiate, since the body is already underway. The only time
 we could renegotiate, would be after the request had been read in its
 entirety. (There is a short window where this could happen, if the
 client had sent a 100-Continue header before the body was even started,
 but I'm excluding this use case for now.)
 
 Since we can't read the entire body without discarding it, we can't
 implement this as the way it has been. Hence the -1.

It isn't clear to me if you are voting -1 on the recent NIO change or on
how the BIO and APR connectors have implemented SSL renegotiation for
years. It is an accepted limitation of SSL renegotiation that you need
to buffer any request body and if the request body is too large
renegotiation will fail. httpd does this exactly the same way. [1]

The rare user for which the above is an issue has several workarounds
available:
1. Increase maxSavePostSize (also increases risk of DOS but may be
acceptable in some circumstances).
2. Set clientAuth=true on the connector as you suggest below.

 Here is the proposed solution:
 1. When SSLAuthenticator starts up, it should inspect configuration of
 the connector to make sure there is at least one connector with
 clientAuth=true in it
  Note: You need to decide what to do if there is only an AJP connector,
 do you trust that the webserver did client-auth on your behalf?
 
 2. If a request comes in, and the system is not configured for
 client-auth, all a server can do is send a 400 Bad Request (or Not
 Authorized) back.
 Note: It should also send a Connection:close so that we can close the
 connection without having to read the body. (See Rainer's email about
 draining implications)
 
 3. We should not implement server initiated renegotiation at this point
 in time, since we can't do it in a way it would allow for the system to
 handle the above use case.
 
 4. We should not implement renegotiation period on the NIO connector at
 this time, since it would still risk a vulnerability with clients that
 are not updated.

Such an approach would be a step backwards from what has been available
for BIO as far back as Tomcat 4 [2] and has been available for APR in
6.0.21 and 5.5.29 [3].

If the issue is that the current NIO implementation is broken, I agree
but I think it is fixable within the same limitations of the current
renegotiation support for BIO and APR. Given that the current patch is
heading in the right direction (hopefully) and that a fix shouldn't be
too hard, I'd rather leave it in and fix it rather than pull it out.
There is no huge rush at the moment for a 7.0.x release but I'd like to
get the 7.0.10 started by the end of this week. If NIO renegotiation
gets in the way then it can be reverted / commented out for now but I am
hopeful of getting this to a working state this week. With the pointers
you have provided on the current code, I think I can see how to get to a
working (in the same way as BIO  APR) solution. It is actually taking
longer to get the test cases working.

If the issue is that you are -1 for the approach of buffering the
request body prior to SSL renegotiation then I think that needs a wider
discussion before any changes are made since that would involving
removing functionality from the current connectors that is currently
being used successfully by the user community.

Mark


[1] http://httpd.apache.org/docs/2.2/mod/mod_ssl.html#sslrenegbuffersize
[2]
http://svn.apache.org/viewvc/tomcat/archive/tc4.1.x/trunk/connectors/http11/src/java/org/apache/coyote/http11/Http11Processor.java?view=annotate
(lines 1118 to 1138)
[3] https://issues.apache.org/bugzilla/show_bug.cgi?id=46950

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-28 Thread Filip Hanik - Dev Lists 

On 2/28/2011 4:49 PM, Mark Thomas wrote:

It isn't clear to me if you are voting -1

on the above commit, and the following commits. r1074675

If you wish to do this, it should at least include:
1. input filters need to check if they retrieved the entire body
if only partial, why even attempt a reneg and make your thread hang for soTimeout while it fails. this is another DoS scenario. the system 
knows if it read the entire body or not. it's part of the protocol itself, no need to rely on timeouts for a reneg to fail.


2. don't change the names of all the flags, since it makes the diffs so much 
harder to review. just change the lines pertinent to the change.

3. implement rehandshake as simple as possible, by using the handshake(...) and 
using its return code

4. SSLAuthenticator should have a flag to fail directly without trying to reneg if the connector is misconfigured to avoid reneg for clients 
vulnerable to the man in the middle reneg attack


5. SSLAuthenticator should be able to find out if the cert truly was client-auth or if it came from another source. otherwise, putting 
httpd/mod_jk in front of it, and I can bypass client-auth as the document states is required


6. And if you want the most performant solution, instead of opening a selector on the same thread, just call sslEngine.beginHandshake, add 
the connection to the poller, and return from the call all together. this way, the worker thread is not in use during a handshake, and it's 
done in the poller just like the initial hand shake. this protects you from slow clients using up threads. this is of course more 
complicated, so I would not expect it in the first iteration.


I would say the other connectors would benefit from improvements in 1,4,5 as 
well.

Filip



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-28 Thread Mark Thomas 
On 01/03/2011 00:16, Filip Hanik - Dev Lists wrote:
 On 2/28/2011 4:49 PM, Mark Thomas wrote:
 It isn't clear to me if you are voting -1
 on the above commit, and the following commits. r1074675

Understood and agree those commits are broken. I'll get those backed out
shortly.

 If you wish to do this, it should at least include:
 1. input filters need to check if they retrieved the entire body
 if only partial, why even attempt a reneg and make your thread hang for
 soTimeout while it fails. this is another DoS scenario. the system knows
 if it read the entire body or not. it's part of the protocol itself, no
 need to rely on timeouts for a reneg to fail.
 
 2. don't change the names of all the flags, since it makes the diffs so
 much harder to review. just change the lines pertinent to the change.
 
 3. implement rehandshake as simple as possible, by using the
 handshake(...) and using its return code
 
 4. SSLAuthenticator should have a flag to fail directly without trying
 to reneg if the connector is misconfigured to avoid reneg for clients
 vulnerable to the man in the middle reneg attack
 
 5. SSLAuthenticator should be able to find out if the cert truly was
 client-auth or if it came from another source. otherwise, putting
 httpd/mod_jk in front of it, and I can bypass client-auth as the
 document states is required
 
 6. And if you want the most performant solution, instead of opening a
 selector on the same thread, just call sslEngine.beginHandshake, add the
 connection to the poller, and return from the call all together. this
 way, the worker thread is not in use during a handshake, and it's done
 in the poller just like the initial hand shake. this protects you from
 slow clients using up threads. this is of course more complicated, so I
 would not expect it in the first iteration.
 
 I would say the other connectors would benefit from improvements in
 1,4,5 as well.

I agree on all of those points (with a few questions - see below). My
current thinking is approaching it in this order.

Do 2 in a separate commit. The flag needs to be renamed to ease
confusion but a separate commit that does just that should be easy to
review.

Address 3 for the NIO connector. That will bring it in line with BIO and
APR.

Fix 1 for all connectors.

I don't understand what you mean in point 4. Could you try and expand on
that.

Fix 5. I may re-word the Javadoc again. Doing the client cert validation
in httpd is valid.

6 is definitely more complicated. I did try this before but gave up.
That was before I had anything working. It may well be easier to get
there from a working solution.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-27 Thread Mark Thomas 
 On 25/02/2011 20:16, Filip Hanik - Dev Lists wrote:
 The simplest solution is, would be to use an individual selector.
 Register the socket and issue a select() on the thread you are running on.
 If you want to use a shared selector (like NIO does for reads and
 writes) it requires a bit more logic.

I have implemented the simple solution and based on a quick test with
the Eclipse debugger the handshake now blocks while waiting for client data.

A review would be good since my understanding of NIO is not as good as
yours.

Mark

-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org

svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-25 Thread markt 
Author: markt
Date: Fri Feb 25 19:19:13 2011
New Revision: 1074675

URL: http://svn.apache.org/viewvc?rev=1074675view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=49284
Support SSL re-negotiation in the HTTP NIO connector
There is a fair amount of renaming in this patch. The real work is in the new 
rehandshake() method in the SecureNioChannel.

Modified:
tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
tomcat/trunk/java/org/apache/tomcat/util/net/NioChannel.java
tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
tomcat/trunk/webapps/docs/changelog.xml

Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java?rev=1074675r1=1074674r2=1074675view=diff
==
--- tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java 
(original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java Fri Feb 
25 19:19:13 2011
@@ -23,6 +23,8 @@ import java.nio.channels.SelectionKey;
 import java.util.Locale;
 import java.util.concurrent.Executor;
 
+import javax.net.ssl.SSLEngine;
+
 import org.apache.coyote.ActionCode;
 import org.apache.coyote.Request;
 import org.apache.coyote.RequestInfo;
@@ -42,7 +44,9 @@ import org.apache.tomcat.util.net.NioCha
 import org.apache.tomcat.util.net.NioEndpoint;
 import org.apache.tomcat.util.net.NioEndpoint.KeyAttachment;
 import org.apache.tomcat.util.net.SSLSupport;
+import org.apache.tomcat.util.net.SecureNioChannel;
 import org.apache.tomcat.util.net.SocketStatus;
+import org.apache.tomcat.util.net.jsse.JSSEFactory;
 
 
 /**
@@ -625,8 +629,25 @@ public class Http11NioProcessor extends 
 .setLimit(maxSavePostSize);
 inputBuffer.addActiveFilter
 (inputFilters[Constants.BUFFERED_FILTER]);
+
+SecureNioChannel sslChannel = (SecureNioChannel) socket;
+SSLEngine engine = sslChannel.getSslEngine();
+if (!engine.getNeedClientAuth()  
!engine.getWantClientAuth()) {
+// Need to re-negotiate SSL connection
+engine.setNeedClientAuth(true);
+try {
+sslChannel.rehandshake();
+sslSupport = (new JSSEFactory()).getSSLSupport(
+engine.getSession());
+} catch (IOException ioe) {
+
log.warn(sm.getString(http11processor.socket.sslreneg,
+ioe));
+}
+}
 try {
-Object sslO = sslSupport.getPeerCertificateChain(true);
+// use force=false since re-negotiation is handled above
+// (and it is a NO-OP for NIO anyway)
+Object sslO = sslSupport.getPeerCertificateChain(false);
 if( sslO != null) {
 request.setAttribute
 (SSLSupport.CERTIFICATE_KEY, sslO);

Modified: tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties?rev=1074675r1=1074674r2=1074675view=diff
==
--- tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties 
(original)
+++ tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties Fri Feb 
25 19:19:13 2011
@@ -31,6 +31,7 @@ http11processor.request.process=Error pr
 http11processor.request.finish=Error finishing request
 http11processor.response.finish=Error finishing response
 http11processor.socket.info=Exception getting socket information
+http11processor.socket.sslreneg=Exception re-negotiating SSL connection
 http11processor.socket.ssl=Exception getting SSL attributes
 http11processor.socket.timeout=Error setting socket timeout
 

Modified: tomcat/trunk/java/org/apache/tomcat/util/net/NioChannel.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/NioChannel.java?rev=1074675r1=1074674r2=1074675view=diff
==
--- tomcat/trunk/java/org/apache/tomcat/util/net/NioChannel.java (original)
+++ tomcat/trunk/java/org/apache/tomcat/util/net/NioChannel.java Fri Feb 25 
19:19:13 2011
@@ -175,7 +175,7 @@ public class NioChannel implements ByteC
  * @return boolean
  * TODO Implement this org.apache.tomcat.util.net.SecureNioChannel method
  */
-public boolean isInitHandshakeComplete() {
+public boolean isHandshakeComplete() {
 return true;
 }
 

Modified:

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-25 Thread Filip Hanik - Dev Lists 

This looks like a CPU spinning handshake to me.
The operation handshake(true, true); returns an IO interest to be 
registered with a selector.
If the client is slow here or misbehaving, you could end up in a end 
less loop, and hence we can have introduced a very simple DoS 
vulnerability here.


The simplest solution is, would be to use an individual selector. 
Register the socket and issue a select() on the thread you are running on.
If you want to use a shared selector (like NIO does for reads and 
writes) it requires a bit more logic.


I understand where you are going with this solution, and I can probably 
fix this today as I sit on the airplane on my way home.


best
Filip



On 02/25/2011 12:19 PM, ma...@apache.org wrote:

Author: markt
Date: Fri Feb 25 19:19:13 2011
New Revision: 1074675

URL: http://svn.apache.org/viewvc?rev=1074675view=rev
Log:
Fix https://issues.apache.org/bugzilla/show_bug.cgi?id=49284
Support SSL re-negotiation in the HTTP NIO connector
There is a fair amount of renaming in this patch. The real work is in the new 
rehandshake() method in the SecureNioChannel.

Modified:
 tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
 tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
 tomcat/trunk/java/org/apache/tomcat/util/net/NioChannel.java
 tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
 tomcat/trunk/webapps/docs/changelog.xml

Modified: tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java?rev=1074675r1=1074674r2=1074675view=diff
==
--- tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java 
(original)
+++ tomcat/trunk/java/org/apache/coyote/http11/Http11NioProcessor.java Fri Feb 
25 19:19:13 2011
@@ -23,6 +23,8 @@ import java.nio.channels.SelectionKey;
  import java.util.Locale;
  import java.util.concurrent.Executor;

+import javax.net.ssl.SSLEngine;
+
  import org.apache.coyote.ActionCode;
  import org.apache.coyote.Request;
  import org.apache.coyote.RequestInfo;
@@ -42,7 +44,9 @@ import org.apache.tomcat.util.net.NioCha
  import org.apache.tomcat.util.net.NioEndpoint;
  import org.apache.tomcat.util.net.NioEndpoint.KeyAttachment;
  import org.apache.tomcat.util.net.SSLSupport;
+import org.apache.tomcat.util.net.SecureNioChannel;
  import org.apache.tomcat.util.net.SocketStatus;
+import org.apache.tomcat.util.net.jsse.JSSEFactory;


  /**
@@ -625,8 +629,25 @@ public class Http11NioProcessor extends
  .setLimit(maxSavePostSize);
  inputBuffer.addActiveFilter
  (inputFilters[Constants.BUFFERED_FILTER]);
+
+SecureNioChannel sslChannel = (SecureNioChannel) socket;
+SSLEngine engine = sslChannel.getSslEngine();
+if (!engine.getNeedClientAuth()  
!engine.getWantClientAuth()) {
+// Need to re-negotiate SSL connection
+engine.setNeedClientAuth(true);
+try {
+sslChannel.rehandshake();
+sslSupport = (new JSSEFactory()).getSSLSupport(
+engine.getSession());
+} catch (IOException ioe) {
+
log.warn(sm.getString(http11processor.socket.sslreneg,
+ioe));
+}
+}
  try {
-Object sslO = sslSupport.getPeerCertificateChain(true);
+// use force=false since re-negotiation is handled above
+// (and it is a NO-OP for NIO anyway)
+Object sslO = sslSupport.getPeerCertificateChain(false);
  if( sslO != null) {
  request.setAttribute
  (SSLSupport.CERTIFICATE_KEY, sslO);

Modified: tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties
URL: 
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties?rev=1074675r1=1074674r2=1074675view=diff
==
--- tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties 
(original)
+++ tomcat/trunk/java/org/apache/coyote/http11/LocalStrings.properties Fri Feb 
25 19:19:13 2011
@@ -31,6 +31,7 @@ http11processor.request.process=Error pr
  http11processor.request.finish=Error finishing request
  http11processor.response.finish=Error finishing response
  http11processor.socket.info=Exception getting socket information
+http11processor.socket.sslreneg=Exception re-negotiating SSL connection
  http11processor.socket.ssl=Exception getting SSL attributes
  http11processor.socket.timeout=Error setting socket timeout


Modified:

Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/

2011-02-25 Thread Mark Thomas 
On 25/02/2011 20:16, Filip Hanik - Dev Lists wrote:
 This looks like a CPU spinning handshake to me.

Opps.

 The operation handshake(true, true); returns an IO interest to be
 registered with a selector.
 If the client is slow here or misbehaving, you could end up in a end
 less loop, and hence we can have introduced a very simple DoS
 vulnerability here.
 
 The simplest solution is, would be to use an individual selector.
 Register the socket and issue a select() on the thread you are running on.
 If you want to use a shared selector (like NIO does for reads and
 writes) it requires a bit more logic.
 
 I understand where you are going with this solution, and I can probably
 fix this today as I sit on the airplane on my way home.

If you could, that would be great.

Mark



-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org