Add support for Encrypt-then-MAC MAC Algorithms (#450)

README.adoc

@@ -83,6 +83,7 @@ signatures::
 
 mac::
   `hmac-md5`, `hmac-md5-96`, `hmac-sha1`, `hmac-sha1-96`, `hmac-sha2-256`, `hmac-sha2-512`, `hmac-ripemd160`, `hmac-ripemd160@openssh.com`
+  `hmac-md5-etm@openssh.com`, `hmac-md5-96-etm@openssh.com`, `hmac-sha1-etm@openssh.com`, `hmac-sha1-96-etm@openssh.com`, `hmac-sha2-256-etm@openssh.com`, `hmac-sha2-512-etm@openssh.com`, `hmac-ripemd160-etm@openssh.com`
 
 compression::
   `zlib` and `zlib@openssh.com` (delayed zlib)

src/itest/docker-image/Dockerfile

@@ -1,4 +1,4 @@
-FROM sickp/alpine-sshd:7.5
+FROM sickp/alpine-sshd:7.5-r2
 
 ADD id_rsa.pub /home/sshj/.ssh/authorized_keys
 
@@ -6,6 +6,7 @@ ADD test-container/ssh_host_ecdsa_key     /etc/ssh/ssh_host_ecdsa_key
 ADD test-container/ssh_host_ecdsa_key.pub /etc/ssh/ssh_host_ecdsa_key.pub
 ADD test-container/sshd_config /etc/ssh/sshd_config
 
+RUN apk add --no-cache tini
 RUN \
   echo "root:smile" | chpasswd && \
   adduser -D -s /bin/ash sshj && \
@@ -15,3 +16,4 @@ RUN \
   chmod 644 /etc/ssh/ssh_host_ecdsa_key.pub && \
   chown -R sshj:sshj /home/sshj
 
+ENTRYPOINT ["/sbin/tini", "/entrypoint.sh"]

src/itest/groovy/com/hierynomus/sshj/transport/mac/MacSpec.groovy

@@ -19,6 +19,7 @@ import com.hierynomus.sshj.IntegrationBaseSpec
 import net.schmizz.sshj.DefaultConfig
 import net.schmizz.sshj.transport.mac.HMACRIPEMD160
 import net.schmizz.sshj.transport.mac.HMACSHA2256
+import spock.lang.AutoCleanup
 import spock.lang.Unroll
 
 class MacSpec extends IntegrationBaseSpec {
@@ -36,8 +37,32 @@ class MacSpec extends IntegrationBaseSpec {
         then:
         client.authenticated
 
+        cleanup:
+        client.disconnect()
+
         where:
         macFactory << [Macs.HMACRIPEMD160(), Macs.HMACRIPEMD160OpenSsh(), Macs.HMACSHA2256(), Macs.HMACSHA2512()]
         mac = macFactory.name
     }
+
+    @Unroll
+    def "should correctly connect with Encrypt-Then-Mac #mac MAC"() {
+        given:
+        def cfg = new DefaultConfig()
+        cfg.setMACFactories(macFactory)
+        def client = getConnectedClient(cfg)
+
+        when:
+        client.authPublickey(USERNAME, KEYFILE)
+
+        then:
+        client.authenticated
+
+        cleanup:
+        client.disconnect()
+
+        where:
+        macFactory << [Macs.HMACRIPEMD160Etm(), Macs.HMACSHA2256Etm(), Macs.HMACSHA2512Etm()]
+        mac = macFactory.name
+    }
 }

src/main/java/com/hierynomus/sshj/transport/mac/Macs.java

@@ -16,48 +16,73 @@
 package com.hierynomus.sshj.transport.mac;
 
 import net.schmizz.sshj.transport.mac.BaseMAC;
+import net.schmizz.sshj.transport.mac.MAC;
 
+@SuppressWarnings("PMD.MethodNamingConventions")
 public class Macs {
     public static Factory HMACMD5() {
-        return new Factory("hmac-md5", "HmacMD5", 16, 16);
+        return new Factory("hmac-md5", "HmacMD5", 16, 16, false);
     }
     public static Factory HMACMD596() {
-        return new Factory("hmac-md5-96", "HmacMD5", 12, 16);
+        return new Factory("hmac-md5-96", "HmacMD5", 12, 16, false);
+    }
+    public static Factory HMACMD5Etm() {
+        return new Factory("hmac-md5-etm@openssh.com", "HmacMD5", 16, 16, true);
+    }
+    public static Factory HMACMD596Etm() {
+        return new Factory("hmac-md5-96-etm@openssh.com", "HmacMD5", 12, 16, true);
     }
     public static Factory HMACRIPEMD160() {
-        return new Factory("hmac-ripemd160", "HMACRIPEMD160", 20, 20);
+        return new Factory("hmac-ripemd160", "HMACRIPEMD160", 20, 20, false);
     }
     public static Factory HMACRIPEMD16096() {
-        return new Factory("hmac-ripemd160-96", "HMACRIPEMD160", 12, 20);
+        return new Factory("hmac-ripemd160-96", "HMACRIPEMD160", 12, 20, false);
+    }
+    public static Factory HMACRIPEMD160Etm() {
+        return new Factory("hmac-ripemd160-etm@openssh.com", "HMACRIPEMD160", 20, 20, true);
     }
     public static Factory HMACRIPEMD160OpenSsh() {
-        return new Factory("hmac-ripemd160@openssh.com", "HMACRIPEMD160", 20, 20);
+        return new Factory("hmac-ripemd160@openssh.com", "HMACRIPEMD160", 20, 20, false);
     }
     public static Factory HMACSHA1() {
-        return new Factory("hmac-sha1", "HmacSHA1", 20, 20);
+        return new Factory("hmac-sha1", "HmacSHA1", 20, 20, false);
     }
     public static Factory HMACSHA196() {
-        return new Factory("hmac-sha1-96", "HmacSHA1", 12, 20);
+        return new Factory("hmac-sha1-96", "HmacSHA1", 12, 20, false);
+    }
+    public static Factory HMACSHA1Etm() {
+        return new Factory("hmac-sha1-etm@openssh.com", "HmacSHA1", 20, 20, true);
+    }
+    public static Factory HMACSHA196Etm() {
+        return new Factory("hmac-sha1-96@openssh.com", "HmacSHA1", 12, 20, true);
     }
     public static Factory HMACSHA2256() {
-        return new Factory("hmac-sha2-256", "HmacSHA256", 32, 32);
+        return new Factory("hmac-sha2-256", "HmacSHA256", 32, 32, false);
+    }
+    public static Factory HMACSHA2256Etm() {
+        return new Factory("hmac-sha2-256-etm@openssh.com", "HmacSHA256", 32, 32, true);
     }
     public static Factory HMACSHA2512() {
-        return new Factory("hmac-sha2-512", "HmacSHA512", 64, 64);
+        return new Factory("hmac-sha2-512", "HmacSHA512", 64, 64, false);
+    }
+    public static Factory HMACSHA2512Etm() {
+        return new Factory("hmac-sha2-512-etm@openssh.com", "HmacSHA512", 64, 64, true);
     }
 
-    private static class Factory implements net.schmizz.sshj.common.Factory.Named<BaseMAC> {
+    private static class Factory implements net.schmizz.sshj.common.Factory.Named<MAC> {
 
         private String name;
         private String algorithm;
         private int bSize;
         private int defBSize;
+        private final boolean etm;
 
-        public Factory(String name, String algorithm, int bSize, int defBSize) {
+        public Factory(String name, String algorithm, int bSize, int defBSize, boolean etm) {
             this.name = name;
             this.algorithm = algorithm;
             this.bSize = bSize;
             this.defBSize = defBSize;
+            this.etm = etm;
         }
 
         @Override
@@ -67,7 +92,7 @@ public class Macs {
 
         @Override
         public BaseMAC create() {
-            return new BaseMAC(algorithm, bSize, defBSize);
+            return new BaseMAC(algorithm, bSize, defBSize, etm);
         }
     }
 }

src/main/java/net/schmizz/sshj/DefaultConfig.java

@@ -20,6 +20,7 @@ import com.hierynomus.sshj.transport.cipher.BlockCiphers;
 import com.hierynomus.sshj.transport.cipher.StreamCiphers;
 import com.hierynomus.sshj.transport.kex.DHGroups;
 import com.hierynomus.sshj.transport.kex.ExtendedDHGroups;
+import com.hierynomus.sshj.transport.mac.Macs;
 import com.hierynomus.sshj.userauth.keyprovider.OpenSSHKeyV1KeyFile;
 import net.schmizz.keepalive.KeepAliveProvider;
 import net.schmizz.sshj.common.Factory;
@@ -219,12 +220,22 @@ public class DefaultConfig
 
     protected void initMACFactories() {
         setMACFactories(
-                new HMACSHA1.Factory(),
+                Macs.HMACSHA1(),
-                new HMACSHA196.Factory(),
+                Macs.HMACSHA1Etm(),
-                new HMACMD5.Factory(),
+                Macs.HMACSHA196(),
-                new HMACMD596.Factory(),
+                Macs.HMACSHA196Etm(),
-                new HMACSHA2256.Factory(),
+                Macs.HMACMD5(),
-                new HMACSHA2512.Factory()
+                Macs.HMACMD5Etm(),
+                Macs.HMACMD596(),
+                Macs.HMACMD596Etm(),
+                Macs.HMACSHA2256(),
+                Macs.HMACSHA2256Etm(),
+                Macs.HMACSHA2512(),
+                Macs.HMACSHA2512Etm(),
+                Macs.HMACRIPEMD160(),
+                Macs.HMACRIPEMD160Etm(),
+                Macs.HMACRIPEMD16096(),
+                Macs.HMACRIPEMD160OpenSsh()
         );
     }
 

src/main/java/net/schmizz/sshj/transport/Converter.java

@@ -44,6 +44,7 @@ abstract class Converter {
     protected int cipherSize = 8;
     protected long seq = -1;
     protected boolean authed;
+    protected boolean etm;
 
     long getSequenceNumber() {
         return seq;
@@ -56,6 +57,7 @@ abstract class Converter {
         if (compression != null)
             compression.init(getCompressionType());
         this.cipherSize = cipher.getIVSize();
+        this.etm = mac.isEtm();
     }
 
     void setAuthenticated() {

src/main/java/net/schmizz/sshj/transport/Decoder.java

@@ -21,7 +21,9 @@ import net.schmizz.sshj.transport.compression.Compression;
 import net.schmizz.sshj.transport.mac.MAC;
 import org.slf4j.Logger;
 
-/** Decodes packets from the SSH binary protocol per the current algorithms. */
+/**
+ * Decodes packets from the SSH binary protocol per the current algorithms.
+ */
 final class Decoder
         extends Converter {
 
@@ -29,16 +31,26 @@ final class Decoder
 
     private final Logger log;
 
-    /** What we pass decoded packets to */
+    /**
+     * What we pass decoded packets to
+     */
     private final SSHPacketHandler packetHandler;
-    /** Buffer where as-yet undecoded data lives */
+    /**
+     * Buffer where as-yet undecoded data lives
+     */
     private final SSHPacket inputBuffer = new SSHPacket();
-    /** Used in case compression is active to store the uncompressed data */
+    /**
+     * Used in case compression is active to store the uncompressed data
+     */
     private final SSHPacket uncompressBuffer = new SSHPacket();
-    /** MAC result is stored here */
+    /**
+     * MAC result is stored here
+     */
     private byte[] macResult;
 
-    /** -1 if packet length not yet been decoded, else the packet length */
+    /**
+     * -1 if packet length not yet been decoded, else the packet length
+     */
     private int packetLength = -1;
 
     /**
@@ -60,53 +72,97 @@ final class Decoder
      */
     private int decode()
             throws SSHException {
+
+        if (etm) {
+            return decodeEtm();
+        } else {
+            return decodeMte();
+        }
+    }
+
+    /**
+     * Decode an Encrypt-Then-Mac packet.
+     */
+    private int decodeEtm() throws SSHException {
+        int bytesNeeded;
+        while (true) {
+            if (packetLength == -1) {
+                assert inputBuffer.rpos() == 0 : "buffer cleared";
+                bytesNeeded = 4 - inputBuffer.available();
+                if (bytesNeeded <= 0) {
+                    // In Encrypt-Then-Mac, the packetlength is sent unencrypted.
+                    packetLength = inputBuffer.readUInt32AsInt();
+                    checkPacketLength(packetLength);
+                } else {
+                    // Needs more data
+                    break;
+                }
+            } else {
+                assert inputBuffer.rpos() == 4 : "packet length read";
+                bytesNeeded = packetLength + mac.getBlockSize() - inputBuffer.available();
+                if (bytesNeeded <= 0) {
+                    seq = seq + 1 & 0xffffffffL;
+                    checkMAC(inputBuffer.array());
+                    decryptBuffer(4, packetLength);
+                    inputBuffer.wpos(packetLength + 4 - inputBuffer.readByte());
+                    final SSHPacket plain = usingCompression() ? decompressed() : inputBuffer;
+                    if (log.isTraceEnabled()) {
+                        log.trace("Received packet #{}: {}", seq, plain.printHex());
+                    }
+                    packetHandler.handle(plain.readMessageID(), plain); // Process the decoded packet
+                    inputBuffer.clear();
+                    packetLength = -1;
+                } else {
+                    // Needs more data
+                    break;
+                }
+            }
+        }
+        return bytesNeeded;
+    }
+
+    /**
+     * Decode a Mac-Then-Encrypt packet
+     * @return
+     * @throws SSHException
+     */
+    private int decodeMte() throws SSHException {
         int need;
 
         /* Decoding loop */
         for (; ; )
 
-            if (packetLength == -1) // Waiting for beginning of packet
+            if (packetLength == -1) { // Waiting for beginning of packet
-            {
-
                 assert inputBuffer.rpos() == 0 : "buffer cleared";
-
                 need = cipherSize - inputBuffer.available();
-                if (need <= 0)
+                if (need <= 0) {
                     packetLength = decryptLength();
-                else
+                } else {
                     // Need more data
                     break;
-
+                }
             } else {
-
                 assert inputBuffer.rpos() == 4 : "packet length read";
-
                 need = packetLength + (mac != null ? mac.getBlockSize() : 0) - inputBuffer.available();
                 if (need <= 0) {
-
+                    decryptBuffer(cipherSize, packetLength + 4 - cipherSize); // Decrypt the rest of the payload
-                    decryptPayload(inputBuffer.array());
-
                     seq = seq + 1 & 0xffffffffL;
-
+                    if (mac != null) {
-                    if (mac != null)
                         checkMAC(inputBuffer.array());
-
+                    }
                     // Exclude the padding & MAC
                     inputBuffer.wpos(packetLength + 4 - inputBuffer.readByte());
-
                     final SSHPacket plain = usingCompression() ? decompressed() : inputBuffer;
-
+                    if (log.isTraceEnabled()) {
-                    if (log.isTraceEnabled())
                         log.trace("Received packet #{}: {}", seq, plain.printHex());
-
+                    }
                     packetHandler.handle(plain.readMessageID(), plain); // Process the decoded packet
-
                     inputBuffer.clear();
                     packetLength = -1;
-
+                } else {
-                } else
                     // Need more data
                     break;
+                }
             }
 
         return need;
@@ -118,8 +174,9 @@ final class Decoder
         mac.update(data, 0, packetLength + 4); // packetLength+4 = entire packet w/o mac
         mac.doFinal(macResult, 0); // compute
         // Check against the received MAC
-        if (!ByteArrayUtils.equals(macResult, 0, data, packetLength + 4, mac.getBlockSize()))
+        if (!ByteArrayUtils.equals(macResult, 0, data, packetLength + 4, mac.getBlockSize())) {
             throw new TransportException(DisconnectReason.MAC_ERROR, "MAC Error");
+        }
     }
 
     private SSHPacket decompressed()
@@ -131,7 +188,7 @@ final class Decoder
 
     private int decryptLength()
             throws TransportException {
-        cipher.update(inputBuffer.array(), 0, cipherSize);
+        decryptBuffer(0, cipherSize);
 
         final int len; // Read packet length
         try {
@@ -140,22 +197,26 @@ final class Decoder
             throw new TransportException(be);
         }
 
-        if (isInvalidPacketLength(len)) { // Check packet length validity
+        checkPacketLength(len);
-            log.error("Error decoding packet (invalid length) {}", inputBuffer.printHex());
-            throw new TransportException(DisconnectReason.PROTOCOL_ERROR, "invalid packet length: " + len);
-        }
 
         return len;
     }
 
-    private static boolean isInvalidPacketLength(int len) {
+    private void decryptBuffer(int offset, int length) {
-        return len < 5 || len > MAX_PACKET_LEN;
+        cipher.update(inputBuffer.array(), offset, length);
     }
 
-    private void decryptPayload(final byte[] data) {
+    private void checkPacketLength(int len) throws TransportException {
-        cipher.update(data, cipherSize, packetLength + 4 - cipherSize);
+        if (len < 5 || len > MAX_PACKET_LEN) { // Check packet length validity
+            log.error("Error decoding packet (invalid length) {}", inputBuffer.printHex());
+            throw new TransportException(DisconnectReason.PROTOCOL_ERROR, "invalid packet length: " + len);
+        }
     }
 
+//    private void decryptPayload(final byte[] data, int offset, int length) {
+//        cipher.update(data, cipherSize, packetLength + 4 - cipherSize);
+//    }
+
     /**
      * Adds {@code len} bytes from {@code b} to the decoder buffer. When a packet has been successfully decoded, hooks
      * in to {@link SSHPacketHandler#handle} of the {@link SSHPacketHandler} this decoder was initialized with.

src/main/java/net/schmizz/sshj/transport/Encoder.java

@@ -67,36 +67,58 @@ final class Encoder
                 log.trace("Encoding packet #{}: {}", seq + 1, buffer.printHex());
             }
 
-            if (usingCompression())
+            if (usingCompression()) {
                 compress(buffer);
+            }
 
             final int payloadSize = buffer.available();
+            int lengthWithoutPadding;
+            if (etm) {
+                // in Encrypt-Then-Mac mode, the length field is not encrypted, so we should keep it out of the
+                // padding length calculation
+                lengthWithoutPadding = 1 + payloadSize; // padLength (1 byte) + payload
+            } else {
+                lengthWithoutPadding = 4 + 1 + payloadSize; // packetLength (4 bytes) + padLength (1 byte) + payload
+            }
 
             // Compute padding length
-            int padLen = -(payloadSize + 5) & cipherSize - 1;
+            int padLen = cipherSize - (lengthWithoutPadding % cipherSize);
-            if (padLen < cipherSize)
+            if (padLen < 4) {
                 padLen += cipherSize;
+            }
 
             final int startOfPacket = buffer.rpos() - 5;
-            final int packetLen = payloadSize + 1 + padLen;
+            int packetLen = 1 + payloadSize + padLen; // packetLength = padLen (1 byte) + payload + padding
+
+            if (packetLen < 16) {
+                padLen += cipherSize;
+                packetLen = 1 + payloadSize + padLen;
+            }
+
+            final int endOfPadding = startOfPacket + 4 + packetLen;
 
             // Put packet header
             buffer.wpos(startOfPacket);
             buffer.putUInt32(packetLen);
             buffer.putByte((byte) padLen);
-
             // Now wpos will mark end of padding
-            buffer.wpos(startOfPacket + 5 + payloadSize + padLen);
+            buffer.wpos(endOfPadding);
+
             // Fill padding
-            prng.fill(buffer.array(), buffer.wpos() - padLen, padLen);
+            prng.fill(buffer.array(), endOfPadding - padLen, padLen);
 
             seq = seq + 1 & 0xffffffffL;
 
-            if (mac != null)
+            if (etm) {
-                putMAC(buffer, startOfPacket, buffer.wpos());
+                cipher.update(buffer.array(), startOfPacket + 4, packetLen);
-
+                putMAC(buffer, startOfPacket, endOfPadding);
-            cipher.update(buffer.array(), startOfPacket, 4 + packetLen);
+            } else {
+                if (mac != null) {
+                    putMAC(buffer, startOfPacket, endOfPadding);
+                }
 
+                cipher.update(buffer.array(), startOfPacket, 4 + packetLen);
+            }
             buffer.rpos(startOfPacket); // Make ready-to-read
 
             return seq;

src/main/java/net/schmizz/sshj/transport/mac/BaseMAC.java

@@ -30,12 +30,18 @@ public class BaseMAC
     private final int defbsize;
     private final int bsize;
     private final byte[] tmp;
+    private final boolean etm;
     private javax.crypto.Mac mac;
 
     public BaseMAC(String algorithm, int bsize, int defbsize) {
+        this(algorithm, bsize, defbsize, false);
+    }
+
+    public BaseMAC(String algorithm, int bsize, int defbsize, boolean isEtm) {
         this.algorithm = algorithm;
         this.bsize = bsize;
         this.defbsize = defbsize;
+        this.etm = isEtm;
         tmp = new byte[defbsize];
     }
 
@@ -112,4 +118,8 @@ public class BaseMAC
         update(tmp, 0, 4);
     }
 
+    @Override
+    public boolean isEtm() {
+        return etm;
+    }
 }

src/main/java/net/schmizz/sshj/transport/mac/MAC.java

@@ -15,7 +15,9 @@
  */
 package net.schmizz.sshj.transport.mac;
 
-/** Message Authentication Code for use in SSH. It usually wraps a javax.crypto.Mac class. */
+/**
+ * Message Authentication Code for use in SSH. It usually wraps a javax.crypto.Mac class.
+ */
 public interface MAC {
 
     byte[] doFinal();
@@ -33,4 +35,40 @@ public interface MAC {
     void update(byte[] foo, int start, int len);
 
     void update(long foo);
+
+    /**
+     * Indicates that an Encrypt-Then-Mac algorithm was selected.
+     * <p>
+     * This has the following implementation details.
+     * 1.5 transport: Protocol 2 Encrypt-then-MAC MAC algorithms
+     * <p>
+     * OpenSSH supports MAC algorithms, whose names contain "-etm", that
+     * perform the calculations in a different order to that defined in RFC
+     * 4253. These variants use the so-called "encrypt then MAC" ordering,
+     * calculating the MAC over the packet ciphertext rather than the
+     * plaintext. This ordering closes a security flaw in the SSH transport
+     * protocol, where decryption of unauthenticated ciphertext provided a
+     * "decryption oracle" that could, in conjunction with cipher flaws, reveal
+     * session plaintext.
+     * <p>
+     * Specifically, the "-etm" MAC algorithms modify the transport protocol
+     * to calculate the MAC over the packet ciphertext and to send the packet
+     * length unencrypted. This is necessary for the transport to obtain the
+     * length of the packet and location of the MAC tag so that it may be
+     * verified without decrypting unauthenticated data.
+     * <p>
+     * As such, the MAC covers:
+     * <p>
+     * mac = MAC(key, sequence_number || packet_length || encrypted_packet)
+     * <p>
+     * where "packet_length" is encoded as a uint32 and "encrypted_packet"
+     * contains:
+     * <p>
+     * byte      padding_length
+     * byte[n1]  payload; n1 = packet_length - padding_length - 1
+     * byte[n2] random padding; n2 = padding_length
+     *
+     * @return Whether the MAC algorithm is an Encrypt-Then-Mac algorithm
+     */
+    boolean isEtm();
 }

src/test/java/net/schmizz/sshj/transport/mac/BaseMacTest.java

@@ -23,6 +23,7 @@ import java.nio.charset.Charset;
 
 import static org.hamcrest.CoreMatchers.is;
 import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
 
 public class BaseMacTest {
     private static final Charset CHARSET = Charset.forName("US-ASCII");
@@ -40,11 +41,9 @@ public class BaseMacTest {
 
     @Test(expected = SSHRuntimeException.class)
     public void testUnknownAlgorithm() {
-        // hopefully a algorithm with that name won't be created :-)
+        BaseMAC hmac = new BaseMAC("AlgorithmThatDoesNotExist", 20, 20, false);
-        BaseMAC hmac = new BaseMAC("AlgorithmThatDoesNotExist", 20, 20);
         hmac.init((KEY + "foo").getBytes(CHARSET));
-        hmac.update(PLAIN_TEXT);
+        fail("Should not initialize a non-existent MAC");
-        assertThat(Hex.toHexString(hmac.doFinal()),  is(EXPECTED_HMAC));
     }
 
     @Test