From 6b0fd46f64bcb75eeeee31d65f10242660aad7c1 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Fri, 29 Dec 2023 17:39:14 +0100 Subject: [PATCH] GH-445: OpenSSH "strict KEX" protocol extension 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/CHANGES.md b/CHANGES.md index 800246c46..d06487adb 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -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 diff --git a/docs/standards.md b/docs/standards.md index cc0cfc144..d223bda4c 100644 --- a/docs/standards.md +++ b/docs/standards.md @@ -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 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 IS_KEX_EXTENSION_SIGNAL - = n -> CLIENT_KEX_EXTENSION.equalsIgnoreCase(n) || SERVER_KEX_EXTENSION.equalsIgnoreCase(n); + public static final Predicate 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. + * + * Note: these values are appended to the initial proposals and removed if received before proceeding + * with the standard KEX proposals negotiation. + * + * @see OpenSSH PROTOCOL - 1.9 transport: + * strict key exchange extension + */ + 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 insensitive 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 Apache MINA SSHD Project */ +@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: + *
    + *
  1. 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.
    2. + *
  2. 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.
    3. + *
+ * 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 options, KexProposalOption option, String toRemove) { + String val = options.get(option); + Set 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 negotiate() throws Exception { Map 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 guess = new EnumMap<>(KexProposalOption.class); Map 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 doStrictKexProposal(Map 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 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 proposal = getKexProposal(); + Map proposal = doStrictKexProposal(getKexProposal()); byte[] seed; synchronized (kexState) { @@ -2588,6 +2718,9 @@ protected void setServerKexData(byte[] data) { protected byte[] receiveKexInit(Buffer buffer) throws Exception { Map 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