From e20370827bf9441c91de0baeb696a4cf9d6f75a6 Mon Sep 17 00:00:00 2001 From: "Brian P. Hinz" Date: Sun, 2 Nov 2014 23:37:54 -0500 Subject: [PATCH] Major overhaul of SSLEngineManager The SSLEngineManager was basically an abomination. The work is now done as it should be, with the buffers being fed and the engine deciding when data is consumed or produced. The engine should be much more robust now as well. Additionally, although JRE 7 supports the TLSv1.1 and TLSv1.2 protocols, they are not actually enabled by default. The JSSE reference cites compatibility reasons for this but this doesn't appear to be the case with the TigerVNC server and they will be enabled by default in JRE 8. The regular expression for enabling anonymous DH cipher suites was too narrow and excluded the elliptic curve ciphers, which are now ordered ahead of the ephemeral ciphers by the default security provider. Lastly, increase the size of the buffer in FdOutStream from 8Kb to 16Kb. I'm not sure why FdInStream and FdOutStream were asymmetric to begin with, but 16Kb is the default size for TLS packets and there seems to be now negative effects on plain text connections. --- .../tigervnc/network/SSLEngineManager.java | 291 ++++++++---------- java/com/tigervnc/rdr/FdInStream.java | 8 +- java/com/tigervnc/rdr/FdOutStream.java | 6 +- java/com/tigervnc/rfb/CSecurityTLS.java | 30 +- 4 files changed, 151 insertions(+), 184 deletions(-) diff --git a/java/com/tigervnc/network/SSLEngineManager.java b/java/com/tigervnc/network/SSLEngineManager.java index 654e602c..cb1f7c42 100644 --- a/java/com/tigervnc/network/SSLEngineManager.java +++ b/java/com/tigervnc/network/SSLEngineManager.java @@ -1,4 +1,4 @@ -/* Copyright (C) 2012 Brian P. Hinz +/* Copyright (C) 2012,2014 Brian P. Hinz * Copyright (C) 2012 D. R. Commander. All Rights Reserved. * * This library is free software; you can redistribute it and/or @@ -32,15 +32,14 @@ public class SSLEngineManager { private SSLEngine engine = null; - private int applicationBufferSize; - private int packetBufferSize; + private int appBufSize; + private int pktBufSize; private ByteBuffer myAppData; private ByteBuffer myNetData; private ByteBuffer peerAppData; private ByteBuffer peerNetData; - private Executor executor; private FdInStream inStream; private FdOutStream outStream; @@ -54,13 +53,13 @@ public class SSLEngineManager { executor = Executors.newSingleThreadExecutor(); - packetBufferSize = engine.getSession().getPacketBufferSize(); - applicationBufferSize = engine.getSession().getApplicationBufferSize(); + pktBufSize = engine.getSession().getPacketBufferSize(); + appBufSize = engine.getSession().getApplicationBufferSize(); - myAppData = ByteBuffer.allocate(applicationBufferSize); - myNetData = ByteBuffer.allocate(packetBufferSize); - peerAppData = ByteBuffer.allocate(applicationBufferSize); - peerNetData = ByteBuffer.allocate(packetBufferSize); + myAppData = ByteBuffer.allocate(appBufSize); + myNetData = ByteBuffer.allocate(pktBufSize); + peerAppData = ByteBuffer.allocate(appBufSize); + peerNetData = ByteBuffer.allocate(pktBufSize); } public void doHandshake() throws Exception { @@ -71,75 +70,77 @@ public class SSLEngineManager { // Process handshaking message while (hs != SSLEngineResult.HandshakeStatus.FINISHED && - hs != SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING) { - - switch (hs) { - - case NEED_UNWRAP: - // Receive handshaking data from peer - pull(peerNetData); - //if (pull(peerNetData) < 0) { - // Handle closed channel - //} - - // Process incoming handshaking data + hs != SSLEngineResult.HandshakeStatus.NOT_HANDSHAKING) { + + switch (hs) { + + case NEED_UNWRAP: + // Receive handshaking data from peer + peerNetData.flip(); + SSLEngineResult res = engine.unwrap(peerNetData, peerAppData); + peerNetData.compact(); + hs = res.getHandshakeStatus(); + switch (res.getStatus()) { + case BUFFER_UNDERFLOW: + int len = Math.min(peerNetData.remaining(), inStream.getBufSize()); + int m = inStream.check(1, len, false); + byte[] buf = new byte[m]; + inStream.readBytes(buf, 0, m); + peerNetData.put(buf, 0, m); peerNetData.flip(); - SSLEngineResult res = engine.unwrap(peerNetData, peerAppData); peerNetData.compact(); - hs = res.getHandshakeStatus(); - - // Check status - switch (res.getStatus()) { - case OK : - // Handle OK status - break; - - // Handle other status: BUFFER_UNDERFLOW, BUFFER_OVERFLOW, CLOSED - //... - } break; - - case NEED_WRAP : - // Empty the local network packet buffer. + + case OK: + // Process incoming handshaking data + break; + + case CLOSED: + engine.closeInbound(); + break; + + } + break; + + case NEED_WRAP : + // Empty the local network packet buffer. + myNetData.clear(); + + // Generate handshaking data + res = engine.wrap(myAppData, myNetData); + hs = res.getHandshakeStatus(); + + // Check status + switch (res.getStatus()) { + case OK : + myAppData.compact(); + myNetData.flip(); + int n = myNetData.remaining(); + byte[] b = new byte[n]; + myNetData.get(b); myNetData.clear(); - - // Generate handshaking data - res = engine.wrap(myAppData, myNetData); - hs = res.getHandshakeStatus(); - - // Check status - switch (res.getStatus()) { - case OK : - //myNetData.flip(); - - push(myNetData); - // Send the handshaking data to peer - //while (myNetData.hasRemaining()) { - // if (push(myNetData) < 0) { - // // Handle closed channel - // } - //} - break; - - // Handle other status: BUFFER_OVERFLOW, BUFFER_UNDERFLOW, CLOSED - //... - } + outStream.writeBytes(b, 0, n); + outStream.flush(); break; - - case NEED_TASK : - // Handle blocking tasks - executeTasks(); + + case BUFFER_OVERFLOW: + // FIXME: How much larger should the buffer be? + // fallthrough + + case CLOSED: + engine.closeOutbound(); break; - - // Handle other status: // FINISHED or NOT_HANDSHAKING - //... + } + break; + case NEED_TASK : + // Handle blocking tasks + executeTasks(); + break; + } hs = engine.getHandshakeStatus(); } - - // Processes after handshaking - //... -} + } private void executeTasks() { Runnable task; @@ -150,112 +151,68 @@ public class SSLEngineManager { public int read(byte[] data, int dataPtr, int length) throws IOException { // Read SSL/TLS encoded data from peer - int bytesRead = 0; - SSLEngineResult res; - do { - // Process incoming data - int packetLength = pull(peerNetData); + int len = Math.min(pktBufSize,inStream.getBufSize()); + int bytesRead = inStream.check(1,len,false); + byte[] buf = new byte[bytesRead]; + inStream.readBytes(buf, 0, bytesRead); + if (peerNetData.remaining() < bytesRead) { peerNetData.flip(); - res = engine.unwrap(peerNetData, peerAppData); - peerNetData.compact(); - bytesRead += packetLength; - } while (res.getStatus() != SSLEngineResult.Status.OK); - peerAppData.flip(); - int n = Math.min(peerAppData.remaining(), length); - peerAppData.get(data, dataPtr, n); - peerAppData.compact(); - return n; + ByteBuffer b = ByteBuffer.allocate(peerNetData.remaining() + bytesRead); + b.put(peerNetData); + peerNetData = b; + } + peerNetData.put(buf); + peerNetData.flip(); + SSLEngineResult res = engine.unwrap(peerNetData, peerAppData); + peerNetData.compact(); + switch (res.getStatus()) { + case OK : + peerAppData.flip(); + peerAppData.get(data, dataPtr, res.bytesProduced()); + peerAppData.compact(); + break; + + case BUFFER_UNDERFLOW: + // normal (need more net data) + break; + + case CLOSED: + engine.closeInbound(); + break; + + } + return res.bytesProduced(); } public int write(byte[] data, int dataPtr, int length) throws IOException { int n = 0; -//while (myAppData.hasRemaining()) { - // Generate SSL/TLS encoded data (handshake or application data) - // FIXME: - // Need to make sure myAppData has space for data! + // FIXME: resize myAppData if necessary myAppData.put(data, dataPtr, length); myAppData.flip(); - SSLEngineResult res = engine.wrap(myAppData, myNetData); - myAppData.compact(); - - // Process status of call - //if (res.getStatus() == SSLEngineResult.Status.OK) { - //myAppData.compact(); - - // Send SSL/TLS encoded data to peer - //while(myNetData.hasRemaining()) { - int num = push(myNetData); - if (num == -1) { - // handle closed channel - } else if (num == 0) { - // no bytes written; try again later - } - //n += num; - //} - //} - - // Handle other status: BUFFER_OVERFLOW, CLOSED - //... - //} - return num; - } - - private int push(ByteBuffer src) throws IOException { - src.flip(); - int n = src.remaining(); - byte[] b = new byte[n]; - src.get(b); - src.clear(); - outStream.writeBytes(b, 0, n); - outStream.flush(); - return n; - } - - private int pull(ByteBuffer dst) throws IOException { - inStream.checkNoWait(5); - //if (!inStream.checkNoWait(5)) { - // return 0; - //} - - byte[] header = new byte[5]; - inStream.readBytes(header, 0, 5); - - // Reference: http://publib.boulder.ibm.com/infocenter/tpfhelp/current/index.jsp?topic=%2Fcom.ibm.ztpf-ztpfdf.doc_put.cur%2Fgtps5%2Fs5rcd.html - int sslRecordType = header[0] & 0xFF; - int sslVersion = header[1] & 0xFF; - int sslDataLength = (int)((header[3] << 8) | (header[4] & 0xFF)); - - if (sslRecordType < 20 || sslRecordType > 23 || sslVersion != 3 || - sslDataLength == 0) { - // Not SSL v3 or TLS. Could be SSL v2 or bad data - - // Reference: http://www.homeport.org/~adam/ssl.html - // and the SSL v2 protocol specification - int headerBytes; - if ((header[0] & 0x80) != 0x80) { - headerBytes = 2; - sslDataLength = (int)(((header[0] & 0x7f) << 8) | header[1]); - } else { - headerBytes = 3; - sslDataLength = (int)(((header[0] & 0x3f) << 8) | header[1]); + while (myAppData.hasRemaining()) { + SSLEngineResult res = engine.wrap(myAppData, myNetData); + n += res.bytesConsumed(); + switch (res.getStatus()) { + case BUFFER_OVERFLOW: + ByteBuffer b = ByteBuffer.allocate(myNetData.capacity() + myAppData.remaining()); + myNetData.flip(); + b.put(myNetData); + myNetData = b; + break; + case CLOSED: + engine.closeOutbound(); + break; } - - // In SSL v2, the version is part of the handshake - sslVersion = header[headerBytes + 1] & 0xFF; - if (sslVersion < 2 || sslVersion > 3 || sslDataLength == 0) - throw new IOException("not an SSL/TLS record"); - - // The number of bytes left to read - sslDataLength -= (5 - headerBytes); } - - assert sslDataLength > 0; - - byte[] buf = new byte[sslDataLength]; - inStream.readBytes(buf, 0, sslDataLength); - dst.put(header); - dst.put(buf); - return sslDataLength; + myAppData.clear(); + myNetData.flip(); + int len = myNetData.remaining(); + byte[] buf = new byte[len]; + myNetData.get(buf); + myNetData.clear(); + outStream.writeBytes(buf, 0, len); + outStream.flush(); + return n; } public SSLSession getSession() { diff --git a/java/com/tigervnc/rdr/FdInStream.java b/java/com/tigervnc/rdr/FdInStream.java index f4e59008..b782f74a 100644 --- a/java/com/tigervnc/rdr/FdInStream.java +++ b/java/com/tigervnc/rdr/FdInStream.java @@ -1,5 +1,5 @@ /* Copyright (C) 2002-2005 RealVNC Ltd. All Rights Reserved. - * Copyright (C) 2012 Brian P. Hinz + * Copyright (C) 2012,2014 Brian P. Hinz * * This is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -27,7 +27,7 @@ import java.util.Iterator; public class FdInStream extends InStream { - static final int DEFAULT_BUF_SIZE = 8192; + static final int DEFAULT_BUF_SIZE = 16384; static final int minBulkSize = 1024; public FdInStream(FileDescriptor fd_, int timeoutms_, int bufSize_, @@ -223,6 +223,10 @@ public class FdInStream extends InStream { fd = fd_; } + public int getBufSize() { + return bufSize; + } + private FileDescriptor fd; boolean closeWhenDone; protected int timeoutms; diff --git a/java/com/tigervnc/rdr/FdOutStream.java b/java/com/tigervnc/rdr/FdOutStream.java index 27db1428..a9dea781 100644 --- a/java/com/tigervnc/rdr/FdOutStream.java +++ b/java/com/tigervnc/rdr/FdOutStream.java @@ -1,6 +1,6 @@ /* Copyright (C) 2002-2005 RealVNC Ltd. All Rights Reserved. * Copyright 2011 Pierre Ossman for Cendio AB - * Copyright (C) 2012 Brian P. Hinz + * Copyright (C) 2012,2014 Brian P. Hinz * * This is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -167,6 +167,10 @@ public class FdOutStream extends OutStream { fd = fd_; } + public int getBufSize() { + return bufSize; + } + protected FileDescriptor fd; protected boolean blocking; protected int timeoutms; diff --git a/java/com/tigervnc/rfb/CSecurityTLS.java b/java/com/tigervnc/rfb/CSecurityTLS.java index 86a3caf7..6f799bb4 100644 --- a/java/com/tigervnc/rfb/CSecurityTLS.java +++ b/java/com/tigervnc/rfb/CSecurityTLS.java @@ -3,7 +3,7 @@ * Copyright (C) 2005 Martin Koegler * Copyright (C) 2010 m-privacy GmbH * Copyright (C) 2010 TigerVNC Team - * Copyright (C) 2011-2012 Brian P. Hinz + * Copyright (C) 2011-2012,2014 Brian P. Hinz * * This is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -32,6 +32,7 @@ import java.io.InputStream; import java.io.FileInputStream; import java.util.ArrayList; import java.util.Collection; +import java.util.Collections; import javax.swing.JOptionPane; import com.tigervnc.rdr.*; @@ -126,13 +127,8 @@ public class CSecurityTLS extends CSecurity { try { manager = new SSLEngineManager(engine, is, os); - } catch(java.lang.Exception e) { - throw new Exception(e.toString()); - } - - try { manager.doHandshake(); - } catch (java.lang.Exception e) { + } catch(java.lang.Exception e) { throw new Exception(e.toString()); } @@ -166,22 +162,28 @@ public class CSecurityTLS extends CSecurity { client.getServerPort()); engine.setUseClientMode(true); - if (anon) { - String[] supported; - ArrayList enabled = new ArrayList(); + String[] supported = engine.getSupportedProtocols(); + ArrayList enabled = new ArrayList(); + for (int i = 0; i < supported.length; i++) + if (supported[i].matches("TLS.*")) + enabled.add(supported[i]); + engine.setEnabledProtocols(enabled.toArray(new String[0])); + if (anon) { supported = engine.getSupportedCipherSuites(); - + enabled = new ArrayList(); + // prefer ECDH over DHE + for (int i = 0; i < supported.length; i++) + if (supported[i].matches("TLS_ECDH_anon.*")) + enabled.add(supported[i]); for (int i = 0; i < supported.length; i++) if (supported[i].matches("TLS_DH_anon.*")) - enabled.add(supported[i]); - + enabled.add(supported[i]); engine.setEnabledCipherSuites(enabled.toArray(new String[0])); } else { engine.setEnabledCipherSuites(engine.getSupportedCipherSuites()); } - engine.setEnabledProtocols(new String[]{"SSLv3","TLSv1"}); } class MyHandshakeListener implements HandshakeCompletedListener { -- 2.39.5