From commits-return-7509-archive-asf-public=cust-asf.ponee.io@zookeeper.apache.org Mon Jan 14 19:40:47 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by mx-eu-01.ponee.io (Postfix) with SMTP id 8C9E4180608 for ; Mon, 14 Jan 2019 19:40:46 +0100 (CET) Received: (qmail 9477 invoked by uid 500); 14 Jan 2019 18:40:40 -0000 Mailing-List: contact commits-help@zookeeper.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@zookeeper.apache.org Delivered-To: mailing list commits@zookeeper.apache.org Received: (qmail 9464 invoked by uid 99); 14 Jan 2019 18:40:40 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 14 Jan 2019 18:40:40 +0000 From: GitBox To: commits@zookeeper.apache.org Subject: [zookeeper] Diff for: [GitHub] asfgit closed pull request #710: ZOOKEEPER-3195: TLS - disable client-initiated renegotiation Message-ID: <154749124006.24276.10452338922484433869.gitbox@gitbox.apache.org> Date: Mon, 14 Jan 2019 18:40:40 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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 4ea105b3e1..310f3e66fa 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 @@ 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 1058010feb..9c4a9b0f0b 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 void testGetSslHandshakeDetectionTimeoutMillisProperty() { } } + 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 acceptFuture; + acceptFuture = workerPool.submit(new Callable() { + @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]); With regards, Apache Git Services