This is an automated email from the ASF dual-hosted git repository. lgoldstein pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mina-sshd.git
commit 06eeff80a7ca2fe7d6b9cfa96f5c081a6ce8c49e Author: Lyor Goldstein <lgoldst...@apache.org> AuthorDate: Sun Oct 13 11:54:08 2019 +0300 [SSHD-946] Supporting 'encrypt-then-MAC' mode --- CHANGES.md | 7 + README.md | 5 +- .../sshd/cli/client/SshClientCliSupport.java | 3 +- .../java/org/apache/sshd/common/SshConstants.java | 3 +- .../apache/sshd/common/kex/KexProposalOption.java | 17 ++ .../java/org/apache/sshd/common/mac/BaseMac.java | 18 ++- .../org/apache/sshd/common/mac/BuiltinMacs.java | 53 +++++-- .../org/apache/sshd/common/mac/MacInformation.java | 4 + .../org/apache/sshd/common/util/buffer/Buffer.java | 18 ++- .../sshd/common/util/buffer/ByteArrayBuffer.java | 5 + .../apache/sshd/util/test/JUnitTestSupport.java | 8 + .../java/org/apache/sshd/common/BaseBuilder.java | 7 +- .../org/apache/sshd/common/io/PacketWriter.java | 22 ++- .../common/session/helpers/AbstractSession.java | 174 ++++++++++++++------- .../sshd/common/session/helpers/SessionHelper.java | 3 +- .../sshd/common/compression/CompressionTest.java | 6 +- .../apache/sshd/common/mac/EncryptThenMacTest.java | 141 +++++++++++++++++ .../{MacTest.java => MacCompatibilityTest.java} | 21 ++- .../apache/sshd/server/auth/WelcomeBannerTest.java | 5 +- 19 files changed, 419 insertions(+), 101 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 0f48121..73664a9 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -37,6 +37,9 @@ the standard does not specifically specify the behavior regarding symbolic links * `Signature` methods accept a `SessionContext` argument representing the session context of their invocation (if any) +* Default MAC(s) list is set according to the [ssh_config(5)](https://www.freebsd.org/cgi/man.cgi?query=ssh_config&sektion=5) +order as **first** ones, where the supported MAC(s) that do no appear in it come last. + ## Minor code helpers * `SessionListener` supports `sessionPeerIdentificationReceived` method that is invoked once successful @@ -57,6 +60,8 @@ the message type=30 (old request). * `AbstractSignature#doInitSignature` is now provided also with the `Key` instance for which it is invoked. +* The `MacInformation` interface has an extra `isEncryptThenMac` method (default=_false_) to enable distinction of this mode. + ## Behavioral changes and enhancements * [SSHD-926](https://issues.apache.org/jira/browse/SSHD-930) - Add support for OpenSSH 'lsets...@openssh.com' SFTP protocol extension. @@ -77,6 +82,8 @@ exchange via properties. * [SSHD-945](https://issues.apache.org/jira/browse/SSHD-945) - Added sshd-contrib code that uses SHA1 with DSA regardless of its key length +* [SSHD-946](https://issues.apache.org/jira/browse/SSHD-946) - Supporting 'encrypt-then-MAC' mode. + * [SSHD-947](https://issues.apache.org/jira/browse/SSHD-947) - Added configuration allowing the user to specify whether client should wait for the server's identification before sending KEX-INIT message. diff --git a/README.md b/README.md index ac35e7b..f6d5399 100644 --- a/README.md +++ b/README.md @@ -47,8 +47,9 @@ based applications requiring SSH support. ## Implemented/available support * **Ciphers**: aes128cbc, aes128ctr, aes192cbc, aes192ctr, aes256cbc, aes256ctr, arcfour128, arcfour256, blowfishcbc, tripledescbc -* **Digests**: md5, sha1, sha224, sha384, sha512 -* **Macs**: hmacmd5, hmacmd596, hmacsha1, hmacsha196, hmacsha256, hmacsha512 +* **Digests**: md5, sha1, sha224, sha256, sha384, sha512 +* **Macs**: hmacmd5, hmacmd596, hmacsha1, hmacsha196, hmacsha256, hmacsha512, hmac-sha2-256-...@openssh.com +, hmac-sha2-512-...@openssh.com, hmac-sha1-...@openssh.com * **Key exchange**: diffie-hellman-group1-sha1, diffie-hellman-group-exchange-sha256, diffie-hellman-group14-sha1, diffie-hellman-group14-sha256 , diffie-hellman-group15-sha512, diffie-hellman-group16-sha512, diffie-hellman-group17-sha512, diffie-hellman-group18-sha512 , ecdh-sha2-nistp256, ecdh-sha2-nistp384, ecdh-sha2-nistp521 diff --git a/sshd-cli/src/main/java/org/apache/sshd/cli/client/SshClientCliSupport.java b/sshd-cli/src/main/java/org/apache/sshd/cli/client/SshClientCliSupport.java index 3d52b5f..ca1919b 100644 --- a/sshd-cli/src/main/java/org/apache/sshd/cli/client/SshClientCliSupport.java +++ b/sshd-cli/src/main/java/org/apache/sshd/cli/client/SshClientCliSupport.java @@ -681,7 +681,8 @@ public abstract class SshClientCliSupport extends CliSupport { : setupMacs(ConfigFileReaderSupport.MACS_CONFIG_PROP, argVal, null, stderr); } - public static List<NamedFactory<Mac>> setupMacs(String argName, String argVal, List<NamedFactory<Mac>> current, PrintStream stderr) { + public static List<NamedFactory<Mac>> setupMacs( + String argName, String argVal, List<NamedFactory<Mac>> current, PrintStream stderr) { if (GenericUtils.size(current) > 0) { showError(stderr, argName + " option value re-specified: " + NamedResource.getNames(current)); return null; diff --git a/sshd-common/src/main/java/org/apache/sshd/common/SshConstants.java b/sshd-common/src/main/java/org/apache/sshd/common/SshConstants.java index 7e73edf..06f7b05 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/SshConstants.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/SshConstants.java @@ -122,7 +122,8 @@ public final class SshConstants { // Some more constants public static final int SSH_EXTENDED_DATA_STDERR = 1; // see RFC4254 section 5.2 - public static final int SSH_PACKET_HEADER_LEN = 5; // 32-bit length + 8-bit pad length + // 32-bit length + 8-bit pad length + public static final int SSH_PACKET_HEADER_LEN = Integer.BYTES + Byte.BYTES; /* * See https://tools.ietf.org/html/rfc4253#section-6.1: * diff --git a/sshd-common/src/main/java/org/apache/sshd/common/kex/KexProposalOption.java b/sshd-common/src/main/java/org/apache/sshd/common/kex/KexProposalOption.java index ae20311..752b41e 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/kex/KexProposalOption.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/kex/KexProposalOption.java @@ -19,6 +19,7 @@ package org.apache.sshd.common.kex; +import java.util.Collection; import java.util.Collections; import java.util.Comparator; import java.util.EnumSet; @@ -42,6 +43,22 @@ public enum KexProposalOption { C2SLANG(Constants.PROPOSAL_LANG_CTOS, "languages (client to server)"), S2CLANG(Constants.PROPOSAL_LANG_STOC, "languages (server to client)"); + public static final Collection<KexProposalOption> CIPHER_PROPOSALS = + Collections.unmodifiableSet( + EnumSet.of(KexProposalOption.C2SENC, KexProposalOption.S2CENC)); + + public static final Collection<KexProposalOption> MAC_PROPOSALS = + Collections.unmodifiableSet( + EnumSet.of(KexProposalOption.C2SMAC, KexProposalOption.S2CMAC)); + + public static final Collection<KexProposalOption> COMPRESSION_PROPOSALS = + Collections.unmodifiableSet( + EnumSet.of(KexProposalOption.C2SCOMP, KexProposalOption.S2CCOMP)); + + public static final Collection<KexProposalOption> LANGUAGE_PROPOSALS = + Collections.unmodifiableSet( + EnumSet.of(KexProposalOption.C2SLANG, KexProposalOption.S2CLANG)); + /** * Compares values according to {@link KexProposalOption#getProposalIndex()} */ diff --git a/sshd-common/src/main/java/org/apache/sshd/common/mac/BaseMac.java b/sshd-common/src/main/java/org/apache/sshd/common/mac/BaseMac.java index e1681d4..036d46f 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/mac/BaseMac.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/mac/BaseMac.java @@ -33,32 +33,39 @@ public class BaseMac implements Mac { private final int defbsize; private final int bsize; private final byte[] tmp; + private final boolean etmMode; private javax.crypto.Mac mac; private String s; - public BaseMac(String algorithm, int bsize, int defbsize) { + public BaseMac(String algorithm, int bsize, int defbsize, boolean etmMode) { this.algorithm = algorithm; this.bsize = bsize; this.defbsize = defbsize; this.tmp = new byte[defbsize]; + this.etmMode = etmMode; } @Override - public final String getAlgorithm() { + public String getAlgorithm() { return algorithm; } @Override - public final int getBlockSize() { + public int getBlockSize() { return bsize; } @Override - public final int getDefaultBlockSize() { + public int getDefaultBlockSize() { return defbsize; } @Override + public boolean isEncryptThenMac() { + return etmMode; + } + + @Override public void init(byte[] key) throws Exception { if (key.length > defbsize) { byte[] tmp = new byte[defbsize]; @@ -102,7 +109,8 @@ public class BaseMac implements Mac { synchronized (this) { if (s == null) { s = getClass().getSimpleName() + "[" + getAlgorithm() + "] - " - + " block=" + getBlockSize() + "/" + getDefaultBlockSize() + " bytes"; + + " block=" + getBlockSize() + "/" + getDefaultBlockSize() + " bytes" + + ", encrypt-then-mac=" + isEncryptThenMac(); } } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/mac/BuiltinMacs.java b/sshd-common/src/main/java/org/apache/sshd/common/mac/BuiltinMacs.java index 3c7beaa..a393082 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/mac/BuiltinMacs.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/mac/BuiltinMacs.java @@ -46,11 +46,29 @@ public enum BuiltinMacs implements MacFactory { hmacmd5(Constants.HMAC_MD5, "HmacMD5", 16, 16), hmacmd596(Constants.HMAC_MD5_96, "HmacMD5", 12, 16), hmacsha1(Constants.HMAC_SHA1, "HmacSHA1", 20, 20), + hmacsha1etm(Constants.ETM_HMAC_SHA1, "HmacSHA1", 20, 20) { + @Override + public boolean isEncryptThenMac() { + return true; + } + }, hmacsha196(Constants.HMAC_SHA1_96, "HmacSHA1", 12, 20), /** See <A HREF="https://tools.ietf.org/html/rfc6668">RFC 6668</A> */ hmacsha256(Constants.HMAC_SHA2_256, "HmacSHA256", 32, 32), + hmacsha256etm(Constants.ETM_HMAC_SHA2_256, "HmacSHA256", 32, 32) { + @Override + public boolean isEncryptThenMac() { + return true; + } + }, /** See <A HREF="https://tools.ietf.org/html/rfc6668">RFC 6668</A> */ - hmacsha512(Constants.HMAC_SHA2_512, "HmacSHA512", 64, 64); + hmacsha512(Constants.HMAC_SHA2_512, "HmacSHA512", 64, 64), + hmacsha512etm(Constants.ETM_HMAC_SHA2_512, "HmacSHA512", 64, 64) { + @Override + public boolean isEncryptThenMac() { + return true; + } + }; public static final Set<BuiltinMacs> VALUES = Collections.unmodifiableSet(EnumSet.allOf(BuiltinMacs.class)); @@ -72,7 +90,7 @@ public enum BuiltinMacs implements MacFactory { @Override public Mac create() { - return new BaseMac(getAlgorithm(), getBlockSize(), getDefaultBlockSize()); + return new BaseMac(getAlgorithm(), getBlockSize(), getDefaultBlockSize(), isEncryptThenMac()); } @Override @@ -116,10 +134,12 @@ public enum BuiltinMacs implements MacFactory { */ public static void registerExtension(MacFactory extension) { String name = Objects.requireNonNull(extension, "No extension provided").getName(); - ValidateUtils.checkTrue(fromFactoryName(name) == null, "Extension overrides built-in: %s", name); + ValidateUtils.checkTrue( + fromFactoryName(name) == null, "Extension overrides built-in: %s", name); synchronized (EXTENSIONS) { - ValidateUtils.checkTrue(!EXTENSIONS.containsKey(name), "Extension overrides existing: %s", name); + ValidateUtils.checkTrue( + !EXTENSIONS.containsKey(name), "Extension overrides existing: %s", name); EXTENSIONS.put(name, extension); } } @@ -130,7 +150,8 @@ public enum BuiltinMacs implements MacFactory { */ public static NavigableSet<MacFactory> getRegisteredExtensions() { synchronized (EXTENSIONS) { - return GenericUtils.asSortedSet(NamedResource.BY_NAME_COMPARATOR, EXTENSIONS.values()); + return GenericUtils.asSortedSet( + NamedResource.BY_NAME_COMPARATOR, EXTENSIONS.values()); } } @@ -152,8 +173,9 @@ public enum BuiltinMacs implements MacFactory { /** * @param s The {@link Enum}'s name - ignored if {@code null}/empty - * @return The matching {@link org.apache.sshd.common.mac.BuiltinMacs} whose {@link Enum#name()} matches - * (case <U>insensitive</U>) the provided argument - {@code null} if no match + * @return The matching {@link org.apache.sshd.common.mac.BuiltinMacs} + * whose {@link Enum#name()} matches (case <U>insensitive</U>) the provided + * argument - {@code null} if no match */ public static BuiltinMacs fromString(String s) { if (GenericUtils.isEmpty(s)) { @@ -193,8 +215,7 @@ public enum BuiltinMacs implements MacFactory { } /** - * @param macs A comma-separated list of MACs' names - ignored - * if {@code null}/empty + * @param macs A comma-separated list of MACs' names - ignored if {@code null}/empty * @return A {@link ParseResult} containing the successfully parsed * factories and the unknown ones. <B>Note:</B> it is up to caller to * ensure that the lists do not contain duplicates @@ -204,7 +225,9 @@ public enum BuiltinMacs implements MacFactory { } public static ParseResult parseMacsList(String... macs) { - return parseMacsList(GenericUtils.isEmpty((Object[]) macs) ? Collections.emptyList() : Arrays.asList(macs)); + return parseMacsList(GenericUtils.isEmpty((Object[]) macs) + ? Collections.emptyList() + : Arrays.asList(macs)); } public static ParseResult parseMacsList(Collection<String> macs) { @@ -250,8 +273,10 @@ public enum BuiltinMacs implements MacFactory { } } - public static final class ParseResult extends NamedFactoriesListParseResult<Mac, MacFactory> { - public static final ParseResult EMPTY = new ParseResult(Collections.emptyList(), Collections.emptyList()); + public static final class ParseResult + extends NamedFactoriesListParseResult<Mac, MacFactory> { + public static final ParseResult EMPTY = + new ParseResult(Collections.emptyList(), Collections.emptyList()); public ParseResult(List<MacFactory> parsed, List<String> unsupported) { super(parsed, unsupported); @@ -266,6 +291,10 @@ public enum BuiltinMacs implements MacFactory { public static final String HMAC_SHA2_256 = "hmac-sha2-256"; public static final String HMAC_SHA2_512 = "hmac-sha2-512"; + public static final String ETM_HMAC_SHA1 = "hmac-sha1-...@openssh.com"; + public static final String ETM_HMAC_SHA2_256 = "hmac-sha2-256-...@openssh.com"; + public static final String ETM_HMAC_SHA2_512 = "hmac-sha2-512-...@openssh.com"; + private Constants() { throw new UnsupportedOperationException("No instance allowed"); } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/mac/MacInformation.java b/sshd-common/src/main/java/org/apache/sshd/common/mac/MacInformation.java index 07a9321..7d5f6a1 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/mac/MacInformation.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/mac/MacInformation.java @@ -37,4 +37,8 @@ public interface MacInformation extends AlgorithmNameProvider { * @return The "natural" MAC block size in bytes */ int getDefaultBlockSize(); + + default boolean isEncryptThenMac() { + return false; + } } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java b/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java index ee2901f..bb29204 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/Buffer.java @@ -114,6 +114,15 @@ public abstract class Buffer implements Readable { public abstract byte[] array(); /** + * @param pos A position in the <U>raw</U> underlying data bytes + * @return The byte at that position + */ + public byte rawByte(int pos) { + byte[] data = array(); + return data[pos]; + } + + /** * "Shift" the internal data so that reading starts from * position zero. */ @@ -221,7 +230,8 @@ public abstract class Buffer implements Readable { } public void dumpHex(SimplifiedLog logger, Level level, String prefix, PropertyResolver resolver) { - BufferUtils.dumpHex(logger, level, prefix, resolver, BufferUtils.DEFAULT_HEX_SEPARATOR, array(), rpos(), available()); + BufferUtils.dumpHex( + logger, level, prefix, resolver, BufferUtils.DEFAULT_HEX_SEPARATOR, array(), rpos(), available()); } /*====================== @@ -482,9 +492,9 @@ public abstract class Buffer implements Readable { public KeyPair getKeyPair() throws SshException { try { - final PublicKey pub; - final PrivateKey prv; - final String keyAlg = getString(); + PublicKey pub; + PrivateKey prv; + String keyAlg = getString(); if (KeyPairProvider.SSH_RSA.equals(keyAlg)) { BigInteger e = getMPInt(); BigInteger n = getMPInt(); diff --git a/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/ByteArrayBuffer.java b/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/ByteArrayBuffer.java index bb9ff8c..e0dee96 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/ByteArrayBuffer.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/util/buffer/ByteArrayBuffer.java @@ -158,6 +158,11 @@ public class ByteArrayBuffer extends Buffer { } @Override + public byte rawByte(int pos) { + return data[pos]; + } + + @Override public void compact() { int avail = available(); if (avail > 0) { diff --git a/sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java b/sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java index 9d57473..aa1e16b 100644 --- a/sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java +++ b/sshd-common/src/test/java/org/apache/sshd/util/test/JUnitTestSupport.java @@ -576,6 +576,14 @@ public abstract class JUnitTestSupport extends Assert { return false; } + /* ---------------------------------------------------------------------------- */ + + public static void outputDebugMessage(String format, Object o) { + if (OUTPUT_DEBUG_MESSAGES) { + outputDebugMessage(String.format(format, o)); + } + } + public static void outputDebugMessage(String format, Object... args) { if (OUTPUT_DEBUG_MESSAGES) { outputDebugMessage(String.format(format, args)); diff --git a/sshd-core/src/main/java/org/apache/sshd/common/BaseBuilder.java b/sshd-core/src/main/java/org/apache/sshd/common/BaseBuilder.java index 904726c..484cc88 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/BaseBuilder.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/BaseBuilder.java @@ -116,10 +116,13 @@ public class BaseBuilder<T extends AbstractFactoryManager, S extends BaseBuilder public static final List<BuiltinMacs> DEFAULT_MAC_PREFERENCE = Collections.unmodifiableList( Arrays.asList( - BuiltinMacs.hmacmd5, - BuiltinMacs.hmacsha1, + BuiltinMacs.hmacsha256etm, + BuiltinMacs.hmacsha512etm, + BuiltinMacs.hmacsha1etm, BuiltinMacs.hmacsha256, BuiltinMacs.hmacsha512, + BuiltinMacs.hmacsha1, + BuiltinMacs.hmacmd5, BuiltinMacs.hmacsha196, BuiltinMacs.hmacmd596 )); diff --git a/sshd-core/src/main/java/org/apache/sshd/common/io/PacketWriter.java b/sshd-core/src/main/java/org/apache/sshd/common/io/PacketWriter.java index 42cda19..59f66b0 100644 --- a/sshd-core/src/main/java/org/apache/sshd/common/io/PacketWriter.java +++ b/sshd-core/src/main/java/org/apache/sshd/common/io/PacketWriter.java @@ -24,8 +24,6 @@ import java.nio.channels.Channel; import org.apache.sshd.common.util.buffer.Buffer; /** - * TODO Add javadoc - * * @author <a href="mailto:d...@mina.apache.org">Apache MINA SSHD Project</a> */ public interface PacketWriter extends Channel { @@ -40,4 +38,24 @@ public interface PacketWriter extends Channel { * @throws IOException if an error occurred when encoding sending the packet */ IoWriteFuture writePacket(Buffer buffer) throws IOException; + + /** + * @param len The packet payload size + * @param blockSize The cipher block size + * @param etmMode Whether using "encrypt-then-MAC" mode + * @return The required padding length + */ + static int calculatePadLength(int len, int blockSize, boolean etmMode) { + len++; // the pad length + if (!etmMode) { + len += Integer.BYTES; + } + + int pad = (-len) & (blockSize - 1); + if (pad < blockSize) { + pad += blockSize; + } + + return pad; + } } 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 27e87a8..241162e 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 @@ -39,6 +39,7 @@ import java.util.concurrent.CopyOnWriteArraySet; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; +import java.util.logging.Level; import org.apache.sshd.common.Closeable; import org.apache.sshd.common.Factory; @@ -61,6 +62,7 @@ import org.apache.sshd.common.future.KeyExchangeFuture; import org.apache.sshd.common.future.SshFutureListener; import org.apache.sshd.common.io.IoSession; import org.apache.sshd.common.io.IoWriteFuture; +import org.apache.sshd.common.io.PacketWriter; import org.apache.sshd.common.kex.KexProposalOption; import org.apache.sshd.common.kex.KexState; import org.apache.sshd.common.kex.KeyExchange; @@ -1030,24 +1032,24 @@ public abstract class AbstractSession extends SessionHelper { } /** - * Encode a buffer into the SSH protocol. - * This method need to be called into a synchronized block around encodeLock + * Encode a buffer into the SSH protocol. <B>Note:</B> This method must be called + * inside a {@code synchronized} block using {@code encodeLock}. * * @param buffer the buffer to encode * @return The encoded buffer - may be different than original if input * buffer does not have enough room for {@link SshConstants#SSH_PACKET_HEADER_LEN}, - * in which a substitute buffer will be created and used. + * in which case a substitute buffer will be created and used. * @throws IOException if an exception occurs during the encoding process */ 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 if (curPos < SshConstants.SSH_PACKET_HEADER_LEN) { - byte[] data = buffer.array(); - int cmd = data[curPos] & 0xFF; // usually the 1st byte is an SSH opcode - log.warn("encode({}) command={} performance cost: available buffer packet header length ({}) below min. required ({})", - this, SshConstants.getCommandMessageName(cmd), curPos, SshConstants.SSH_PACKET_HEADER_LEN); + log.warn("encode({}) command={}[{}] performance cost: available buffer packet header length ({}) below min. required ({})", + this, cmd, SshConstants.getCommandMessageName(cmd), + curPos, SshConstants.SSH_PACKET_HEADER_LEN); Buffer nb = new ByteArrayBuffer(buffer.available() + Long.SIZE, false); nb.wpos(SshConstants.SSH_PACKET_HEADER_LEN); nb.putBuffer(buffer); @@ -1057,57 +1059,63 @@ public abstract class AbstractSession extends SessionHelper { // 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); + } + int off = curPos - SshConstants.SSH_PACKET_HEADER_LEN; // Debug log the packet - if (log.isTraceEnabled()) { - buffer.dumpHex(getSimplifiedLogger(), "encode(" + this + ") packet #" + seqo, this); + boolean traceEnabled = log.isTraceEnabled(); + if (traceEnabled) { + buffer.dumpHex(getSimplifiedLogger(), Level.FINEST, "encode(" + this + ") packet #" + seqo, this); } // Compress the packet if needed 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); + } } // Compute padding length - int bsize = outCipherSize; + boolean etmMode = (outMac == null) ? false : outMac.isEncryptThenMac(); + int pad = PacketWriter.calculatePadLength(len, outCipherSize, etmMode); int oldLen = len; - len += SshConstants.SSH_PACKET_HEADER_LEN; - int pad = (-len) & (bsize - 1); - if (pad < bsize) { - pad += bsize; + len = len + pad + Byte.BYTES /* the pad length byte */; + + if (traceEnabled) { + log.trace("encode({}) packet #{} command={}[{}] len={}, pad={}, mac={}", + this, seqo, cmd, SshConstants.getCommandMessageName(cmd), len, pad, outMac); } - len = len + pad - 4; + // Write 5 header bytes buffer.wpos(off); buffer.putInt(len); buffer.putByte((byte) pad); - // Fill padding + // Make sure enough room for padding and then fill it buffer.wpos(off + oldLen + SshConstants.SSH_PACKET_HEADER_LEN + pad); synchronized (random) { random.fill(buffer.array(), buffer.wpos() - pad, pad); } - // Compute mac - if (outMac != null) { - int macSize = outMac.getBlockSize(); - int l = buffer.wpos(); - buffer.wpos(l + macSize); - outMac.updateUInt(seqo); - outMac.update(buffer.array(), off, l); - outMac.doFinal(buffer.array(), l); + if (etmMode) { + // Do not encrypt the length field + encryptOutgoingBuffer(buffer, off + Integer.BYTES, len); + appendOutgoingMac(buffer, off, len); + } else { + appendOutgoingMac(buffer, off, len); + encryptOutgoingBuffer(buffer, off, len + Integer.BYTES); } - // Encrypt packet, excluding mac - if (outCipher != null) { - outCipher.update(buffer.array(), off, len + 4); - int blocksCount = (len + 4) / outCipher.getCipherBlockSize(); - outBlocksCount.addAndGet(Math.max(1, blocksCount)); - } // Increment packet id - seqo = (seqo + 1) & 0xffffffffL; + seqo = (seqo + 1L) & 0x0ffffffffL; // Update stats outPacketsCount.incrementAndGet(); outBytesCount.addAndGet(len); @@ -1121,6 +1129,33 @@ public abstract class AbstractSession extends SessionHelper { } } + protected void appendOutgoingMac(Buffer buf, int offset, int len) throws Exception { + if (outMac == null) { + return; + } + + int macSize = outMac.getBlockSize(); + int l = buf.wpos(); + // ensure enough room for MAC in outgoing buffer + buf.wpos(l + macSize); + // Include sequence number + outMac.updateUInt(seqo); + // Include the length field in the MAC calculation + outMac.update(buf.array(), offset, len + Integer.BYTES); + // Append MAC to end of packet + outMac.doFinal(buf.array(), l); + } + + protected void encryptOutgoingBuffer(Buffer buf, int offset, int len) throws Exception { + if (outCipher == null) { + return; + } + outCipher.update(buf.array(), offset, len); + + int blocksCount = len / outCipher.getCipherBlockSize(); + outBlocksCount.addAndGet(Math.max(1, blocksCount)); + } + /** * Decode the incoming buffer and handle packets as needed. * @@ -1129,14 +1164,16 @@ public abstract class AbstractSession extends SessionHelper { protected void decode() throws Exception { // Decoding loop for (;;) { + boolean etmMode = (inMac == null) ? false : 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 assert decoderBuffer.rpos() == 0; + int minBufLen = etmMode ? Integer.BYTES : inCipherSize; // If we have received enough bytes, start processing those - if (decoderBuffer.available() > inCipherSize) { + if (decoderBuffer.available() > minBufLen) { // Decrypt the first bytes - if (inCipher != null) { + if ((inCipher != null) && (!etmMode)) { inCipher.update(decoderBuffer.array(), 0, inCipherSize); int blocksCount = inCipherSize / inCipher.getCipherBlockSize(); @@ -1151,7 +1188,7 @@ public abstract class AbstractSession extends SessionHelper { if ((decoderLength < SshConstants.SSH_PACKET_HEADER_LEN) || (decoderLength > (8 * SshConstants.SSH_REQUIRED_PAYLOAD_PACKET_LENGTH_SUPPORT))) { log.warn("decode({}) Error decoding packet(invalid length): {}", this, decoderLength); - decoderBuffer.dumpHex(getSimplifiedLogger(), "decode(" + this + ") invalid length packet", this); + decoderBuffer.dumpHex(getSimplifiedLogger(), Level.FINEST, "decode(" + this + ") invalid length packet", this); throw new SshException(SshConstants.SSH2_DISCONNECT_PROTOCOL_ERROR, "Invalid packet length: " + decoderLength); } @@ -1163,35 +1200,36 @@ public abstract class AbstractSession extends SessionHelper { } // We have received the beginning of the packet } else if (decoderState == 1) { - // The read position should always be 4 at this point - assert decoderBuffer.rpos() == 4; - int macSize = inMac != null ? inMac.getBlockSize() : 0; + // The read position should always be after reading the packet length at this point + assert decoderBuffer.rpos() == Integer.BYTES; + int macSize = (inMac != null) ? inMac.getBlockSize() : 0; // Check if the packet has been fully received if (decoderBuffer.available() >= (decoderLength + macSize)) { byte[] data = decoderBuffer.array(); - // Decrypt the remaining of the packet - if (inCipher != null) { - int updateLen = decoderLength + 4 - inCipherSize; - inCipher.update(data, inCipherSize, updateLen); + if (etmMode) { + validateIncomingMac(data, 0, decoderLength + Integer.BYTES); - int blocksCount = updateLen / inCipher.getCipherBlockSize(); - inBlocksCount.addAndGet(Math.max(1, blocksCount)); - } - // Check the mac of the packet - if (inMac != null) { - // Update mac with packet id - inMac.updateUInt(seqi); - // Update mac with packet data - inMac.update(data, 0, decoderLength + 4); - // Compute mac result - inMac.doFinal(inMacResult, 0); - // Check the computed result with the received mac (just after the packet data) - if (!BufferUtils.equals(inMacResult, 0, data, decoderLength + 4, macSize)) { - throw new SshException(SshConstants.SSH2_DISCONNECT_MAC_ERROR, "MAC Error"); + if (inCipher != null) { + inCipher.update(data, Integer.BYTES, decoderLength); + + int blocksCount = decoderLength / inCipher.getCipherBlockSize(); + inBlocksCount.addAndGet(Math.max(1, blocksCount)); + } + } else { + // Decrypt the remaining of the packet + if (inCipher != null) { + int updateLen = decoderLength + Integer.BYTES - inCipherSize; + inCipher.update(data, inCipherSize, updateLen); + + int blocksCount = updateLen / inCipher.getCipherBlockSize(); + inBlocksCount.addAndGet(Math.max(1, blocksCount)); } + + validateIncomingMac(data, 0, decoderLength + Integer.BYTES); } + // Increment incoming packet sequence number - seqi = (seqi + 1) & 0xffffffffL; + seqi = (seqi + 1L) & 0x0ffffffffL; // Get padding int pad = decoderBuffer.getUByte(); Buffer packet; @@ -1210,12 +1248,12 @@ public abstract class AbstractSession extends SessionHelper { inCompression.uncompress(decoderBuffer, uncompressBuffer); packet = uncompressBuffer; } else { - decoderBuffer.wpos(decoderLength + 4 - pad); + decoderBuffer.wpos(decoderLength + Integer.BYTES - pad); packet = decoderBuffer; } if (log.isTraceEnabled()) { - packet.dumpHex(getSimplifiedLogger(), "decode(" + this + ") packet #" + seqi, this); + packet.dumpHex(getSimplifiedLogger(), Level.FINEST, "decode(" + this + ") packet #" + seqi, this); } // Update stats @@ -1224,7 +1262,7 @@ public abstract class AbstractSession extends SessionHelper { // Process decoded packet handleMessage(packet); // Set ready to handle next packet - decoderBuffer.rpos(decoderLength + 4 + macSize); + decoderBuffer.rpos(decoderLength + Integer.BYTES + macSize); decoderBuffer.wpos(wpos); decoderBuffer.compact(); decoderState = 0; @@ -1236,6 +1274,24 @@ public abstract class AbstractSession extends SessionHelper { } } + protected void validateIncomingMac(byte[] data, int offset, int len) throws Exception { + if (inMac == null) { + return; + } + + // Update mac with packet id + inMac.updateUInt(seqi); + // Update mac with packet data + inMac.update(data, offset, len); + // Compute mac result + inMac.doFinal(inMacResult, 0); + + // Check the computed result with the received mac (just after the packet data) + if (!BufferUtils.equals(inMacResult, 0, data, offset + len, inMac.getBlockSize())) { + throw new SshException(SshConstants.SSH2_DISCONNECT_MAC_ERROR, "MAC Error"); + } + } + /** * Read the other side identification. * This method is specific to the client or server side, but both should call 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 b9ed2f1..3a24647 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 @@ -921,7 +921,8 @@ public abstract class SessionHelper extends AbstractKexFactoryManager implements return proposal; } - protected void signalNegotiationStart(Map<KexProposalOption, String> c2sOptions, Map<KexProposalOption, String> s2cOptions) { + protected void signalNegotiationStart( + Map<KexProposalOption, String> c2sOptions, Map<KexProposalOption, String> s2cOptions) { try { invokeSessionSignaller(l -> { signalNegotiationStart(l, c2sOptions, s2cOptions); diff --git a/sshd-core/src/test/java/org/apache/sshd/common/compression/CompressionTest.java b/sshd-core/src/test/java/org/apache/sshd/common/compression/CompressionTest.java index a05b05f..e0fbedb 100644 --- a/sshd-core/src/test/java/org/apache/sshd/common/compression/CompressionTest.java +++ b/sshd-core/src/test/java/org/apache/sshd/common/compression/CompressionTest.java @@ -28,7 +28,7 @@ import java.util.List; import org.apache.sshd.common.channel.Channel; import org.apache.sshd.common.kex.KexProposalOption; -import org.apache.sshd.common.mac.MacTest; +import org.apache.sshd.common.mac.MacCompatibilityTest; import org.apache.sshd.common.session.Session; import org.apache.sshd.common.session.SessionListener; import org.apache.sshd.server.SshServer; @@ -97,8 +97,8 @@ public class CompressionTest extends BaseTestSupport { public static void setupClientAndServer() throws Exception { JSchLogger.init(); - sshd = CoreTestSupportUtils.setupTestServer(MacTest.class); - sshd.setKeyPairProvider(CommonTestSupportUtils.createTestHostKeyProvider(MacTest.class)); + sshd = CoreTestSupportUtils.setupTestServer(MacCompatibilityTest.class); + sshd.setKeyPairProvider(CommonTestSupportUtils.createTestHostKeyProvider(MacCompatibilityTest.class)); sshd.start(); port = sshd.getPort(); } diff --git a/sshd-core/src/test/java/org/apache/sshd/common/mac/EncryptThenMacTest.java b/sshd-core/src/test/java/org/apache/sshd/common/mac/EncryptThenMacTest.java new file mode 100644 index 0000000..794d308 --- /dev/null +++ b/sshd-core/src/test/java/org/apache/sshd/common/mac/EncryptThenMacTest.java @@ -0,0 +1,141 @@ +/* + * 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.mac; + +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.concurrent.TimeUnit; + +import org.apache.sshd.client.SshClient; +import org.apache.sshd.client.session.ClientSession; +import org.apache.sshd.common.kex.KexProposalOption; +import org.apache.sshd.server.SshServer; +import org.apache.sshd.util.test.BaseTestSupport; +import org.apache.sshd.util.test.CommonTestSupportUtils; +import org.apache.sshd.util.test.CoreTestSupportUtils; +import org.apache.sshd.util.test.JUnit4ClassRunnerWithParametersFactory; +import org.junit.AfterClass; +import org.junit.Before; +import org.junit.BeforeClass; +import org.junit.FixMethodOrder; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.MethodSorters; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +import org.junit.runners.Parameterized.UseParametersRunnerFactory; + +/** + * @author <a href="mailto:d...@mina.apache.org">Apache MINA SSHD Project</a> + */ +@FixMethodOrder(MethodSorters.NAME_ASCENDING) +@RunWith(Parameterized.class) // see https://github.com/junit-team/junit/wiki/Parameterized-tests +@UseParametersRunnerFactory(JUnit4ClassRunnerWithParametersFactory.class) +public class EncryptThenMacTest extends BaseTestSupport { + private static SshServer sshd; + private static int port; + private static SshClient client; + + private final MacFactory factory; + + public EncryptThenMacTest(MacFactory factory) { + this.factory = factory; + } + + @BeforeClass + public static void setupClientAndServer() throws Exception { + sshd = CoreTestSupportUtils.setupTestServer(EncryptThenMacTest.class); + sshd.setKeyPairProvider(CommonTestSupportUtils.createTestHostKeyProvider(EncryptThenMacTest.class)); + sshd.start(); + port = sshd.getPort(); + + client = CoreTestSupportUtils.setupTestClient(EncryptThenMacTest.class); + client.start(); + } + + @AfterClass + public static void tearDownClientAndServer() throws Exception { + if (sshd != null) { + try { + sshd.stop(true); + } finally { + sshd = null; + } + } + + if (client != null) { + try { + client.stop(); + } finally { + client = null; + } + } + } + + @Parameters(name = "{0}") + public static Collection<Object[]> parameters() { + List<Object[]> ret = new ArrayList<>(); + for (MacFactory f : BuiltinMacs.VALUES) { + if (!f.isSupported()) { + outputDebugMessage("Skip unsupported MAC %s", f); + continue; + } + + // We want only encrypt-then-mac mode + if (!f.isEncryptThenMac()) { + outputDebugMessage("Skip Mac-Then-Encrypt %s", f); + continue; + } + + ret.add(new Object[]{f}); + } + + return ret; + } + + @Before + public void setUp() throws Exception { + sshd.setMacFactories(Collections.singletonList(factory)); + client.setMacFactories(Collections.singletonList(factory)); + } + + @Test + public void testClientConnection() throws Exception { + try (ClientSession session = client.connect(getCurrentTestName(), TEST_LOCALHOST, port) + .verify(7L, TimeUnit.SECONDS) + .getSession()) { + session.addPasswordIdentity(getCurrentTestName()); + session.auth().verify(11L, TimeUnit.SECONDS); + + String expected = factory.getName(); + for (KexProposalOption opt : KexProposalOption.MAC_PROPOSALS) { + String actual = session.getNegotiatedKexParameter(opt); + assertEquals("Mismatched " + opt + " negotiation", expected, actual); + } + } + } + + @Override + public String toString() { + return getClass().getSimpleName() + "[" + factory + "]"; + } +} diff --git a/sshd-core/src/test/java/org/apache/sshd/common/mac/MacTest.java b/sshd-core/src/test/java/org/apache/sshd/common/mac/MacCompatibilityTest.java similarity index 91% rename from sshd-core/src/test/java/org/apache/sshd/common/mac/MacTest.java rename to sshd-core/src/test/java/org/apache/sshd/common/mac/MacCompatibilityTest.java index 3ea363b..9a687f2 100644 --- a/sshd-core/src/test/java/org/apache/sshd/common/mac/MacTest.java +++ b/sshd-core/src/test/java/org/apache/sshd/common/mac/MacCompatibilityTest.java @@ -55,14 +55,14 @@ import ch.ethz.ssh2.Connection; import ch.ethz.ssh2.ConnectionInfo; /** - * Test MAC algorithms. + * Test MAC algorithms with other known implementations. * * @author <a href="mailto:d...@mina.apache.org">Apache MINA SSHD Project</a> */ @FixMethodOrder(MethodSorters.NAME_ASCENDING) @RunWith(Parameterized.class) // see https://github.com/junit-team/junit/wiki/Parameterized-tests @UseParametersRunnerFactory(JUnit4ClassRunnerWithParametersFactory.class) -public class MacTest extends BaseTestSupport { +public class MacCompatibilityTest extends BaseTestSupport { private static final Collection<String> GANYMEDE_MACS = Collections.unmodifiableSet( GenericUtils.asSortedSet(String.CASE_INSENSITIVE_ORDER, Connection.getAvailableMACs())); @@ -73,7 +73,7 @@ public class MacTest extends BaseTestSupport { private final MacFactory factory; private final String jschMacClass; - public MacTest(MacFactory factory, String jschMacClass) { + public MacCompatibilityTest(MacFactory factory, String jschMacClass) { this.factory = factory; this.jschMacClass = jschMacClass; } @@ -83,7 +83,13 @@ public class MacTest extends BaseTestSupport { List<Object[]> ret = new ArrayList<>(); for (MacFactory f : BuiltinMacs.VALUES) { if (!f.isSupported()) { - System.out.println("Skip unsupported MAC " + f); + outputDebugMessage("Skip unsupported MAC %s", f); + continue; + } + + // None of the implementations we use support encrypt-then-mac mode + if (f.isEncryptThenMac()) { + outputDebugMessage("Skip Encrypt-Then-Mac %s", f); continue; } @@ -116,8 +122,8 @@ public class MacTest extends BaseTestSupport { public static void setupClientAndServer() throws Exception { JSchLogger.init(); - sshd = CoreTestSupportUtils.setupTestServer(MacTest.class); - sshd.setKeyPairProvider(CommonTestSupportUtils.createTestHostKeyProvider(MacTest.class)); + sshd = CoreTestSupportUtils.setupTestServer(MacCompatibilityTest.class); + sshd.setKeyPairProvider(CommonTestSupportUtils.createTestHostKeyProvider(MacCompatibilityTest.class)); sshd.start(); port = sshd.getPort(); } @@ -180,7 +186,8 @@ public class MacTest extends BaseTestSupport { try { conn.setClient2ServerMACs(new String[]{macName}); - ConnectionInfo info = conn.connect(null, (int) TimeUnit.SECONDS.toMillis(5L), (int) TimeUnit.SECONDS.toMillis(11L)); + ConnectionInfo info = conn.connect(null, + (int) TimeUnit.SECONDS.toMillis(5L), (int) TimeUnit.SECONDS.toMillis(11L)); outputDebugMessage("Connected: kex=%s, key-type=%s, c2senc=%s, s2cenc=%s, c2mac=%s, s2cmac=%s", info.keyExchangeAlgorithm, info.serverHostKeyAlgorithm, info.clientToServerCryptoAlgorithm, info.serverToClientCryptoAlgorithm, diff --git a/sshd-core/src/test/java/org/apache/sshd/server/auth/WelcomeBannerTest.java b/sshd-core/src/test/java/org/apache/sshd/server/auth/WelcomeBannerTest.java index 4ae73b6..f3a5400 100644 --- a/sshd-core/src/test/java/org/apache/sshd/server/auth/WelcomeBannerTest.java +++ b/sshd-core/src/test/java/org/apache/sshd/server/auth/WelcomeBannerTest.java @@ -90,8 +90,9 @@ public class WelcomeBannerTest extends BaseTestSupport { @Test public void testSimpleBanner() throws Exception { - final String expectedWelcome = "Welcome to SSHD WelcomeBannerTest"; - PropertyResolverUtils.updateProperty(sshd, ServerAuthenticationManager.WELCOME_BANNER, expectedWelcome); + String expectedWelcome = "Welcome to SSHD WelcomeBannerTest"; + PropertyResolverUtils.updateProperty( + sshd, ServerAuthenticationManager.WELCOME_BANNER, expectedWelcome); testBanner(expectedWelcome); }