zookeeper-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From an...@apache.org
Subject [zookeeper] branch master updated: ZOOKEEPER-3195: TLS - disable client-initiated renegotiation
Date Mon, 14 Jan 2019 18:40:22 GMT
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 <ilyam@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 <ilyam@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]);


Mime
View raw message