ecki commented on code in PR #446: URL: https://github.com/apache/mina-sshd/pull/446#discussion_r1435258249
########## 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: When you check if the peer value is present you can also check that the own (I.e. c instead of s for a client connection) is not present, because that would not be allowed. RFC8303 (which is analogous to the strict kex) says “may disconnect” in 2.2: > Implementations MUST NOT send an incorrect indicator name for their role. Implementations MAY disconnect if the counterparty sends an incorrect indicator. If "ext-info-c" or "ext-info-s" ends up being negotiated as a key exchange method, the parties MUST disconnect. -- 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