From 23a9c6c1bef7e69697bb694c8f28e8004a566fbe Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Wed, 12 Apr 2017 14:54:18 +1000 Subject: [PATCH] Issue #1463 --- .../io/ssl/SslClientConnectionFactory.java | 1 + .../eclipse/jetty/io/ssl/SslConnection.java | 122 ++++++++++++------ .../io/SelectChannelEndPointSslTest.java | 1 + .../eclipse/jetty/io/SslConnectionTest.java | 112 +++++++++++++++- .../src/main/config/etc/jetty-ssl-context.xml | 2 + jetty-server/src/main/config/modules/ssl.mod | 4 + .../jetty/server/SslConnectionFactory.java | 1 + .../jetty/util/ssl/SslContextFactory.java | 20 +++ .../io/WebSocketClientSelectorManager.java | 1 + 9 files changed, 224 insertions(+), 40 deletions(-) diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java index 648ec687a26..60662741ad8 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslClientConnectionFactory.java @@ -83,6 +83,7 @@ public class SslClientConnectionFactory implements ClientConnectionFactory { SslConnection sslConnection = (SslConnection)connection; sslConnection.setRenegotiationAllowed(sslContextFactory.isRenegotiationAllowed()); + sslConnection.setRenegotiationLimit(sslContextFactory.getRenegotiationLimit()); ContainerLifeCycle connector = (ContainerLifeCycle)context.get(ClientConnectionFactory.CONNECTOR_CONTEXT_KEY); connector.getBeans(SslHandshakeListener.class).forEach(sslConnection::addHandshakeListener); } diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java index 264785be7a1..632265c8172 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/ssl/SslConnection.java @@ -90,6 +90,7 @@ public class SslConnection extends AbstractConnection private final boolean _encryptedDirectBuffers = true; private final boolean _decryptedDirectBuffers = false; private boolean _renegotiationAllowed; + private int _renegotiationLimit = -1; private boolean _closedOutbound; private final Runnable _runCompletWrite = new Runnable() { @@ -170,7 +171,26 @@ public class SslConnection extends AbstractConnection public void setRenegotiationAllowed(boolean renegotiationAllowed) { - this._renegotiationAllowed = renegotiationAllowed; + _renegotiationAllowed = renegotiationAllowed; + } + + /** + * @return The number of renegotions allowed for this connection. When the limit + * is 0 renegotiation will be denied. If the limit is <0 then no limit is applied. + */ + public int getRenegotiationLimit() + { + return _renegotiationLimit; + } + + /** + * @param renegotiationLimit The number of renegotions allowed for this connection. + * When the limit is 0 renegotiation will be denied. If the limit is <0 then no limit is applied. + * Default -1. + */ + public void setRenegotiationLimit(int renegotiationLimit) + { + _renegotiationLimit = renegotiationLimit; } @Override @@ -323,7 +343,7 @@ public class SslConnection extends AbstractConnection synchronized (DecryptedEndPoint.this) { if (LOG.isDebugEnabled()) - LOG.debug("{} write failed", SslConnection.this, x); + LOG.debug("write failed {}", SslConnection.this, x); BufferUtil.clear(_encryptedOutput); releaseEncryptedOutputBuffer(); @@ -567,8 +587,8 @@ public class SslConnection extends AbstractConnection } if (LOG.isDebugEnabled()) { - LOG.debug("{} net={} unwrap {}", SslConnection.this, net_filled, unwrapResult.toString().replace('\n',' ')); - LOG.debug("{} filled {}",SslConnection.this,BufferUtil.toHexSummary(buffer)); + LOG.debug("net={} unwrap {} {}", net_filled, unwrapResult.toString().replace('\n',' '), SslConnection.this); + LOG.debug("filled {} {}",BufferUtil.toHexSummary(buffer), SslConnection.this); } HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus(); @@ -626,25 +646,13 @@ public class SslConnection extends AbstractConnection case BUFFER_UNDERFLOW: case OK: { - if (unwrapHandshakeStatus == HandshakeStatus.FINISHED && !_handshaken) - { - _handshaken = true; - if (LOG.isDebugEnabled()) - LOG.debug("{} {} handshake succeeded {}/{}", SslConnection.this, - _sslEngine.getUseClientMode() ? "client" : "resumed server", - _sslEngine.getSession().getProtocol(),_sslEngine.getSession().getCipherSuite()); - notifyHandshakeSucceeded(_sslEngine); - } + if (unwrapHandshakeStatus == HandshakeStatus.FINISHED) + handshakeFinished(); - // Check whether renegotiation is allowed - if (_handshaken && handshakeStatus != HandshakeStatus.NOT_HANDSHAKING && !isRenegotiationAllowed()) - { - if (LOG.isDebugEnabled()) - LOG.debug("{} renegotiation denied", SslConnection.this); - closeInbound(); + // Check whether re-negotiation is allowed + if (!allowRenegotiate(handshakeStatus)) return -1; - } - + // If bytes were produced, don't bother with the handshake status; // pass the decrypted data to the application, which will perform // another call to fill() or flush(). @@ -762,6 +770,51 @@ public class SslConnection extends AbstractConnection } } + private void handshakeFinished() + { + if (_handshaken) + { + if (LOG.isDebugEnabled()) + LOG.debug("Renegotiated {}", SslConnection.this); + if (_renegotiationLimit>0) + _renegotiationLimit--; + } + else + { + _handshaken = true; + if (LOG.isDebugEnabled()) + LOG.debug("{} handshake succeeded {}/{} {}", + _sslEngine.getUseClientMode() ? "client" : "resumed server", + _sslEngine.getSession().getProtocol(),_sslEngine.getSession().getCipherSuite(), + SslConnection.this); + notifyHandshakeSucceeded(_sslEngine); + } + } + + private boolean allowRenegotiate(HandshakeStatus handshakeStatus) + { + if (!_handshaken || handshakeStatus == HandshakeStatus.NOT_HANDSHAKING) + return true; + + if (!isRenegotiationAllowed()) + { + if (LOG.isDebugEnabled()) + LOG.debug("Renegotiation denied {}", SslConnection.this); + closeInbound(); + return false; + } + + if (_renegotiationLimit==0) + { + if (LOG.isDebugEnabled()) + LOG.debug("Renegotiation limit exceeded {}", SslConnection.this); + closeInbound(); + return false; + } + + return true; + } + private void closeInbound() { try @@ -787,7 +840,7 @@ public class SslConnection extends AbstractConnection if (LOG.isDebugEnabled()) { for (ByteBuffer b : appOuts) - LOG.debug("{} flush {}", SslConnection.this, BufferUtil.toHexSummary(b)); + LOG.debug("flush {} {}", BufferUtil.toHexSummary(b), SslConnection.this); } try @@ -822,7 +875,7 @@ public class SslConnection extends AbstractConnection BufferUtil.flipToFlush(_encryptedOutput, pos); } if (LOG.isDebugEnabled()) - LOG.debug("{} wrap {}", SslConnection.this, wrapResult.toString().replace('\n',' ')); + LOG.debug("wrap {} {}", wrapResult.toString().replace('\n',' '), SslConnection.this); Status wrapResultStatus = wrapResult.getStatus(); @@ -863,29 +916,20 @@ public class SslConnection extends AbstractConnection default: { if (LOG.isDebugEnabled()) - LOG.debug("{} wrap {} {}", SslConnection.this, wrapResultStatus, BufferUtil.toHexSummary(_encryptedOutput)); + LOG.debug("wrap {} {} {}", wrapResultStatus, BufferUtil.toHexSummary(_encryptedOutput), SslConnection.this); - if (wrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED && !_handshaken) - { - _handshaken = true; - if (LOG.isDebugEnabled()) - LOG.debug("{} {} handshake succeeded {}/{}", SslConnection.this, - _sslEngine.getUseClientMode() ? "resumed client" : "server", - _sslEngine.getSession().getProtocol(),_sslEngine.getSession().getCipherSuite()); - notifyHandshakeSucceeded(_sslEngine); - } + if (wrapResult.getHandshakeStatus() == HandshakeStatus.FINISHED) + handshakeFinished(); HandshakeStatus handshakeStatus = _sslEngine.getHandshakeStatus(); - // Check whether renegotiation is allowed - if (_handshaken && handshakeStatus != HandshakeStatus.NOT_HANDSHAKING && !isRenegotiationAllowed()) + // Check whether re-negotiation is allowed + if (!allowRenegotiate(handshakeStatus)) { - if (LOG.isDebugEnabled()) - LOG.debug("{} renegotiation denied", SslConnection.this); getEndPoint().shutdownOutput(); return allConsumed; } - + // if we have net bytes, let's try to flush them if (BufferUtil.hasContent(_encryptedOutput)) if (!getEndPoint().flush(_encryptedOutput)) @@ -975,7 +1019,7 @@ public class SslConnection extends AbstractConnection boolean ishut = isInputShutdown(); boolean oshut = isOutputShutdown(); if (LOG.isDebugEnabled()) - LOG.debug("{} shutdownOutput: oshut={}, ishut={}", SslConnection.this, oshut, ishut); + LOG.debug("shutdownOutput: oshut={}, ishut={} {}", oshut, ishut, SslConnection.this); if (oshut) return; diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/SelectChannelEndPointSslTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/SelectChannelEndPointSslTest.java index 0ad8cec2fa4..acc9b944b74 100644 --- a/jetty-io/src/test/java/org/eclipse/jetty/io/SelectChannelEndPointSslTest.java +++ b/jetty-io/src/test/java/org/eclipse/jetty/io/SelectChannelEndPointSslTest.java @@ -77,6 +77,7 @@ public class SelectChannelEndPointSslTest extends SelectChannelEndPointTest engine.setUseClientMode(false); SslConnection sslConnection = new SslConnection(__byteBufferPool, _threadPool, endpoint, engine); sslConnection.setRenegotiationAllowed(__sslCtxFactory.isRenegotiationAllowed()); + sslConnection.setRenegotiationLimit(__sslCtxFactory.getRenegotiationLimit()); Connection appConnection = super.newConnection(channel,sslConnection.getDecryptedEndPoint()); sslConnection.getDecryptedEndPoint().setConnection(appConnection); return sslConnection; diff --git a/jetty-io/src/test/java/org/eclipse/jetty/io/SslConnectionTest.java b/jetty-io/src/test/java/org/eclipse/jetty/io/SslConnectionTest.java index 98768f441bc..5bcb14db76c 100644 --- a/jetty-io/src/test/java/org/eclipse/jetty/io/SslConnectionTest.java +++ b/jetty-io/src/test/java/org/eclipse/jetty/io/SslConnectionTest.java @@ -33,6 +33,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLException; import javax.net.ssl.SSLSocket; import org.eclipse.jetty.io.ssl.SslConnection; @@ -80,6 +81,7 @@ public class SslConnectionTest engine.setUseClientMode(false); SslConnection sslConnection = new SslConnection(__byteBufferPool, getExecutor(), endpoint, engine); sslConnection.setRenegotiationAllowed(__sslCtxFactory.isRenegotiationAllowed()); + sslConnection.setRenegotiationLimit(__sslCtxFactory.getRenegotiationLimit()); Connection appConnection = new TestConnection(sslConnection.getDecryptedEndPoint()); sslConnection.getDecryptedEndPoint().setConnection(appConnection); return sslConnection; @@ -149,6 +151,8 @@ public class SslConnectionTest _threadPool.start(); _scheduler.start(); _manager.start(); + __sslCtxFactory.setRenegotiationAllowed(true); + __sslCtxFactory.setRenegotiationLimit(-1); } @@ -248,7 +252,7 @@ public class SslConnectionTest } } } - protected Socket newClient() throws IOException + protected SSLSocket newClient() throws IOException { SSLSocket socket = __sslCtxFactory.newSslSocket(); socket.connect(_connector.socket().getLocalSocketAddress()); @@ -280,6 +284,112 @@ public class SslConnectionTest client.close(); } + @Test + public void testRenegotiate() throws Exception + { + SSLSocket client = newClient(); + client.setSoTimeout(60000); + + SocketChannel server = _connector.accept(); + server.configureBlocking(false); + _manager.accept(server); + + client.getOutputStream().write("Hello".getBytes(StandardCharsets.UTF_8)); + byte[] buffer = new byte[1024]; + int len=client.getInputStream().read(buffer); + Assert.assertEquals(5, len); + Assert.assertEquals("Hello",new String(buffer,0,len,StandardCharsets.UTF_8)); + + client.startHandshake(); + + client.getOutputStream().write("World".getBytes(StandardCharsets.UTF_8)); + len=client.getInputStream().read(buffer); + Assert.assertEquals(5, len); + Assert.assertEquals("World",new String(buffer,0,len,StandardCharsets.UTF_8)); + + client.close(); + } + + @Test + public void testRenegotiateNotAllowed() throws Exception + { + __sslCtxFactory.setRenegotiationAllowed(false); + + SSLSocket client = newClient(); + client.setSoTimeout(60000); + + SocketChannel server = _connector.accept(); + server.configureBlocking(false); + _manager.accept(server); + + client.getOutputStream().write("Hello".getBytes(StandardCharsets.UTF_8)); + byte[] buffer = new byte[1024]; + int len=client.getInputStream().read(buffer); + Assert.assertEquals(5, len); + Assert.assertEquals("Hello",new String(buffer,0,len,StandardCharsets.UTF_8)); + + client.startHandshake(); + + client.getOutputStream().write("World".getBytes(StandardCharsets.UTF_8)); + try + { + client.getInputStream().read(buffer); + Assert.fail(); + } + catch(SSLException e) + { + // expected + } + } + + @Test + public void testRenegotiateLimit() throws Exception + { + __sslCtxFactory.setRenegotiationAllowed(true); + __sslCtxFactory.setRenegotiationLimit(2); + + SSLSocket client = newClient(); + client.setSoTimeout(60000); + + SocketChannel server = _connector.accept(); + server.configureBlocking(false); + _manager.accept(server); + + client.getOutputStream().write("Good".getBytes(StandardCharsets.UTF_8)); + byte[] buffer = new byte[1024]; + int len=client.getInputStream().read(buffer); + Assert.assertEquals(4, len); + Assert.assertEquals("Good",new String(buffer,0,len,StandardCharsets.UTF_8)); + + client.startHandshake(); + + client.getOutputStream().write("Bye".getBytes(StandardCharsets.UTF_8)); + len=client.getInputStream().read(buffer); + Assert.assertEquals(3, len); + Assert.assertEquals("Bye",new String(buffer,0,len,StandardCharsets.UTF_8)); + + client.startHandshake(); + + client.getOutputStream().write("Cruel".getBytes(StandardCharsets.UTF_8)); + len=client.getInputStream().read(buffer); + Assert.assertEquals(5, len); + Assert.assertEquals("Cruel",new String(buffer,0,len,StandardCharsets.UTF_8)); + + client.startHandshake(); + + client.getOutputStream().write("World".getBytes(StandardCharsets.UTF_8)); + try + { + client.getInputStream().read(buffer); + Assert.fail(); + } + catch(SSLException e) + { + // expected + } + } + + @Test public void testWriteOnConnect() throws Exception diff --git a/jetty-server/src/main/config/etc/jetty-ssl-context.xml b/jetty-server/src/main/config/etc/jetty-ssl-context.xml index 1b266d1c744..3e84a9e0e0c 100644 --- a/jetty-server/src/main/config/etc/jetty-ssl-context.xml +++ b/jetty-server/src/main/config/etc/jetty-ssl-context.xml @@ -26,4 +26,6 @@ + + diff --git a/jetty-server/src/main/config/modules/ssl.mod b/jetty-server/src/main/config/modules/ssl.mod index 75263c6c994..7638d2594cd 100644 --- a/jetty-server/src/main/config/modules/ssl.mod +++ b/jetty-server/src/main/config/modules/ssl.mod @@ -95,3 +95,7 @@ https://raw.githubusercontent.com/eclipse/jetty.project/master/jetty-server/src/ ## Set the timeout (in seconds) of the SslSession cache timeout # jetty.sslContext.sslSessionTimeout=-1 + +## Allow SSL renegotiation +# jetty.sslContext.renegotiationAllowed=true +# jetty.sslContext.renegotiationLimit=5 diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/SslConnectionFactory.java b/jetty-server/src/main/java/org/eclipse/jetty/server/SslConnectionFactory.java index 7a8f0c66a9e..cdb63609022 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/SslConnectionFactory.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/SslConnectionFactory.java @@ -87,6 +87,7 @@ public class SslConnectionFactory extends AbstractConnectionFactory SslConnection sslConnection = newSslConnection(connector, endPoint, engine); sslConnection.setRenegotiationAllowed(_sslContextFactory.isRenegotiationAllowed()); + sslConnection.setRenegotiationLimit(_sslContextFactory.getRenegotiationLimit()); configure(sslConnection, connector, endPoint); ConnectionFactory next = connector.getConnectionFactory(_nextProtocol); diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java index 345482bdaf3..c2d7fa3961d 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java @@ -166,6 +166,7 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable private String _endpointIdentificationAlgorithm = null; private boolean _trustAll; private boolean _renegotiationAllowed = true; + private int _renegotiationLimit = 5; private Factory _factory; /** @@ -930,6 +931,25 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable _renegotiationAllowed = renegotiationAllowed; } + /** + * @return The number of renegotions allowed for this connection. When the limit + * is 0 renegotiation will be denied. If the limit is <0 then no limit is applied. + */ + public int getRenegotiationLimit() + { + return _renegotiationLimit; + } + + /** + * @param renegotiationLimit The number of renegotions allowed for this connection. + * When the limit is 0 renegotiation will be denied. If the limit is <0 then no limit is applied. + * Default 5. + */ + public void setRenegotiationLimit(int renegotiationLimit) + { + _renegotiationLimit = renegotiationLimit; + } + /** * @return Path to file that contains Certificate Revocation List */ diff --git a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/io/WebSocketClientSelectorManager.java b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/io/WebSocketClientSelectorManager.java index d6235aa3c0a..33981d9d267 100644 --- a/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/io/WebSocketClientSelectorManager.java +++ b/jetty-websocket/websocket-client/src/main/java/org/eclipse/jetty/websocket/client/io/WebSocketClientSelectorManager.java @@ -86,6 +86,7 @@ public class WebSocketClientSelectorManager extends SelectorManager SSLEngine engine = newSSLEngine(sslContextFactory,channel); SslConnection sslConnection = new SslConnection(bufferPool,getExecutor(),endPoint,engine); sslConnection.setRenegotiationAllowed(sslContextFactory.isRenegotiationAllowed()); + sslConnection.setRenegotiationLimit(sslContextFactory.getRenegotiationLimit()); EndPoint sslEndPoint = sslConnection.getDecryptedEndPoint(); Connection connection = newUpgradeConnection(channel,sslEndPoint,connectPromise);