tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Filip Hanik - Dev Lists <devli...@hanik.com>
Subject Re: svn commit: r1074675 - in /tomcat/trunk: java/org/apache/coyote/http11/ java/org/apache/tomcat/util/net/ webapps/docs/
Date Fri, 25 Feb 2011 20:16:03 GMT
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


Mime
View raw message