From a0905e9abf06ddb527f3557e73cfdb75e54edec5 Mon Sep 17 00:00:00 2001 From: Lyor Goldstein Date: Thu, 21 Dec 2023 19:12:42 +0200 Subject: [PATCH] [GH-445] Implemented OpenSSH strict KEX protocol --- CHANGES.md | 13 + docs/standards.md | 34 +- docs/technical/kex.md | 8 + .../sshd/cli/client/SftpCommandMain.java | 184 ++-- .../config/ConfigFileReaderSupport.java | 2 + .../common/kex/extension/KexExtensions.java | 19 + .../helpers/SessionCountersDetails.java | 158 ++++ .../session/helpers/SessionKexDetails.java | 123 +++ .../common/config/SshConfigFileReader.java | 6 + .../apache/sshd/common/session/Session.java | 9 + .../session/helpers/AbstractSession.java | 844 +++++++++++++----- .../common/session/helpers/SessionHelper.java | 1 + .../sshd/core/CoreModuleProperties.java | 7 + 13 files changed, 1109 insertions(+), 299 deletions(-) create mode 100644 sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionCountersDetails.java create mode 100644 sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionKexDetails.java diff --git a/CHANGES.md b/CHANGES.md index 800246c46..a3f065759 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -36,6 +36,12 @@ ## Behavioral changes and enhancements +### [GH-445 - Terrapin attack mitigation](https://github.com/apache/mina-sshd/issues/429) + +There is a **new** `CoreModuleProperties` property that controls the mitigation for the [Terrapin attach](https://terrapin-attack.com/) via what is known as +"strict-KEX" (see [OpenSSH PROTOCOL - 1.9 transport: strict key exchange extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL)). +It is **disabled** by default due to its experimental nature and possible interoperability issues, so users who wish to use this feature must turn it on *explicitly*. + ### New `ScpTransferEventListener` callback method Following [GH-428/GH-392](https://github.com/apache/mina-sshd/issues/428) a new `handleReceiveCommandAckInfo` method has been added to enable users to inspect @@ -43,6 +49,13 @@ 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. +### Public `Session` methods to query internal session state values + +Provide (read-only) public access to internal session state values related to KEX, counters, etc..: + +* *getSessionKexDetails* +* *getSessionCountersDetails* + ## Potential compatibility issues ## Major Code Re-factoring diff --git a/docs/standards.md b/docs/standards.md index cc0cfc144..5a24b0784 100644 --- a/docs/standards.md +++ b/docs/standards.md @@ -29,23 +29,31 @@ 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 1.9 transport: 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..fb5647742 100644 --- a/docs/technical/kex.md +++ b/docs/technical/kex.md @@ -129,3 +129,11 @@ 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. + +## [OpenSSH 1.9 transport: strict key exchange extension](https://github.com/openssh/openssh-portable/blob/master/PROTOCOL) + + +There is a **new** `CoreModuleProperties` property that controls the mitigation for the [Terrapin attack](https://terrapin-attack.com/) via what is known as "strict-KEX" +It is **disabled** by default due to its experimental nature and possible interoperability issues, so users who wish to use this feature must turn it on *explicitly*. +The pseudo KEX values are *appended* to the initial proposals sent to the peer and removed when received before proceeding with the standard KEX proposals negotiation so +as not to interfere with it (other than marking that they were detected). diff --git a/sshd-cli/src/main/java/org/apache/sshd/cli/client/SftpCommandMain.java b/sshd-cli/src/main/java/org/apache/sshd/cli/client/SftpCommandMain.java index fde0e8868..a6bf85335 100644 --- a/sshd-cli/src/main/java/org/apache/sshd/cli/client/SftpCommandMain.java +++ b/sshd-cli/src/main/java/org/apache/sshd/cli/client/SftpCommandMain.java @@ -32,7 +32,11 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.time.Instant; +import java.time.ZoneId; +import java.time.format.DateTimeFormatter; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.Map; import java.util.Objects; @@ -57,6 +61,8 @@ import org.apache.sshd.common.kex.KeyExchange; import org.apache.sshd.common.mac.MacFactory; import org.apache.sshd.common.session.Session; +import org.apache.sshd.common.session.helpers.SessionCountersDetails; +import org.apache.sshd.common.session.helpers.SessionKexDetails; import org.apache.sshd.common.signature.SignatureFactory; import org.apache.sshd.common.util.ExceptionUtils; import org.apache.sshd.common.util.GenericUtils; @@ -112,7 +118,19 @@ public SftpCommandMain(SftpClient client) { this.client = Objects.requireNonNull(client, "No client"); Map map = new TreeMap<>(String.CASE_INSENSITIVE_ORDER); - for (SftpCommandExecutor e : Arrays.asList( + Collection commands = obtainAvailableCommands(); + for (SftpCommandExecutor e : commands) { + String name = e.getName(); + ValidateUtils.checkTrue(map.put(name, e) == null, "Multiple commands named '%s'", name); + } + commandsMap = Collections.unmodifiableMap(map); + + Path cwdPath = OsUtils.getCurrentWorkingDirectory(); + cwdLocal = Objects.toString(cwdPath, null); + } + + protected Collection obtainAvailableCommands() { + return Arrays.asList( new ExitCommandExecutor(), new PwdCommandExecutor(), new InfoCommandExecutor(), @@ -135,14 +153,8 @@ public SftpCommandMain(SftpClient client) { new PutCommandExecutor(), new ProgressCommandExecutor(), new LimitsCommandExecutor(), - new HelpCommandExecutor())) { - String name = e.getName(); - ValidateUtils.checkTrue(map.put(name, e) == null, "Multiple commands named '%s'", name); - } - commandsMap = Collections.unmodifiableMap(map); - - Path cwdPath = OsUtils.getCurrentWorkingDirectory(); - cwdLocal = Objects.toString(cwdPath, null); + new RekeyCommandExecutor(), + new HelpCommandExecutor()); } @Override @@ -411,8 +423,8 @@ public static void main(String[] args) throws Exception { ////////////////////////////////////////////////////////////////////////// - private static class ExitCommandExecutor implements SftpCommandExecutor { - ExitCommandExecutor() { + protected static class ExitCommandExecutor implements SftpCommandExecutor { + protected ExitCommandExecutor() { super(); } @@ -433,7 +445,7 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class PwdCommandExecutor implements SftpCommandExecutor { + protected class PwdCommandExecutor implements SftpCommandExecutor { protected PwdCommandExecutor() { super(); } @@ -456,8 +468,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class SessionCommandExecutor implements SftpCommandExecutor { - SessionCommandExecutor() { + protected class SessionCommandExecutor implements SftpCommandExecutor { + protected SessionCommandExecutor() { super(); } @@ -482,14 +494,35 @@ public boolean executeCommand( appendInfoValue(stdout, "Client version", session.getClientVersion()).println(); appendInfoValue(stdout, "Server version", session.getServerVersion()).println(); + + SessionCountersDetails details = session.getSessionCountersDetails(); + + stdout.println("Input counters:"); + appendInfoValue(stdout, "inputBlocksCount", details.getInputBlocksCount()).println(); + appendInfoValue(stdout, "inputBytesCount", details.getInputBytesCount()).println(); + appendInfoValue(stdout, "inputPacketsCount", details.getInputPacketsCount()).println(); + appendInfoValue(stdout, "inputPacketSequenceNumber", details.getInputPacketSequenceNumber()).println(); + appendInfoValue(stdout, "totalIncomingBlocksCount", details.getTotalIncomingBlocksCount()).println(); + appendInfoValue(stdout, "totalIncomingBytesCount", details.getTotalIncomingBytesCount()).println(); + appendInfoValue(stdout, "totalIncomingPacketsCount", details.getTotalIncomingPacketsCount()).println(); + + stdout.println("Output counters:"); + appendInfoValue(stdout, "outputBlocksCount", details.getOutputBlocksCount()).println(); + appendInfoValue(stdout, "outputBytesCount", details.getOutputBytesCount()).println(); + appendInfoValue(stdout, "outputPacketsCount", details.getOutputPacketsCount()).println(); + appendInfoValue(stdout, "outputPacketSequenceNumber", details.getOutputPacketSequenceNumber()).println(); + appendInfoValue(stdout, "totalOutgoingBlocksCount", details.getTotalOutgoingBlocksCount()).println(); + appendInfoValue(stdout, "totalOutgoingBytesCount", details.getTotalOutgoingBytesCount()).println(); + appendInfoValue(stdout, "totalOutgoingPacketsCount", details.getTotalOutgoingPacketsCount()).println(); + return false; } } /* -------------------------------------------------------------------- */ - private class KexCommandExecutor implements SftpCommandExecutor { - KexCommandExecutor() { + protected class KexCommandExecutor implements SftpCommandExecutor { + protected KexCommandExecutor() { super(); } @@ -516,14 +549,29 @@ public boolean executeCommand( appendInfoValue(stdout, description + "[negotiated]", negotiated.get(option)).println(); } + SessionKexDetails details = session.getSessionKexDetails(); + stdout.println("details:"); + appendInfoValue(stdout, "initialKexDone", details.isInitialKexDone()).println(); + appendInfoValue(stdout, "kexState", details.getKexState()).println(); + appendInfoValue(stdout, "strictKexEnabled", details.isStrictKexEnabled()).println(); + appendInfoValue(stdout, "strictKexSignalled", details.isStrictKexSignalled()).println(); + + Instant lastKeyTimeValue = details.getLastKeyTimeValue(); + String lastKeyTimestamp = (lastKeyTimeValue == null) + ? null + : DateTimeFormatter.ISO_LOCAL_DATE_TIME.format(lastKeyTimeValue.atZone(ZoneId.systemDefault())); + appendInfoValue(stdout, "lastKeyTimeValue", lastKeyTimestamp).println(); + appendInfoValue(stdout, "newKeysSentCount", details.getNewKeysSentCount()).println(); + appendInfoValue(stdout, "newKeysReceivedCount", details.getNewKeysReceivedCount()).println(); + return false; } } /* -------------------------------------------------------------------- */ - private class ClientCommandExecutor implements SftpCommandExecutor { - ClientCommandExecutor() { + protected class ClientCommandExecutor implements SftpCommandExecutor { + protected ClientCommandExecutor() { super(); } @@ -562,8 +610,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class InfoCommandExecutor implements SftpCommandExecutor { - InfoCommandExecutor() { + protected class InfoCommandExecutor implements SftpCommandExecutor { + protected InfoCommandExecutor() { super(); } @@ -604,8 +652,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class VersionCommandExecutor implements SftpCommandExecutor { - VersionCommandExecutor() { + protected class VersionCommandExecutor implements SftpCommandExecutor { + protected VersionCommandExecutor() { super(); } @@ -627,8 +675,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class CdCommandExecutor extends PwdCommandExecutor { - CdCommandExecutor() { + protected class CdCommandExecutor extends PwdCommandExecutor { + protected CdCommandExecutor() { super(); } @@ -652,8 +700,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class LcdCommandExecutor extends PwdCommandExecutor { - LcdCommandExecutor() { + protected class LcdCommandExecutor extends PwdCommandExecutor { + protected LcdCommandExecutor() { super(); } @@ -681,8 +729,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class MkdirCommandExecutor implements SftpCommandExecutor { - MkdirCommandExecutor() { + protected class MkdirCommandExecutor implements SftpCommandExecutor { + protected MkdirCommandExecutor() { super(); } @@ -706,8 +754,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class LsCommandExecutor implements SftpCommandExecutor { - LsCommandExecutor() { + protected class LsCommandExecutor implements SftpCommandExecutor { + protected LsCommandExecutor() { super(); } @@ -750,8 +798,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class LlsCommandExecutor implements SftpCommandExecutor { - LlsCommandExecutor() { + protected class LlsCommandExecutor implements SftpCommandExecutor { + protected LlsCommandExecutor() { super(); } @@ -805,8 +853,8 @@ protected void displayLocalPathInfo(Path path, PrintStream stdout) throws IOExce /* -------------------------------------------------------------------- */ - private class RmCommandExecutor implements SftpCommandExecutor { - RmCommandExecutor() { + protected class RmCommandExecutor implements SftpCommandExecutor { + protected RmCommandExecutor() { super(); } @@ -863,7 +911,7 @@ public boolean executeCommand( return false; } - private void removeRecursive( + protected void removeRecursive( SftpClient sftp, String path, Attributes attrs, PrintStream stdout, boolean verbose) throws IOException { if (attrs.isDirectory()) { @@ -894,8 +942,8 @@ private void removeRecursive( /* -------------------------------------------------------------------- */ - private class RmdirCommandExecutor implements SftpCommandExecutor { - RmdirCommandExecutor() { + protected class RmdirCommandExecutor implements SftpCommandExecutor { + protected RmdirCommandExecutor() { super(); } @@ -919,8 +967,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class RenameCommandExecutor implements SftpCommandExecutor { - RenameCommandExecutor() { + protected class RenameCommandExecutor implements SftpCommandExecutor { + protected RenameCommandExecutor() { super(); } @@ -946,8 +994,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class LimitsCommandExecutor implements SftpCommandExecutor { - LimitsCommandExecutor() { + protected class LimitsCommandExecutor implements SftpCommandExecutor { + protected LimitsCommandExecutor() { super(); } @@ -975,8 +1023,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class StatVfsCommandExecutor implements SftpCommandExecutor { - StatVfsCommandExecutor() { + protected class StatVfsCommandExecutor implements SftpCommandExecutor { + protected StatVfsCommandExecutor() { super(); } @@ -1007,8 +1055,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class LStatCommandExecutor implements SftpCommandExecutor { - LStatCommandExecutor() { + protected class LStatCommandExecutor implements SftpCommandExecutor { + protected LStatCommandExecutor() { super(); } @@ -1034,8 +1082,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class ReadLinkCommandExecutor implements SftpCommandExecutor { - ReadLinkCommandExecutor() { + protected class ReadLinkCommandExecutor implements SftpCommandExecutor { + protected ReadLinkCommandExecutor() { super(); } @@ -1060,8 +1108,8 @@ public boolean executeCommand(String args, BufferedReader stdin, PrintStream std /* -------------------------------------------------------------------- */ - private class HelpCommandExecutor implements SftpCommandExecutor { - HelpCommandExecutor() { + protected class HelpCommandExecutor implements SftpCommandExecutor { + protected HelpCommandExecutor() { super(); } @@ -1085,7 +1133,7 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private abstract class TransferCommandExecutor implements SftpCommandExecutor { + protected abstract class TransferCommandExecutor implements SftpCommandExecutor { protected TransferCommandExecutor() { super(); } @@ -1270,8 +1318,8 @@ protected void executeCommand(String args, boolean upload, PrintStream stdout) t /* -------------------------------------------------------------------- */ - private class GetCommandExecutor extends TransferCommandExecutor { - GetCommandExecutor() { + protected class GetCommandExecutor extends TransferCommandExecutor { + protected GetCommandExecutor() { super(); } @@ -1291,8 +1339,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class PutCommandExecutor extends TransferCommandExecutor { - PutCommandExecutor() { + protected class PutCommandExecutor extends TransferCommandExecutor { + protected PutCommandExecutor() { super(); } @@ -1312,8 +1360,8 @@ public boolean executeCommand( /* -------------------------------------------------------------------- */ - private class ProgressCommandExecutor implements SftpCommandExecutor { - ProgressCommandExecutor() { + protected class ProgressCommandExecutor implements SftpCommandExecutor { + protected ProgressCommandExecutor() { super(); } @@ -1346,4 +1394,28 @@ public boolean executeCommand( return false; } } + + /* -------------------------------------------------------------------- */ + + protected class RekeyCommandExecutor implements SftpCommandExecutor { + protected RekeyCommandExecutor() { + super(); + } + + @Override + public String getName() { + return "rekey"; + } + + @Override + public boolean executeCommand( + String args, BufferedReader stdin, PrintStream stdout, PrintStream stderr) + throws Exception { + SftpClient sftp = getClient(); + ClientSession session = sftp.getSession(); + session.reExchangeKeys(); + stdout.println(); + return false; + } + } } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/config/ConfigFileReaderSupport.java b/sshd-common/src/main/java/org/apache/sshd/common/config/ConfigFileReaderSupport.java index 8a5b2486f..680f786a8 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/config/ConfigFileReaderSupport.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/config/ConfigFileReaderSupport.java @@ -89,6 +89,8 @@ public final class ConfigFileReaderSupport { public static final String MACS_CONFIG_PROP = "MACs"; // see http://manpages.ubuntu.com/manpages/precise/en/man5/sshd_config.5.html public static final String KEX_ALGORITHMS_CONFIG_PROP = "KexAlgorithms"; + // see https://github.com/openssh/openssh-portable/blob/master/PROTOCOL - section 1.9 + public static final String STRICT_KEX_CUSTOM_CONFIG_PROP = "UseStrictKex"; // see http://linux.die.net/man/5/ssh_config public static final String HOST_KEY_ALGORITHMS_CONFIG_PROP = "HostKeyAlgorithms"; // see http://manpages.ubuntu.com/manpages/precise/en/man5/sshd_config.5.html 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..0f0746425 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 @@ -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; @@ -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. + * + * 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"; + public static final List STRICT_KEX_EXTENSIONS = Collections.unmodifiableList( + Arrays.asList( + STRICT_KEX_CLIENT_EXTENSION, STRICT_KEX_SERVER_EXTENSION)); + @SuppressWarnings("checkstyle:Indentation") public static final Predicate IS_KEX_EXTENSION_SIGNAL = n -> CLIENT_KEX_EXTENSION.equalsIgnoreCase(n) || SERVER_KEX_EXTENSION.equalsIgnoreCase(n); diff --git a/sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionCountersDetails.java b/sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionCountersDetails.java new file mode 100644 index 000000000..0d9a5ad94 --- /dev/null +++ b/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 Apache MINA SSHD Project + */ +public class SessionCountersDetails { + private long inputPacketSequenceNumber; + private long outputPacketSequenceNumber; + private long inputPacketsCount; + private long outputPacketsCount; + private long totalIncomingPacketsCount; + private long totalOutgoingPacketsCount; + private long inputBytesCount; + private long outputBytesCount; + private long totalIncomingBytesCount; + private long totalOutgoingBytesCount; + private long inputBlocksCount; + private long outputBlocksCount; + private long totalIncomingBlocksCount; + private long totalOutgoingBlocksCount; + + public SessionCountersDetails() { + super(); + } + + public long getInputPacketSequenceNumber() { + return inputPacketSequenceNumber; + } + + public void setInputPacketSequenceNumber(long inputPacketSequenceNumber) { + this.inputPacketSequenceNumber = inputPacketSequenceNumber; + } + + public long getOutputPacketSequenceNumber() { + return outputPacketSequenceNumber; + } + + public void setOutputPacketSequenceNumber(long outputPacketSequenceNumber) { + this.outputPacketSequenceNumber = outputPacketSequenceNumber; + } + + public long getInputPacketsCount() { + return inputPacketsCount; + } + + public void setInputPacketsCount(long inputPacketsCount) { + this.inputPacketsCount = inputPacketsCount; + } + + public long getOutputPacketsCount() { + return outputPacketsCount; + } + + public void setOutputPacketsCount(long outputPacketsCount) { + this.outputPacketsCount = outputPacketsCount; + } + + public long getTotalIncomingPacketsCount() { + return totalIncomingPacketsCount; + } + + public void setTotalIncomingPacketsCount(long totalIncomingPacketsCount) { + this.totalIncomingPacketsCount = totalIncomingPacketsCount; + } + + public long getTotalOutgoingPacketsCount() { + return totalOutgoingPacketsCount; + } + + public void setTotalOutgoingPacketsCount(long totalOutgoingPacketsCount) { + this.totalOutgoingPacketsCount = totalOutgoingPacketsCount; + } + + public long getInputBytesCount() { + return inputBytesCount; + } + + public void setInputBytesCount(long inputBytesCount) { + this.inputBytesCount = inputBytesCount; + } + + public long getOutputBytesCount() { + return outputBytesCount; + } + + public void setOutputBytesCount(long outputBytesCount) { + this.outputBytesCount = outputBytesCount; + } + + public long getTotalIncomingBytesCount() { + return totalIncomingBytesCount; + } + + public void setTotalIncomingBytesCount(long totalIncomingBytesCount) { + this.totalIncomingBytesCount = totalIncomingBytesCount; + } + + public long getTotalOutgoingBytesCount() { + return totalOutgoingBytesCount; + } + + public void setTotalOutgoingBytesCount(long totalOutgoingBytesCount) { + this.totalOutgoingBytesCount = totalOutgoingBytesCount; + } + + public long getInputBlocksCount() { + return inputBlocksCount; + } + + public void setInputBlocksCount(long inputBlocksCount) { + this.inputBlocksCount = inputBlocksCount; + } + + public long getOutputBlocksCount() { + return outputBlocksCount; + } + + public void setOutputBlocksCount(long outputBlocksCount) { + this.outputBlocksCount = outputBlocksCount; + } + + public long getTotalIncomingBlocksCount() { + return totalIncomingBlocksCount; + } + + public void setTotalIncomingBlocksCount(long totalIncomingBlocksCount) { + this.totalIncomingBlocksCount = totalIncomingBlocksCount; + } + + public long getTotalOutgoingBlocksCount() { + return totalOutgoingBlocksCount; + } + + public void setTotalOutgoingBlocksCount(long totalOutgoingBlocksCount) { + this.totalOutgoingBlocksCount = totalOutgoingBlocksCount; + } +} diff --git a/sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionKexDetails.java b/sshd-common/src/main/java/org/apache/sshd/common/session/helpers/SessionKexDetails.java new file mode 100644 index 000000000..723f41dbb --- /dev/null +++ b/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 Apache MINA SSHD Project + */ +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) { + this.strictKexSignalled = strictKexSignalled; + } + + // TODO add the KEX extensions values (if any) + + /** + * @return Number of times the session sent the {@code SSH_MSG_NEWKEYS} command + */ + public int getNewKeysSentCount() { + return newKeysSentCount; + } + + public void setNewKeysSentCount(int newKeysSentCount) { + this.newKeysSentCount = newKeysSentCount; + } + + /** + * @return Number of times the session received the {@code SSH_MSG_NEWKEYS} command + */ + public int getNewKeysReceivedCount() { + return newKeysReceivedCount; + } + + public void setNewKeysReceivedCount(int newKeysReceivedCount) { + this.newKeysReceivedCount = newKeysReceivedCount; + } + + /** + * @return Last {@link Instant} when new keys were established - may be {@code null} if new keys not yet established + */ + public Instant getLastKeyTimeValue() { + return lastKeyTimeValue; + } + + public void setLastKeyTimeValue(Instant lastKeyTimeValue) { + this.lastKeyTimeValue = lastKeyTimeValue; + } + + @Override + public String toString() { + return getClass().getSimpleName() + + "[initialKexDone=" + isInitialKexDone() + + ", kexState=" + getKexState() + + ", strictKexEnabled=" + isStrictKexEnabled() + + ", strictKexSignalled=" + isStrictKexSignalled() + + ", newKeysSentCount=" + getNewKeysSentCount() + + ", newKeysReceivedCount=" + getNewKeysReceivedCount() + + ", lastKeyTimeValue=" + getLastKeyTimeValue() + + "]"; + } +} diff --git a/sshd-core/src/main/java/org/apache/sshd/common/config/SshConfigFileReader.java b/sshd-core/src/main/java/org/apache/sshd/common/config/SshConfigFileReader.java index e4244a184..7fa1b80fd 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/config/SshConfigFileReader.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/config/SshConfigFileReader.java @@ -45,6 +45,7 @@ import org.apache.sshd.common.signature.Signature; import org.apache.sshd.common.util.GenericUtils; import org.apache.sshd.common.util.ValidateUtils; +import org.apache.sshd.core.CoreModuleProperties; /** * Reads and interprets some useful configurations from an OpenSSH configuration file. @@ -256,6 +257,11 @@ public static M configureKeyExchanges( M manager, PropertyResolver props, boolean lenient, Function xformer, boolean ignoreUnsupported) { Objects.requireNonNull(props, "No properties to configure"); + Boolean useStrictKex = props.getBoolean(ConfigFileReaderSupport.STRICT_KEX_CUSTOM_CONFIG_PROP); + if (useStrictKex != null) { + CoreModuleProperties.USE_STRICT_KEX.set(manager, useStrictKex); + } + return configureKeyExchanges(manager, props.getString(ConfigFileReaderSupport.KEX_ALGORITHMS_CONFIG_PROP), lenient, xformer, ignoreUnsupported); diff --git a/sshd-core/src/main/java/org/apache/sshd/common/session/Session.java b/sshd-core/src/main/java/org/apache/sshd/common/session/Session.java index 07eb80fd7..c3b28801b 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/session/Session.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/session/Session.java @@ -41,6 +41,8 @@ import org.apache.sshd.common.io.IoWriteFuture; import org.apache.sshd.common.kex.KexFactoryManager; import org.apache.sshd.common.kex.KeyExchange; +import org.apache.sshd.common.session.helpers.SessionCountersDetails; +import org.apache.sshd.common.session.helpers.SessionKexDetails; import org.apache.sshd.common.session.helpers.TimeoutIndicator; import org.apache.sshd.common.util.ValidateUtils; import org.apache.sshd.common.util.buffer.Buffer; @@ -65,6 +67,8 @@ public interface Session FactoryManagerHolder, PortForwardingInformationProvider { + SessionCountersDetails getSessionCountersDetails(); + /** * Create a new buffer for the specified SSH packet and reserve the needed space (5 bytes) for the packet header. * @@ -255,6 +259,11 @@ GlobalRequestFuture request(Buffer buffer, String request, ReplyHandler replyHan */ void exceptionCaught(Throwable t); + /** + * @return Information about internal KEX related state values + */ + SessionKexDetails getSessionKexDetails(); + /** * Initiate a new key exchange. * 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..58bc541bb 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 @@ -41,10 +41,14 @@ import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.function.LongConsumer; import java.util.logging.Level; +import java.util.stream.Collectors; +import java.util.stream.Stream; import org.apache.sshd.common.Closeable; import org.apache.sshd.common.Factory; @@ -109,6 +113,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 @@ -151,24 +156,34 @@ public abstract class AbstractSession extends SessionHelper { protected final Map clientProposal = new EnumMap<>(KexProposalOption.class); protected final Map unmodClientProposal = Collections.unmodifiableMap(clientProposal); protected final Map negotiationResult = new EnumMap<>(KexProposalOption.class); - protected final Map unmodNegotiationResult = Collections.unmodifiableMap(negotiationResult); + protected final Map 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 = new AtomicReference<>(KexState.UNKNOWN); protected final AtomicReference 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; protected Cipher outCipher; protected Cipher inCipher; protected int outCipherSize = 8; @@ -180,10 +195,6 @@ public abstract class AbstractSession extends SessionHelper { protected byte[] inMacResult; protected Compression outCompression; protected Compression inCompression; - /** Input packet sequence number. */ - protected long seqi; - /** Output packet sequence number. */ - protected long seqo; protected SessionWorkBuffer uncompressBuffer; protected final SessionWorkBuffer decoderBuffer; protected int decoderState; @@ -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 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 protected long maxRekyPackets; protected long maxRekeyBytes; protected Duration maxRekeyInterval; @@ -236,10 +254,10 @@ public abstract class AbstractSession extends SessionHelper { protected long ignorePacketsFrequency; protected int ignorePacketsVariance; - protected final AtomicLong maxRekeyBlocks - = new AtomicLong(CoreModuleProperties.REKEY_BYTES_LIMIT.getRequiredDefault() / 16); - protected final AtomicLong ignorePacketsCount - = new AtomicLong(CoreModuleProperties.IGNORE_MESSAGE_FREQUENCY.getRequiredDefault()); + protected final AtomicLong maxRekeyBlocks = new AtomicLong( + CoreModuleProperties.REKEY_BYTES_LIMIT.getRequiredDefault() / 16); + protected final AtomicLong ignorePacketsCount = new AtomicLong( + CoreModuleProperties.IGNORE_MESSAGE_FREQUENCY.getRequiredDefault()); /** * Used to wait for results of global requests sent with {@code want-reply = true}. Note that per RFC 4254, global @@ -299,19 +317,15 @@ protected AbstractSession(boolean serverSession, FactoryManager factoryManager, attachSession(ioSession, this); - Factory factory = ValidateUtils.checkNotNull( - factoryManager.getRandomFactory(), "No random factory for %s", ioSession); - random = ValidateUtils.checkNotNull( - factory.create(), "No randomizer instance for %s", ioSession); + Factory factory = ValidateUtils.checkNotNull(factoryManager.getRandomFactory(), + "No random factory for %s", ioSession); + random = ValidateUtils.checkNotNull(factory.create(), "No randomizer instance for %s", ioSession); refreshConfiguration(); - sessionListenerProxy = EventListenerUtils.proxyWrapper( - SessionListener.class, sessionListeners); - channelListenerProxy = EventListenerUtils.proxyWrapper( - ChannelListener.class, channelListeners); - tunnelListenerProxy = EventListenerUtils.proxyWrapper( - PortForwardingEventListener.class, tunnelListeners); + sessionListenerProxy = EventListenerUtils.proxyWrapper(SessionListener.class, sessionListeners); + channelListenerProxy = EventListenerUtils.proxyWrapper(ChannelListener.class, channelListeners); + tunnelListenerProxy = EventListenerUtils.proxyWrapper(PortForwardingEventListener.class, tunnelListeners); try { signalSessionEstablished(ioSession); @@ -358,11 +372,14 @@ public static int calculatePadLength(int len, int blockSize, boolean etmMode) { /* * Note: according to RFC-4253 section 6: * - * The minimum size of a packet is 16 (or the cipher block size, whichever is larger) bytes (plus 'mac'). + * The minimum size of a packet is 16 (or the cipher block size, + * whichever is larger) bytes (plus 'mac'). * - * Since all out ciphers, MAC(s), etc. have a block size > 8 then the minimum size of the packet will be at - * least 16 due to the padding at the very least - so even packets that contain an opcode with no arguments will - * be above this value. This avoids an un-necessary call to Math.max(len, 16) for each and every packet + * Since all out ciphers, MAC(s), etc. have a block size > 8 then the + * minimum size of the packet will be at least 16 due to the padding at + * the very least - so even packets that contain an opcode with no + * arguments will be above this value. This avoids an un-necessary call + * to Math.max(len, 16) for each and every packet */ len++; // the pad length @@ -373,12 +390,14 @@ public static int calculatePadLength(int len, int blockSize, boolean etmMode) { /* * Note: according to RFC-4253 section 6: * - * Note that the length of the concatenation of 'packet_length', 'padding_length', 'payload', and 'random - * padding' MUST be a multiple of the cipher block size or 8, whichever is larger. + * Note that the length of the concatenation of 'packet_length', + * 'padding_length', 'payload', and 'random padding' MUST be a multiple + * of the cipher block size or 8, whichever is larger. * - * However, we currently do not have ciphers with a block size of less than 8 so we do not take this into - * account in order to accelerate the calculation and avoiding an un-necessary call to Math.max(blockSize, 8) - * for each and every packet. + * However, we currently do not have ciphers with a block size of less + * than 8 so we do not take this into account in order to accelerate the + * calculation and avoiding an un-necessary call to Math.max(blockSize, + * 8) for each and every packet. */ int pad = (-len) & (blockSize - 1); if (pad < blockSize) { @@ -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 // one should be set by the readIdentification method if ((clientVersion == null) || (serverVersion == null)) { if (readIdentification(decoderBuffer)) { @@ -502,12 +522,35 @@ protected void refreshConfiguration() { ignorePacketsVariance = 0; } - long countValue = calculateNextIgnorePacketCount( - random, ignorePacketsFrequency, ignorePacketsVariance); + long countValue = calculateNextIgnorePacketCount(random, ignorePacketsFrequency, ignorePacketsVariance); ignorePacketsCount.set(countValue); } } + @Override + public SessionCountersDetails getSessionCountersDetails() { + SessionCountersDetails details = new SessionCountersDetails(); + details.setInputBlocksCount(inBlocksCount.get()); + details.setInputBytesCount(inBytesCount.get()); + details.setInputPacketsCount(inPacketsCount.get()); + details.setInputPacketSequenceNumber(seqi); + + details.setOutputBlocksCount(outBlocksCount.get()); + details.setOutputBytesCount(outBytesCount.get()); + details.setOutputPacketsCount(outPacketsCount.get()); + details.setOutputPacketSequenceNumber(seqo); + + details.setTotalIncomingBlocksCount(totalIncomingBlocksCount.get()); + details.setTotalIncomingBytesCount(totalIncomingBytesCount.get()); + details.setTotalIncomingPacketsCount(totalIncomingPacketsCount.get()); + + details.setTotalOutgoingBlocksCount(totalOutgoingBlocksCount.get()); + details.setTotalOutgoingBytesCount(totalOutgoingBytesCount.get()); + details.setTotalOutgoingPacketsCount(totalOutgingPacketsCount.get()); + + return details; + } + /** * Abstract method for processing incoming decoded packets. The given buffer will hold the decoded packet, starting * from the command byte at the read position. @@ -540,9 +583,29 @@ 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. + */ + if ((totalIncomingPacketsCount.get() == 1L) + && CoreModuleProperties.USE_STRICT_KEX.getRequired(this) + && (cmd != SshConstants.SSH_MSG_KEXINIT)) { + log.error("doHandleMessage({}) invalid 1st message: {}", this, SshConstants.getCommandMessageName(cmd)); + throw new SshException(SshConstants.SSH2_DISCONNECT_PROTOCOL_ERROR, "Strict KEX Error"); + } + if (log.isDebugEnabled()) { - log.debug("doHandleMessage({}) process #{} {}", this, seqi - 1, - SshConstants.getCommandMessageName(cmd)); + log.debug("doHandleMessage({}) process #{} {}", this, seqi - 1L, SshConstants.getCommandMessageName(cmd)); } switch (cmd) { @@ -595,12 +658,12 @@ protected void doHandleMessage(Buffer buffer) throws Exception { /* * 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. + * 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)); + log.debug("process({}) Unsupported command: {}", this, SshConstants.getCommandMessageName(cmd)); } notImplemented(cmd, buffer); } @@ -611,13 +674,15 @@ protected void doHandleMessage(Buffer buffer) throws Exception { protected boolean handleFirstKexPacketFollows(int cmd, Buffer buffer, boolean followFlag) { if (!followFlag) { - return true; // if 1st KEX packet does not follow then process the command + return true; // if 1st KEX packet does not follow then process the + // command } /* * According to RFC4253 section 7.1: * - * If the other party's guess was wrong, and this field was TRUE, the next packet MUST be silently ignored + * If the other party's guess was wrong, and this field was TRUE, the + * next packet MUST be silently ignored */ boolean debugEnabled = log.isDebugEnabled(); for (KexProposalOption option : KexProposalOption.FIRST_KEX_PACKET_GUESS_MATCHES) { @@ -670,10 +735,16 @@ protected IoWriteFuture sendNewKeys() throws Exception { Buffer buffer = createBuffer(SshConstants.SSH_MSG_NEWKEYS, Byte.SIZE); IoWriteFuture future; synchronized (encodeLock) { - // writePacket() would also work since it would never try to queue the packet, and would never try to - // initiate a new KEX, and thus would never try to get the kexLock monitor. If it did, we might get a - // deadlock due to lock inversion. It seems safer to push this out directly, though. + // writePacket() would also work since it would never try to queue + // the packet, and would never try to + // initiate a new KEX, and thus would never try to get the kexLock + // monitor. If it did, we might get a + // deadlock due to lock inversion. It seems safer to push this out + // directly, though. future = doWritePacket(buffer); + + newKeysSignalled(true); + // Use the new settings from now on for any outgoing packet setOutputEncoding(); } @@ -684,10 +755,12 @@ protected IoWriteFuture sendNewKeys() throws Exception { * According to https://tools.ietf.org/html/rfc8308#section-2.4: * * - * If a client sends SSH_MSG_EXT_INFO, it MUST send it as the next packet following the client's first - * SSH_MSG_NEWKEYS message to the server. + * If a client sends SSH_MSG_EXT_INFO, it MUST send it as the next + * packet following the client's first SSH_MSG_NEWKEYS message to the + * server. * - * If a server sends SSH_MSG_EXT_INFO, it MAY send it at zero, one, or both of the following opportunities: + * If a server sends SSH_MSG_EXT_INFO, it MAY send it at zero, one, or + * both of the following opportunities: * * + As the next packet following the server's first SSH_MSG_NEWKEYS. */ @@ -721,15 +794,13 @@ protected void handleKexMessage(int cmd, Buffer buffer) throws Exception { boolean debugEnabled = log.isDebugEnabled(); if (kex.next(cmd, buffer)) { if (debugEnabled) { - log.debug("handleKexMessage({})[{}] KEX processing complete after cmd={}", - this, kex.getName(), cmd); + log.debug("handleKexMessage({})[{}] KEX processing complete after cmd={}", this, kex.getName(), cmd); } checkKeys(); sendNewKeys(); } else { if (debugEnabled) { - log.debug("handleKexMessage({})[{}] more KEX packets expected after cmd={}", - this, kex.getName(), cmd); + log.debug("handleKexMessage({})[{}] more KEX packets expected after cmd={}", this, kex.getName(), cmd); } } } @@ -776,8 +847,8 @@ protected boolean handleServiceRequest(String serviceName, Buffer buffer) throws try { startService(serviceName, buffer); } catch (Throwable e) { - debug("handleServiceRequest({}) Service {} rejected: {} = {}", - this, serviceName, e.getClass().getSimpleName(), e.getMessage(), e); + debug("handleServiceRequest({}) Service {} rejected: {} = {}", this, serviceName, + e.getClass().getSimpleName(), e.getMessage(), e); disconnect(SshConstants.SSH2_DISCONNECT_SERVICE_NOT_AVAILABLE, "Bad service request: " + serviceName); return false; } @@ -786,8 +857,8 @@ protected boolean handleServiceRequest(String serviceName, Buffer buffer) throws log.debug("handleServiceRequest({}) Accepted service {}", this, serviceName); } - Buffer response = createBuffer( - SshConstants.SSH_MSG_SERVICE_ACCEPT, Byte.SIZE + GenericUtils.length(serviceName)); + Buffer response = createBuffer(SshConstants.SSH_MSG_SERVICE_ACCEPT, + Byte.SIZE + GenericUtils.length(serviceName)); response.putString(serviceName); writePacket(response); return true; @@ -797,8 +868,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 + // before the initial KEX. return initialKexDone; } return false; @@ -820,6 +893,19 @@ protected void handleServiceAccept(String serviceName, Buffer buffer) throws Exc } } + @Override + public SessionKexDetails getSessionKexDetails() { + SessionKexDetails details = new SessionKexDetails(); + details.setInitialKexDone(initialKexDone); + details.setKexState(getKexState()); + details.setStrictKexEnabled(CoreModuleProperties.USE_STRICT_KEX.getRequired(this)); + details.setStrictKexSignalled(strictKexSignalled.get()); + details.setLastKeyTimeValue(lastKeyTimeValue.get()); + details.setNewKeysSentCount(newKeysSentCount.get()); + details.setNewKeysReceivedCount(newKeysReceivedCount.get()); + return details; + } + protected void handleKexInit(Buffer buffer) throws Exception { if (log.isDebugEnabled()) { log.debug("handleKexInit({}) SSH_MSG_KEXINIT", this); @@ -858,9 +944,12 @@ protected void doKexNegotiation() throws Exception { kexInitializedFuture = initFuture; } } - // requestNewKeyExchange() is running in some other thread: wait until it has set up our own proposal. - // The timeout is a last resort only to avoid blocking indefinitely in case something goes - // catastrophically wrong somewhere; it should never be hit. If it is, an exception will be thrown. + // requestNewKeyExchange() is running in some other thread: wait + // until it has set up our own proposal. + // The timeout is a last resort only to avoid blocking indefinitely + // in case something goes + // catastrophically wrong somewhere; it should never be hit. If it + // is, an exception will be thrown. // // See https://issues.apache.org/jira/browse/SSHD-1197 initFuture.await(CoreModuleProperties.KEX_PROPOSAL_SETUP_TIMEOUT.getRequired(this)); @@ -872,8 +961,8 @@ protected void doKexNegotiation() throws Exception { Map result = negotiate(); String kexAlgorithm = result.get(KexProposalOption.ALGORITHMS); Collection kexFactories = getKeyExchangeFactories(); - KeyExchangeFactory kexFactory = NamedResource.findByName( - kexAlgorithm, String.CASE_INSENSITIVE_ORDER, kexFactories); + KeyExchangeFactory kexFactory = NamedResource.findByName(kexAlgorithm, String.CASE_INSENSITIVE_ORDER, + kexFactories); ValidateUtils.checkNotNull(kexFactory, "Unknown negotiated KEX algorithm: %s", kexAlgorithm); byte[] v_s = serverVersion.getBytes(StandardCharsets.UTF_8); @@ -897,11 +986,22 @@ protected void doKexNegotiation() throws Exception { protected void handleNewKeys(int cmd, Buffer buffer) throws Exception { boolean debugEnabled = log.isDebugEnabled(); if (debugEnabled) { - log.debug("handleNewKeys({}) SSH_MSG_NEWKEYS command={}", - this, SshConstants.getCommandMessageName(cmd)); + log.debug("handleNewKeys({}) SSH_MSG_NEWKEYS command={}", this, SshConstants.getCommandMessageName(cmd)); } validateKexState(cmd, KexState.KEYS); - // It is guaranteed that we handle the peer's SSH_MSG_NEWKEYS after having sent our own. + + /* + * Terrapin attack mitigation - see + * https://github.com/openssh/openssh-portable/blob/master/PROTOCOL + * section 1.9: transport: strict key exchange extension + * + * After sending or receiving a SSH2_MSG_NEWKEYS message, reset the + * packet sequence number to zero. + */ + newKeysSignalled(false); + + // It is guaranteed that we handle the peer's SSH_MSG_NEWKEYS after + // having sent our own. // prepareNewKeys() was already called in sendNewKeys(). // // From now on, use the new settings for any incoming message. @@ -939,10 +1039,7 @@ protected void validateKexState(int cmd, KexState expected) { @Override protected Closeable getInnerCloseable() { - Closeable closer = builder() - .parallel(toString(), getServices()) - .close(getIoSession()) - .build(); + Closeable closer = builder().parallel(toString(), getServices()).close(getIoSession()).build(); closer.addCloseFutureListener(future -> clearAttributes()); return closer; } @@ -963,7 +1060,8 @@ protected void preClose() { } kexHandler.shutdown(); - // if anyone waiting for global response notify them about the closing session + // if anyone waiting for global response notify them about the closing + // session boolean debugEnabled = log.isDebugEnabled(); for (;;) { GlobalRequestFuture future = pendingGlobalRequests.pollLast(); @@ -971,7 +1069,8 @@ protected void preClose() { break; } if (debugEnabled) { - log.debug("preClose({}): Session closing; failing still pending global request {}", this, future.getId()); + log.debug("preClose({}): Session closing; failing still pending global request {}", this, + future.getId()); } future.setValue(new SshException("Session is closing")); } @@ -991,16 +1090,14 @@ protected void preClose() { protected List getServices() { Service service = currentService.getService(); - return (service != null) - ? Collections.singletonList(service) - : Collections.emptyList(); + return (service != null) ? Collections.singletonList(service) : Collections.emptyList(); } @Override public T getService(Class clazz) { Collection registeredServices = getServices(); - ValidateUtils.checkState(GenericUtils.isNotEmpty(registeredServices), - "No registered services to look for %s", clazz.getSimpleName()); + ValidateUtils.checkState(GenericUtils.isNotEmpty(registeredServices), "No registered services to look for %s", + clazz.getSimpleName()); for (Service s : registeredServices) { if (clazz.isInstance(s)) { @@ -1117,21 +1214,105 @@ protected IoWriteFuture doWritePacket(Buffer buffer) throws IOException { } } + /** + * Called to indicate that {@link SshConstants#SSH_MSG_NEWKEYS} was either sent or received + * + * @param sentNewKeys Indicates whether the message was sent or received + * @return The number of times this has occurred (including this one) + */ + protected int newKeysSignalled(boolean sentNewKeys) { + AtomicInteger holder = sentNewKeys + ? newKeysSentCount + : newKeysReceivedCount; + int count = holder.incrementAndGet(); + if (log.isDebugEnabled()) { + log.debug("newKeysSignalled({})[sentNewKeys={}] count={}", this, sentNewKeys, count); + } + + /* + * Terrapin attack mitigation - see + * https://github.com/openssh/openssh-portable/blob/master/PROTOCOL + * section 1.9: transport: strict key exchange extension + * + * After sending or receiving a SSH2_MSG_NEWKEYS message, reset the + * packet sequence number to zero. + */ + if (strictKexSignalled.get()) { + resetSequenceNumbers(sentNewKeys); + } + + return count; + } + + protected void resetSequenceNumbers(boolean sentNewkeys) { + /* + * We rely on the fact that SSH_MSG_NEWKEYS is symmetric and if we + * initiated one then an incoming one is due from our peer (and vice + * versa). Therefore: + * + * - if we initiated the message, we can reset our sequence number and + * rely on receiving the peer's response to reset our tracking of its + * counter. We still need it to decode our peer's response and thus have + * to wait for it before resetting our tracking value. + * + * - if we are the peer that received the message then we can reset our + * tracking of the initiator's counter, relying on the fact that it did + * it to its own counter. After (!) we send our response we will reset + * our counter as well. + */ + long prevSeqno; + synchronized (strictKexSignalled) { + if (sentNewkeys) { + prevSeqno = seqo; + seqo = 0L; + } else { + prevSeqno = seqi; + seqi = 0L; + } + } + + if (log.isDebugEnabled()) { + log.debug("resetSequenceNumbers({})[sentNewKeys={}] packet couter={}", this, sentNewkeys, prevSeqno); + } + } + protected int resolveIgnoreBufferDataLength() { + // Ignore if feature disabled if ((ignorePacketDataLength <= 0) || (ignorePacketsFrequency <= 0L) || (ignorePacketsVariance < 0)) { return 0; } + /* + * Terrapin attack mitigation - see + * https://github.com/openssh/openssh-portable/blob/master/PROTOCOL + * section 1.9: transport: strict key exchange extension + * + * We need to defer sending any stuffing SSH_MSG_IGNORE messages so that + * the peer does not close the connection + */ + + if (CoreModuleProperties.USE_STRICT_KEX.getRequired(this)) { + // Do not prepend a stuffed message before 1st packet in order to enabled strict KEX + if (totalOutgingPacketsCount.get() == 0L) { + return 0; + } + + // Do not prepend a stuffed message if using strict KEX and NEWKEYS message neither sent nor received + if (strictKexSignalled.get() + && ((newKeysReceivedCount.get() <= 0) || (newKeysSentCount.get() <= 0))) { + return 0; + } + } + long count = ignorePacketsCount.decrementAndGet(); if (count > 0L) { return 0; } synchronized (random) { - count = calculateNextIgnorePacketCount( - random, ignorePacketsFrequency, ignorePacketsVariance); + count = calculateNextIgnorePacketCount(random, ignorePacketsFrequency, ignorePacketsVariance); ignorePacketsCount.set(count); return ignorePacketDataLength + random.random(ignorePacketDataLength); } @@ -1173,18 +1354,22 @@ public Buffer request(String request, Buffer buffer, long maxWaitMillis) throws if (withReply) { if (debugEnabled) { - log.debug("request({}) request={}, timeout={}ms, requestSeqNo={}, done {}, result received={}", this, request, - maxWaitMillis, future.getSequenceNumber(), done, result instanceof Buffer); + log.debug("request({}) request={}, timeout={}ms, requestSeqNo={}, done {}, result received={}", this, + request, maxWaitMillis, future.getSequenceNumber(), done, result instanceof Buffer); } if (!done || result == null) { - throw new SocketTimeoutException("No response received after " + maxWaitMillis + "ms for request=" + request); + throw new SocketTimeoutException( + "No response received after " + maxWaitMillis + "ms for request=" + request); } - // The operation is specified to return null if the request could be made, but got an error reply. - // The caller cannot distinguish between SSH_MSG_UNIMPLEMENTED and SSH_MSG_REQUEST_FAILURE. + // The operation is specified to return null if the request could be + // made, but got an error reply. + // The caller cannot distinguish between SSH_MSG_UNIMPLEMENTED and + // SSH_MSG_REQUEST_FAILURE. if (result instanceof GlobalRequestException) { if (debugEnabled) { - log.debug("request({}) request={}, requestSeqNo={}: received={}", this, request, future.getSequenceNumber(), + log.debug("request({}) request={}, requestSeqNo={}: received={}", this, request, + future.getSequenceNumber(), SshConstants.getCommandMessageName(((GlobalRequestException) result).getCode())); } return null; @@ -1208,8 +1393,10 @@ public GlobalRequestFuture request(Buffer buffer, String request, GlobalRequestF if (!isOpen()) { throw new IOException("Global request " + request + ": session is closing or closed."); } - // Fire-and-forget global requests (want-reply = false) are always allowed; we don't need to register the - // future, nor do we have to wait for anything. Client code can wait on the returned future if it wants to + // Fire-and-forget global requests (want-reply = false) are always + // allowed; we don't need to register the + // future, nor do we have to wait for anything. Client code can wait + // on the returned future if it wants to // be sure the message has been sent. globalRequest = new GlobalRequestFuture(request, replyHandler) { @@ -1231,23 +1418,31 @@ public void operationComplete(IoWriteFuture future) { writePacket(buffer).addListener(globalRequest); return globalRequest; } - // We do expect a reply. The packet may get queued or otherwise delayed for an unknown time. We must - // consider this request pending only once its sequence number is known. If sending the message fails, - // the writeFuture will set an exception on the globalRequest, or will fail it. + // We do expect a reply. The packet may get queued or otherwise delayed + // for an unknown time. We must + // consider this request pending only once its sequence number is known. + // If sending the message fails, + // the writeFuture will set an exception on the globalRequest, or will + // fail it. globalRequest = new GlobalRequestFuture(request, replyHandler) { @Override + @SuppressWarnings("synthetic-access") public void operationComplete(IoWriteFuture future) { if (!future.isWritten()) { - // If it was not written after all, make sure it's not considered pending anymore. + // If it was not written after all, make sure it's not + // considered pending anymore. pendingGlobalRequests.removeFirstOccurrence(this); } // Super call will fulfill the future if not written super.operationComplete(future); if (future.isWritten() && getHandler() != null) { - // Fulfill this future now. The GlobalRequestFuture can thus be used to wait for the - // successful sending of the request, the framework will invoke the handler whenever - // the reply arrives. The buffer cannot be obtained though the future. + // Fulfill this future now. The GlobalRequestFuture can thus + // be used to wait for the + // successful sending of the request, the framework will + // invoke the handler whenever + // the reply arrives. The buffer cannot be obtained though + // the future. setValue(null); } } @@ -1255,13 +1450,16 @@ public void operationComplete(IoWriteFuture future) { if (!isOpen()) { throw new IOException("Global request " + request + ": session is closing or closed."); } - // This consumer will be invoked once before the packet actually goes out. Some servers respond to global - // requests with SSH_MSG_UNIMPLEMENTED instead of SSH_MSG_REQUEST_FAILURE (see SSHD-968), so we need to make + // This consumer will be invoked once before the packet actually goes + // out. Some servers respond to global + // requests with SSH_MSG_UNIMPLEMENTED instead of + // SSH_MSG_REQUEST_FAILURE (see SSHD-968), so we need to make // sure we do know the sequence number. globalSequenceNumbers.put(buffer, seqNo -> { globalRequest.setSequenceNumber(seqNo); if (log.isDebugEnabled()) { - log.debug("makeGlobalRequest({})[{}] want-reply=true with seqNo={}", this, globalRequest.getId(), seqNo); + log.debug("makeGlobalRequest({})[{}] want-reply=true with seqNo={}", this, globalRequest.getId(), + seqNo); } // Insert at front pendingGlobalRequests.push(globalRequest); @@ -1269,42 +1467,47 @@ public void operationComplete(IoWriteFuture future) { writePacket(buffer).addListener(f -> { Throwable t = f.getException(); if (t != null) { - // Just in case we get an exception before preProcessEncodeBuffer was even called + // Just in case we get an exception before + // preProcessEncodeBuffer was even called globalSequenceNumbers.remove(buffer); } - }).addListener(globalRequest); // Report errors through globalRequest, fulfilling globalRequest + }).addListener(globalRequest); // Report errors through globalRequest, + // fulfilling globalRequest return globalRequest; } @Override protected boolean doInvokeUnimplementedMessageHandler(int cmd, Buffer buffer) throws Exception { /* - * SSHD-968 Some servers respond to global requests with SSH_MSG_UNIMPLEMENTED instead of - * SSH_MSG_REQUEST_FAILURE (as mandated by https://tools.ietf.org/html/rfc4254#section-4) so deal with it. + * SSHD-968 Some servers respond to global requests with + * SSH_MSG_UNIMPLEMENTED instead of SSH_MSG_REQUEST_FAILURE (as mandated + * by https://tools.ietf.org/html/rfc4254#section-4) so deal with it. */ if (!pendingGlobalRequests.isEmpty() && cmd == SshConstants.SSH_MSG_UNIMPLEMENTED) { // We do have ongoing global requests. long msgSeqNo = buffer.rawUInt(buffer.rpos()); // Find the global request this applies to - GlobalRequestFuture future = pendingGlobalRequests.stream().filter(f -> f.getSequenceNumber() == msgSeqNo).findAny() - .orElse(null); + GlobalRequestFuture future = pendingGlobalRequests.stream().filter(f -> f.getSequenceNumber() == msgSeqNo) + .findAny().orElse(null); if (future != null && pendingGlobalRequests.removeFirstOccurrence(future)) { // This SSH_MSG_UNIMPLEMENTED was the reply to a global request. if (log.isDebugEnabled()) { - log.debug("doInvokeUnimplementedMessageHandler({}) report global request={} failure for seqNo={}", this, - future.getId(), msgSeqNo); + log.debug("doInvokeUnimplementedMessageHandler({}) report global request={} failure for seqNo={}", + this, future.getId(), msgSeqNo); } GlobalRequestFuture.ReplyHandler handler = future.getHandler(); if (handler != null) { - Buffer resultBuf = ByteArrayBuffer.getCompactClone(buffer.array(), buffer.rpos(), buffer.available()); + Buffer resultBuf = ByteArrayBuffer.getCompactClone(buffer.array(), buffer.rpos(), + buffer.available()); handler.accept(cmd, resultBuf); } else { future.setValue(new GlobalRequestException(cmd)); } return true; // message handled internally } else if (future != null) { - // The SSH_MSG_UNIMPLEMENTED was for a global request, but that request is no longer in the list: it + // The SSH_MSG_UNIMPLEMENTED was for a global request, but that + // request is no longer in the list: it // got terminated otherwise. return true; } @@ -1360,10 +1563,10 @@ public Buffer prepareBuffer(byte cmd, Buffer buffer) { */ protected B validateTargetBuffer(int cmd, B buffer) { ValidateUtils.checkNotNull(buffer, "No target buffer to examine for command=%d", cmd); - ValidateUtils.checkTrue( - buffer != decoderBuffer, "Not allowed to use the internal decoder buffer for command=%d", cmd); - ValidateUtils.checkTrue( - buffer != uncompressBuffer, "Not allowed to use the internal uncompress buffer for command=%d", cmd); + ValidateUtils.checkTrue(buffer != decoderBuffer, + "Not allowed to use the internal decoder buffer for command=%d", cmd); + ValidateUtils.checkTrue(buffer != uncompressBuffer, + "Not allowed to use the internal uncompress buffer for command=%d", cmd); return buffer; } @@ -1381,7 +1584,8 @@ protected Buffer encode(Buffer buffer) throws IOException { try { // Check that the packet has some free space for the header int curPos = buffer.rpos(); - int cmd = buffer.rawByte(curPos) & 0xFF; // usually the 1st byte is an SSH opcode + int cmd = buffer.rawByte(curPos) & 0xFF; // usually the 1st byte is + // an SSH opcode Buffer nb = preProcessEncodeBuffer(cmd, buffer); if (nb != buffer) { buffer = nb; @@ -1389,9 +1593,9 @@ protected Buffer encode(Buffer buffer) throws IOException { int newCmd = buffer.rawByte(curPos) & 0xFF; if (cmd != newCmd) { - log.warn("encode({}) - command changed from {}[{}] to {}[{}] by pre-processor", - this, cmd, SshConstants.getCommandMessageName(cmd), - newCmd, SshConstants.getCommandMessageName(newCmd)); + log.warn("encode({}) - command changed from {}[{}] to {}[{}] by pre-processor", this, cmd, + SshConstants.getCommandMessageName(cmd), newCmd, + SshConstants.getCommandMessageName(newCmd)); cmd = newCmd; } } @@ -1399,28 +1603,26 @@ protected Buffer encode(Buffer buffer) throws IOException { // Grab the length of the packet (excluding the 5 header bytes) int len = buffer.available(); if (log.isDebugEnabled()) { - log.debug("encode({}) packet #{} sending command={}[{}] len={}", - this, seqo, cmd, SshConstants.getCommandMessageName(cmd), len); + log.debug("encode({}) packet #{} sending command={}[{}] len={}", this, seqo, cmd, + SshConstants.getCommandMessageName(cmd), len); } int off = curPos - SshConstants.SSH_PACKET_HEADER_LEN; // Debug log the packet boolean traceEnabled = log.isTraceEnabled(); if (traceEnabled) { - buffer.dumpHex(getSimplifiedLogger(), Level.FINEST, - "encode(" + this + ") packet #" + seqo, this); + buffer.dumpHex(getSimplifiedLogger(), Level.FINEST, "encode(" + this + ") packet #" + seqo, this); } // Compress the packet if needed - if ((outCompression != null) - && outCompression.isCompressionExecuted() + if ((outCompression != null) && outCompression.isCompressionExecuted() && (isAuthenticated() || (!outCompression.isDelayed()))) { int oldLen = len; outCompression.compress(buffer); len = buffer.available(); if (traceEnabled) { - log.trace("encode({}) packet #{} command={}[{}] compressed {} -> {}", - this, seqo, cmd, SshConstants.getCommandMessageName(cmd), oldLen, len); + log.trace("encode({}) packet #{} command={}[{}] compressed {} -> {}", this, seqo, cmd, + SshConstants.getCommandMessageName(cmd), oldLen, len); } } @@ -1435,8 +1637,8 @@ protected Buffer encode(Buffer buffer) throws IOException { len += Byte.BYTES + pad; if (traceEnabled) { - log.trace("encode({}) packet #{} command={}[{}] len={}, pad={}, mac={}", - this, seqo, cmd, SshConstants.getCommandMessageName(cmd), len, pad, outMac); + log.trace("encode({}) packet #{} command={}[{}] len={}, pad={}, mac={}", this, seqo, cmd, + SshConstants.getCommandMessageName(cmd), len, pad, outMac); } // Write 5 header bytes @@ -1467,7 +1669,9 @@ protected Buffer encode(Buffer buffer) throws IOException { // Update counters used to track re-keying outPacketsCount.incrementAndGet(); + totalOutgingPacketsCount.incrementAndGet(); outBytesCount.addAndGet(len); + totalOutgoingBytesCount.addAndGet(len); // Make buffer ready to be read buffer.rpos(off); @@ -1483,10 +1687,14 @@ protected void aeadOutgoingBuffer(Buffer buf, int offset, int len) throws Except if (outCipher == null || outCipher.getAuthenticationTagSize() == 0) { throw new IllegalArgumentException("AEAD mode requires an AEAD cipher"); } + byte[] data = buf.array(); outCipher.updateWithAAD(data, offset, Integer.BYTES, len); + int blocksCount = len / outCipherSize; - outBlocksCount.addAndGet(Math.max(1, blocksCount)); + int deltaCount = Math.max(1, blocksCount); + outBlocksCount.addAndGet(deltaCount); + totalOutgoingBlocksCount.addAndGet(deltaCount); } protected void appendOutgoingMac(Buffer buf, int offset, int len) throws Exception { @@ -1509,10 +1717,13 @@ protected void encryptOutgoingBuffer(Buffer buf, int offset, int len) throws Exc if (outCipher == null) { return; } + outCipher.update(buf.array(), offset, len); int blocksCount = len / outCipherSize; - outBlocksCount.addAndGet(Math.max(1, blocksCount)); + int deltaCount = Math.max(1, blocksCount); + outBlocksCount.addAndGet(deltaCount); + totalOutgoingBlocksCount.addAndGet(deltaCount); } /** @@ -1530,16 +1741,18 @@ protected void decode() throws Exception { boolean etmMode = inMac != null && inMac.isEncryptThenMac(); // Wait for beginning of packet if (decoderState == 0) { - // The read position should always be 0 at this point because we have compacted this buffer + // The read position should always be 0 at this point because we + // have compacted this buffer assert decoderBuffer.rpos() == 0; /* * Note: according to RFC-4253 section 6: * - * Implementations SHOULD decrypt the length after receiving the first 8 (or cipher block size whichever - * is larger) bytes + * Implementations SHOULD decrypt the length after receiving the + * first 8 (or cipher block size whichever is larger) bytes * - * However, we currently do not have ciphers with a block size of less than 8 we avoid un-necessary - * Math.max(minBufLen, 8) for each and every packet + * However, we currently do not have ciphers with a block size + * of less than 8 we avoid un-necessary Math.max(minBufLen, 8) + * for each and every packet */ int minBufLen = etmMode || authMode ? Integer.BYTES : inCipherSize; // If we have received enough bytes, start processing those @@ -1548,17 +1761,21 @@ protected void decode() throws Exception { // RFC 5647: packet length encoded in additional data inCipher.updateAAD(decoderBuffer.array(), 0, Integer.BYTES); } else if ((inCipher != null) && (!etmMode)) { - // Decrypt the first bytes so we can extract the packet length + // Decrypt the first bytes so we can extract the packet + // length inCipher.update(decoderBuffer.array(), 0, inCipherSize); int blocksCount = inCipherSize / inCipher.getCipherBlockSize(); - inBlocksCount.addAndGet(Math.max(1, blocksCount)); + int countDelta = Math.max(1, blocksCount); + inBlocksCount.addAndGet(countDelta); + totalIncomingBlocksCount.addAndGet(countDelta); } // Read packet length decoderLength = decoderBuffer.getInt(); /* - * Check packet length validity - we allow 8 times the minimum required packet length support in - * order to be aligned with some OpenSSH versions that allow up to 256k + * Check packet length validity - we allow 8 times the + * minimum required packet length support in order to be + * aligned with some OpenSSH versions that allow up to 256k */ if ((decoderLength < SshConstants.SSH_PACKET_HEADER_LEN) || (decoderLength > (8 * SshConstants.SSH_REQUIRED_PAYLOAD_PACKET_LENGTH_SUPPORT))) { @@ -1576,36 +1793,51 @@ protected void decode() throws Exception { } // We have received the beginning of the packet } else if (decoderState == 1) { - // The read position should always be after reading the packet length at this point + // The read position should always be after reading the packet + // length at this point assert decoderBuffer.rpos() == Integer.BYTES; // Check if the packet has been fully received if (decoderBuffer.available() >= (decoderLength + macSize + authSize)) { byte[] data = decoderBuffer.array(); if (authMode) { - inCipher.update(data, Integer.BYTES /* packet length is handled by AAD */, decoderLength); + inCipher.update(data, + Integer.BYTES /* + * packet length is handled by AAD + */, decoderLength); int blocksCount = decoderLength / inCipherSize; - inBlocksCount.addAndGet(Math.max(1, blocksCount)); + int deltaCount = Math.max(1, blocksCount); + inBlocksCount.addAndGet(deltaCount); + totalIncomingBlocksCount.addAndGet(deltaCount); } else if (etmMode) { validateIncomingMac(data, 0, decoderLength + Integer.BYTES); if (inCipher != null) { - inCipher.update(data, Integer.BYTES /* packet length is unencrypted */, decoderLength); + inCipher.update(data, + Integer.BYTES /* + * packet length is + * unencrypted + */, decoderLength); int blocksCount = decoderLength / inCipherSize; - inBlocksCount.addAndGet(Math.max(1, blocksCount)); + int deltaCount = Math.max(1, blocksCount); + inBlocksCount.addAndGet(deltaCount); + totalIncomingBlocksCount.addAndGet(deltaCount); } } else { /* - * Decrypt the remaining of the packet - skip the block we already decoded in order to extract - * the packet length + * Decrypt the remaining of the packet - skip the block + * we already decoded in order to extract the packet + * length */ if (inCipher != null) { int updateLen = decoderLength + Integer.BYTES - inCipherSize; inCipher.update(data, inCipherSize, updateLen); int blocksCount = updateLen / inCipherSize; - inBlocksCount.addAndGet(Math.max(1, blocksCount)); + int deltaCount = Math.max(1, blocksCount); + inBlocksCount.addAndGet(deltaCount); + totalIncomingBlocksCount.addAndGet(deltaCount); } validateIncomingMac(data, 0, decoderLength + Integer.BYTES); @@ -1619,8 +1851,7 @@ protected void decode() throws Exception { Buffer packet; int wpos = decoderBuffer.wpos(); // Decompress if needed - if ((inCompression != null) - && inCompression.isCompressionExecuted() + if ((inCompression != null) && inCompression.isCompressionExecuted() && (isAuthenticated() || (!inCompression.isDelayed()))) { if (uncompressBuffer == null) { uncompressBuffer = new SessionWorkBuffer(this); @@ -1637,13 +1868,16 @@ protected void decode() throws Exception { } if (log.isTraceEnabled()) { - packet.dumpHex(getSimplifiedLogger(), Level.FINEST, - "decode(" + this + ") packet #" + seqi, this); + packet.dumpHex(getSimplifiedLogger(), Level.FINEST, "decode(" + this + ") packet #" + seqi, + this); } // Update counters used to track re-keying inPacketsCount.incrementAndGet(); - inBytesCount.addAndGet(packet.available()); + totalIncomingPacketsCount.incrementAndGet(); + int availableByteCount = packet.available(); + inBytesCount.addAndGet(availableByteCount); + totalIncomingBytesCount.addAndGet(availableByteCount); // Process decoded packet handleMessage(packet); @@ -1673,7 +1907,8 @@ protected void validateIncomingMac(byte[] data, int offset, int len) throws Exce // Compute mac result inMac.doFinal(inMacResult, 0); - // Check the computed result with the received mac (just after the packet data) + // Check the computed result with the received mac (just after the + // packet data) if (!Mac.equals(inMacResult, 0, data, offset + len, inMacSize)) { throw new SshException(SshConstants.SSH2_DISCONNECT_MAC_ERROR, "MAC Error"); } @@ -1712,19 +1947,21 @@ protected byte[] sendKexInit(Map proposal) throws Exc boolean traceEnabled = log.isTraceEnabled(); if (traceEnabled) { - log.trace("sendKexInit({}) cookie={}", - this, BufferUtils.toHex(buffer.array(), p, SshConstants.MSG_KEX_COOKIE_SIZE, ':')); + log.trace("sendKexInit({}) cookie={}", this, + BufferUtils.toHex(buffer.array(), p, SshConstants.MSG_KEX_COOKIE_SIZE, ':')); } for (KexProposalOption paramType : KexProposalOption.VALUES) { String s = proposal.get(paramType); + String paramValue = adjustOutgoingKexProposalOption( + paramType, GenericUtils.trimToEmpty(s), debugEnabled); if (traceEnabled) { - log.trace("sendKexInit({})[{}] {}", this, paramType.getDescription(), s); + log.trace("sendKexInit({})[{}] {}", this, paramType.getDescription(), paramValue); } - buffer.putString(GenericUtils.trimToEmpty(s)); + buffer.putString(paramValue); } - buffer.putBoolean(false); // first kex packet follows + buffer.putBoolean(false); // first KEX packet follows buffer.putUInt(0L); // reserved (FFU) ReservedSessionMessagesHandler handler = getReservedSessionMessagesHandler(); @@ -1741,6 +1978,143 @@ protected byte[] sendKexInit(Map 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-v00@openssh.com" to its kex_algorithms + * and the server may append "kex-strict-s-v00@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... + String peerFound = Stream.of(GenericUtils.split(value, ',')) + .filter(v -> !peerValue.equals(v)) + .findFirst() + .orElse(null); + long incomingCount = totalIncomingPacketsCount.get(); + /* + * According to https://github.com/openssh/openssh-portable/blob/master/PROTOCOL + * + * 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 incoming packet don't consider it (and obviously, + * neither if we do not use strict KEX ourselves) + */ + if ((incomingCount == 1L) && GenericUtils.isNotBlank(peerFound)) { + strictKexSignalled.set(true); + } + + if (debugEnabled) { + log.debug( + "preProcessIncomingKexProposalOption({})[{}] incomingCount={}, peerValue={}, strictKexSignalled={}", + this, option, incomingCount, peerValue, strictKexSignalled); + } + + return value; + } + + protected String preProcessNegotiatedKexProposalOption( + KexProposalOption option, String value, boolean clientOption, boolean debugEnabled) { + if (GenericUtils.isBlank(value)) { + return value; // can happen for language (e.g.) + } + + if (option != KexProposalOption.ALGORITHMS) { + return value; + } + + /* + * Be strict - strip ANY strict KEX extension from the proposal regardless of + * whether it is expected or not so as not to impact the standard negotiation + */ + String strictKexName = KexExtensions.STRICT_KEX_EXTENSIONS + .stream() + .filter(value::contains) + .findAny() + .orElse(null); + if (GenericUtils.isBlank(strictKexName)) { + return value; + } + + if (debugEnabled) { + log.debug("preProcessNegotiatedKexProposalOption({})[{}] detected strict KEX={} signal in proposal={}", + this, option, strictKexName, value); + } + + String adjusted = Stream.of(GenericUtils.split(value, ',')) + .filter(v -> !KexExtensions.STRICT_KEX_EXTENSIONS.contains(v)) + .collect(Collectors.joining(",")); + if (debugEnabled) { + log.debug("preProcessNegotiatedKexProposalOption({})[{}] strict KEX={} adjusted result={}", + this, option, strictKexName, adjusted); + } + + return adjusted; + } + /** * Receive the remote key exchange init message. The packet data is returned for later use. * @@ -1764,17 +2138,19 @@ protected byte[] receiveKexInit(Buffer buffer, Map pr boolean traceEnabled = log.isTraceEnabled(); if (traceEnabled) { - log.trace("receiveKexInit({}) cookie={}", - this, BufferUtils.toHex(d, cookieStartPos, SshConstants.MSG_KEX_COOKIE_SIZE, ':')); + log.trace("receiveKexInit({}) cookie={}", this, + BufferUtils.toHex(d, cookieStartPos, SshConstants.MSG_KEX_COOKIE_SIZE, ':')); } // Read proposal + boolean debugEnabled = log.isDebugEnabled(); for (KexProposalOption paramType : KexProposalOption.VALUES) { int lastPos = buffer.rpos(); - String value = buffer.getString(); + String value = preProcessIncomingKexProposalOption(paramType, buffer.getString(), debugEnabled); if (traceEnabled) { log.trace("receiveKexInit({})[{}] {}", this, paramType.getDescription(), value); } + int curPos = buffer.rpos(); int readLen = curPos - lastPos; proposal.put(paramType, value); @@ -1868,8 +2244,8 @@ protected void prepareNewKeys() throws Exception { boolean serverSession = isServerSession(); String value = getNegotiatedKexParameter(KexProposalOption.S2CENC); - Cipher s2ccipher = ValidateUtils.checkNotNull( - NamedFactory.create(getCipherFactories(), value), "Unknown s2c cipher: %s", value); + Cipher s2ccipher = ValidateUtils.checkNotNull(NamedFactory.create(getCipherFactories(), value), + "Unknown s2c cipher: %s", value); e_s2c = resizeKey(e_s2c, s2ccipher.getKdfSize(), hash, k, h); Mac s2cmac; @@ -1892,8 +2268,8 @@ protected void prepareNewKeys() throws Exception { } value = getNegotiatedKexParameter(KexProposalOption.C2SENC); - Cipher c2scipher = ValidateUtils.checkNotNull( - NamedFactory.create(getCipherFactories(), value), "Unknown c2s cipher: %s", value); + Cipher c2scipher = ValidateUtils.checkNotNull(NamedFactory.create(getCipherFactories(), value), + "Unknown c2s cipher: %s", value); e_c2s = resizeKey(e_c2s, c2scipher.getKdfSize(), hash, k, h); Mac c2smac; @@ -1950,8 +2326,8 @@ protected void setOutputEncoding() throws Exception { firstKexPacketFollows = null; if (log.isDebugEnabled()) { - log.debug("setOutputEncoding({}): cipher {}; mac {}; compression {}; blocks limit {}", this, outCipher, outMac, - outCompression, maxRekeyBlocks); + log.debug("setOutputEncoding({}): cipher {}; mac {}; compression {}; blocks limit {}", this, outCipher, + outMac, outCompression, maxRekeyBlocks); } } @@ -2005,18 +2381,28 @@ protected long determineRekeyBlockLimit(int inCipherBlockSize, int outCipherBloc if (minCipherBlockBytes >= 16) { rekeyBlocksLimit = 1L << Math.min(minCipherBlockBytes * 2, 63); } else { - // With a block size of 8 we'd end up with 2^16. That would re-key very often. - // RFC 4344: "If L is less than 128 [...], then, although it may be too - // expensive to rekey every 2**(L/4) blocks, it is still advisable for SSH - // implementations to follow the original recommendation in [RFC4253]: rekey at + // With a block size of 8 we'd end up with 2^16. That would + // re-key very often. + // RFC 4344: "If L is less than 128 [...], then, although it may + // be too + // expensive to rekey every 2**(L/4) blocks, it is still + // advisable for SSH + // implementations to follow the original recommendation in + // [RFC4253]: rekey at // least once for every gigabyte of transmitted data." // - // Note that chacha20-poly1305 has a block size of 8. The OpenSSH recommendation - // is: "ChaCha20 must never reuse a {key, nonce} for encryption nor may it be - // used to encrypt more than 2^70 bytes under the same {key, nonce}. The - // SSH Transport protocol (RFC4253) recommends a far more conservative - // rekeying every 1GB of data sent or received. If this recommendation - // is followed, then chacha20-poly1305@openssh.com requires no special + // Note that chacha20-poly1305 has a block size of 8. The + // OpenSSH recommendation + // is: "ChaCha20 must never reuse a {key, nonce} for encryption + // nor may it be + // used to encrypt more than 2^70 bytes under the same {key, + // nonce}. The + // SSH Transport protocol (RFC4253) recommends a far more + // conservative + // rekeying every 1GB of data sent or received. If this + // recommendation + // is followed, then chacha20-poly1305@openssh.com requires no + // special // handling in this area." rekeyBlocksLimit = (1L << 30) / minCipherBlockBytes; // 1GB } @@ -2064,14 +2450,17 @@ protected Map negotiate() throws Exception { SessionDisconnectHandler discHandler = getSessionDisconnectHandler(); KexExtensionHandler extHandler = getKexExtensionHandler(); for (KexProposalOption paramType : KexProposalOption.VALUES) { - String clientParamValue = c2sOptions.get(paramType); - String serverParamValue = s2cOptions.get(paramType); + String clientParamValue = preProcessNegotiatedKexProposalOption( + paramType, c2sOptions.get(paramType), true, debugEnabled); String[] c = GenericUtils.split(clientParamValue, ','); + String serverParamValue = preProcessNegotiatedKexProposalOption( + paramType, s2cOptions.get(paramType), false, debugEnabled); String[] s = GenericUtils.split(serverParamValue, ','); /* * According to https://tools.ietf.org/html/rfc8308#section-2.2: * - * Implementations MAY disconnect if the counterpart sends an incorrect (KEX extension) indicator + * Implementations MAY disconnect if the counterpart sends an + * incorrect (KEX extension) indicator * * TODO - for now we do not enforce this */ @@ -2092,35 +2481,35 @@ protected Map negotiate() throws Exception { // check if reached an agreement String value = guess.get(paramType); if (extHandler != null) { - extHandler.handleKexExtensionNegotiation( - this, paramType, value, c2sOptions, clientParamValue, s2cOptions, serverParamValue); + extHandler.handleKexExtensionNegotiation(this, paramType, value, c2sOptions, clientParamValue, + s2cOptions, serverParamValue); } if (value != null) { if (traceEnabled) { - log.trace("negotiate({})[{}] guess={} (client={} / server={})", - this, paramType.getDescription(), value, clientParamValue, serverParamValue); + log.trace("negotiate({})[{}] guess={} (client={} / server={})", this, + paramType.getDescription(), value, clientParamValue, serverParamValue); } continue; } try { - if ((discHandler != null) - && discHandler.handleKexDisconnectReason( - this, c2sOptions, s2cOptions, negotiatedGuess, paramType)) { + if ((discHandler != null) && discHandler.handleKexDisconnectReason(this, c2sOptions, s2cOptions, + negotiatedGuess, paramType)) { if (debugEnabled) { log.debug("negotiate({}) ignore missing value for KEX option={}", this, paramType); } continue; } } catch (IOException | RuntimeException e) { - // If disconnect handler throws an exception continue with the disconnect + // If disconnect handler throws an exception continue with + // the disconnect debug("negotiate({}) failed ({}) to invoke disconnect handler due to mismatched KEX option={}: {}", this, e.getClass().getSimpleName(), paramType, e.getMessage(), e); } - String message = "Unable to negotiate key exchange for " + paramType.getDescription() - + " (client: " + clientParamValue + " / server: " + serverParamValue + ")"; + String message = "Unable to negotiate key exchange for " + paramType.getDescription() + " (client: " + + clientParamValue + " / server: " + serverParamValue + ")"; // OK if could not negotiate languages if (KexProposalOption.S2CLANG.equals(paramType) || KexProposalOption.C2SLANG.equals(paramType)) { if (traceEnabled) { @@ -2134,14 +2523,13 @@ protected Map negotiate() throws Exception { /* * According to https://tools.ietf.org/html/rfc8308#section-2.2: * - * If "ext-info-c" or "ext-info-s" ends up being negotiated as a key exchange method, the parties MUST - * disconnect. + * If "ext-info-c" or "ext-info-s" ends up being negotiated as a key + * exchange method, the parties MUST disconnect. */ String kexOption = guess.get(KexProposalOption.ALGORITHMS); if (KexExtensions.IS_KEX_EXTENSION_SIGNAL.test(kexOption)) { - if ((discHandler != null) - && discHandler.handleKexDisconnectReason( - this, c2sOptions, s2cOptions, negotiatedGuess, KexProposalOption.ALGORITHMS)) { + if ((discHandler != null) && discHandler.handleKexDisconnectReason(this, c2sOptions, s2cOptions, + negotiatedGuess, KexProposalOption.ALGORITHMS)) { if (debugEnabled) { log.debug("negotiate({}) ignore violating {} KEX option={}", this, KexProposalOption.ALGORITHMS, kexOption); @@ -2315,7 +2703,8 @@ public void addPortForwardingEventListener(PortForwardingEventListener listener) PortForwardingEventListener.validateListener(listener); // avoid race conditions on notifications while session is being closed if (!isOpen()) { - log.warn("addPortForwardingEventListener({})[{}] ignore registration while session is closing", this, listener); + log.warn("addPortForwardingEventListener({})[{}] ignore registration while session is closing", this, + listener); return; } @@ -2353,19 +2742,18 @@ public KeyExchangeFuture reExchangeKeys() throws IOException { try { requestNewKeysExchange(); } catch (GeneralSecurityException e) { - debug("reExchangeKeys({}) failed ({}) to request new keys: {}", - this, e.getClass().getSimpleName(), e.getMessage(), e); - throw ValidateUtils.initializeExceptionCause( - new ProtocolException("Failed (" + e.getClass().getSimpleName() + ")" - + " to generate keys for exchange: " + e.getMessage()), + debug("reExchangeKeys({}) failed ({}) to request new keys: {}", this, e.getClass().getSimpleName(), + e.getMessage(), e); + throw ValidateUtils.initializeExceptionCause(new ProtocolException("Failed (" + e.getClass().getSimpleName() + + ")" + " to generate keys for exchange: " + + e.getMessage()), e); } catch (Exception e) { ExceptionUtils.rethrowAsIoException(e); - return null; // actually dead code + return null; // actually dead code } - return ValidateUtils.checkNotNull( - kexFutureHolder.get(), "No current KEX future on state=%s", kexState); + return ValidateUtils.checkNotNull(kexFutureHolder.get(), "No current KEX future on state=%s", kexState); } /** @@ -2410,8 +2798,10 @@ protected KeyExchangeFuture requestNewKeysExchange() throws Exception { DefaultKeyExchangeFuture newFuture = new DefaultKeyExchangeFuture(toString(), null); DefaultKeyExchangeFuture kexFuture = kexFutureHolder.getAndSet(newFuture); if (kexFuture != null) { - // Should actually never do anything. We don't reset the kexFuture at the end of KEX, and we do check for a - // running KEX above. The old future should in all cases be fulfilled already. + // Should actually never do anything. We don't reset the kexFuture + // at the end of KEX, and we do check for a + // running KEX above. The old future should in all cases be + // fulfilled already. kexFuture.setValue(new SshException("New KEX started while previous one still ongoing")); } @@ -2429,9 +2819,7 @@ protected boolean isRekeyRequired() { return false; } - return isRekeyTimeIntervalExceeded() - || isRekeyPacketCountsExceeded() - || isRekeyBlocksCountExceeded() + return isRekeyTimeIntervalExceeded() || isRekeyPacketCountsExceeded() || isRekeyBlocksCountExceeded() || isRekeyDataSizeExceeded(); } @@ -2445,8 +2833,8 @@ protected boolean isRekeyTimeIntervalExceeded() { boolean rekey = rekeyDiff.compareTo(maxRekeyInterval) > 0; if (rekey) { if (log.isDebugEnabled()) { - log.debug("isRekeyTimeIntervalExceeded({}) re-keying: last={}, now={}, diff={}, max={}", - this, lastKeyTimeValue.get(), now, rekeyDiff, maxRekeyInterval); + log.debug("isRekeyTimeIntervalExceeded({}) re-keying: last={}, now={}, diff={}, max={}", this, + lastKeyTimeValue.get(), now, rekeyDiff, maxRekeyInterval); } } @@ -2458,12 +2846,11 @@ protected boolean isRekeyPacketCountsExceeded() { return false; // disabled } - boolean rekey = (inPacketsCount.get() > maxRekyPackets) - || (outPacketsCount.get() > maxRekyPackets); + boolean rekey = (inPacketsCount.get() > maxRekyPackets) || (outPacketsCount.get() > maxRekyPackets); if (rekey) { if (log.isDebugEnabled()) { - log.debug("isRekeyPacketCountsExceeded({}) re-keying: in={}, out={}, max={}", - this, inPacketsCount, outPacketsCount, maxRekyPackets); + log.debug("isRekeyPacketCountsExceeded({}) re-keying: in={}, out={}, max={}", this, inPacketsCount, + outPacketsCount, maxRekyPackets); } } @@ -2478,8 +2865,8 @@ protected boolean isRekeyDataSizeExceeded() { boolean rekey = (inBytesCount.get() > maxRekeyBytes) || (outBytesCount.get() > maxRekeyBytes); if (rekey) { if (log.isDebugEnabled()) { - log.debug("isRekeyDataSizeExceeded({}) re-keying: in={}, out={}, max={}", - this, inBytesCount, outBytesCount, maxRekeyBytes); + log.debug("isRekeyDataSizeExceeded({}) re-keying: in={}, out={}, max={}", this, inBytesCount, + outBytesCount, maxRekeyBytes); } } @@ -2495,8 +2882,8 @@ protected boolean isRekeyBlocksCountExceeded() { boolean rekey = (inBlocksCount.get() > maxBlocks) || (outBlocksCount.get() > maxBlocks); if (rekey) { if (log.isDebugEnabled()) { - log.debug("isRekeyBlocksCountExceeded({}) re-keying: in={}, out={}, max={}", - this, inBlocksCount, outBlocksCount, maxBlocks); + log.debug("isRekeyBlocksCountExceeded({}) re-keying: in={}, out={}, max={}", this, inBlocksCount, + outBlocksCount, maxBlocks); } } @@ -2595,16 +2982,13 @@ protected byte[] receiveKexInit(Buffer buffer) throws Exception { } if (log.isTraceEnabled()) { - log.trace("receiveKexInit({}) proposal={} seed: {}", - this, proposal, BufferUtils.toHex(':', seed)); + log.trace("receiveKexInit({}) proposal={} seed: {}", this, proposal, BufferUtils.toHex(':', seed)); } return seed; } - protected abstract void receiveKexInit( - Map proposal, byte[] seed) - throws IOException; + protected abstract void receiveKexInit(Map proposal, byte[] seed) throws IOException; /** * Retrieve the SSH session from the I/O session. If the session has not been attached, an exception will be thrown @@ -2614,8 +2998,7 @@ protected abstract void receiveKexInit( * @see #getSession(IoSession, boolean) * @throws MissingAttachedSessionException if no attached SSH session */ - public static AbstractSession getSession(IoSession ioSession) - throws MissingAttachedSessionException { + public static AbstractSession getSession(IoSession ioSession) throws MissingAttachedSessionException { return getSession(ioSession, false); } @@ -2662,19 +3045,20 @@ public static AbstractSession getSession(IoSession ioSession, boolean allowNull) */ protected static class MessageCodingSettings { - private final Cipher cipher; + protected final Cipher cipher; - private final Mac mac; + protected final Mac mac; - private final Compression compression; + protected final Compression compression; - private final Cipher.Mode mode; + protected final Cipher.Mode mode; - private byte[] key; + protected byte[] key; - private byte[] iv; + protected byte[] iv; - public MessageCodingSettings(Cipher cipher, Mac mac, Compression compression, Cipher.Mode mode, byte[] key, byte[] iv) { + public MessageCodingSettings(Cipher cipher, Mac mac, Compression compression, Cipher.Mode mode, byte[] key, + byte[] iv) { this.cipher = cipher; this.mac = mac; this.compression = compression; @@ -2683,7 +3067,7 @@ public MessageCodingSettings(Cipher cipher, Mac mac, Compression compression, Ci this.iv = iv.clone(); } - private void initCipher(long packetSequenceNumber) throws Exception { + protected void initCipher(long packetSequenceNumber) throws Exception { if (key != null) { if (cipher.getAlgorithm().startsWith("ChaCha")) { BufferUtils.putLong(packetSequenceNumber, iv, 0, iv.length); diff --git a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java index 267e9bed4..a5b45a722 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/session/helpers/SessionHelper.java @@ -127,6 +127,7 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements */ protected SessionHelper(boolean serverSession, FactoryManager factoryManager, IoSession ioSession) { super(Objects.requireNonNull(factoryManager, "No factory manager provided")); + this.serverSession = serverSession; this.ioSession = Objects.requireNonNull(ioSession, "No IoSession provided"); } diff --git a/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java b/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java index a46ed0081..b276b42ff 100644 --- a/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java +++ b/sshd-core/src/main/java/org/apache/sshd/core/CoreModuleProperties.java @@ -147,6 +147,13 @@ public final class CoreModuleProperties { public static final Property KEX_PROPOSAL_SETUP_TIMEOUT = Property.durationSec("kex-proposal-setup-timeout", Duration.ofSeconds(42), Duration.ofSeconds(5)); + /** + * @see OpenSSH PROTOCOL - 1.9 transport: + * strict key exchange extension + */ + public static final Property USE_STRICT_KEX + = Property.bool("use-strict-kex", false); + /** * Key used to set the heartbeat interval in milliseconds (0 to disable = default) */