ecki commented on code in PR #449:
URL: https://github.com/apache/mina-sshd/pull/449#discussion_r1442111543
##########
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.
+ *
+ * @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
Review Comment:
It is a bit unsymmetrical with the rfc3808 extension (strict-kex has no such
predicate and instead defines the list), but I guess it’s ok since both have a
bit different scope.
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -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);
+ }
+ }
Review Comment:
Not handling the case of a wrong client or server type extension? (It is
correctly handles so it could be ignored or logged. Aborting would be extra
paranoid.
##########
CHANGES.md:
##########
@@ -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
Review Comment:
As mentioned, I think it will help admins if they can set a flag to
disconnect connections if they do not negotiate strict-kex. This makes is much
easier to re-enable the vulnerable (but modern) ciphers. Maybe it’s an option
for later, but why not offer it from the get-go. If not offered maybe a sample
listener who does it would be good (validating the info is exposed to make that
call by the app).
nb: I am not saying make sending of the offer configurable, it’s perfectly
fine with me to do that unconditional (even though some other impls offer this
option).
##########
sshd-core/src/main/java/org/apache/sshd/common/session/helpers/AbstractSession.java:
##########
@@ -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
Review Comment:
Servers could include it only if client requested it, this might be good for
interop (but it also might be less secure, not sure?). Maybe add a comment
documenting the decision.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
