Skip to content

Commit d0005d8

Browse files
yadvryadvr
committed
CLOUDSTACK-9348: Improve Nio SSH handshake buffers
Use a holder class to pass buffers, fixes potential leak. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com>
1 parent 893f2af commit d0005d8

4 files changed

Lines changed: 58 additions & 20 deletions

File tree

‎engine/orchestration/src/com/cloud/agent/manager/ClusteredAgentManagerImpl.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -519,7 +519,7 @@ public SocketChannel connectToPeer(final String peerName, final SocketChannel pr
519519
sslEngine.setUseClientMode(true);
520520
sslEngine.setEnabledProtocols(SSLUtils.getSupportedProtocols(sslEngine.getEnabledProtocols()));
521521
sslEngine.beginHandshake();
522-
if (!Link.doHandshake(ch1, sslEngine, true)) {
522+
if (!Link.doHandshake(ch1, sslEngine)) {
523523
ch1.close();
524524
throw new IOException(String.format("SSL: Handshake failed with peer management server '%s' on %s:%d ", peerName, ip, port));
525525
}

‎utils/src/main/java/com/cloud/utils/nio/Link.java‎

Lines changed: 55 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@
3232
import java.security.KeyStore;
3333
import java.security.SecureRandom;
3434
import java.util.concurrent.ConcurrentLinkedQueue;
35+
import java.util.concurrent.Executor;
36+
import java.util.concurrent.Executors;
3537

3638
import javax.net.ssl.KeyManagerFactory;
3739
import javax.net.ssl.SSLContext;
@@ -462,7 +464,7 @@ public static ByteBuffer enlargeBuffer(ByteBuffer buffer, final int sessionPropo
462464
return buffer;
463465
}
464466

465-
public static ByteBuffer handleBufferUnderflow(final SSLEngine engine, ByteBuffer buffer) {
467+
public static ByteBuffer handleBufferUnderflow(final SSLEngine engine, final ByteBuffer buffer) {
466468
if (engine == null || buffer == null) {
467469
return buffer;
468470
}
@@ -475,14 +477,14 @@ public static ByteBuffer handleBufferUnderflow(final SSLEngine engine, ByteBuffe
475477
return replaceBuffer;
476478
}
477479

478-
private static boolean doHandshakeUnwrap(final SocketChannel socketChannel, final SSLEngine sslEngine,
480+
private static HandshakeHolder doHandshakeUnwrap(final SocketChannel socketChannel, final SSLEngine sslEngine,
479481
ByteBuffer peerAppData, ByteBuffer peerNetData, final int appBufferSize) throws IOException {
480482
if (socketChannel == null || sslEngine == null || peerAppData == null || peerNetData == null || appBufferSize < 0) {
481-
return false;
483+
return new HandshakeHolder(peerAppData, peerNetData, false);
482484
}
483485
if (socketChannel.read(peerNetData) < 0) {
484486
if (sslEngine.isInboundDone() && sslEngine.isOutboundDone()) {
485-
return false;
487+
return new HandshakeHolder(peerAppData, peerNetData, false);
486488
}
487489
try {
488490
sslEngine.closeInbound();
@@ -492,7 +494,7 @@ private static boolean doHandshakeUnwrap(final SocketChannel socketChannel, fina
492494
sslEngine.closeOutbound();
493495
// After closeOutbound the engine will be set to WRAP state,
494496
// in order to try to send a close message to the client.
495-
return true;
497+
return new HandshakeHolder(peerAppData, peerNetData, true);
496498
}
497499
peerNetData.flip();
498500
SSLEngineResult result = null;
@@ -503,7 +505,10 @@ private static boolean doHandshakeUnwrap(final SocketChannel socketChannel, fina
503505
s_logger.error(String.format("SSL error caught during unwrap data: %s, for local address=%s, remote address=%s. The client may have invalid ca-certificates.",
504506
sslException.getMessage(), socketChannel.getLocalAddress(), socketChannel.getRemoteAddress()));
505507
sslEngine.closeOutbound();
506-
return false;
508+
return new HandshakeHolder(peerAppData, peerNetData, true);
509+
}
510+
if (result == null) {
511+
return new HandshakeHolder(peerAppData, peerNetData, false);
507512
}
508513
switch (result.getStatus()) {
509514
case OK:
@@ -519,23 +524,23 @@ private static boolean doHandshakeUnwrap(final SocketChannel socketChannel, fina
519524
break;
520525
case CLOSED:
521526
if (sslEngine.isOutboundDone()) {
522-
return false;
527+
return new HandshakeHolder(peerAppData, peerNetData, false);
523528
} else {
524529
sslEngine.closeOutbound();
525-
break;
526530
}
531+
break;
527532
default:
528533
throw new IllegalStateException("Invalid SSL status: " + result.getStatus());
529534
}
530-
return true;
535+
return new HandshakeHolder(peerAppData, peerNetData, true);
531536
}
532537

533-
private static boolean doHandshakeWrap(final SocketChannel socketChannel, final SSLEngine sslEngine,
538+
private static HandshakeHolder doHandshakeWrap(final SocketChannel socketChannel, final SSLEngine sslEngine,
534539
ByteBuffer myAppData, ByteBuffer myNetData, ByteBuffer peerNetData,
535540
final int netBufferSize) throws IOException {
536541
if (socketChannel == null || sslEngine == null || myNetData == null || peerNetData == null
537542
|| myAppData == null || netBufferSize < 0) {
538-
return false;
543+
return new HandshakeHolder(myAppData, myNetData, false);
539544
}
540545
myNetData.clear();
541546
SSLEngineResult result = null;
@@ -545,7 +550,10 @@ private static boolean doHandshakeWrap(final SocketChannel socketChannel, final
545550
s_logger.error(String.format("SSL error caught during wrap data: %s, for local address=%s, remote address=%s.",
546551
sslException.getMessage(), socketChannel.getLocalAddress(), socketChannel.getRemoteAddress()));
547552
sslEngine.closeOutbound();
548-
return false;
553+
return new HandshakeHolder(myAppData, myNetData, true);
554+
}
555+
if (result == null) {
556+
return new HandshakeHolder(myAppData, myNetData, false);
549557
}
550558
switch (result.getStatus()) {
551559
case OK :
@@ -579,10 +587,10 @@ private static boolean doHandshakeWrap(final SocketChannel socketChannel, final
579587
default:
580588
throw new IllegalStateException("Invalid SSL status: " + result.getStatus());
581589
}
582-
return true;
590+
return new HandshakeHolder(myAppData, myNetData, true);
583591
}
584592

585-
public static boolean doHandshake(final SocketChannel socketChannel, final SSLEngine sslEngine, final boolean isClient) throws IOException {
593+
public static boolean doHandshake(final SocketChannel socketChannel, final SSLEngine sslEngine) throws IOException {
586594
if (socketChannel == null || sslEngine == null) {
587595
return false;
588596
}
@@ -593,6 +601,7 @@ public static boolean doHandshake(final SocketChannel socketChannel, final SSLEn
593601
ByteBuffer myNetData = ByteBuffer.allocate(netBufferSize);
594602
ByteBuffer peerNetData = ByteBuffer.allocate(netBufferSize);
595603

604+
final Executor executor = Executors.newSingleThreadExecutor();
596605
final long startTimeMills = System.currentTimeMillis();
597606

598607
HandshakeStatus handshakeStatus = sslEngine.getHandshakeStatus();
@@ -606,12 +615,17 @@ public static boolean doHandshake(final SocketChannel socketChannel, final SSLEn
606615
}
607616
switch (handshakeStatus) {
608617
case NEED_UNWRAP:
609-
if (!doHandshakeUnwrap(socketChannel, sslEngine, peerAppData, peerNetData, appBufferSize)) {
618+
final HandshakeHolder unwrapResult = doHandshakeUnwrap(socketChannel, sslEngine, peerAppData, peerNetData, appBufferSize);
619+
peerAppData = unwrapResult.getAppDataBuffer();
620+
peerNetData = unwrapResult.getNetDataBuffer();
621+
if (!unwrapResult.isSuccess()) {
610622
return false;
611623
}
612624
break;
613625
case NEED_WRAP:
614-
if (!doHandshakeWrap(socketChannel, sslEngine, myAppData, myNetData, peerNetData, netBufferSize)) {
626+
final HandshakeHolder wrapResult = doHandshakeWrap(socketChannel, sslEngine, myAppData, myNetData, peerNetData, netBufferSize);
627+
myNetData = wrapResult.getNetDataBuffer();
628+
if (!wrapResult.isSuccess()) {
615629
return false;
616630
}
617631
break;
@@ -621,7 +635,7 @@ public static boolean doHandshake(final SocketChannel socketChannel, final SSLEn
621635
if (s_logger.isTraceEnabled()) {
622636
s_logger.trace("SSL: Running delegated task!");
623637
}
624-
task.run();
638+
executor.execute(task);
625639
}
626640
break;
627641
case FINISHED:
@@ -636,4 +650,28 @@ public static boolean doHandshake(final SocketChannel socketChannel, final SSLEn
636650
return true;
637651
}
638652

653+
private static class HandshakeHolder {
654+
private ByteBuffer appData;
655+
private ByteBuffer netData;
656+
private boolean success = true;
657+
658+
HandshakeHolder(ByteBuffer appData, ByteBuffer netData, boolean success) {
659+
this.appData = appData;
660+
this.netData = netData;
661+
this.success = success;
662+
}
663+
664+
ByteBuffer getAppDataBuffer() {
665+
return appData;
666+
}
667+
668+
ByteBuffer getNetDataBuffer() {
669+
return netData;
670+
}
671+
672+
boolean isSuccess() {
673+
return success;
674+
}
675+
}
676+
639677
}

‎utils/src/main/java/com/cloud/utils/nio/NioClient.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ protected void init() throws IOException {
6161
sslEngine.setUseClientMode(true);
6262
sslEngine.setEnabledProtocols(SSLUtils.getSupportedProtocols(sslEngine.getEnabledProtocols()));
6363
sslEngine.beginHandshake();
64-
if (!Link.doHandshake(_clientConnection, sslEngine, true)) {
64+
if (!Link.doHandshake(_clientConnection, sslEngine)) {
6565
s_logger.error("SSL Handshake failed while connecting to host: " + _host + " port: " + _port);
6666
_selector.close();
6767
throw new IOException("SSL Handshake failed while connecting to host: " + _host + " port: " + _port);

‎utils/src/main/java/com/cloud/utils/nio/NioConnection.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -213,7 +213,7 @@ public void run() {
213213
_selector.wakeup();
214214
try {
215215
sslEngine.beginHandshake();
216-
if (!Link.doHandshake(socketChannel, sslEngine, false)) {
216+
if (!Link.doHandshake(socketChannel, sslEngine)) {
217217
throw new IOException("SSL handshake timed out with " + socketChannel.getRemoteAddress());
218218
}
219219
if (s_logger.isTraceEnabled()) {

0 commit comments

Comments
 (0)