When a mina 2.1.X server (presumably also 2.0.X) is used with the
BouncyCastle JSSE provider ("BCJSSE" - I am a BC dev), then it may get
into an infinite loop if a connected client requests renegotiation (by
sending a new ClientHello) and BCJSSE (default behaviour) rejects that
by responding with a no_renegotiation alert.
The infinite loop happens in the 'SslHandler.handshake' method (via
'renegotiateIfNeeded'). The (renegotiation) ClientHello is unwrapped and
the server SSLEngine then signals NEED_WRAP because it has an alert to
send. 'renegotiateIfNeeded' then calls through to 'handshake' and the
alert is wrapped, with the SSLEngine returning NOT_HANDSHAKING. This
ought to be an appropriate time to exit the switch loop, but it appears
this loop requires getting a FINISHED status, which will never happen of
course since no new handshake was initiated.
Simply returning from 'handshake' when NOT_HANDSHAKING is reached seems
to cause other issues, so presumably the handling of NOT_HANDSHAKING in
the same way as NEED_WRAP is a workaround best left as is. Therefore I
propose a more targeted fix that returns once a wrap call returns
NOT_HANDSHAKING and without any progress (no bytes consumed or
produced). The exit condition could be made even more specific by
requiring the previous status to also have been NOT_HANDSHAKING, if that
is preferable.
The attached patch includes a reproducer unit test (in SslTest) and the
proposed fix in SslHandler.handshake() method.
Regards,
Pete Dettman
diff --git a/mina-core/src/main/java/org/apache/mina/filter/ssl/SslHandler.java
b/mina-core/src/main/java/org/apache/mina/filter/ssl/SslHandler.java
index aaae15066..2a3390a0f 100644
--- a/mina-core/src/main/java/org/apache/mina/filter/ssl/SslHandler.java
+++ b/mina-core/src/main/java/org/apache/mina/filter/ssl/SslHandler.java
@@ -654,6 +654,17 @@ class SslHandler {
outNetBuffer.flip();
handshakeStatus = result.getHandshakeStatus();
writeNetBuffer(nextFilter);
+
+ /*
+ * Avoid an infinite loop if we are NOT_HANDSHAKING and wrap
made no progress.
+ */
+ if (HandshakeStatus.NOT_HANDSHAKING == handshakeStatus &&
+ result.bytesConsumed() == 0 &&
+ result.bytesProduced() == 0)
+ {
+ return;
+ }
+
break;
default:
diff --git a/mina-core/src/test/java/org/apache/mina/filter/ssl/SslTest.java
b/mina-core/src/test/java/org/apache/mina/filter/ssl/SslTest.java
index e61bad654..c4616d24d 100644
--- a/mina-core/src/test/java/org/apache/mina/filter/ssl/SslTest.java
+++ b/mina-core/src/test/java/org/apache/mina/filter/ssl/SslTest.java
@@ -31,6 +31,7 @@ import java.security.Security;
import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLSocket;
import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.TrustManagerFactory;
@@ -41,6 +42,8 @@ import org.apache.mina.filter.codec.ProtocolCodecFilter;
import org.apache.mina.filter.codec.textline.TextLineCodecFactory;
import org.apache.mina.transport.socket.nio.NioSocketAcceptor;
import org.apache.mina.util.AvailablePortFinder;
+import org.bouncycastle.jce.provider.BouncyCastleProvider;
+import org.bouncycastle.jsse.provider.BouncyCastleJsseProvider;
import org.junit.Test;
/**
@@ -116,6 +119,24 @@ public class SslTest {
acceptor.bind(new InetSocketAddress(port));
}
+ private static void startServerBCJSSE() throws Exception {
+ acceptor = new NioSocketAcceptor();
+
+ acceptor.setReuseAddress(true);
+ DefaultIoFilterChainBuilder filters = acceptor.getFilterChain();
+
+ // Inject the SSL filter
+ SslFilter sslFilter = new SslFilter(createSSLContextBCJSSE());
+ filters.addLast("sslFilter", sslFilter);
+ sslFilter.setNeedClientAuth(true);
+
+ // Inject the TestLine codec filter
+ filters.addLast("text", new ProtocolCodecFilter(new
TextLineCodecFactory()));
+
+ acceptor.setHandler(new TestHandler());
+ acceptor.bind(new InetSocketAddress(port));
+ }
+
private static void stopServer() {
acceptor.dispose();
}
@@ -135,6 +156,29 @@ public class SslTest {
connectAndSend();
}
+ /**
+ * Starts a client which will connect using SSL and then attempt
renegotiation
+ */
+ private static void startClientReneg() throws Exception {
+ address = InetAddress.getByName("localhost");
+
+ SSLContext context = createSSLContext();
+ factory = context.getSocketFactory();
+
+ connectAndRenegotiate();
+ }
+
+ private static void connectAndRenegotiate() throws Exception {
+ Socket parent = new Socket(address, port);
+ SSLSocket socket = (SSLSocket)factory.createSocket(parent,
address.getCanonicalHostName(), port, false);
+
+ socket.setEnabledProtocols(new String[]{ "TLSv1.2" });
+ socket.startHandshake();
+
+ // Attempt renegotiation on the connection
+ socket.startHandshake();
+ }
+
private static void connectAndSend() throws Exception {
Socket parent = new Socket(address, port);
Socket socket = factory.createSocket(parent,
address.getCanonicalHostName(), port, false);
@@ -174,6 +218,28 @@ public class SslTest {
return ctx;
}
+ private static SSLContext createSSLContextBCJSSE() throws IOException,
GeneralSecurityException {
+ char[] passphrase = "password".toCharArray();
+
+ SSLContext ctx = SSLContext.getInstance("TLS", "BCJSSE");
+// KeyManagerFactory kmf =
KeyManagerFactory.getInstance(KEY_MANAGER_FACTORY_ALGORITHM);
+// TrustManagerFactory tmf =
TrustManagerFactory.getInstance(KEY_MANAGER_FACTORY_ALGORITHM);
+ KeyManagerFactory kmf = KeyManagerFactory.getInstance("PKIX",
"BCJSSE");
+ TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX",
"BCJSSE");
+
+ KeyStore ks = KeyStore.getInstance("JKS");
+ KeyStore ts = KeyStore.getInstance("JKS");
+
+ ks.load(SslTest.class.getResourceAsStream("keystore.sslTest"),
passphrase);
+ ts.load(SslTest.class.getResourceAsStream("truststore.sslTest"),
passphrase);
+
+ kmf.init(ks, passphrase);
+ tmf.init(ts);
+ ctx.init(kmf.getKeyManagers(), tmf.getTrustManagers(), null);
+
+ return ctx;
+ }
+
@Test
public void testSSL() throws Exception {
try {
@@ -200,6 +266,48 @@ public class SslTest {
}
+ @Test
+ public void testSSLClientRenegRejected() throws Exception {
+ boolean missingBC = Security.getProvider("BC") == null;
+ boolean missingBCJSSE = Security.getProvider("BCJSSE") == null;
+
+ if (missingBC) {
+ Security.addProvider(new BouncyCastleProvider());
+ }
+ if (missingBCJSSE) {
+ Security.addProvider(new BouncyCastleJsseProvider());
+ }
+
+ try {
+ startServerBCJSSE();
+
+ Thread t = new Thread() {
+ public void run() {
+ try {
+ startClientReneg();
+ } catch (Exception e) {
+ clientError = e;
+ }
+ }
+ };
+ t.start();
+ t.join();
+
+ if (clientError != null) {
+ throw clientError;
+ }
+ } finally {
+ stopServer();
+
+ if (missingBC) {
+ Security.removeProvider("BC");
+ }
+ if (missingBCJSSE) {
+ Security.removeProvider("BCJSSE");
+ }
+ }
+ }
+
@Test
public void unsecureClientTryToConnectoToSecureServer() throws Exception {
try {
diff --git a/pom.xml b/pom.xml
index 81154972c..89ecca016 100644
--- a/pom.xml
+++ b/pom.xml
@@ -370,6 +370,24 @@
<artifactId>junit</artifactId>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.bouncycastle</groupId>
+ <artifactId>bcprov-debug-jdk18on</artifactId>
+ <version>1.77</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.bouncycastle</groupId>
+ <artifactId>bctls-debug-jdk18on</artifactId>
+ <version>1.77</version>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>org.bouncycastle</groupId>
+ <artifactId>bcutil-debug-jdk18on</artifactId>
+ <version>1.77</version>
+ <scope>test</scope>
+ </dependency>
</dependencies>
<profiles>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
