From b90ad09443e1771e37d23e393afe842759c20454 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Sun, 15 Nov 2009 23:41:46 +0000 Subject: [PATCH] CVE-2009-3555 prevent renegotiation git-svn-id: svn+ssh://dev.eclipse.org/svnroot/rt/org.eclipse.jetty/jetty/trunk@1047 7e9141cc-0065-0410-87d8-b60c137991c4 --- VERSION.txt | 1 + .../http/ssl/SslSelectChannelEndPoint.java | 61 +++- .../jetty/server/ssl/SslConnector.java | 17 ++ .../server/ssl/SslSelectChannelConnector.java | 29 +- .../jetty/server/ssl/SslSocketConnector.java | 44 ++- .../jetty/server/ssl/SslRenegotiateTest.java | 271 ++++++++++++++++++ 6 files changed, 411 insertions(+), 12 deletions(-) create mode 100644 jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslRenegotiateTest.java diff --git a/VERSION.txt b/VERSION.txt index 3cf7aee417f..4b2b1e0620f 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -38,6 +38,7 @@ jetty-7.0.1-SNAPSHOT + CQ-3581 jetty OSGi contribution + Moved centralized logging and verifier back to sandbox + 294345 Support for HTTP/301 + HTTP/302 response codes + + CVE-2009-3555 Prevent SSL renegotiate for SSL vulnerability jetty-7.0.0.v20091005 5 October 2009 291340 Race condition in onException() notifications diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/ssl/SslSelectChannelEndPoint.java b/jetty-http/src/main/java/org/eclipse/jetty/http/ssl/SslSelectChannelEndPoint.java index de2543baf1e..34f2976c3c0 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/ssl/SslSelectChannelEndPoint.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/ssl/SslSelectChannelEndPoint.java @@ -65,6 +65,9 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint private boolean _closing=false; private SSLEngineResult _result; + private boolean _handshook=false; + private boolean _allowRenegotiate=false; + private final boolean _debug = __log.isDebugEnabled(); // snapshot debug status for optimizer @@ -88,12 +91,34 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint if (_debug) __log.debug(_session+" channel="+channel); } + + /* ------------------------------------------------------------ */ + /** + * @return True if SSL re-negotiation is allowed (default false) + */ + public boolean isAllowRenegotiate() + { + return _allowRenegotiate; + } + + /* ------------------------------------------------------------ */ + /** + * Set if SSL re-negotiation is allowed. CVE-2009-3555 discovered + * a vulnerability in SSL/TLS with re-negotiation. If your JVM + * does not have CVE-2009-3555 fixed, then re-negotiation should + * not be allowed. + * @param allowRenegotiate true if re-negotiation is allowed (default false) + */ + public void setAllowRenegotiate(boolean allowRenegotiate) + { + _allowRenegotiate = allowRenegotiate; + } + + /* ------------------------------------------------------------ */ // TODO get rid of these dumps public void dump() { Log.info(""+_result); - // System.err.println(h.toString()); - // System.err.println("--"); } /* ------------------------------------------------------------ */ @@ -163,6 +188,7 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint { case FINISHED: case NOT_HANDSHAKING: + _handshook=true; break loop; case NEED_UNWRAP: @@ -240,6 +266,8 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint } } + + /* ------------------------------------------------------------ */ /** Fill the buffer with unencrypted bytes. * Called by a {@link Parser} instance when more data is @@ -302,6 +330,7 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint break loop; case NEED_UNWRAP: + checkRenegotiate(); // Need more data to be unwrapped so try another call to unwrap if (!unwrap(bbuf) && _engine.getHandshakeStatus()==HandshakeStatus.NEED_UNWRAP) { @@ -339,6 +368,7 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint case NEED_WRAP: { + checkRenegotiate(); // The SSL needs to send some handshake data to the other side, // so let fill become a flush for a little bit. wraps++; @@ -391,11 +421,13 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint buffer.setPutIndex(bbuf.position()); bbuf.position(0); } - } - - // return the number of unencrypted bytes filled. - return buffer.length()-size; + // return the number of unencrypted bytes filled. + int filled=buffer.length()-size; + if (filled>0) + _handshook=true; + return filled; + } } /* ------------------------------------------------------------ */ @@ -426,12 +458,11 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint if (isBufferingOutput()) break loop; } - + switch(_engine.getHandshakeStatus()) { case FINISHED: case NOT_HANDSHAKING: - if (_closing || available==0) { if (consumed==0) @@ -450,8 +481,10 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint else c=wrap(buffer); + if (c>0) { + _handshook=true; consumed+=c; available-=c; } @@ -465,6 +498,7 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint break; case NEED_UNWRAP: + checkRenegotiate(); Buffer buf =_buffers.getBuffer(_engine.getSession().getApplicationBufferSize()); try { @@ -493,6 +527,7 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint case NEED_WRAP: { + checkRenegotiate(); synchronized(_outBuffer) { try @@ -549,6 +584,16 @@ public class SslSelectChannelEndPoint extends SelectChannelEndPoint } } } + + /* ------------------------------------------------------------ */ + private void checkRenegotiate() throws IOException + { + if (_handshook && !_allowRenegotiate && _channel!=null && _channel.isOpen()) + { + Log.warn("SSL renegotiate denied: "+_channel); + close(); + } + } /* ------------------------------------------------------------ */ private ByteBuffer extractInputBuffer(Buffer buffer) diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslConnector.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslConnector.java index a6714e65c24..13d8cf31441 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslConnector.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslConnector.java @@ -217,4 +217,21 @@ public interface SslConnector extends Connector * @return The SSLContext */ public abstract SSLContext getSslContext(); + + + /* ------------------------------------------------------------ */ + /** + * @return True if SSL re-negotiation is allowed (default false) + */ + public boolean isAllowRenegotiate(); + + /* ------------------------------------------------------------ */ + /** + * Set if SSL re-negotiation is allowed. CVE-2009-3555 discovered + * a vulnerability in SSL/TLS with re-negotiation. If your JVM + * does not have CVE-2009-3555 fixed, then re-negotiation should + * not be allowed. + * @param allowRenegotiate true if re-negotiation is allowed (default false) + */ + public void setAllowRenegotiate(boolean allowRenegotiate); } \ No newline at end of file diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslSelectChannelConnector.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslSelectChannelConnector.java index 08a5a3dd48e..21751c619f7 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslSelectChannelConnector.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslSelectChannelConnector.java @@ -82,6 +82,7 @@ public class SslSelectChannelConnector extends SelectChannelConnector implements /** Set to true if we require client certificate authentication. */ private boolean _needClientAuth=false; private boolean _wantClientAuth=false; + private boolean _allowRenegotiate=false; private transient Password _password; private transient Password _keyPassword; @@ -223,6 +224,28 @@ public class SslSelectChannelConnector extends SelectChannelConnector implements { } + /* ------------------------------------------------------------ */ + /** + * @return True if SSL re-negotiation is allowed (default false) + */ + public boolean isAllowRenegotiate() + { + return _allowRenegotiate; + } + + /* ------------------------------------------------------------ */ + /** + * Set if SSL re-negotiation is allowed. CVE-2009-3555 discovered + * a vulnerability in SSL/TLS with re-negotiation. If your JVM + * does not have CVE-2009-3555 fixed, then re-negotiation should + * not be allowed. + * @param allowRenegotiate true if re-negotiation is allowed (default false) + */ + public void setAllowRenegotiate(boolean allowRenegotiate) + { + _allowRenegotiate = allowRenegotiate; + } + /* ------------------------------------------------------------ */ /** * @see org.eclipse.jetty.server.ssl.SslConnector#getExcludeCipherSuites() @@ -554,7 +577,9 @@ public class SslSelectChannelConnector extends SelectChannelConnector implements @Override protected SelectChannelEndPoint newEndPoint(SocketChannel channel, SelectSet selectSet, SelectionKey key) throws IOException { - return new SslSelectChannelEndPoint(_sslBuffers,channel,selectSet,key,createSSLEngine()); + SslSelectChannelEndPoint endp = new SslSelectChannelEndPoint(_sslBuffers,channel,selectSet,key,createSSLEngine()); + endp.setAllowRenegotiate(_allowRenegotiate); + return endp; } /* ------------------------------------------------------------------------------- */ @@ -597,7 +622,6 @@ public class SslSelectChannelConnector extends SelectChannelConnector implements engine.setEnabledCipherSuites(enabledCipherSuites); } - } catch (Exception e) { @@ -607,7 +631,6 @@ public class SslSelectChannelConnector extends SelectChannelConnector implements } return engine; } - @Override protected void doStart() throws Exception diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslSocketConnector.java b/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslSocketConnector.java index c2379df5d85..48ce925c315 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslSocketConnector.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/ssl/SslSocketConnector.java @@ -29,6 +29,8 @@ import java.util.Arrays; import java.util.Iterator; import java.util.List; +import javax.net.ssl.HandshakeCompletedEvent; +import javax.net.ssl.HandshakeCompletedListener; import javax.net.ssl.KeyManager; import javax.net.ssl.KeyManagerFactory; import javax.net.ssl.SSLContext; @@ -144,6 +146,7 @@ public class SslSocketConnector extends SocketConnector implements SslConnector private int _handshakeTimeout = 0; //0 means use maxIdleTime private SSLContext _context; + private boolean _allowRenegotiate =false; /* ------------------------------------------------------------ */ @@ -155,6 +158,27 @@ public class SslSocketConnector extends SocketConnector implements SslConnector super(); } + /* ------------------------------------------------------------ */ + /** + * @return True if SSL re-negotiation is allowed (default false) + */ + public boolean isAllowRenegotiate() + { + return _allowRenegotiate; + } + + /* ------------------------------------------------------------ */ + /** + * Set if SSL re-negotiation is allowed. CVE-2009-3555 discovered + * a vulnerability in SSL/TLS with re-negotiation. If your JVM + * does not have CVE-2009-3555 fixed, then re-negotiation should + * not be allowed. + * @param allowRenegotiate true if re-negotiation is allowed (default false) + */ + public void setAllowRenegotiate(boolean allowRenegotiate) + { + _allowRenegotiate = allowRenegotiate; + } /* ------------------------------------------------------------ */ @Override @@ -687,7 +711,25 @@ public class SslSocketConnector extends SocketConnector implements SslConnector if (handshakeTimeout > 0) _socket.setSoTimeout(handshakeTimeout); - ((SSLSocket)_socket).startHandshake(); + final SSLSocket ssl=(SSLSocket)_socket; + ssl.addHandshakeCompletedListener(new HandshakeCompletedListener() + { + boolean handshook=false; + public void handshakeCompleted(HandshakeCompletedEvent event) + { + if (handshook) + { + if (!_allowRenegotiate) + { + Log.warn("SSL renegotiate denied: "+ssl); + try{ssl.close();}catch(IOException e){Log.warn(e);} + } + } + else + handshook=true; + } + }); + ssl.startHandshake(); if (handshakeTimeout>0) _socket.setSoTimeout(oldTimeout); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslRenegotiateTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslRenegotiateTest.java new file mode 100644 index 00000000000..9ca4d0be8ba --- /dev/null +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SslRenegotiateTest.java @@ -0,0 +1,271 @@ +package org.eclipse.jetty.server.ssl; + +import java.io.File; +import java.io.IOException; +import java.io.PrintWriter; +import java.net.InetSocketAddress; +import java.net.SocketAddress; +import java.nio.ByteBuffer; +import java.nio.channels.SocketChannel; + +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLEngineResult; +import javax.net.ssl.SSLSession; +import javax.net.ssl.TrustManager; +import javax.net.ssl.X509TrustManager; +import javax.net.ssl.SSLEngineResult.HandshakeStatus; +import javax.servlet.ServletException; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.io.nio.IndirectNIOBuffer; +import org.eclipse.jetty.server.Connector; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.handler.AbstractHandler; +import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.util.log.Log; + +import junit.framework.TestCase; + + +public class SslRenegotiateTest extends TestCase +{ + + static TrustManager[] trustAllCerts = new TrustManager[] { new X509TrustManager() + { + public java.security.cert.X509Certificate[] getAcceptedIssuers() + { + return null; + } + + public void checkClientTrusted( java.security.cert.X509Certificate[] certs, String authType ) + { + } + + public void checkServerTrusted( java.security.cert.X509Certificate[] certs, String authType ) + { + } + } }; + + static HostnameVerifier hostnameVerifier = new HostnameVerifier() + { + public boolean verify( String urlHostName, SSLSession session ) + { + Log.warn( "Warning: URL Host: " + urlHostName + " vs." + session.getPeerHost() ); + return true; + } + }; + + ByteBuffer _outAppB; + ByteBuffer _outPacketB; + ByteBuffer _inAppB; + ByteBuffer _inPacketB; + SocketChannel _socket; + SSLEngine _engine; + + + public void testRenegNIO() throws Exception + { + doRequests(new SslSelectChannelConnector(),true); + } + + public void testNoRenegNIO() throws Exception + { + doRequests(new SslSelectChannelConnector(),false); + } + public void testRenegBIO() throws Exception + { + doRequests(new SslSocketConnector(),true); + } + + public void testNoRenegBIO() throws Exception + { + /* TODO - this test does not always work??? need to investigate why + doRequests(new SslSocketConnector(),false); + */ + } + + public void doRequests(SslConnector connector,boolean reneg) throws Exception + { + Server server=new Server(); + try + { + String keystore = System.getProperty("user.dir")+File.separator+"src"+File.separator+"test"+File.separator+"resources"+File.separator+"keystore"; + connector.setPort(0); + connector.setKeystore(keystore); + connector.setPassword("storepwd"); + connector.setKeyPassword("keypwd"); + connector.setAllowRenegotiate(reneg); + + server.setConnectors(new Connector[] { connector }); + server.setHandler(new HelloWorldHandler()); + + server.start(); + + SocketAddress addr = new InetSocketAddress("localhost",connector.getLocalPort()); + _socket = SocketChannel.open(addr); + _socket.configureBlocking(true); + + SSLContext context=SSLContext.getInstance("SSL"); + context.init( null, trustAllCerts, new java.security.SecureRandom() ); + + _engine = context.createSSLEngine(); + _engine.setUseClientMode(true); + SSLSession session=_engine.getSession(); + + _outAppB = ByteBuffer.allocate(session.getApplicationBufferSize()); + _outPacketB = ByteBuffer.allocate(session.getPacketBufferSize()); + _inAppB = ByteBuffer.allocate(session.getApplicationBufferSize()); + _inPacketB = ByteBuffer.allocate(session.getPacketBufferSize()); + + + _outAppB.put("GET /1 HTTP/1.1\r\nHost: localhost\r\n\r\n".getBytes(StringUtil.__ISO_8859_1)); + _outAppB.flip(); + + _engine.beginHandshake(); + + runHandshake(); + + doWrap(); + doUnwrap(); + _inAppB.flip(); + String response=new IndirectNIOBuffer(_inAppB,true).toString(); + // System.err.println(response); + assertTrue(response.startsWith("HTTP/1.1 200 OK")); + + if (response.indexOf("HELLO WORLD")<0) + { + _inAppB.clear(); + doUnwrap(); + _inAppB.flip(); + response=new IndirectNIOBuffer(_inAppB,true).toString(); + } + + assertTrue(response.indexOf("HELLO WORLD")>=0); + + _inAppB.clear(); + _outAppB.clear(); + _outAppB.put("GET /2 HTTP/1.1\r\nHost: localhost\r\n\r\n".getBytes(StringUtil.__ISO_8859_1)); + _outAppB.flip(); + + try + { + session.invalidate(); + _engine.beginHandshake(); + runHandshake(); + + doWrap(); + doUnwrap(); + _inAppB.flip(); + response=new IndirectNIOBuffer(_inAppB,true).toString(); + assertTrue(response.startsWith("HTTP/1.1 200 OK")); + assertTrue(response.indexOf("HELLO WORLD")>0); + + assertTrue(reneg); + } + catch(IOException e) + { + // System.err.println(e); + assertFalse(reneg); + return; + } + + } + finally + { + server.stop(); + } + } + + void runHandshake() throws Exception + { + SSLEngineResult result; + + while (true) + { + //System.err.println(); + //System.err.println(_engine.getHandshakeStatus()); + + switch(_engine.getHandshakeStatus()) + { + case NEED_TASK: + { + //System.err.println("running task"); + _engine.getDelegatedTask().run(); + break; + } + + case NEED_WRAP: + { + doWrap(); + break; + } + + case NEED_UNWRAP: + { + doUnwrap(); + break; + } + + default: + return; + } + } + } + + private void doWrap() throws Exception + { + SSLEngineResult result =_engine.wrap(_outAppB,_outPacketB); +// System.err.println("wrapped "+result.bytesConsumed()+" to "+result.bytesProduced()); + _outPacketB.flip(); + while (_outPacketB.hasRemaining()) + { + int p = _outPacketB.remaining(); + int l =_socket.write(_outPacketB); + // System.err.println("wrote "+l+" of "+p); + } + _outPacketB.clear(); + } + + private void doUnwrap() throws Exception + { + _inPacketB.clear(); + int l=_socket.read(_inPacketB); + // System.err.println("read "+l); + if (l<0) + throw new IOException("EOF"); + + _inPacketB.flip(); + + SSLEngineResult result; + do + { + result =_engine.unwrap(_inPacketB,_inAppB); +// System.err.println("unwrapped "+result.bytesConsumed()+" to "+result.bytesProduced()+" "+_engine.getHandshakeStatus()); + + } + while(result.bytesConsumed()>0 && + _inPacketB.remaining()>0 && + (_engine.getHandshakeStatus()==HandshakeStatus.NEED_UNWRAP || _engine.getHandshakeStatus()==HandshakeStatus.NOT_HANDSHAKING)); + + } + + private static class HelloWorldHandler extends AbstractHandler + { + + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + { + baseRequest.setHandled(true); + //System.err.println("HELLO WORLD HANDLING"); + +// System.err.println("hello "+baseRequest.getUri()); + byte[] b=("HELLO WORLD "+baseRequest.getUri()).getBytes(StringUtil.__UTF8); + response.setContentLength(b.length); + response.getOutputStream().write(b); + response.getOutputStream().flush(); + } + } +}