Projects
Mega:24.09
apache-sshd
_service:tar_scm:CVE-2023-48795.patch
Sign Up
Log In
Username
Password
Overview
Repositories
Revisions
Requests
Users
Attributes
Meta
File _service:tar_scm:CVE-2023-48795.patch of Package apache-sshd
From 6b0fd46f64bcb75eeeee31d65f10242660aad7c1 Mon Sep 17 00:00:00 2001 From: Thomas Wolf <twolf@apache.org> Date: Fri, 29 Dec 2023 17:39:14 +0100 Subject: [PATCH 1/3] GH-445: OpenSSH "strict KEX" protocol extension Origin: https://github.com/apache/mina-sshd/pull/449 Implements the OpenSSH "strict KEX" protocol extension.[1] If both parties in a an SSH connection announce support for strict KEX in the initial KEX_INIT message, strict KEX is active; otherwise it isn't. With strict KEX active, there must be only KEX-related messages during the initial key exchange (no IGNORE or DEBUG messages are allowed), and the KEX_INIT message must be the first one to have been received after the initial version exchange. If these conditions are violated, the connection is terminated. Strict KEX also resets message sequence numbers to zero after each NEW_KEYS message sent or received. [1] https://github.com/openssh/openssh-portable/blob/master/PROTOCOL --- CHANGES.md | 11 ++ docs/standards.md | 35 ++-- docs/technical/kex.md | 15 ++ .../common/kex/extension/KexExtensions.java | 20 ++- .../session/helpers/AbstractSession.java | 161 ++++++++++++++++-- .../session/helpers/AbstractSessionTest.java | 1 + 6 files changed, 213 insertions(+), 30 deletions(-) diff --git a/docs/technical/kex.md b/docs/technical/kex.md index e5d353a92..a3f5facc1 100644 --- a/docs/technical/kex.md +++ b/docs/technical/kex.md @@ -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). + 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 index 9fac45c13..f275227e1 100644 --- 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,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 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 4bdb39c4c..b05a3ab92 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 @@ -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); diff --git a/sshd-core/src/test/java/org/apache/sshd/common/session/helpers/AbstractSessionTest.java b/sshd-core/src/test/java/org/apache/sshd/common/session/helpers/AbstractSessionTest.java index 6fe6e8104..8d74d51b7 100644 --- a/sshd-core/src/test/java/org/apache/sshd/common/session/helpers/AbstractSessionTest.java +++ b/sshd-core/src/test/java/org/apache/sshd/common/session/helpers/AbstractSessionTest.java @@ -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 From 315739e4e9d1dc7a4ff32ea64936982ed0b73e76 Mon Sep 17 00:00:00 2001 From: Thomas Wolf <twolf@apache.org> Date: Mon, 1 Jan 2024 15:00:40 +0100 Subject: [PATCH 2/3] GH-445: Unit tests for strict KEX Add tests for the restricted message handling if strict KEX is active: * Initial KEX fails if KEX_INIT is not the first message * Initial KEX fails if there are spurious messages like DEBUG during KEX * Re-KEX succeeds even if there are spurious messages --- .../common/kex/extension/StrictKexTest.java | 264 ++++++++++++++++++ 1 file changed, 264 insertions(+) create mode 100644 sshd-core/src/test/java/org/apache/sshd/common/kex/extension/StrictKexTest.java diff --git a/sshd-core/src/test/java/org/apache/sshd/common/kex/extension/StrictKexTest.java b/sshd-core/src/test/java/org/apache/sshd/common/kex/extension/StrictKexTest.java new file mode 100644 index 000000000..6d6c2ce8c --- /dev/null +++ b/sshd-core/src/test/java/org/apache/sshd/common/kex/extension/StrictKexTest.java @@ -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: "strict-kex"</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); + } +} From 7b2c781640a7a78a9455b86593a1f63c9e8cab92 Mon Sep 17 00:00:00 2001 From: Thomas Wolf <twolf@apache.org> Date: Tue, 2 Jan 2024 22:35:42 +0100 Subject: [PATCH 3/3] GH-445: strict KEX interoperability tests Run an Apache MINA sshd client against OpenSSH servers that do have or do not have strict KEX. --- .../StrictKexInteroperabilityTest.java | 192 ++++++++++++++++++ .../sshd/common/kex/extensions/client/bob_key | 27 +++ .../common/kex/extensions/client/bob_key.pub | 1 + .../kex/extensions/client/entrypoint.sh | 6 + sshd-mina/pom.xml | 1 + sshd-netty/pom.xml | 1 + 6 files changed, 228 insertions(+) create mode 100644 sshd-core/src/test/java/org/apache/sshd/common/kex/extension/StrictKexInteroperabilityTest.java create mode 100644 sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/bob_key create mode 100644 sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/bob_key.pub create mode 100644 sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/entrypoint.sh diff --git a/sshd-core/src/test/java/org/apache/sshd/common/kex/extension/StrictKexInteroperabilityTest.java b/sshd-core/src/test/java/org/apache/sshd/common/kex/extension/StrictKexInteroperabilityTest.java new file mode 100644 index 000000000..43e6d8a8e --- /dev/null +++ b/sshd-core/src/test/java/org/apache/sshd/common/kex/extension/StrictKexInteroperabilityTest.java @@ -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: "strict-kex"</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; + } + } +} diff --git a/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/bob_key b/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/bob_key new file mode 100644 index 000000000..b5b70aeaa --- /dev/null +++ b/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/bob_key @@ -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----- diff --git a/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/bob_key.pub b/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/bob_key.pub new file mode 100644 index 000000000..efecd1b08 --- /dev/null +++ b/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/bob_key.pub @@ -0,0 +1 @@ +ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDFjcevVKqkkhD1JsV8YwbB5XLyODZMQfcYLUrQu51AcnCiBGL515t0KpS25IhNuk+GOR5lKT2urX3x3pGFPpNCZj0nzAsvb/5VdmT+sxUQSN5dBLciZGcva9WaQC6e/ovB8MYcii+SHCPf+WoZamEMAqcfJe6w8PBDc3gsUaDHBe9EapyXOY/wf4oNZpo9qmTUBkIEigPglsvx9oG0s1TTWv6kHewgq9+w8nkPE8StOF+s+4OohsDdrESoWWBfhpWXfIydJaN+EuE7vEPdjCu1SFqece3yllCjkWTAIZYOmQbfGz6ordkAKuEwUxO8+4NLzeMO0TM13kphvgqeQ48x user01 diff --git a/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/entrypoint.sh b/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/entrypoint.sh new file mode 100644 index 000000000..26489c5f0 --- /dev/null +++ b/sshd-core/src/test/resources/org/apache/sshd/common/kex/extensions/client/entrypoint.sh @@ -0,0 +1,6 @@ +#!/bin/sh + +chown -R bob /home/bob +chmod 0600 /home/bob/.ssh/* + +/usr/sbin/sshd -D -ddd diff --git a/sshd-mina/pom.xml b/sshd-mina/pom.xml index 967b12930..6d1c4ed6a 100644 --- a/sshd-mina/pom.xml +++ b/sshd-mina/pom.xml @@ -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> diff --git a/sshd-netty/pom.xml b/sshd-netty/pom.xml index 5d774029f..ac34b5094 100644 --- a/sshd-netty/pom.xml +++ b/sshd-netty/pom.xml @@ -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>
Locations
Projects
Search
Status Monitor
Help
Open Build Service
OBS Manuals
API Documentation
OBS Portal
Reporting a Bug
Contact
Mailing List
Forums
Chat (IRC)
Twitter
Open Build Service (OBS)
is an
openSUSE project
.
浙ICP备2022010568号-2