You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zookeeper.apache.org by an...@apache.org on 2019/01/14 18:40:22 UTC
[zookeeper] branch master updated: ZOOKEEPER-3195: TLS - disable
client-initiated renegotiation
This is an automated email from the ASF dual-hosted git repository.
andor pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 38fab85 ZOOKEEPER-3195: TLS - disable client-initiated renegotiation
38fab85 is described below
commit 38fab85ab476ffea3dd9668eac6fd6e09a190a09
Author: Ilya Maykov <il...@fb.com>
AuthorDate: Mon Jan 14 11:40:19 2019 -0700
ZOOKEEPER-3195: TLS - disable client-initiated renegotiation
Summary: client-initiated renegotiation is insecure and is vulnerable to MITM attacks.
Unfortunately, the feature is enabled in Java by default. This disables it.
See https://bugs.openjdk.java.net/browse/JDK-7188658 and
https://www.oracle.com/technetwork/java/javase/documentation/tlsreadme-141115.html
Test Plan: manually tested by running a secure ZK server and probing the listening port
with python's sslyze tool (using `sslyze --reneg ...`). Tested on Java 8, 9, 10, and 11.
Author: Ilya Maykov <il...@fb.com>
Reviewers: andor@apache.org
Closes #710 from ivmaykov/ZOOKEEPER-3195
---
.../java/org/apache/zookeeper/common/X509Util.java | 15 ++++
.../org/apache/zookeeper/common/X509UtilTest.java | 95 +++++++++++++++++++++-
2 files changed, 109 insertions(+), 1 deletion(-)
diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
index 4ea105b..310f3e6 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
@@ -66,6 +66,21 @@ import org.slf4j.LoggerFactory;
public abstract class X509Util implements Closeable, AutoCloseable {
private static final Logger LOG = LoggerFactory.getLogger(X509Util.class);
+ private static final String REJECT_CLIENT_RENEGOTIATION_PROPERTY =
+ "jdk.tls.rejectClientInitiatedRenegotiation";
+ static {
+ // Client-initiated renegotiation in TLS is unsafe and
+ // allows MITM attacks, so we should disable it unless
+ // it was explicitly enabled by the user.
+ // A brief summary of the issue can be found at
+ // https://www.ietf.org/proceedings/76/slides/tls-7.pdf
+ if (System.getProperty(REJECT_CLIENT_RENEGOTIATION_PROPERTY) == null) {
+ LOG.info("Setting -D {}=true to disable client-initiated TLS renegotiation",
+ REJECT_CLIENT_RENEGOTIATION_PROPERTY);
+ System.setProperty(REJECT_CLIENT_RENEGOTIATION_PROPERTY, Boolean.TRUE.toString());
+ }
+ }
+
static final String DEFAULT_PROTOCOL = "TLSv1.2";
private static final String[] DEFAULT_CIPHERS_JAVA8 = {
"TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256",
diff --git a/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java b/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
index 1058010..9c4a9b0 100644
--- a/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
+++ b/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java
@@ -17,10 +17,24 @@
*/
package org.apache.zookeeper.common;
+import java.io.IOException;
+import java.net.InetAddress;
+import java.net.InetSocketAddress;
+import java.net.ServerSocket;
+import java.net.Socket;
import java.security.Security;
import java.util.Collection;
-
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+import java.util.concurrent.atomic.AtomicInteger;
+
+import javax.net.ssl.HandshakeCompletedEvent;
+import javax.net.ssl.HandshakeCompletedListener;
import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLHandshakeException;
import javax.net.ssl.SSLServerSocket;
import javax.net.ssl.SSLSocket;
import javax.net.ssl.X509KeyManager;
@@ -389,6 +403,85 @@ public class X509UtilTest extends BaseX509ParameterizedTestCase {
}
}
+ private static void forceClose(Socket s) {
+ if (s == null || s.isClosed()) {
+ return;
+ }
+ try {
+ s.close();
+ } catch (IOException e) {
+ }
+ }
+
+ private static void forceClose(ServerSocket s) {
+ if (s == null || s.isClosed()) {
+ return;
+ }
+ try {
+ s.close();
+ } catch (IOException e) {
+ }
+ }
+
+ // This test makes sure that client-initiated TLS renegotiation does not
+ // succeed. We explicitly disable it at the top of X509Util.java.
+ @Test(expected = SSLHandshakeException.class)
+ public void testClientRenegotiationFails() throws Throwable {
+ int port = PortAssignment.unique();
+ ExecutorService workerPool = Executors.newCachedThreadPool();
+ final SSLServerSocket listeningSocket = x509Util.createSSLServerSocket();
+ SSLSocket clientSocket = null;
+ SSLSocket serverSocket = null;
+ final AtomicInteger handshakesCompleted = new AtomicInteger(0);
+ try {
+ InetSocketAddress localServerAddress = new InetSocketAddress(
+ InetAddress.getLoopbackAddress(), port);
+ listeningSocket.bind(localServerAddress);
+ Future<SSLSocket> acceptFuture;
+ acceptFuture = workerPool.submit(new Callable<SSLSocket>() {
+ @Override
+ public SSLSocket call() throws Exception {
+ SSLSocket sslSocket = (SSLSocket) listeningSocket.accept();
+ sslSocket.addHandshakeCompletedListener(new HandshakeCompletedListener() {
+ @Override
+ public void handshakeCompleted(HandshakeCompletedEvent handshakeCompletedEvent) {
+ handshakesCompleted.getAndIncrement();
+ }
+ });
+ Assert.assertEquals(1, sslSocket.getInputStream().read());
+ try {
+ // 2nd read is after the renegotiation attempt and will fail
+ sslSocket.getInputStream().read();
+ return sslSocket;
+ } catch (Exception e) {
+ forceClose(sslSocket);
+ throw e;
+ }
+ }
+ });
+ clientSocket = x509Util.createSSLSocket();
+ clientSocket.connect(localServerAddress);
+ clientSocket.getOutputStream().write(1);
+ // Attempt to renegotiate after establishing the connection
+ clientSocket.startHandshake();
+ clientSocket.getOutputStream().write(1);
+ // The exception is thrown on the server side, we need to unwrap it
+ try {
+ serverSocket = acceptFuture.get();
+ } catch (ExecutionException e) {
+ throw e.getCause();
+ }
+ } finally {
+ forceClose(serverSocket);
+ forceClose(clientSocket);
+ forceClose(listeningSocket);
+ workerPool.shutdown();
+ // Make sure the first handshake completed and only the second
+ // one failed.
+ Assert.assertEquals(1, handshakesCompleted.get());
+ }
+ }
+
// Warning: this will reset the x509Util
private void setCustomCipherSuites() {
System.setProperty(x509Util.getCipherSuitesProperty(), customCipherSuites[0] + "," + customCipherSuites[1]);