[mina-sshd] 03/03: KEX: change output encoding atomically

[tw...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

twolf pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mina-sshd.git

commit d7b7649f56983d470a79c0fdb720cfd548227b87
Author: Thomas Wolf <tw...@apache.org>
AuthorDate: Sun Apr 10 18:53:34 2022 +0200

    KEX: change output encoding atomically
    
    Ensure the `encodeLock` is held for sending the `SSH_MSG_NEWKEYS`
    message and changing the output encoding.
    
    For the input encoding a similar change is not needed since the
    `decodeLock` is held anyway.
    
    Strictly speaking acquiring the `encodeLock` should not be necessary
    since any other potentially concurrent writes should be queued because
    the KEX state is not yet `DONE`.
---
 .../common/session/helpers/AbstractSession.java    | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
index e4032b9aa..a556a2b4e 100644
--- a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
+++ b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
@@ -608,11 +608,18 @@ public abstract class AbstractSession extends SessionHelper {
             log.debug("sendNewKeys({}) Send SSH_MSG_NEWKEYS", this);
         }
 
-        Buffer buffer = createBuffer(SshConstants.SSH_MSG_NEWKEYS, Byte.SIZE);
-        IoWriteFuture future = writePacket(buffer);
         prepareNewKeys();
-        // Use the new settings from now on for any outgoing packet
-        setOutputEncoding();
+        Buffer buffer = createBuffer(SshConstants.SSH_MSG_NEWKEYS, Byte.SIZE);
+        IoWriteFuture future;
+        synchronized (encodeLock) {
+            // writePacket() would also work since it would never try to queue the packet, and would never try to
+            // initiate a new KEX, and thus would never try to get the kexLock monitor. If it did, we might get a
+            // deadlock due to lock inversion. It seems safer to push this out directly, though.
+            future = doWritePacket(buffer);
+            // Use the new settings from now on for any outgoing packet
+            setOutputEncoding();
+        }
+        resetIdleTimeout();
         /*
          * According to https://tools.ietf.org/html/rfc8308#section-2.4:
          *
@@ -625,12 +632,9 @@ public abstract class AbstractSession extends SessionHelper {
          * + As the next packet following the server's first SSH_MSG_NEWKEYS.
          */
         KexExtensionHandler extHandler = getKexExtensionHandler();
-        if ((extHandler == null)
-                || (!extHandler.isKexExtensionsAvailable(this, AvailabilityPhase.NEWKEYS))) {
-            return future;
+        if ((extHandler != null) && extHandler.isKexExtensionsAvailable(this, AvailabilityPhase.NEWKEYS)) {
+            extHandler.sendKexExtensions(this, KexPhase.NEWKEYS);
         }
-
-        extHandler.sendKexExtensions(this, KexPhase.NEWKEYS);
         return future;
     }