Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Received-SPF: pass (athena.apache.org: local policy)
Message-ID: <4D6C26B9.2040005@hanik.com>
Date: Mon, 28 Feb 2011 15:50:33 -0700
From: Filip Hanik - Dev Lists <devlists@hanik.com>
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US;
 rv:1.9.2.13) Gecko/20101207 Thunderbird/3.1.7
MIME-Version: 1.0
To: Tomcat Developers List <dev@tomcat.apache.org>
Subject: Re: svn commit: r1074675 - in /tomcat/trunk:
 java/org/apache/coyote/http11/
 java/org/apache/tomcat/util/net/ webapps/docs/
References: <20110225191914.8D80B23888CB@eris.apache.org>
 <4D680E03.607@hanik.com>
In-Reply-To: <4D680E03.607@hanik.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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

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, markt@apache.org wrote:
>> Author: markt
>> Date: Fri Feb 25 19:19:13 2011
>> New Revision: 1074675
>>
>> URL: http://svn.apache.org/viewvc?rev=1074675&view=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=1074675&r1=1074674&r2=1074675&view=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=1074675&r1=1074674&r2=1074675&view=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=1074675&r1=1074674&r2=1074675&view=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: tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java
>> URL: 
>> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java?rev=1074675&r1=1074674&r2=1074675&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java (original)
>> +++ tomcat/trunk/java/org/apache/tomcat/util/net/SecureNioChannel.java Fri Feb 25 19:19:13 2011
>> @@ -43,8 +43,8 @@ public class SecureNioChannel extends Ni
>>
>>       protected SSLEngine sslEngine;
>>
>> -    protected boolean initHandshakeComplete = false;
>> -    protected HandshakeStatus initHandshakeStatus; //gets set by begin handshake
>> +    protected boolean handshakeComplete = false;
>> +    protected HandshakeStatus handshakeStatus; //gets set by begin handshake
>>
>>       protected boolean closed = false;
>>       protected boolean closing = false;
>> @@ -82,12 +82,12 @@ public class SecureNioChannel extends Ni
>>           netOutBuffer.limit(0);
>>           netInBuffer.position(0);
>>           netInBuffer.limit(0);
>> -        initHandshakeComplete = false;
>> +        handshakeComplete = false;
>>           closed = false;
>>           closing = false;
>>           //initiate handshake
>>           sslEngine.beginHandshake();
>> -        initHandshakeStatus = sslEngine.getHandshakeStatus();
>> +        handshakeStatus = sslEngine.getHandshakeStatus();
>>       }
>>
>>       @Override
>> @@ -146,35 +146,35 @@ public class SecureNioChannel extends Ni
>>        */
>>       @Override
>>       public int handshake(boolean read, boolean write) throws IOException {
>> -        if ( initHandshakeComplete ) return 0; //we have done our initial handshake
>> +        if ( handshakeComplete ) return 0; //we have done our initial handshake
>>
>>           if (!flush(netOutBuffer)) return SelectionKey.OP_WRITE; //we still have data to write
>>
>>           SSLEngineResult handshake = null;
>>
>> -        while (!initHandshakeComplete) {
>> -            switch ( initHandshakeStatus ) {
>> +        while (!handshakeComplete) {
>> +            switch ( handshakeStatus ) {
>>                   case NOT_HANDSHAKING: {
>>                       //should never happen
>>                       throw new IOException("NOT_HANDSHAKING during handshake");
>>                   }
>>                   case FINISHED: {
>>                       //we are complete if we have delivered the last package
>> -                    initHandshakeComplete = !netOutBuffer.hasRemaining();
>> +                    handshakeComplete = !netOutBuffer.hasRemaining();
>>                       //return 0 if we are complete, otherwise we still have data to write
>> -                    return initHandshakeComplete?0:SelectionKey.OP_WRITE;
>> +                    return handshakeComplete?0:SelectionKey.OP_WRITE;
>>                   }
>>                   case NEED_WRAP: {
>>                       //perform the wrap function
>>                       handshake = handshakeWrap(write);
>>                       if ( handshake.getStatus() == Status.OK ){
>> -                        if (initHandshakeStatus == HandshakeStatus.NEED_TASK)
>> -                            initHandshakeStatus = tasks();
>> +                        if (handshakeStatus == HandshakeStatus.NEED_TASK)
>> +                            handshakeStatus = tasks();
>>                       } else {
>>                           //wrap should always work with our buffers
>>                           throw new IOException("Unexpected status:" + handshake.getStatus() + " during handshake WRAP.");
>>                       }
>> -                    if ( initHandshakeStatus != HandshakeStatus.NEED_UNWRAP || (!flush(netOutBuffer)) ) {
>> +                    if ( handshakeStatus != HandshakeStatus.NEED_UNWRAP || (!flush(netOutBuffer)) ) {
>>                           //should actually return OP_READ if we have NEED_UNWRAP
>>                           return SelectionKey.OP_WRITE;
>>                       }
>> @@ -185,26 +185,26 @@ public class SecureNioChannel extends Ni
>>                       //perform the unwrap function
>>                       handshake = handshakeUnwrap(read);
>>                       if ( handshake.getStatus() == Status.OK ) {
>> -                        if (initHandshakeStatus == HandshakeStatus.NEED_TASK)
>> -                            initHandshakeStatus = tasks();
>> +                        if (handshakeStatus == HandshakeStatus.NEED_TASK)
>> +                            handshakeStatus = tasks();
>>                       } else if ( handshake.getStatus() == Status.BUFFER_UNDERFLOW ){
>>                           //read more data, reregister for OP_READ
>>                           return SelectionKey.OP_READ;
>>                       } else {
>> -                        throw new IOException("Invalid handshake status:"+initHandshakeStatus+" during handshake UNWRAP.");
>> +                        throw new IOException("Invalid handshake status:"+handshakeStatus+" during handshake UNWRAP.");
>>                       }//switch
>>                       break;
>>                   }
>>                   case NEED_TASK: {
>> -                    initHandshakeStatus = tasks();
>> +                    handshakeStatus = tasks();
>>                       break;
>>                   }
>> -                default: throw new IllegalStateException("Invalid handshake status:"+initHandshakeStatus);
>> +                default: throw new IllegalStateException("Invalid handshake status:"+handshakeStatus);
>>               }//switch
>>           }//while
>>           //return 0 if we are complete, otherwise reregister for any activity that
>>           //would cause this method to be called again.
>> -        return initHandshakeComplete?0:(SelectionKey.OP_WRITE|SelectionKey.OP_READ);
>> +        return handshakeComplete?0:(SelectionKey.OP_WRITE|SelectionKey.OP_READ);
>>       }
>>
>>       /**
>> @@ -234,7 +234,7 @@ public class SecureNioChannel extends Ni
>>           //prepare the results to be written
>>           netOutBuffer.flip();
>>           //set the status
>> -        initHandshakeStatus = result.getHandshakeStatus();
>> +        handshakeStatus = result.getHandshakeStatus();
>>           //optimization, if we do have a writable channel, write it now
>>           if ( doWrite ) flush(netOutBuffer);
>>           return result;
>> @@ -268,19 +268,42 @@ public class SecureNioChannel extends Ni
>>               //compact the buffer, this is an optional method, wonder what would happen if we didn't
>>               netInBuffer.compact();
>>               //read in the status
>> -            initHandshakeStatus = result.getHandshakeStatus();
>> +            handshakeStatus = result.getHandshakeStatus();
>>               if ( result.getStatus() == SSLEngineResult.Status.OK&&
>>                    result.getHandshakeStatus() == HandshakeStatus.NEED_TASK ) {
>>                   //execute tasks if we need to
>> -                initHandshakeStatus = tasks();
>> +                handshakeStatus = tasks();
>>               }
>>               //perform another unwrap?
>>               cont = result.getStatus() == SSLEngineResult.Status.OK&&
>> -                   initHandshakeStatus == HandshakeStatus.NEED_UNWRAP;
>> +                   handshakeStatus == HandshakeStatus.NEED_UNWRAP;
>>           }while ( cont );
>>           return result;
>>       }
>>
>> +    public void rehandshake() throws IOException {
>> +        int readBufLimit = getBufHandler().getReadBuffer().limit();
>> +        try {
>> +            // Expand read buffer to maximum to allow handshaking to take place
>> +            getBufHandler().getReadBuffer().limit(
>> +                    getBufHandler().getReadBuffer().capacity());
>> +            sslEngine.getSession().invalidate();
>> +            sslEngine.beginHandshake();
>> +            handshakeComplete = false;
>> +            handshakeStatus = sslEngine.getHandshakeStatus();
>> +            while (!handshakeComplete) {
>> +                handshake(true, true);
>> +                if (handshakeStatus == HandshakeStatus.NEED_UNWRAP)  {
>> +                    handshakeUnwrap(true);
>> +                }
>> +            }
>> +        } finally {
>> +            // Restore the pre-handshak value
>> +            getBufHandler().getReadBuffer().limit(readBufLimit);
>> +        }
>> +    }
>> +
>> +
>>       /**
>>        * Sends a SSL close message, will not physically close the connection here.<br>
>>        * To close the connection, you could do something like
>> @@ -353,7 +376,7 @@ public class SecureNioChannel extends Ni
>>           //are we in the middle of closing or closed?
>>           if ( closing || closed) return -1;
>>           //did we finish our handshake?
>> -        if (!initHandshakeComplete) throw new IllegalStateException("Handshake incomplete, you must complete handshake before reading 
>> data.");
>> +        if (!handshakeComplete) throw new IllegalStateException("Handshake incomplete, you must complete handshake before reading data.");
>>
>>           //read from the network
>>           int netread = sc.read(netInBuffer);
>> @@ -474,8 +497,8 @@ public class SecureNioChannel extends Ni
>>       }
>>
>>       @Override
>> -    public boolean isInitHandshakeComplete() {
>> -        return initHandshakeComplete;
>> +    public boolean isHandshakeComplete() {
>> +        return handshakeComplete;
>>       }
>>
>>       @Override
>>
>> Modified: tomcat/trunk/webapps/docs/changelog.xml
>> URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1074675&r1=1074674&r2=1074675&view=diff
>> ==============================================================================
>> --- tomcat/trunk/webapps/docs/changelog.xml (original)
>> +++ tomcat/trunk/webapps/docs/changelog.xml Fri Feb 25 19:19:13 2011
>> @@ -151,6 +151,10 @@
>> </subsection>
>> <subsection name="Coyote">
>> <changelog>
>> +<add>
>> +<bug>49284</bug>: Add SSL re-negotiation support to the HTTP NIO
>> +        connector. (markt)
>> +</add>
>> <fix>
>> <bug>50780</bug>: Fix memory leak in APR implementation of AJP
>>           connector introduced by the refactoring for<bug>49884</bug>. (markt)
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@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-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org