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

Lyor Goldstein · Lyor Goldstein · commit f992796a01d0 · 2023-12-21T20:55:33.000+02:00
diff --git a/CHANGES.md b/CHANGES.md
@@ -36,6 +36,12 @@
 
 ## Behavioral changes and enhancements
 
+### [GH-445 - Terrapin attack mitigation](https://github.com/apache/mina-sshd/issues/429)
+
+There is a **new** `CoreModuleProperties` property that controls the mitigation for the [Terrapin attach](https://terrapin-attack.com/) via what is known as
+"strict-KEX" (see [OpenSSH PROTOCOL - 1.9 transport: strict key exchange extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)).
+It is **disabled** by default due to its experimental nature and possible interoperability issues, so users who wish to use this feature must turn it on *explicitly*.
+
 ### New `ScpTransferEventListener` callback method
 
 Following [GH-428/GH-392](https://github.com/apache/mina-sshd/issues/428) a new `handleReceiveCommandAckInfo` method has been added to enable users to inspect
diff --git a/docs/standards.md b/docs/standards.md
@@ -29,23 +29,31 @@
     above mentioned hooks for [RFC 8308](https://tools.ietf.org/html/rfc8308).
 * [RFC 8731 - Secure Shell (SSH) Key Exchange Method Using Curve25519 and Curve448](https://tools.ietf.org/html/rfc8731)
 * [Key Exchange (KEX) Method Updates and Recommendations for Secure Shell](https://tools.ietf.org/html/draft-ietf-curdle-ssh-kex-sha2-03)
+
+## *OpenSSH*
 * [OpenSSH support for U2F/FIDO security keys](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.u2f)
     * **Note:** the server side supports these keys by default. The client side requires specific initialization
 * [OpenSSH public-key certificate authentication system for use by SSH](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.certkeys)
+* [OpenSSH 1.9 transport: strict key exchange extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+
+## SFTP version 3-6 + extensions
+
+* `supported` - [DRAFT 05 - section 4.4](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-05#section-4.4)
+* `supported2` - [DRAFT 13 section 5.4](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-13#section-5.4)
+* `versions` - [DRAFT 09 Section 4.6](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-4.6)
+* `vendor-id` - [DRAFT 09 - section 4.4](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-4.4)
+* `acl-supported` - [DRAFT 11 - section 5.4](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-11#section-5.4)
+* `newline` - [DRAFT 09 Section 4.3](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-4.3)
+* `md5-hash`, `md5-hash-handle` - [DRAFT 09 - section 9.1.1](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-9.1.1)
+* `check-file-handle`, `check-file-name` - [DRAFT 09 - section 9.1.2](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-9.1.2)
+* `copy-file`, `copy-data` - [DRAFT 00 - sections 6, 7](https://tools.ietf.org/id/draft-ietf-secsh-filexfer-extensions-00.txt)
+* `space-available` - [DRAFT 09 - section 9.2](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-9.2)
+* `filename-charset`, `filename-translation-control` - [DRAFT 13 - section 6](https://tools.ietf.org/html/draft-ietf-secsh-filexfer-13#section-6) - only client side
+* Several [OpenSSH SFTP extensions](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+	
+## Miscellaneous
+
 * [SSH proxy jumps](./internals.md#ssh-jumps)
-* SFTP version 3-6 + extensions
-    * `supported` - [DRAFT 05 - section 4.4](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-05#section-4.4)
-    * `supported2` - [DRAFT 13 section 5.4](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-13#section-5.4)
-    * `versions` - [DRAFT 09 Section 4.6](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-4.6)
-    * `vendor-id` - [DRAFT 09 - section 4.4](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-4.4)
-    * `acl-supported` - [DRAFT 11 - section 5.4](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-11#section-5.4)
-    * `newline` - [DRAFT 09 Section 4.3](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-4.3)
-    * `md5-hash`, `md5-hash-handle` - [DRAFT 09 - section 9.1.1](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-9.1.1)
-    * `check-file-handle`, `check-file-name` - [DRAFT 09 - section 9.1.2](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-9.1.2)
-    * `copy-file`, `copy-data` - [DRAFT 00 - sections 6, 7](https://tools.ietf.org/id/draft-ietf-secsh-filexfer-extensions-00.txt)
-    * `space-available` - [DRAFT 09 - section 9.2](https://datatracker.ietf.org/doc/html/draft-ietf-secsh-filexfer-09#section-9.2)
-    * `filename-charset`, `filename-translation-control` - [DRAFT 13 - section 6](https://tools.ietf.org/html/draft-ietf-secsh-filexfer-13#section-6) - only client side
-    * Several [OpenSSH SFTP extensions](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
 * [Endless tarpit](https://nullprogram.com/blog/2019/03/22/) - see [HOWTO(s)](./howto.md) section.
 
 ## Implemented/available support
diff --git a/docs/technical/kex.md b/docs/technical/kex.md
@@ -129,3 +129,11 @@ thread is not overrun by producers and actually can finish.
 Again, "client" and "server" could also be inverted. For instance, a client uploading
 files via SFTP might have an application thread pumping data through a channel, which
 might be blocked during KEX.
+
+## [OpenSSH 1.9 transport: strict key exchange extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+
+
+There is a **new** `CoreModuleProperties` property that controls the mitigation for the [Terrapin attack](https://terrapin-attack.com/) via what is known as "strict-KEX"
+It is **disabled** by default due to its experimental nature and possible interoperability issues, so users who wish to use this feature must turn it on *explicitly*.
+The pseudo KEX values are *appended* to the initial proposals sent to the peer and removed when received before proceeding with the standard KEX proposals negotiation so
+as not to interfere with it (other than marking that they were detected).
diff --git a/sshd-common/src/main/java/org/apache/sshd/common/kex/extension/KexExtensions.java b/sshd-common/src/main/java/org/apache/sshd/common/kex/extension/KexExtensions.java
@@ -59,6 +59,21 @@ public final class KexExtensions {
     public static final String CLIENT_KEX_EXTENSION = "ext-info-c";
     public static final String SERVER_KEX_EXTENSION = "ext-info-s";
 
+    /**
+     * Reminder:
+     *
+     * These pseudo-algorithms are only valid in the initial SSH2_MSG_KEXINIT and MUST be ignored if they are present in
+     * subsequent SSH2_MSG_KEXINIT packets.
+     *
+     * <B>Note:</B> these values are <U>appended</U> to the initial proposals and removed if received before proceeding
+     * with the standard KEX proposals negotiation.
+     *
+     * @see <A HREF="https://github.com/openssh/openssh-portable/blob/master/PROTOCOL">OpenSSH PROTOCOL - 1.9 transport:
+     *      strict key exchange extension</A>
+     */
+    public static final String STRICT_KEX_CLIENT_EXTENSION = "kex-strict-c-v00@openssh.com";
+    public static final String STRICT_KEX_SERVER_EXTENSION = "kex-strict-s-v00@openssh.com";
+
     @SuppressWarnings("checkstyle:Indentation")
     public static final Predicate<String> IS_KEX_EXTENSION_SIGNAL
             = n -> CLIENT_KEX_EXTENSION.equalsIgnoreCase(n) || SERVER_KEX_EXTENSION.equalsIgnoreCase(n);
diff --git a/sshd-core/src/main/java/org/apache/sshd/common/kex/AbstractKexFactoryManager.java b/sshd-core/src/main/java/org/apache/sshd/common/kex/AbstractKexFactoryManager.java
@@ -22,6 +22,7 @@
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import org.apache.sshd.common.NamedFactory;
 import org.apache.sshd.common.cipher.Cipher;
@@ -38,6 +39,13 @@
 public abstract class AbstractKexFactoryManager
         extends AbstractInnerCloseable
         implements KexFactoryManager {
+    /** Input packet sequence number. */
+    protected long seqi;
+    /** Output packet sequence number. */
+    protected long seqo;
+    protected final AtomicBoolean newKeysSignaledHolder = new AtomicBoolean();
+    protected final AtomicBoolean strictKexSignalled = new AtomicBoolean();
+
     private final KexFactoryManager delegate;
     private List<KeyExchangeFactory> keyExchangeFactories;
     private List<NamedFactory<Cipher>> cipherFactories;
@@ -130,6 +138,14 @@ public void setKexExtensionHandler(KexExtensionHandler kexExtensionHandler) {
         this.kexExtensionHandler = kexExtensionHandler;
     }
 
+    protected boolean isNewKeysSignalled() {
+        return newKeysSignaledHolder.get();
+    }
+
+    protected boolean isStrictKexSignalled() {
+        return strictKexSignalled.get();
+    }
+
     protected <V, C extends Collection<V>> C resolveEffectiveFactories(C local, C inherited) {
         if (GenericUtils.isEmpty(local)) {
             return inherited;
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
@@ -180,10 +180,6 @@ public abstract class AbstractSession extends SessionHelper {
     protected byte[] inMacResult;
     protected Compression outCompression;
     protected Compression inCompression;
-    /** Input packet sequence number. */
-    protected long seqi;
-    /** Output packet sequence number. */
-    protected long seqo;
     protected SessionWorkBuffer uncompressBuffer;
     protected final SessionWorkBuffer decoderBuffer;
     protected int decoderState;
@@ -202,7 +198,9 @@ public abstract class AbstractSession extends SessionHelper {
      * Rekeying
      */
     protected final AtomicLong inPacketsCount = new AtomicLong(0L);
+    protected final AtomicLong totalIncomingPacketCount = new AtomicLong(0L);
     protected final AtomicLong outPacketsCount = new AtomicLong(0L);
+    protected final AtomicLong totalOutgingPacketCount = new AtomicLong(0L);
     protected final AtomicLong inBytesCount = new AtomicLong(0L);
     protected final AtomicLong outBytesCount = new AtomicLong(0L);
     protected final AtomicLong inBlocksCount = new AtomicLong(0L);
@@ -540,8 +538,29 @@ 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 ((totalIncomingPacketCount.get() == 1L)
+                && 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));
         }
 
@@ -674,6 +693,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 +923,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 +1150,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 messages so that
+         * the peer does not close the connection
+         */
         if ((ignorePacketDataLength <= 0)
                 || (ignorePacketsFrequency <= 0L)
-                || (ignorePacketsVariance < 0)) {
+                || (ignorePacketsVariance < 0)
+                || (isStrictKexSignalled() && (!isNewKeysSignalled()))) {
             return 0;
         }
 
@@ -1467,6 +1507,7 @@ protected Buffer encode(Buffer buffer) throws IOException {
 
             // Update counters used to track re-keying
             outPacketsCount.incrementAndGet();
+            totalOutgingPacketCount.incrementAndGet();
             outBytesCount.addAndGet(len);
 
             // Make buffer ready to be read
@@ -1643,6 +1684,7 @@ protected void decode() throws Exception {
 
                     // Update counters used to track re-keying
                     inPacketsCount.incrementAndGet();
+                    totalIncomingPacketCount.incrementAndGet();
                     inBytesCount.addAndGet(packet.available());
 
                     // Process decoded packet
@@ -2683,7 +2725,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);
diff --git a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java
@@ -77,6 +77,7 @@
 /**
  * Contains split code in order to make {@link AbstractSession} class smaller
  */
+@SuppressWarnings("checkstyle:MethodCount")
 public abstract class SessionHelper extends AbstractKexFactoryManager implements Session {
 
     // Session timeout measurements
@@ -127,6 +128,7 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements
      */
     protected SessionHelper(boolean serverSession, FactoryManager factoryManager, IoSession ioSession) {
         super(Objects.requireNonNull(factoryManager, "No factory manager provided"));
+
         this.serverSession = serverSession;
         this.ioSession = Objects.requireNonNull(ioSession, "No IoSession provided");
     }
@@ -224,6 +226,65 @@ public void setAuthenticated() throws IOException {
         }
     }
 
+    /**
+     * 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;
+    }
+
+    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 our 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);
+        }
+    }
+
     /**
      * Checks whether the session has timed out (both authentication and idle timeouts are checked). If the session has
      * timed out, a DISCONNECT message will be sent.
diff --git a/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java b/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java
@@ -147,6 +147,13 @@ public final class CoreModuleProperties {
     public static final Property<Duration> KEX_PROPOSAL_SETUP_TIMEOUT
             = Property.durationSec("kex-proposal-setup-timeout", Duration.ofSeconds(42), Duration.ofSeconds(5));
 
+    /**
+     * @see <A HREF="https://github.com/openssh/openssh-portable/blob/master/PROTOCOL">OpenSSH PROTOCOL - 1.9 transport:
+     *      strict key exchange extension</A>
+     */
+    public static final Property<Boolean> USE_STRICT_KEX
+            = Property.bool("use-strict-kex", false);
+
     /**
      * Key used to set the heartbeat interval in milliseconds (0 to disable = default)
      */
