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