[apacheGH-445] lay down the groundwork for mitigating the Terrapin attack

Author: Lyor Goldstein

sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java

@@ -41,6 +41,7 @@
 import java.util.concurrent.ScheduledFuture;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.LongConsumer;
@@ -188,6 +189,8 @@ public abstract class AbstractSession extends SessionHelper {
     protected final SessionWorkBuffer decoderBuffer;
     protected int decoderState;
     protected int decoderLength;
+    protected final AtomicBoolean newKeysSignaledHolder = new AtomicBoolean();
+    protected final AtomicBoolean strictKexSignalled = new AtomicBoolean();
     protected final Object encodeLock = new Object();
     protected final Object decodeLock = new Object();
     protected final Object requestLock = new Object();
@@ -540,8 +543,27 @@ protected void handleMessage(Buffer buffer) throws Exception {
 
     protected void doHandleMessage(Buffer buffer) throws Exception {
         int cmd = buffer.getUByte();
+
+        /*
+         * Terrapin attack mitigation - see https://github.com/openssh/openssh-portable/blob/master/PROTOCOL
+         * section 1.9 transport: strict key exchange extension
+         *
+         *      During initial KEX, terminate the connection if any unexpected or
+         *      out-of-sequence packet is received. This includes terminating the
+         *      connection if the first packet received is not SSH2_MSG_KEXINIT.
+         *
+         *      Unexpected packets for the purpose of strict KEX include messages
+         *      that are otherwise valid at any time during the connection such as
+         *      SSH2_MSG_DEBUG and SSH2_MSG_IGNORE.
+         */
+        if (isStrictKexSignalled() && (cmd != SshConstants.SSH_MSG_KEXINIT)) {
+            log.error("doHandleMessage({}) invalid 1st message: {}",
+                    this, SshConstants.getCommandMessageName(cmd));
+            throw new SshException(SshConstants.SSH2_DISCONNECT_PROTOCOL_ERROR, "Strict KEX Error");
+        }
+
         if (log.isDebugEnabled()) {
-            log.debug("doHandleMessage({}) process #{} {}", this, seqi - 1,
+            log.debug("doHandleMessage({}) process #{} {}", this, seqi - 1L,
                     SshConstants.getCommandMessageName(cmd));
         }
 
@@ -655,6 +677,73 @@ protected Map.Entry<String, String> comparePreferredKexProposalOption(KexProposa
         return null;
     }
 
+    protected void resetSequenceNumbers(boolean sentNewkeys) {
+        /*
+         * We rely on the fact that SSH_MSG_NEWKEYS is symmetric and if we initiated one then an
+         * incoming one is due from our peer (and vice versa). Therefore:
+         *
+         *      - if we initiated the message, we can reset our sequence number and
+         *      rely on receiving the peer's response to reset our tracking of
+         *      its counter. We still need it to decode our peer's response and
+         *      thus have to wait for it before resetting out tracking value.
+         *
+         *      - if we are the peer that received the message then we can reset
+         *      our tracking of the initiator's counter, relying on the fact that
+         *      it did it to its own counter. After (!) we send our response we will
+         *      reset our counter as well.
+         */
+        long prevSeqno;
+        synchronized (newKeysSignaledHolder) {
+            if (sentNewkeys) {
+                prevSeqno = seqo;
+                seqo = 0L;
+            } else {
+                prevSeqno = seqi;
+                seqi = 0L;
+            }
+
+        }
+
+        if (log.isDebugEnabled()) {
+            log.debug("resetSequenceNumbers({})[sentNewKeys={}] packet couter={}", this, sentNewkeys, prevSeqno);
+        }
+    }
+
+    protected boolean isNewKeysSignalled() {
+        return newKeysSignaledHolder.get();
+    }
+
+    protected boolean isStrictKexSignalled() {
+        return strictKexSignalled.get();
+    }
+
+    /**
+     * Called to indicate that {@link SshConstants#SSH_MSG_NEWKEYS} was either sent or received
+     *
+     * @param sentNewKeys Indicates whether the message was sent or received
+     * @return The previous state of the signalling holder
+     * @see #isNewKeysSignalled()
+     */
+    protected boolean newKeysSignalled(boolean sentNewKeys) {
+        boolean prev = newKeysSignaledHolder.getAndSet(true);
+        if (log.isDebugEnabled()) {
+            log.debug("newKeysSignalled({})[sentNewKeys={}] signalState={} -> {}", this, sentNewKeys, prev, true);
+        }
+
+        /*
+         * Terrapin attack mitigation - see https://github.com/openssh/openssh-portable/blob/master/PROTOCOL
+         * section 1.9: transport: strict key exchange extension
+         *
+         *      After sending or receiving a SSH2_MSG_NEWKEYS message,
+         *      reset the packet sequence number to zero.
+         */
+        if (isStrictKexSignalled()) {
+            resetSequenceNumbers(sentNewKeys);
+        }
+
+        return prev;
+    }
+
     /**
      * Send a message to put new keys into use.
      *
@@ -674,6 +763,9 @@ protected IoWriteFuture sendNewKeys() throws Exception {
             // 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);
+
+            newKeysSignalled(true);
+
             // Use the new settings from now on for any outgoing packet
             setOutputEncoding();
         }
@@ -901,6 +993,16 @@ protected void handleNewKeys(int cmd, Buffer buffer) throws Exception {
                     this, SshConstants.getCommandMessageName(cmd));
         }
         validateKexState(cmd, KexState.KEYS);
+
+        /*
+         * Terrapin attack mitigation - see https://github.com/openssh/openssh-portable/blob/master/PROTOCOL
+         * section 1.9: transport: strict key exchange extension
+         *
+         *      After sending or receiving a SSH2_MSG_NEWKEYS message,
+         *      reset the packet sequence number to zero.
+         */
+        newKeysSignalled(false);
+
         // It is guaranteed that we handle the peer's SSH_MSG_NEWKEYS after having sent our own.
         // prepareNewKeys() was already called in sendNewKeys().
         //
@@ -1118,9 +1220,17 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws IOException {
     }
 
     protected int resolveIgnoreBufferDataLength() {
+        /*
+         * Terrapin attack mitigation - see https://github.com/openssh/openssh-portable/blob/master/PROTOCOL
+         * section 1.9: transport: strict key exchange extension
+         *
+         * We need to defer sending any stuffing SSH_MSG_IGNORE message so that
+         * the peer does not close the connection
+         */
         if ((ignorePacketDataLength <= 0)
                 || (ignorePacketsFrequency <= 0L)
-                || (ignorePacketsVariance < 0)) {
+                || (ignorePacketsVariance < 0)
+                || isStrictKexSignalled() && (!isNewKeysSignalled())) {
             return 0;
         }
 
@@ -2683,7 +2793,7 @@ public MessageCodingSettings(Cipher cipher, Mac mac, Compression compression, Ci
             this.iv = iv.clone();
         }
 
-        private void initCipher(long packetSequenceNumber) throws Exception {
+        protected void initCipher(long packetSequenceNumber) throws Exception {
             if (key != null) {
                 if (cipher.getAlgorithm().startsWith("ChaCha")) {
                     BufferUtils.putLong(packetSequenceNumber, iv, 0, iv.length);