Skip to content

Commit

Permalink
apacheGH-445: OpenSSH "strict KEX" protocol extension
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@tomaswolf
tomaswolf committed Jan 1, 2024
1 parent 6f69e41 commit d67b6f5
Show file tree
Hide file tree
Showing 4 changed files with 167 additions and 14 deletions.
10 changes: 10 additions & 0 deletions CHANGES.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

## Behavioral changes and enhancements

Expand All @@ -43,6 +44,15 @@ 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/). 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
Expand Down
19 changes: 19 additions & 0 deletions sshd-common/src/main/java/org/apache/sshd/common/kex/extension/KexExtensions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.nio.ByteBuffer;
import java.util.AbstractMap.SimpleImmutableEntry;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
Expand Down Expand Up @@ -59,6 +60,24 @@ public final class KexExtensions {
public static final String CLIENT_KEX_EXTENSION = "ext-info-c";
public static final String SERVER_KEX_EXTENSION = "ext-info-s";

/**
* Reminder:
*
* These pseudo-algorithms are only valid in the initial SSH2_MSG_KEXINIT and MUST be ignored if they are present in
* subsequent SSH2_MSG_KEXINIT packets.
*
* <B>Note:</B> these values are <U>appended</U> to the initial proposals and removed if received before proceeding
* with the standard KEX proposals negotiation.
*
* @see <A HREF="https://github.com/openssh/openssh-portable/blob/master/PROTOCOL">OpenSSH PROTOCOL - 1.9 transport:
* strict key exchange extension</A>
*/
public static final String STRICT_KEX_CLIENT_EXTENSION = "[email protected]";
public static final String STRICT_KEX_SERVER_EXTENSION = "[email protected]";
public static final List<String> STRICT_KEX_EXTENSIONS = Collections.unmodifiableList(
Arrays.asList(
STRICT_KEX_CLIENT_EXTENSION, STRICT_KEX_SERVER_EXTENSION));

@SuppressWarnings("checkstyle:Indentation")
public static final Predicate<String> IS_KEX_EXTENSION_SIGNAL
= n -> CLIENT_KEX_EXTENSION.equalsIgnoreCase(n) || SERVER_KEX_EXTENSION.equalsIgnoreCase(n);
Expand Down
151 changes: 137 additions & 14 deletions sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -109,6 +114,7 @@
*
* @author <a href="mailto:[email protected]">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
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand All @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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.
Expand All @@ -2056,11 +2128,33 @@ 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);
// 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) {
Expand Down Expand Up @@ -2520,8 +2614,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) {
Expand Down Expand Up @@ -2588,6 +2708,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);
Expand Down
1 change: 1 addition & 0 deletions sshd-core/src/test/java/org/apache/sshd/common/session/helpers/AbstractSessionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit d67b6f5

Please sign in to comment.