ecki commented on code in PR #446:
URL: https://github.com/apache/mina-sshd/pull/446#discussion_r1435101866
##########
sshd-common/src/main/java/org/apache/sshd/common/kex/extension/KexExtensions.java:
##########
@@ -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.
+ *
Review Comment:
Do you also want to add verbiage that it should never be negotiated (implied
by “removed”?)
##########
sshd-core/src/main/java/org/apache/sshd/common/session/SessionListener.java:
##########
@@ -103,6 +103,22 @@ default void sessionPeerIdentificationLine(
// ignored
}
+ /**
+ * Identification sent to peer
+ *
+ * @param session The {@link Session} instance
+ * @param version The resolved identification version
+ * @param extraLines Extra data preceding the identification to be sent.
<B>Note:</B> the list is modifiable only if
Review Comment:
Not related to terrapin, but the “list is modifiable” does that imply is
fired before sending identification string?
##########
sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionKexDetails.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.session.helpers;
+
+import java.time.Instant;
+
+import org.apache.sshd.common.kex.KexState;
+
+/**
+ * Provides some useful internal information about the session's KEX
+ *
+ * @author <a href="mailto:dev@mina.apache.org";>Apache MINA SSHD Project</a>
+ */
+public class SessionKexDetails {
+ private KexState kexState;
+ private boolean initialKexDone;
+ private boolean strictKexEnabled;
+ private boolean strictKexSignalled;
+ private int newKeysSentCount;
+ private int newKeysReceivedCount;
+ private Instant lastKeyTimeValue;
+
+ public SessionKexDetails() {
+ super();
+ }
+
+ public KexState getKexState() {
+ return kexState;
+ }
+
+ public void setKexState(KexState kexState) {
+ this.kexState = kexState;
+ }
+
+ public boolean isInitialKexDone() {
+ return initialKexDone;
+ }
+
+ public void setInitialKexDone(boolean initialKexDone) {
+ this.initialKexDone = initialKexDone;
+ }
+
+ public boolean isStrictKexEnabled() {
+ return strictKexEnabled;
+ }
+
+ public void setStrictKexEnabled(boolean strictKexEnabled) {
+ this.strictKexEnabled = strictKexEnabled;
+ }
+
+ public boolean isStrictKexSignalled() {
Review Comment:
Should maybe document that this means it’s negotiated by both sides (as
opposed to be offered by client?)
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -1741,6 +1979,143 @@ protected byte[] sendKexInit(Map<KexProposalOption,
String> proposal) throws Exc
return data;
}
+ protected String adjustOutgoingKexProposalOption(
+ KexProposalOption option, String value, boolean debugEnabled) {
+ if (GenericUtils.isBlank(value)) {
+ return value; // can happen for language (e.g.)
+ }
+
+ if (option != KexProposalOption.ALGORITHMS) {
+ return value;
+ }
+
+ if (!CoreModuleProperties.USE_STRICT_KEX.getRequired(this)) {
+ return value;
+ }
+
+ /*
+ * According to
https://github.com/openssh/openssh-portable/blob/master/PROTOCOL - section 1.9
+ *
+ * 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.
+ *
+ * Therefore, if this is not the 1st outgoing packet don't bother
appending
+ */
+ long outgoingCount = totalOutgingPacketsCount.get();
+ if (outgoingCount > 0L) {
+ if (debugEnabled) {
+ log.debug("adjustKexOutgoingProposalOption({})[{}] not first
outgoing packet ({}) for proposal={}",
+ this, option, outgoingCount, value);
+ }
+ return value;
+ }
+
+ /*
+ * According to
https://github.com/openssh/openssh-portable/blob/master/PROTOCOL - section 1.9
+ *
+ * The client may append "kex-strict-c-...@openssh.com" to its
kex_algorithms
+ * and the server may append "kex-strict-s-...@openssh.com"
+ *
+ */
+ String proposal = isServerSession()
Review Comment:
Server could skip it, if client did not request it, maybe better for interop?
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -1741,6 +1979,143 @@ protected byte[] sendKexInit(Map<KexProposalOption,
String> proposal) throws Exc
return data;
}
+ protected String adjustOutgoingKexProposalOption(
+ KexProposalOption option, String value, boolean debugEnabled) {
+ if (GenericUtils.isBlank(value)) {
+ return value; // can happen for language (e.g.)
+ }
+
+ if (option != KexProposalOption.ALGORITHMS) {
+ return value;
+ }
+
+ if (!CoreModuleProperties.USE_STRICT_KEX.getRequired(this)) {
+ return value;
+ }
+
+ /*
+ * According to
https://github.com/openssh/openssh-portable/blob/master/PROTOCOL - section 1.9
+ *
+ * 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.
+ *
+ * Therefore, if this is not the 1st outgoing packet don't bother
appending
+ */
+ long outgoingCount = totalOutgingPacketsCount.get();
+ if (outgoingCount > 0L) {
+ if (debugEnabled) {
+ log.debug("adjustKexOutgoingProposalOption({})[{}] not first
outgoing packet ({}) for proposal={}",
+ this, option, outgoingCount, value);
+ }
+ return value;
+ }
+
+ /*
+ * According to
https://github.com/openssh/openssh-portable/blob/master/PROTOCOL - section 1.9
+ *
+ * The client may append "kex-strict-c-...@openssh.com" to its
kex_algorithms
+ * and the server may append "kex-strict-s-...@openssh.com"
+ *
+ */
+ String proposal = isServerSession()
+ ? KexExtensions.STRICT_KEX_SERVER_EXTENSION
+ : KexExtensions.STRICT_KEX_CLIENT_EXTENSION;
+ String adjusted = value + "," + proposal;
+ if (debugEnabled) {
+ log.debug("adjustOutgoingKexProposalOption({})[{}] adjusted={}",
this, option, adjusted);
+ }
+
+ return adjusted;
+ }
+
+ protected String preProcessIncomingKexProposalOption(
+ KexProposalOption option, String value, boolean debugEnabled) {
+ if (GenericUtils.isBlank(value)) {
+ return value; // can happen for language (e.g.)
+ }
+
+ if (option != KexProposalOption.ALGORITHMS) {
+ return value;
+ }
+
+ if (!CoreModuleProperties.USE_STRICT_KEX.getRequired(this)) {
+ return value;
+ }
+
+ String peerValue = isServerSession()
+ ? KexExtensions.STRICT_KEX_CLIENT_EXTENSION
+ : KexExtensions.STRICT_KEX_SERVER_EXTENSION;
+ if (!value.contains(peerValue)) {
+ return value;
+ }
+
+ // Just a bit of paranoia...
Review Comment:
Should it also reject the “ownValue” (might be attack or error)?
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -471,7 +490,8 @@ public MacInformation getMacInformation(boolean incoming) {
public void messageReceived(Readable buffer) throws Exception {
synchronized (decodeLock) {
decoderBuffer.putBuffer(buffer);
- // One of those properties will be set by the constructor and the
other
+ // One of those properties will be set by the constructor and the
+ // other
Review Comment:
Nit: That’s an ugly reformat
##########
sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionCountersDetails.java:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.session.helpers;
+
+/**
+ * Provides several internal session counters details
+ *
+ * @author <a href="mailto:dev@mina.apache.org";>Apache MINA SSHD Project</a>
+ */
+public class SessionCountersDetails {
Review Comment:
This is helpful, as related to terrapin, maybe count ignored or dummy blocks
separately (debug, ignore, ping pong). So an app can add additional
protection/intelligence?
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -540,9 +583,30 @@ protected void handleMessage(Buffer buffer) throws
Exception {
protected void doHandleMessage(Buffer buffer) throws Exception {
int cmd = buffer.getUByte();
+
+ /*
+ * Terrapin attack mitigation - see
+ * https://github.com/openssh/openssh-portable/blob/master/PROTOCOL
+ * section 1.9 transport: strict key exchange extension
+ *
+ * During initial KEX, terminate the connection if any unexpected or
+ * out-of-sequence packet is received. This includes terminating the
+ * connection if the first packet received is not SSH2_MSG_KEXINIT.
+ *
+ * Unexpected packets for the purpose of strict KEX include messages
+ * that are otherwise valid at any time during the connection such as
+ * SSH2_MSG_DEBUG and SSH2_MSG_IGNORE.
Review Comment:
It also includes invalid packets, I think they are part of the attack to
provoke send counter increase from unimpl responses.
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -202,13 +213,20 @@ public abstract class AbstractSession extends
SessionHelper {
* Rekeying
*/
protected final AtomicLong inPacketsCount = new AtomicLong(0L);
+ protected final AtomicLong totalIncomingPacketsCount = new AtomicLong(0L);
protected final AtomicLong outPacketsCount = new AtomicLong(0L);
+ protected final AtomicLong totalOutgingPacketsCount = new AtomicLong(0L);
protected final AtomicLong inBytesCount = new AtomicLong(0L);
+ protected final AtomicLong totalIncomingBytesCount = new AtomicLong(0L);
protected final AtomicLong outBytesCount = new AtomicLong(0L);
+ protected final AtomicLong totalOutgoingBytesCount = new AtomicLong(0L);
protected final AtomicLong inBlocksCount = new AtomicLong(0L);
+ protected final AtomicLong totalIncomingBlocksCount = new AtomicLong(0L);
protected final AtomicLong outBlocksCount = new AtomicLong(0L);
+ protected final AtomicLong totalOutgoingBlocksCount = new AtomicLong(0L);
protected final AtomicReference<Instant> lastKeyTimeValue = new
AtomicReference<>(Instant.now());
- // we initialize them here in case super constructor calls some methods
that use these values
+ // we initialize them here in case super constructor calls some methods
that
+ // use these values
Review Comment:
I guess that comment means the block above, maybe add a linebreak or move it
up?
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -797,8 +869,10 @@ protected boolean validateServiceKexState(KexState state) {
if (KexState.DONE.equals(state)) {
return true;
} else if (KexState.INIT.equals(state)) {
- // Allow service requests that were "in flight" when we sent our
own KEX_INIT. We will send back the accept
- // only after KEX is done. However, we will refuse a service
request before the initial KEX.
+ // Allow service requests that were "in flight" when we sent our
own
+ // KEX_INIT. We will send back the accept
+ // only after KEX is done. However, we will refuse a service
request
Review Comment:
NIt: reflow
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -151,24 +156,34 @@ public abstract class AbstractSession extends
SessionHelper {
protected final Map<KexProposalOption, String> clientProposal = new
EnumMap<>(KexProposalOption.class);
protected final Map<KexProposalOption, String> unmodClientProposal =
Collections.unmodifiableMap(clientProposal);
protected final Map<KexProposalOption, String> negotiationResult = new
EnumMap<>(KexProposalOption.class);
- protected final Map<KexProposalOption, String> unmodNegotiationResult =
Collections.unmodifiableMap(negotiationResult);
+ protected final Map<KexProposalOption, String> unmodNegotiationResult =
Collections
+ .unmodifiableMap(negotiationResult);
protected KeyExchange kex;
protected Boolean firstKexPacketFollows;
protected boolean initialKexDone;
+ protected final AtomicBoolean strictKexSignalled = new AtomicBoolean();
+ protected final AtomicInteger newKeysReceivedCount = new AtomicInteger();
+ protected final AtomicInteger newKeysSentCount = new AtomicInteger();
+
/**
* Holds the current key exchange state.
*/
protected final AtomicReference<KexState> kexState = new
AtomicReference<>(KexState.UNKNOWN);
protected final AtomicReference<DefaultKeyExchangeFuture> kexFutureHolder
= new AtomicReference<>(null);
- // The kexInitializedFuture is fulfilled when this side (client or server)
has prepared its own proposal. Access is
+ // The kexInitializedFuture is fulfilled when this side (client or server)
+ // has prepared its own proposal. Access is
// synchronized on kexState.
protected DefaultKeyExchangeFuture kexInitializedFuture;
/*
* SSH packets encoding / decoding support
*/
+ /** Input packet sequence number. */
+ protected long seqi;
+ /** Output packet sequence number. */
+ protected long seqo;
Review Comment:
Maybe add guardedby comment to describe which monitor must be held?
##########
sshd-core/src/main/java/org/apache/sshd/common/config/SshConfigFileReader.java:
##########
@@ -256,6 +257,11 @@ public static <M extends AbstractFactoryManager> M
configureKeyExchanges(
M manager, PropertyResolver props, boolean lenient,
Function<? super DHFactory, ? extends KeyExchangeFactory> xformer,
boolean ignoreUnsupported) {
Objects.requireNonNull(props, "No properties to configure");
+ Boolean useStrictKex =
props.getBoolean(ConfigFileReaderSupport.STRICT_KEX_CUSTOM_CONFIG_PROP);
Review Comment:
Maybe add a moment that this overwrites the setting for debug only?
##########
sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionKexDetails.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.session.helpers;
+
+import java.time.Instant;
+
+import org.apache.sshd.common.kex.KexState;
+
+/**
+ * Provides some useful internal information about the session's KEX
+ *
+ * @author <a href="mailto:dev@mina.apache.org";>Apache MINA SSHD Project</a>
+ */
+public class SessionKexDetails {
+ private KexState kexState;
+ private boolean initialKexDone;
+ private boolean strictKexEnabled;
+ private boolean strictKexSignalled;
+ private int newKeysSentCount;
+ private int newKeysReceivedCount;
+ private Instant lastKeyTimeValue;
+
+ public SessionKexDetails() {
+ super();
+ }
+
+ public KexState getKexState() {
+ return kexState;
+ }
+
+ public void setKexState(KexState kexState) {
+ this.kexState = kexState;
+ }
+
+ public boolean isInitialKexDone() {
+ return initialKexDone;
+ }
+
+ public void setInitialKexDone(boolean initialKexDone) {
+ this.initialKexDone = initialKexDone;
+ }
+
+ public boolean isStrictKexEnabled() {
+ return strictKexEnabled;
+ }
+
+ public void setStrictKexEnabled(boolean strictKexEnabled) {
+ this.strictKexEnabled = strictKexEnabled;
+ }
+
+ public boolean isStrictKexSignalled() {
+ return strictKexSignalled;
+ }
+
+ public void setStrictKexSignalled(boolean strictKexSignalled) {
Review Comment:
Do the setters need to be public? Or maybe use constructor to make it
immutable?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@mina.apache.org
For additional commands, e-mail: dev-h...@mina.apache.org
