[PATCH] JDK-8190917 : SSL session resumption, through handshake, in SSLEngine is broken for any protocols lesser than TLSv1.2

Jaikiran Pai jai.forums2013 at gmail.com
Fri Feb 23 05:58:15 UTC 2018


Given some recent changes to the class involved in this patch, in the 
jdk repo (http://hg.openjdk.java.net/jdk/jdk), I noticed some merge 
conflicts to this patch today. So I've now attached an updated patch 
which resolves those merge issues. This has been tested with latest 
jtreg (tip).

-Jaikiran


On 06/12/17 9:35 PM, Jaikiran Pai wrote:
> Thank you Xuelei. Please take your time.
>
> -Jaikiran
>
>
>
> On Wednesday, December 6, 2017, Xuelei Fan <xuelei.fan at oracle.com 
> <mailto:xuelei.fan at oracle.com>> wrote:
>
>     Hi Jaikiran,
>
>     I will sponsor this contribution.  I need more time for the review
>     and testing.
>
>     Thanks,
>     Xuelei
>
>     On 11/23/2017 9:22 PM, Jaikiran Pai wrote:
>
>         As noted in [1], there's a regression in Java 9, where SSL
>         session resumption no longer works for SSL protocols other
>         than TLSv1.2. The code which is responsible for session
>         resumption resides in the ServerHandshaker[2], in the
>         clientHello method. This method, in its logic to decide
>         whether or not to resume a (previously) cached session, checks
>         if the client hello message has a session id associated. If it
>         does, it then fetches a session from the cache. If it does
>         find a previously cached session for that id, it then goes
>         ahead to check if the SSL protocol associated with the cached
>         session matches with the protocol version that's
>         "applicable/selected" of the incoming client hello message.
>         However, a relatively recent change[3] has, IMO,
>         unintentionally caused the protocol version check logic to
>         fail for protocols other than TLSv1.2. The protocol version
>         check looks like:
>
>
>         // cannot resume session with different
>
>         if (oldVersion != protocolVersion) {
>         resumingSession = false;
>         }
>
>         The `protocolVersion` variable in this case happens to be a
>         _default initialized value_ (TLSv1.2) instead of the one
>         that's "selected" based on the incoming client hello message.
>         As a result the `protocolVersion` value is always TLSv1.2 and
>         thus for any other protocols, this comparison fails, even when
>         the client hello message uses the right session id and the
>         right protocol that matches the previous session.
>
>         The attached patch, includes a potential fix to this issue.
>         The change makes sure that the protocol selection, based on
>         the client hello message, is done before this session
>         resumption check and also makes sure it uses that "selected"
>         protocol in the version comparison check instead of the class
>         member `protocolVersion` (which gets set when the server hello
>         message is finally being sent).
>
>         The attached patch also contains a (jtreg) test case which
>         reproduces this bug and verifies this fix. The test case tests
>         the session resumption against TLSv1, TLSv1.1 and TLSv1.2. The
>         test code itself is a minor modification of the SSL demo
>         that's available here [4].
>
>         This is my first OpenJDK patch and my OCA got approved a few
>         days back. Could someone please help with the review of this
>         patch?
>
>         P.S: I noticed that the JIRA got assigned a few days back. I
>         am not sure if that means the fix is already being worked upon
>         by someone else from the team. If that's the case, then let me
>         know and I'll let it be handled by them.
>
>         [1] https://bugs.openjdk.java.net/browse/JDK-8190917
>         <https://bugs.openjdk.java.net/browse/JDK-8190917>
>
>         [2]
>         http://hg.openjdk.java.net/jdk/jdk/file/b7ae1437111b/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java
>         <http://hg.openjdk.java.net/jdk/jdk/file/b7ae1437111b/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java>
>
>
>         [3] http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/42268eb6e04e
>         <http://hg.openjdk.java.net/jdk9/jdk9/jdk/rev/42268eb6e04e>
>
>         [4]
>         https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/samples/sslengine/SSLEngineSimpleDemo.java
>         <https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/samples/sslengine/SSLEngineSimpleDemo.java>
>
>
>         -Jaikiran
>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20180223/c8da0fe1/attachment-0001.html>
-------------- next part --------------
# HG changeset patch
# User Jaikiran Pai <jaikiran.pai at gmail.com>
# Date 1511499148 -19800
#      Fri Nov 24 10:22:28 2017 +0530
# Node ID ddfa1ea49b5a0316988eb8b218f19a6c20ca529c
# Parent  03ae177c26b016353e5ea1cab6ffd051dfa086ca
JDK-8190917 Make sure SSL session resumption works (not just for TLSv1.2)

diff --git a/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java b/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java
--- a/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java
+++ b/src/java.base/share/classes/sun/security/ssl/ServerHandshaker.java
@@ -589,6 +589,18 @@
         }  // Otherwise, applicationProtocol will be set by the callback.
 
         session = null; // forget about the current session
+        clientRequestedVersion = mesg.protocolVersion;
+
+        // select a proper protocol version.
+        final ProtocolVersion selectedVersion =
+               selectProtocolVersion(clientRequestedVersion);
+        if (selectedVersion == null ||
+                selectedVersion.v == ProtocolVersion.SSL20Hello.v) {
+            fatalSE(Alerts.alert_handshake_failure,
+                "Client requested protocol " + clientRequestedVersion +
+                " not enabled or not supported");
+        }
+
         //
         // Here we go down either of two paths:  (a) the fast one, where
         // the client's asked to rejoin an existing session, and the server
@@ -613,7 +625,7 @@
                 if (resumingSession) {
                     ProtocolVersion oldVersion = previous.getProtocolVersion();
                     // cannot resume session with different version
-                    if (oldVersion != mesg.protocolVersion) {
+                    if (oldVersion != selectedVersion) {
                         resumingSession = false;
                     }
                 }
@@ -764,18 +776,6 @@
          */
         ServerHello m1 = new ServerHello();
 
-        clientRequestedVersion = mesg.protocolVersion;
-
-        // select a proper protocol version.
-        ProtocolVersion selectedVersion =
-               selectProtocolVersion(clientRequestedVersion);
-        if (selectedVersion == null ||
-                selectedVersion.v == ProtocolVersion.SSL20Hello.v) {
-            fatalSE(Alerts.alert_handshake_failure,
-                "Client requested protocol " + clientRequestedVersion +
-                " not enabled or not supported");
-        }
-
         handshakeHash.protocolDetermined(selectedVersion);
         setVersion(selectedVersion);
 
diff --git a/test/jdk/sun/security/ssl/SessionResumption/SSLSessionResumption.java b/test/jdk/sun/security/ssl/SessionResumption/SSLSessionResumption.java
new file mode 100644
--- /dev/null
+++ b/test/jdk/sun/security/ssl/SessionResumption/SSLSessionResumption.java
@@ -0,0 +1,371 @@
+/*
+ * Copyright (c) 2015, 2017, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import javax.net.ssl.KeyManagerFactory;
+import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLEngine;
+import javax.net.ssl.SSLEngineResult;
+import javax.net.ssl.SSLEngineResult.HandshakeStatus;
+import javax.net.ssl.SSLSession;
+import javax.net.ssl.TrustManagerFactory;
+import java.io.FileInputStream;
+import java.nio.ByteBuffer;
+import java.security.KeyStore;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+/*
+ * @test
+ * @bug 8190917
+ * @summary Verify that SSL session resumption works for all SSL protocols
+ * @run main SSLSessionResumption
+ */
+public class SSLSessionResumption {
+
+    private static final boolean debug = false;
+    private static String pathToStores = "../../../../javax/net/ssl/etc";
+    private static String keyStoreFile = "keystore";
+    private static String trustStoreFile = "truststore";
+    private static String passwd = "passphrase";
+
+    private static String keyFilename =
+            System.getProperty("test.src", "./") + "/" + pathToStores +
+                    "/" + keyStoreFile;
+    private static String trustFilename =
+            System.getProperty("test.src", "./") + "/" + pathToStores +
+                    "/" + trustStoreFile;
+
+
+    private final String sslProtocol;
+    private final SSLContext sslc;
+
+    private SSLEngine clientEngine;     // client Engine
+    private ByteBuffer clientOut;       // write side of clientEngine
+    private ByteBuffer clientIn;        // read side of clientEngine
+
+    private SSLEngine serverEngine;     // server Engine
+    private ByteBuffer serverOut;       // write side of serverEngine
+    private ByteBuffer serverIn;        // read side of serverEngine
+
+    private ByteBuffer cTOs;            // client->server
+    private ByteBuffer sTOc;            // server->client
+
+
+    private byte[] previousSessionId;    // session id of a previous established SSL session
+
+
+    private SSLSessionResumption(final String sslProtocol) throws Exception {
+        this.sslProtocol = sslProtocol;
+
+        final KeyStore ks = KeyStore.getInstance("JKS");
+        final KeyStore ts = KeyStore.getInstance("JKS");
+        final char[] passphrase = passwd.toCharArray();
+
+        ks.load(new FileInputStream(keyFilename), passphrase);
+        ts.load(new FileInputStream(trustFilename), passphrase);
+
+        final KeyManagerFactory kmf = KeyManagerFactory.getInstance("SunX509");
+        kmf.init(ks, passphrase);
+
+        final TrustManagerFactory tmf = TrustManagerFactory.getInstance("SunX509");
+        tmf.init(ts);
+
+        final SSLContext sslCtx = SSLContext.getInstance(this.sslProtocol);
+        sslCtx.init(kmf.getKeyManagers(), tmf.getTrustManagers(), null);
+
+        this.sslc = sslCtx;
+    }
+
+    public static void main(final String[] args) throws Exception {
+        // for each of these protocols we run the session resumption test
+        // individually
+        final String[] protocols = new String[]{"TLSv1", "TLSv1.1", "TLSv1.2"};
+        final Set<String> failedProtocols = new HashSet();
+        for (final String protocol : protocols) {
+            final SSLSessionResumption test = new SSLSessionResumption(protocol);
+            // verify that the session resumption works
+            if (!test.verifySessionResumption()) {
+                failedProtocols.add(protocol);
+            }
+        }
+        if (!failedProtocols.isEmpty()) {
+            throw new Exception("SSL session resumption failed for protocols " + failedProtocols);
+        }
+    }
+
+    /**
+     * Uses {@link SSLEngine} in the context of a {@link SSLContext} to do some SSL backed
+     * communication and verifies that the communication reuses a session
+     *
+     * @return Returns true if the SSL session was reused. False otherwise
+     * @throws Exception
+     */
+    private boolean verifySessionResumption() throws Exception {
+        try {
+            for (int i = 0; i < 2; i++) {
+                log("Starting SSL communication " + (i + 1) + " using protocol " + this.sslProtocol);
+                this.doSSLCommunication();
+            }
+        } catch (SessionIdMisMatchException e) {
+            return false;
+        }
+        return true;
+    }
+
+
+    private void doSSLCommunication() throws Exception {
+        boolean dataDone = false;
+
+        createSSLEngines();
+        createBuffers();
+
+        SSLEngineResult clientResult;   // results from client's last operation
+        SSLEngineResult serverResult;   // results from server's last operation
+
+        /*
+         * Examining the SSLEngineResults could be much more involved,
+         * and may alter the overall flow of the application.
+         *
+         * For example, if we received a BUFFER_OVERFLOW when trying
+         * to write to the output pipe, we could reallocate a larger
+         * pipe, but instead we wait for the peer to drain it.
+         */
+        while (!isEngineClosed(clientEngine) ||
+                !isEngineClosed(serverEngine)) {
+
+            log("================");
+
+            clientResult = clientEngine.wrap(clientOut, cTOs);
+            if (clientResult.getHandshakeStatus() == HandshakeStatus.FINISHED) {
+                assertSessionId(this.previousSessionId, clientEngine);
+                this.previousSessionId = clientEngine.getSession().getId();
+            }
+            log("client wrap: ", clientResult);
+            runDelegatedTasks(clientResult, clientEngine);
+
+            serverResult = serverEngine.wrap(serverOut, sTOc);
+            log("server wrap: ", serverResult);
+            runDelegatedTasks(serverResult, serverEngine);
+
+            cTOs.flip();
+            sTOc.flip();
+
+            log("----");
+
+            clientResult = clientEngine.unwrap(sTOc, clientIn);
+            if (clientResult.getHandshakeStatus() == HandshakeStatus.FINISHED) {
+                assertSessionId(this.previousSessionId, clientEngine);
+                this.previousSessionId = clientEngine.getSession().getId();
+            }
+            log("client unwrap: ", clientResult);
+            runDelegatedTasks(clientResult, clientEngine);
+
+            serverResult = serverEngine.unwrap(cTOs, serverIn);
+            log("server unwrap: ", serverResult);
+            runDelegatedTasks(serverResult, serverEngine);
+
+            cTOs.compact();
+            sTOc.compact();
+
+            /*
+             * After we've transfered all application data between the client
+             * and server, we close the clientEngine's outbound stream.
+             * This generates a close_notify handshake message, which the
+             * server engine receives and responds by closing itself.
+             *
+             * In normal operation, each SSLEngine should call
+             * closeOutbound().  To protect against truncation attacks,
+             * SSLEngine.closeInbound() should be called whenever it has
+             * determined that no more input data will ever be
+             * available (say a closed input stream).
+             */
+            if (!dataDone && (clientOut.limit() == serverIn.position()) &&
+                    (serverOut.limit() == clientIn.position())) {
+
+                /*
+                 * A sanity check to ensure we got what was sent.
+                 */
+                checkTransfer(serverOut, clientIn);
+                checkTransfer(clientOut, serverIn);
+
+                log("\tClosing clientEngine's *OUTBOUND*...");
+                clientEngine.closeOutbound();
+                // serverEngine.closeOutbound();
+                dataDone = true;
+            }
+        }
+    }
+
+    /*
+     * Using the SSLContext created during object creation,
+     * create/configure the SSLEngines we'll use for this demo.
+     */
+    private void createSSLEngines() throws Exception {
+        /*
+         * Configure the serverEngine to act as a server in the SSL/TLS
+         * handshake.
+         */
+        serverEngine = sslc.createSSLEngine();
+        serverEngine.setUseClientMode(false);
+        serverEngine.setNeedClientAuth(false);
+
+        /*
+         * Similar to above, but using client mode instead.
+         */
+        clientEngine = sslc.createSSLEngine("client", 80);
+        clientEngine.setUseClientMode(true);
+    }
+
+    /*
+     * Create and size the buffers appropriately.
+     */
+    private void createBuffers() {
+
+        /*
+         * We'll assume the buffer sizes are the same
+         * between client and server.
+         */
+        SSLSession session = clientEngine.getSession();
+        int appBufferMax = session.getApplicationBufferSize();
+        int netBufferMax = session.getPacketBufferSize();
+
+        /*
+         * We'll make the input buffers a bit bigger than the max needed
+         * size, so that unwrap()s following a successful data transfer
+         * won't generate BUFFER_OVERFLOWS.
+         *
+         * We'll use a mix of direct and indirect ByteBuffers for
+         * tutorial purposes only.  In reality, only use direct
+         * ByteBuffers when they give a clear performance enhancement.
+         */
+        clientIn = ByteBuffer.allocate(appBufferMax + 50);
+        serverIn = ByteBuffer.allocate(appBufferMax + 50);
+
+        cTOs = ByteBuffer.allocateDirect(netBufferMax);
+        sTOc = ByteBuffer.allocateDirect(netBufferMax);
+
+        clientOut = ByteBuffer.wrap("JDK-8190917 : Hi Server, I'm Client".getBytes());
+        serverOut = ByteBuffer.wrap("JDK-8190917 : Hello Client, I'm Server".getBytes());
+    }
+
+    /*
+     * If the result indicates that we have outstanding tasks to do,
+     * go ahead and run them in this thread.
+     */
+    private static void runDelegatedTasks(final SSLEngineResult result,
+                                          final SSLEngine engine) throws Exception {
+
+        if (result.getHandshakeStatus() == HandshakeStatus.NEED_TASK) {
+            Runnable runnable;
+            while ((runnable = engine.getDelegatedTask()) != null) {
+                log("\trunning delegated task...");
+                runnable.run();
+            }
+            final HandshakeStatus hsStatus = engine.getHandshakeStatus();
+            if (hsStatus == HandshakeStatus.NEED_TASK) {
+                throw new Exception(
+                        "handshake shouldn't need additional tasks");
+            }
+            log("\tnew HandshakeStatus: " + hsStatus);
+        }
+    }
+
+    private static boolean isEngineClosed(SSLEngine engine) {
+        return (engine.isOutboundDone() && engine.isInboundDone());
+    }
+
+    /*
+     * Simple check to make sure everything came across as expected.
+     */
+    private static void checkTransfer(ByteBuffer a, ByteBuffer b)
+            throws Exception {
+        a.flip();
+        b.flip();
+
+        if (!a.equals(b)) {
+            throw new Exception("Data didn't transfer cleanly");
+        } else {
+            log("\tData transferred cleanly");
+        }
+
+        a.position(a.limit());
+        b.position(b.limit());
+        a.limit(a.capacity());
+        b.limit(b.capacity());
+    }
+
+    private static void assertSessionId(final byte[] expected, final SSLEngine engine) throws SessionIdMisMatchException {
+        if (expected == null) {
+            // we haven't yet created a session previously, so there isn't any
+            // session to be expected to resume
+            return;
+        }
+        final byte[] sessionId = engine.getSession().getId();
+        // compare and verify if they are same
+        if (!java.util.Arrays.equals(expected, sessionId)) {
+            throw new SessionIdMisMatchException(expected, sessionId);
+        }
+    }
+
+    private static boolean resultOnce = true;
+
+    private static void log(final String str, final SSLEngineResult result) {
+        if (!debug) {
+            return;
+        }
+        if (resultOnce) {
+            resultOnce = false;
+            log("The format of the SSLEngineResult is: \n" +
+                    "\t\"getStatus() / getHandshakeStatus()\" +\n" +
+                    "\t\"bytesConsumed() / bytesProduced()\"\n");
+        }
+        final HandshakeStatus hsStatus = result.getHandshakeStatus();
+        log(str +
+                result.getStatus() + "/" + hsStatus + ", " +
+                result.bytesConsumed() + "/" + result.bytesProduced() +
+                " bytes");
+        if (hsStatus == HandshakeStatus.FINISHED) {
+            log("\t...ready for application data");
+        }
+    }
+
+    private static void log(final String message) {
+        if (debug) {
+            System.out.println(message);
+        }
+    }
+
+    private static final class SessionIdMisMatchException extends Exception {
+
+        private final byte[] expected;
+        private final byte[] actual;
+
+        private SessionIdMisMatchException(final byte[] expected, final byte[] actual) {
+            super("SSL session id mismatch - expected " + Arrays.toString(expected) + " actual " + Arrays.toString(actual));
+            this.expected = expected;
+            this.actual = actual;
+        }
+    }
+
+}
\ No newline at end of file


More information about the security-dev mailing list