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....@oracle.com <mailto:xuelei....@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




# HG changeset patch
# User Jaikiran Pai <jaikiran....@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

Reply via email to