Apache MINA SSHD: integrity check bypass

@@ -0,0 +1,192 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.kex.extension;
+
+import java.io.PipedInputStream;
+import java.io.PipedOutputStream;
+import java.nio.charset.StandardCharsets;
+
+import org.apache.sshd.client.ClientFactoryManager;
+import org.apache.sshd.client.SshClient;
+import org.apache.sshd.client.channel.ChannelShell;
+import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.client.session.ClientSessionImpl;
+import org.apache.sshd.client.session.SessionFactory;
+import org.apache.sshd.common.channel.StreamingChannel;
+import org.apache.sshd.common.io.IoSession;
+import org.apache.sshd.common.keyprovider.FileKeyPairProvider;
+import org.apache.sshd.util.test.BaseTestSupport;
+import org.apache.sshd.util.test.CommonTestSupportUtils;
+import org.apache.sshd.util.test.ContainerTestCase;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.testcontainers.containers.GenericContainer;
+import org.testcontainers.containers.output.Slf4jLogConsumer;
+import org.testcontainers.containers.wait.strategy.Wait;
+import org.testcontainers.images.builder.ImageFromDockerfile;
+import org.testcontainers.images.builder.dockerfile.DockerfileBuilder;
+import org.testcontainers.utility.MountableFile;
+
+/**
+ * Tests to ensure that an Apache MINA sshd client can talk to OpenSSH servers with or without "strict KEX". This
+ * implicitly tests the message sequence number handling; if sequence numbers get out of sync or are reset wrongly,
+ * subsequent messages cannot be decrypted correctly and there will be exceptions.
+ *
+ * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
+ * @see    <A HREF="https://github.com/apache/mina-sshd/issues/445">Terrapin Mitigation: &quot;strict-kex&quot;</A>
+ */
+@Category(ContainerTestCase.class)
+public class StrictKexInteroperabilityTest extends BaseTestSupport {
+
+    private static final Logger LOG = LoggerFactory.getLogger(StrictKexInteroperabilityTest.class);
+
+    private static final String TEST_RESOURCES = "org/apache/sshd/common/kex/extensions/client";
+
+    private SshClient client;
+
+    public StrictKexInteroperabilityTest() {
+        super();
+    }
+
+    @Before
+    public void setUp() throws Exception {
+        client = setupTestClient();
+        SessionFactory factory = new TestSessionFactory(client);
+        client.setSessionFactory(factory);
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        if (client != null) {
+            client.stop();
+        }
+    }
+
+    private DockerfileBuilder strictKexImage(DockerfileBuilder builder, boolean withStrictKex) {
+        if (!withStrictKex) {
+            return builder
+                    // CentOS 7 is EOL and thus unlikely to get the security update for strict KEX.
+                    .from("centos:7.9.2009") //
+                    .run("yum install -y openssh-server") // Installs OpenSSH 7.4
+                    .run("/usr/sbin/sshd-keygen") // Generate multiple host keys
+                    .run("adduser bob"); // Add a user
+        } else {
+            return builder
+                    .from("alpine:20231219") //
+                    .run("apk --update add openssh-server") // Installs OpenSSH 9.6
+                    .run("ssh-keygen -A") // Generate multiple host keys
+                    .run("adduser -D bob") // Add a user
+                    .run("echo 'bob:passwordBob' | chpasswd"); // Give it a password to unlock the user
+        }
+    }
+
+    @Test
+    public void testStrictKexOff() throws Exception {
+        testStrictKex(false);
+    }
+
+    @Test
+    public void testStrictKexOn() throws Exception {
+        testStrictKex(true);
+    }
+
+    private void testStrictKex(boolean withStrictKex) throws Exception {
+        // This tests that the message sequence numbers are handled correctly. Strict KEX resets them to zero on any
+        // KEX, without strict KEX, they're not reset. If sequence numbers get out of sync, received messages are
+        // decrypted wrongly and there will be exceptions.
+        @SuppressWarnings("resource")
+        GenericContainer<?> sshdContainer = new GenericContainer<>(new ImageFromDockerfile()
+                .withDockerfileFromBuilder(builder -> strictKexImage(builder, withStrictKex) //
+                        .run("mkdir -p /home/bob/.ssh") // Create the SSH config directory
+                        .entryPoint("/entrypoint.sh") //
+                        .build())) //
+                                .withCopyFileToContainer(MountableFile.forClasspathResource(TEST_RESOURCES + "/bob_key.pub"),
+                                        "/home/bob/.ssh/authorized_keys")
+                                // entrypoint must be executable. Spotbugs doesn't like 0777, so use hex
+                                .withCopyFileToContainer(
+                                        MountableFile.forClasspathResource(TEST_RESOURCES + "/entrypoint.sh", 0x1ff),
+                                        "/entrypoint.sh")
+                                .waitingFor(Wait.forLogMessage(".*Server listening on :: port 22.*\\n", 1)) //
+                                .withExposedPorts(22) //
+                                .withLogConsumer(new Slf4jLogConsumer(LOG));
+        sshdContainer.start();
+        try {
+            FileKeyPairProvider keyPairProvider = CommonTestSupportUtils.createTestKeyPairProvider(TEST_RESOURCES + "/bob_key");
+            client.setKeyIdentityProvider(keyPairProvider);
+            client.start();
+            try (ClientSession session = client.connect("bob", sshdContainer.getHost(), sshdContainer.getMappedPort(22))
+                    .verify(CONNECT_TIMEOUT).getSession()) {
+                session.auth().verify(AUTH_TIMEOUT);
+                assertTrue("Should authenticate", session.isAuthenticated());
+                assertTrue("Unexpected session type " + session.getClass().getName(), session instanceof TestSession);
+                assertEquals("Unexpected strict KEX usage", withStrictKex, ((TestSession) session).usesStrictKex());
+                try (ChannelShell channel = session.createShellChannel()) {
+                    channel.setOut(System.out);
+                    channel.setErr(System.err);
+                    channel.setStreaming(StreamingChannel.Streaming.Sync);
+                    PipedOutputStream pos = new PipedOutputStream();
+                    PipedInputStream pis = new PipedInputStream(pos);
+                    channel.setIn(pis);
+                    assertTrue("Could not open session", channel.open().await(DEFAULT_TIMEOUT));
+                    LOG.info("writing some data...");
+                    pos.write("\n\n".getBytes(StandardCharsets.UTF_8));
+                    assertTrue("Channel should be open", channel.isOpen());
+                    assertTrue(session.reExchangeKeys().verify(CONNECT_TIMEOUT).isDone());
+                    assertTrue("Channel should be open", channel.isOpen());
+                    LOG.info("writing some data...");
+                    pos.write("\n\n".getBytes(StandardCharsets.UTF_8));
+                    assertTrue("Channel should be open", channel.isOpen());
+                    channel.close(true);
+                }
+            }
+        } finally {
+            sshdContainer.stop();
+        }
+    }
+
+    // Subclass ClientSessionImpl to get access to the strictKex flag.
+
+    private static class TestSessionFactory extends SessionFactory {
+
+        TestSessionFactory(ClientFactoryManager client) {
+            super(client);
+        }
+
+        @Override
+        protected ClientSessionImpl doCreateSession(IoSession ioSession) throws Exception {
+            return new TestSession(getClient(), ioSession);
+        }
+    }
+
+    private static class TestSession extends ClientSessionImpl {
+
+        TestSession(ClientFactoryManager client, IoSession ioSession) throws Exception {
+            super(client, ioSession);
+        }
+
+        boolean usesStrictKex() {
+            return strictKex;
+        }
+    }
+}

@@ -0,0 +1,27 @@
+-----BEGIN RSA PRIVATE KEY-----
+MIIEpAIBAAKCAQEAxY3Hr1SqpJIQ9SbFfGMGweVy8jg2TEH3GC1K0LudQHJwogRi
++debdCqUtuSITbpPhjkeZSk9rq198d6RhT6TQmY9J8wLL2/+VXZk/rMVEEjeXQS3
+ImRnL2vVmkAunv6LwfDGHIovkhwj3/lqGWphDAKnHyXusPDwQ3N4LFGgxwXvRGqc
+lzmP8H+KDWaaPapk1AZCBIoD4JbL8faBtLNU01r+pB3sIKvfsPJ5DxPErThfrPuD
+qIbA3axEqFlgX4aVl3yMnSWjfhLhO7xD3YwrtUhannHt8pZQo5FkwCGWDpkG3xs+
+qK3ZACrhMFMTvPuDS83jDtEzNd5KYb4KnkOPMQIDAQABAoIBAQCE5GktgrD/39pU
+b25tzFehW25FjpbIGZ/UvbMUUwDnd5RZCMZj9yv1qyc7GOSwFOKmEgpmVqXNuZt9
+dxFBJuT8x7Xf7Zygnp/icbBivakvuTUMMb3X/t6CwfGAwCgcgHMXVZaPYE275f4k
+Dq3Wxv7di3NMusGkeY/GcAipF4gmGKKe7Ck1ifRypF2cDJsgTtsoFUHNNKfnT3gf
+OcJsVLRl0osbsxdqU+Tep46+jHrNt8J9n2VeRNRIqGHj0CkNdpLQOs+MjvIO3Hgq
+9NUxwIExwaPnBpTLlWwfemCz3JQnlAineMbYBGa1tpAA3Iw56NWcNbiOPyUyffbI
+wBC4r1uZAoGBAPESsergFD+ontChEI+h38oM/D9DKCObZR2kz6WArZ54i1dJWOgh
+HCsuxgPjxmaddPKghfNhUORdZBynuS5G7n6BfItNilDiFm2KBk12d38OVovUFo1Q
+r5akclKf0kFxHt5TzHIrNAv7B4OF0Uk3kuDHM7ITX3qDpTSBLlzPAUUHAoGBANHJ
+QIPmuF2q+PXnnSgdEyiETfl/IqUTXQyxda8kRIPJKKHZKPHZePhgJKUq9VP32PrP
+AxIBNrS3Netsp+EAApj09hmWUcgJRIU1/wjpVGqUmguYgh8nVFOPDudOJD5ltQ/A
+enzQ19IkGroaQB8CBGZsPaBAvqRZ5PLbm+BZEPQHAoGAblaMMGCXY/udlQfjOJpy
+f1wqKBpoyMNbKJJCqBGZZaruu+jKVJSy++DQqP8b0+PFnzdxl8+24o8MP0FVNKUq
+i6RgiLHY2ORiN4ixEctjLjg1zJIqMEv50g06di7IYUORSVk5fhfgHourCLu66rQQ
++eiy9JKBZOXUO4/U1I26mwkCgYAhfuCuLsiBLCtUGAcfwISuk3FfxMzjTpQs0qjX
+rhLCd/vk26eN9gs6nR88v/8ryQb8BNGYrljtwdL6I/8qDbZcdcBVlYq5RcGLA3QV
+GCxCWDfAYjlkgAMW1GCsze07iUG/ohvskevjwaAC1u4mBUxujhnI3I2T8EZ+AFKD
+H7V1QQKBgQDNt+zjSdLtA9AczxDwWmi5SbS+k+nGbi6AQO9i73wky/wxx7FonfWS
+2skkOUIst3HBc0Oz+CJTfNFQK6GVqtzTdlZFhMYS0ua1Djd6q6S648+K0cieY4r5
+5irivHYVN8t7lBcvbA7E7yD6dHXSHsn6yOLTrV382qRfJTbxG7ZVWA==
+-----END RSA PRIVATE KEY-----

@@ -0,0 +1 @@
+ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDFjcevVKqkkhD1JsV8YwbB5XLyODZMQfcYLUrQu51AcnCiBGL515t0KpS25IhNuk+GOR5lKT2urX3x3pGFPpNCZj0nzAsvb/5VdmT+sxUQSN5dBLciZGcva9WaQC6e/ovB8MYcii+SHCPf+WoZamEMAqcfJe6w8PBDc3gsUaDHBe9EapyXOY/wf4oNZpo9qmTUBkIEigPglsvx9oG0s1TTWv6kHewgq9+w8nkPE8StOF+s+4OohsDdrESoWWBfhpWXfIydJaN+EuE7vEPdjCu1SFqece3yllCjkWTAIZYOmQbfGz6ordkAKuEwUxO8+4NLzeMO0TM13kphvgqeQ48x user01

@@ -0,0 +1,6 @@
+#!/bin/sh
+
+chown -R bob /home/bob
+chmod 0600 /home/bob/.ssh/*
+
+/usr/sbin/sshd -D -ddd

@@ -124,6 +124,7 @@
                         <exclude>**/SessionReKeyHostKeyExchangeTest.java</exclude>
                         <exclude>**/HostBoundPubKeyAuthTest.java</exclude>
                         <exclude>**/PortForwardingWithOpenSshTest.java</exclude>
+                        <exclude>**/StrictKexInteroperabilityTest.java</exclude>
                         <!-- reading files from classpath doesn't work correctly w/ reusable test jar -->
                         <exclude>**/OpenSSHCertificateTest.java</exclude>
                     </excludes>

@@ -143,6 +143,7 @@
                         <exclude>**/SessionReKeyHostKeyExchangeTest.java</exclude>
                         <exclude>**/HostBoundPubKeyAuthTest.java</exclude>
                         <exclude>**/PortForwardingWithOpenSshTest.java</exclude>
+                        <exclude>**/StrictKexInteroperabilityTest.java</exclude>
                         <!-- reading files from classpath doesn't work correctly w/ reusable test jar -->
                         <exclude>**/OpenSSHCertificateTest.java</exclude>
                     </excludes>

@@ -0,0 +1,264 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sshd.common.kex.extension;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicReference;
+
+import org.apache.sshd.client.SshClient;
+import org.apache.sshd.client.session.ClientSession;
+import org.apache.sshd.common.SshConstants;
+import org.apache.sshd.common.SshException;
+import org.apache.sshd.common.io.IoWriteFuture;
+import org.apache.sshd.common.kex.KexProposalOption;
+import org.apache.sshd.common.session.Session;
+import org.apache.sshd.common.session.SessionListener;
+import org.apache.sshd.common.util.GenericUtils;
+import org.apache.sshd.server.SshServer;
+import org.apache.sshd.util.test.BaseTestSupport;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.FixMethodOrder;
+import org.junit.Test;
+import org.junit.runners.MethodSorters;
+
+/**
+ * Tests for message handling during "strict KEX" is active: initial KEX must fail and disconnect if the KEX_INIT
+ * message is not first, or if there are spurious extra messages like IGNORE or DEBUG during KEX. Later KEXes must
+ * succeed even if there are spurious messages.
+ * <p>
+ * The other part of "strict KEX" is resetting the message sequence numbers after KEX. This is not tested here but in
+ * the {@link StrictKexInteroperabilityTest}, which runs an Apache MINA sshd client against OpenSSH servers that have or
+ * do not have the "strict KEX" extension. If the sequence number handling was wrong, those tests would fail.
+ * </p>
+ *
+ * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
+ * @see    <A HREF="https://github.com/apache/mina-sshd/issues/445">Terrapin Mitigation: &quot;strict-kex&quot;</A>
+ */
+@FixMethodOrder(MethodSorters.NAME_ASCENDING)
+public class StrictKexTest extends BaseTestSupport {
+    private SshServer sshd;
+    private SshClient client;
+
+    public StrictKexTest() {
+        super();
+    }
+
+    @Before
+    public void setUp() throws Exception {
+        sshd = setupTestServer();
+        client = setupTestClient();
+    }
+
+    @After
+    public void tearDown() throws Exception {
+        if (sshd != null) {
+            sshd.stop(true);
+        }
+        if (client != null) {
+            client.stop();
+        }
+    }
+
+    @Test
+    public void connectionClosedIfFirstPacketFromClientNotKexInit() throws Exception {
+        testConnectionClosedIfFirstPacketFromPeerNotKexInit(true);
+    }
+
+    @Test
+    public void connectionClosedIfFirstPacketFromServerNotKexInit() throws Exception {
+        testConnectionClosedIfFirstPacketFromPeerNotKexInit(false);
+    }
+
+    private void testConnectionClosedIfFirstPacketFromPeerNotKexInit(boolean clientInitiates) throws Exception {
+        AtomicReference<IoWriteFuture> debugMsg = new AtomicReference<>();
+        SessionListener messageInitiator = new SessionListener() {
+            @Override // At this stage KEX-INIT not sent yet
+            public void sessionNegotiationOptionsCreated(Session session, Map<KexProposalOption, String> proposal) {
+                try {
+                    debugMsg.set(session.sendDebugMessage(true, getCurrentTestName(), null));
+                } catch (Exception e) {
+                    throw new RuntimeException(e);
+                }
+            }
+        };
+
+        if (clientInitiates) {
+            client.addSessionListener(messageInitiator);
+        } else {
+            sshd.addSessionListener(messageInitiator);
+        }
+
+        try (ClientSession session = obtainInitialTestClientSession()) {
+            fail("Unexpected session success");
+        } catch (SshException e) {
+            IoWriteFuture future = debugMsg.get();
+            assertNotNull("No SSH_MSG_DEBUG", future);
+            assertTrue("SSH_MSG_DEBUG should have been sent", future.isWritten());
+            // Due to a race condition in the Nio2 transport when closing a connection due to an exception it's possible
+            // that we do _not_ get the expected disconnection code. The race condition may lead to the IoSession being
+            // closed in the peer before it has sent the DISCONNECT message. Happens in particular on Windows.
+            if (e.getDisconnectCode() == SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED) {
+                assertTrue("Unexpected disconnect reason: " + e.getMessage(), e.getMessage()
+                        .startsWith("Strict KEX negotiated but sequence number of first KEX_INIT received is not 1"));
+            }
+        }
+    }
+
+    @Test
+    public void connectionClosedIfSpuriousPacketFromClientInKex() throws Exception {
+        testConnectionClosedIfSupriousPacketInKex(true);
+    }
+
+    @Test
+    public void connectionClosedIfSpuriousPacketFromServerInKex() throws Exception {
+        testConnectionClosedIfSupriousPacketInKex(false);
+    }
+
+    private void testConnectionClosedIfSupriousPacketInKex(boolean clientInitiates) throws Exception {
+        AtomicReference<IoWriteFuture> debugMsg = new AtomicReference<>();
+        SessionListener messageInitiator = new SessionListener() {
+            @Override // At this stage the peer's KEX_INIT has been received
+            public void sessionNegotiationEnd(
+                    Session session, Map<KexProposalOption, String> clientProposal,
+                    Map<KexProposalOption, String> serverProposal, Map<KexProposalOption, String> negotiatedOptions,
+                    Throwable reason) {
+                try {
+                    debugMsg.set(session.sendDebugMessage(true, getCurrentTestName(), null));
+                } catch (Exception e) {
+                    throw new RuntimeException(e);
+                }
+            }
+        };
+
+        if (clientInitiates) {
+            client.addSessionListener(messageInitiator);
+        } else {
+            sshd.addSessionListener(messageInitiator);
+        }
+
+        try (ClientSession session = obtainInitialTestClientSession()) {
+            fail("Unexpected session success");
+        } catch (SshException e) {
+            IoWriteFuture future = debugMsg.get();
+            assertNotNull("No SSH_MSG_DEBUG", future);
+            assertTrue("SSH_MSG_DEBUG should have been sent", future.isWritten());
+            if (e.getDisconnectCode() == SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED) {
+                assertEquals("Unexpected disconnect reason",
+                        "SSH_MSG_DEBUG not allowed during initial key exchange in strict KEX", e.getMessage());
+            }
+        }
+    }
+
+    @Test
+    public void reKeyAllowsDebugInKexFromClient() throws Exception {
+        testReKeyAllowsDebugInKex(true);
+    }
+
+    @Test
+    public void reKeyAllowsDebugInKexFromServer() throws Exception {
+        testReKeyAllowsDebugInKex(false);
+    }
+
+    private void testReKeyAllowsDebugInKex(boolean clientInitiates) throws Exception {
+        AtomicBoolean sendDebug = new AtomicBoolean();
+        AtomicReference<IoWriteFuture> debugMsg = new AtomicReference<>();
+        SessionListener messageInitiator = new SessionListener() {
+            @Override // At this stage the peer's KEX_INIT has been received
+            public void sessionNegotiationEnd(
+                    Session session, Map<KexProposalOption, String> clientProposal,
+                    Map<KexProposalOption, String> serverProposal, Map<KexProposalOption, String> negotiatedOptions,
+                    Throwable reason) {
+                if (sendDebug.get()) {
+                    try {
+                        debugMsg.set(session.sendDebugMessage(true, getCurrentTestName(), null));
+                    } catch (Exception e) {
+                        throw new RuntimeException(e);
+                    }
+                }
+            }
+        };
+
+        if (clientInitiates) {
+            client.addSessionListener(messageInitiator);
+        } else {
+            sshd.addSessionListener(messageInitiator);
+        }
+
+        try (ClientSession session = obtainInitialTestClientSession()) {
+            assertTrue("Session should be stablished", session.isOpen());
+            sendDebug.set(true);
+            assertTrue("KEX not done", session.reExchangeKeys().verify(CONNECT_TIMEOUT).isDone());
+            IoWriteFuture future = debugMsg.get();
+            assertNotNull("No SSH_MSG_DEBUG", future);
+            assertTrue("SSH_MSG_DEBUG should have been sent", future.isWritten());
+            assertTrue(session.isOpen());
+        }
+    }
+
+    @Test
+    public void strictKexWorksWithServerFlagInClientProposal() throws Exception {
+        testStrictKexWorksWithWrongFlag(true);
+    }
+
+    @Test
+    public void strictKexWorksWithClientFlagInServerProposal() throws Exception {
+        testStrictKexWorksWithWrongFlag(false);
+    }
+
+    private void testStrictKexWorksWithWrongFlag(boolean clientInitiates) throws Exception {
+        SessionListener messageInitiator = new SessionListener() {
+            @Override
+            public void sessionNegotiationOptionsCreated(Session session, Map<KexProposalOption, String> proposal) {
+                // Modify the proposal by including the *wrong* flag. (The framework will also add the correct flag.)
+                String value = proposal.get(KexProposalOption.ALGORITHMS);
+                String toAdd = clientInitiates
+                        ? KexExtensions.STRICT_KEX_SERVER_EXTENSION
+                        : KexExtensions.STRICT_KEX_CLIENT_EXTENSION;
+                if (GenericUtils.isEmpty(value)) {
+                    value = toAdd;
+                } else {
+                    value += ',' + toAdd;
+                }
+                proposal.put(KexProposalOption.ALGORITHMS, value);
+            }
+        };
+
+        if (clientInitiates) {
+            client.addSessionListener(messageInitiator);
+        } else {
+            sshd.addSessionListener(messageInitiator);
+        }
+
+        try (ClientSession session = obtainInitialTestClientSession()) {
+            assertTrue("Session should be stablished", session.isOpen());
+        }
+    }
+
+    private ClientSession obtainInitialTestClientSession() throws IOException {
+        sshd.start();
+        int port = sshd.getPort();
+
+        client.start();
+        return createAuthenticatedClientSession(client, port);
+    }
+}

@@ -33,6 +33,7 @@
 ## New Features
 
 * [GH-429](https://github.com/apache/mina-sshd/issues/429) Support GIT protocol-v2
+* [GH-445](https://github.com/apache/mina-sshd/issues/445) OpenSSH "strict key exchange" protocol extension ([CVE-2023-48795](https://nvd.nist.gov/vuln/detail/CVE-2023-48795) mitigation)
 
 ## Behavioral changes and enhancements
 
@@ -43,6 +44,16 @@ acknowledgements of a `receive` related command. The user is free to inspect the
 to handle it - including even throwing an exception if OK status (if this makes sense for whatever reason). The default implementation checks for ERROR code and throws
 an exception if so.
 
+### OpenSSH protocol extension: strict key exchange
+
+[GH-445](https://github.com/apache/mina-sshd/issues/445) implements an extension to the SSH protocol introduced
+in OpenSSH 9.6. This ["strict key exchange" extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)
+hardens the SSH key exchange against the ["Terrapin attack"](https://www.terrapin-attack.com/)
+([CVE-2023-48795](https://nvd.nist.gov/vuln/detail/CVE-2023-48795)). The extension is active if both parties
+announce their support for it at the start of the initial key exchange. If only one party announces support,
+it is not activated to ensure compatibility with SSH implementations that do not implement it. Apache MINA sshd
+clients and servers always announce their support for strict key exchange.
+
 ## Potential compatibility issues
 
 ## Major Code Re-factoring

@@ -29,23 +29,32 @@
     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 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

@@ -129,3 +129,18 @@ 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.
+
+### Strict Key Exchange
+
+"Strict KEX" is an SSH protocol extension introduced in 2023 to harden the protocol against
+a particular form of attack. For details, see ["Terrapin attack"](https://www.terrapin-attack.com/)
+and [CVE-2023-48795](https://nvd.nist.gov/vuln/detail/CVE-2023-48795). The "strict KEX"
+counter-measures are active if both peers indicate support for it at the start of the initial
+key exchange. By default, Apache MINA sshd always supports "strict kex" and advertises it, and
+thus it will always be active if the other party also supports it.
+
+If for whatever reason you want to disable using "strict KEX", this can be achieved by setting
+a custom session factory on the `SshClient` or `SshServer`. This custom session factory would create
+custom sessions subclassed from `ClientSessionImpl`or `ServerSessionImpl` that do not do anything
+in method `doStrictKexProposal()` (just return the proposal unchanged).
+

@@ -59,9 +59,23 @@ public final class KexExtensions {
     public static final String CLIENT_KEX_EXTENSION = "ext-info-c";
     public static final String SERVER_KEX_EXTENSION = "ext-info-s";
 
-    @SuppressWarnings("checkstyle:Indentation")
-    public static final Predicate<String> IS_KEX_EXTENSION_SIGNAL
-            = n -> CLIENT_KEX_EXTENSION.equalsIgnoreCase(n) || SERVER_KEX_EXTENSION.equalsIgnoreCase(n);
+    public static final Predicate<String> IS_KEX_EXTENSION_SIGNAL = //
+            n -> CLIENT_KEX_EXTENSION.equalsIgnoreCase(n) || SERVER_KEX_EXTENSION.equalsIgnoreCase(n);
+
+    /**
+     * 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";
 
     /**
      * A case <U>insensitive</U> map of all the default known {@link KexExtensionParser} where key=the extension name

@@ -27,13 +27,17 @@
 import java.time.Duration;
 import java.time.Instant;
 import java.util.AbstractMap.SimpleImmutableEntry;
+import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Deque;
 import java.util.EnumMap;
+import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedDeque;
 import java.util.concurrent.CopyOnWriteArraySet;
@@ -45,6 +49,7 @@
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.LongConsumer;
 import java.util.logging.Level;
+import java.util.stream.Collectors;
 
 import org.apache.sshd.common.Closeable;
 import org.apache.sshd.common.Factory;
@@ -109,6 +114,7 @@
  *
  * @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
  */
+@SuppressWarnings("checkstyle:MethodCount")
 public abstract class AbstractSession extends SessionHelper {
     /**
      * Name of the property where this session is stored in the attributes of the underlying MINA session. See
@@ -192,6 +198,22 @@ public abstract class AbstractSession extends SessionHelper {
     protected final Object decodeLock = new Object();
     protected final Object requestLock = new Object();
 
+    /**
+     * "Strict KEX" is a mitigation for the "Terrapin attack". The KEX protocol is modified as follows:
+     * <ol>
+     * <li>During the initial (unencrypted) KEX, no extra messages not strictly necessary for KEX are allowed. The
+     * KEX_INIT message must be the first one after the version identification, and no IGNORE or DEBUG messages are
+     * allowed until the KEX is completed. If a party receives such a message, it terminates the connection.</li>
+     * <li>Message sequence numbers are reset to zero after a key exchange (initial or later). When the NEW_KEYS message
+     * has been sent, the outgoing message number is reset; after a NEW_KEYS message has been received, the incoming
+     * message number is reset.</li>
+     * </ol>
+     * Strict KEX is negotiated in the original KEX proposal; it is active if and only if both parties indicate that
+     * they support strict KEX.
+     */
+    protected boolean strictKex;
+    protected long initialKexInitSequenceNumber = -1;
+
     /**
      * The {@link KeyExchangeMessageHandler} instance also serves as lock protecting {@link #kexState} changes from DONE
      * to INIT or RUN, and from KEYS to DONE.
@@ -550,18 +572,24 @@ protected void doHandleMessage(Buffer buffer) throws Exception {
                 handleDisconnect(buffer);
                 break;
             case SshConstants.SSH_MSG_IGNORE:
+                failStrictKex(cmd);
                 handleIgnore(buffer);
                 break;
             case SshConstants.SSH_MSG_UNIMPLEMENTED:
+                failStrictKex(cmd);
                 handleUnimplemented(buffer);
                 break;
             case SshConstants.SSH_MSG_DEBUG:
+                // Fail after handling -- by default a message will be logged, which might be helpful.
                 handleDebug(buffer);
+                failStrictKex(cmd);
                 break;
             case SshConstants.SSH_MSG_SERVICE_REQUEST:
+                failStrictKex(cmd);
                 handleServiceRequest(buffer);
                 break;
             case SshConstants.SSH_MSG_SERVICE_ACCEPT:
+                failStrictKex(cmd);
                 handleServiceAccept(buffer);
                 break;
             case SshConstants.SSH_MSG_KEXINIT:
@@ -571,9 +599,11 @@ protected void doHandleMessage(Buffer buffer) throws Exception {
                 handleNewKeys(cmd, buffer);
                 break;
             case KexExtensions.SSH_MSG_EXT_INFO:
+                failStrictKex(cmd);
                 handleKexExtension(cmd, buffer);
                 break;
             case KexExtensions.SSH_MSG_NEWCOMPRESS:
+                failStrictKex(cmd);
                 handleNewCompression(cmd, buffer);
                 break;
             default:
@@ -589,26 +619,35 @@ protected void doHandleMessage(Buffer buffer) throws Exception {
                     }
 
                     handleKexMessage(cmd, buffer);
-                } else if (currentService.process(cmd, buffer)) {
-                    resetIdleTimeout();
                 } else {
-                    /*
-                     * According to https://tools.ietf.org/html/rfc4253#section-11.4
-                     *
-                     * An implementation MUST respond to all unrecognized messages with an SSH_MSG_UNIMPLEMENTED message
-                     * in the order in which the messages were received.
-                     */
-                    if (log.isDebugEnabled()) {
-                        log.debug("process({}) Unsupported command: {}",
-                                this, SshConstants.getCommandMessageName(cmd));
+                    failStrictKex(cmd);
+                    if (currentService.process(cmd, buffer)) {
+                        resetIdleTimeout();
+                    } else {
+                        /*
+                         * According to https://tools.ietf.org/html/rfc4253#section-11.4
+                         *
+                         * An implementation MUST respond to all unrecognized messages with an SSH_MSG_UNIMPLEMENTED
+                         * message in the order in which the messages were received.
+                         */
+                        if (log.isDebugEnabled()) {
+                            log.debug("process({}) Unsupported command: {}", this, SshConstants.getCommandMessageName(cmd));
+                        }
+                        notImplemented(cmd, buffer);
                     }
-                    notImplemented(cmd, buffer);
                 }
                 break;
         }
         checkRekey();
     }
 
+    protected void failStrictKex(int cmd) throws SshException {
+        if (!initialKexDone && strictKex) {
+            throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED,
+                    SshConstants.getCommandMessageName(cmd) + " not allowed during initial key exchange in strict KEX");
+        }
+    }
+
     protected boolean handleFirstKexPacketFollows(int cmd, Buffer buffer, boolean followFlag) {
         if (!followFlag) {
             return true; // if 1st KEX packet does not follow then process the command
@@ -1118,7 +1157,7 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws IOException {
     }
 
     protected int resolveIgnoreBufferDataLength() {
-        if ((ignorePacketDataLength <= 0)
+        if (!initialKexDone || (ignorePacketDataLength <= 0)
                 || (ignorePacketsFrequency <= 0L)
                 || (ignorePacketsVariance < 0)) {
             return 0;
@@ -1931,6 +1970,13 @@ protected void prepareNewKeys() throws Exception {
      * @throws Exception on errors
      */
     protected void setOutputEncoding() throws Exception {
+        if (strictKex) {
+            if (log.isDebugEnabled()) {
+                log.debug("setOutputEncoding({}): strict KEX resets output message sequence number from {} to 0", this, seqo);
+            }
+            seqo = 0;
+        }
+
         outCipher = outSettings.getCipher(seqo);
         outMac = outSettings.getMac();
         outCompression = outSettings.getCompression();
@@ -1962,6 +2008,13 @@ protected void setOutputEncoding() throws Exception {
      * @throws Exception on errors
      */
     protected void setInputEncoding() throws Exception {
+        if (strictKex) {
+            if (log.isDebugEnabled()) {
+                log.debug("setInputEncoding({}): strict KEX resets input message sequence number from {} to 0", this, seqi);
+            }
+            seqi = 0;
+        }
+
         inCipher = inSettings.getCipher(seqi);
         inMac = inSettings.getMac();
         inCompression = inSettings.getCompression();
@@ -2044,6 +2097,25 @@ protected IoWriteFuture notImplemented(int cmd, Buffer buffer) throws Exception
         return sendNotImplemented(seqi - 1L);
     }
 
+    /**
+     * Given a KEX proposal and a {@link KexProposalOption}, removes all occurrences of a value from a comma-separated
+     * value list.
+     *
+     * @param  options  {@link Map} holding the Kex proposal
+     * @param  option   {@link KexProposalOption} to modify
+     * @param  toRemove value to remove
+     * @return          {@code true} if the option contained the value (and it was removed); {@code false} otherwise
+     */
+    protected boolean removeValue(Map<KexProposalOption, String> options, KexProposalOption option, String toRemove) {
+        String val = options.get(option);
+        Set<String> algorithms = new LinkedHashSet<>(Arrays.asList(val.split(",")));
+        boolean result = algorithms.remove(toRemove);
+        if (result) {
+            options.put(option, algorithms.stream().collect(Collectors.joining(",")));
+        }
+        return result;
+    }
+
     /**
      * Compute the negotiated proposals by merging the client and server proposal. The negotiated proposal will also be
      * stored in the {@link #negotiationResult} property.
@@ -2056,11 +2128,43 @@ protected Map<KexProposalOption, String> negotiate() throws Exception {
         Map<KexProposalOption, String> s2cOptions = getServerKexProposals();
         signalNegotiationStart(c2sOptions, s2cOptions);
 
+        // Make modifiable. Strict KEX flags are to be heeded only in initial KEX, and to be ignored afterwards.
+        c2sOptions = new EnumMap<>(c2sOptions);
+        s2cOptions = new EnumMap<>(s2cOptions);
+        boolean strictKexClient = removeValue(c2sOptions, KexProposalOption.ALGORITHMS,
+                KexExtensions.STRICT_KEX_CLIENT_EXTENSION);
+        boolean strictKexServer = removeValue(s2cOptions, KexProposalOption.ALGORITHMS,
+                KexExtensions.STRICT_KEX_SERVER_EXTENSION);
+        if (removeValue(c2sOptions, KexProposalOption.ALGORITHMS, KexExtensions.STRICT_KEX_SERVER_EXTENSION)
+                && !initialKexDone) {
+            log.warn("negotiate({}) client proposal contains server flag {}; will be ignored", this,
+                    KexExtensions.STRICT_KEX_SERVER_EXTENSION);
+        }
+        if (removeValue(s2cOptions, KexProposalOption.ALGORITHMS, KexExtensions.STRICT_KEX_CLIENT_EXTENSION)
+                && !initialKexDone) {
+            log.warn("negotiate({}) server proposal contains client flag {}; will be ignored", this,
+                    KexExtensions.STRICT_KEX_CLIENT_EXTENSION);
+        }
+        // Make unmodifiable again
+        c2sOptions = Collections.unmodifiableMap(c2sOptions);
+        s2cOptions = Collections.unmodifiableMap(s2cOptions);
         Map<KexProposalOption, String> guess = new EnumMap<>(KexProposalOption.class);
         Map<KexProposalOption, String> negotiatedGuess = Collections.unmodifiableMap(guess);
         try {
             boolean debugEnabled = log.isDebugEnabled();
             boolean traceEnabled = log.isTraceEnabled();
+            if (!initialKexDone) {
+                strictKex = strictKexClient && strictKexServer;
+                if (debugEnabled) {
+                    log.debug("negotiate({}) strict KEX={} client={} server={}", this, strictKex, strictKexClient,
+                            strictKexServer);
+                }
+                if (strictKex && initialKexInitSequenceNumber != 1) {
+                    throw new SshException(SshConstants.SSH2_DISCONNECT_KEY_EXCHANGE_FAILED,
+                            "Strict KEX negotiated but sequence number of first KEX_INIT received is not 1: "
+                                                                                             + initialKexInitSequenceNumber);
+                }
+            }
             SessionDisconnectHandler discHandler = getSessionDisconnectHandler();
             KexExtensionHandler extHandler = getKexExtensionHandler();
             for (KexProposalOption paramType : KexProposalOption.VALUES) {
@@ -2520,8 +2624,34 @@ protected String resolveSessionKexProposal(String hostKeyTypes) throws IOExcepti
         }
     }
 
+    protected Map<KexProposalOption, String> doStrictKexProposal(Map<KexProposalOption, String> proposal) {
+        String value = proposal.get(KexProposalOption.ALGORITHMS);
+        String askForStrictKex = isServerSession()
+                ? KexExtensions.STRICT_KEX_SERVER_EXTENSION
+                : KexExtensions.STRICT_KEX_CLIENT_EXTENSION;
+        if (!initialKexDone) {
+            // On the initial KEX, include the strict KEX flag
+            if (GenericUtils.isEmpty(value)) {
+                value = askForStrictKex;
+            } else {
+                value += "," + askForStrictKex;
+            }
+        } else if (!GenericUtils.isEmpty(value)) {
+            // On subsequent KEXes, do not include ext-info-c/ext-info-s or the strict KEX flag in the proposal.
+            List<String> algorithms = new ArrayList<>(Arrays.asList(value.split(",")));
+            String extType = isServerSession() ? KexExtensions.SERVER_KEX_EXTENSION : KexExtensions.CLIENT_KEX_EXTENSION;
+            boolean changed = algorithms.remove(extType);
+            changed |= algorithms.remove(askForStrictKex);
+            if (changed) {
+                value = algorithms.stream().collect(Collectors.joining(","));
+            }
+        }
+        proposal.put(KexProposalOption.ALGORITHMS, value);
+        return proposal;
+    }
+
     protected byte[] sendKexInit() throws Exception {
-        Map<KexProposalOption, String> proposal = getKexProposal();
+        Map<KexProposalOption, String> proposal = doStrictKexProposal(getKexProposal());
 
         byte[] seed;
         synchronized (kexState) {
@@ -2588,6 +2718,9 @@ protected void setServerKexData(byte[] data) {
     protected byte[] receiveKexInit(Buffer buffer) throws Exception {
         Map<KexProposalOption, String> proposal = new EnumMap<>(KexProposalOption.class);
 
+        if (!initialKexDone) {
+            initialKexInitSequenceNumber = seqi;
+        }
         byte[] seed;
         synchronized (kexState) {
             seed = receiveKexInit(buffer, proposal);

@@ -437,6 +437,7 @@ public static class MySession extends AbstractSession {
         public MySession() {
             super(true, org.apache.sshd.util.test.CoreTestSupportUtils.setupTestServer(AbstractSessionTest.class),
                   new MyIoSession());
+            initialKexDone = true;
         }
 
         @Override