[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: <https://mail.openjdk.org/pipermail/security-dev/attachments/20180223/c8da0fe1/attachment.htm>
-------------- 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