From ee382b084a0f4dc0dbeca21079fce3bfa67ccda1 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Thu, 1 Feb 2024 20:44:08 +0100 Subject: [PATCH] GH-455: ensure BaseCipher.update() fulfills the contract The org.apache.sshd.common.cipher.Cipher interface specifies for update(byte[] buffer, int offset, int length) that length bytes are encrypted or decrypted in-place in the given buffer, starting at the given offset. The BaseCipher implementation just called javax.crypto.Cipher.update(). That, however, may buffer blocks and not update all data right away. (For instance, AES pipelined implementations may behave that way.) Buffered blocks may be returned/updated in subsequent update() calls. To ensure that really all bytes given are updated, one needs to call doFinal(), which always returns/updates such buffered blocks. But javax.crypto.Cipher.doFinal() resets the cipher to its initial state. For use in SSH, this is not appropriate: the cipher must be reset not to the initial state but to the final state. This is done for CTR ciphers by adding the number of processed blocks to the initial IV and then using that IV for re-initialization. For CBC ciphers, the re-initialization IV must be the last encrypted block processed. Note that in CTR mode, we cannot check for IV re-use. This is not a problem in practice because in the SSH protocol key exchanges happen long before an IV can wrap around. --- CHANGES.md | 2 + .../sshd/common/cipher/BaseCBCCipher.java | 55 ++++++ .../sshd/common/cipher/BaseCTRCipher.java | 78 ++++++++ .../apache/sshd/common/cipher/BaseCipher.java | 57 +++++- .../sshd/common/cipher/BaseRC4Cipher.java | 5 + .../sshd/common/cipher/BuiltinCiphers.java | 64 ++++++- .../org/apache/sshd/common/cipher/Cipher.java | 14 +- .../common/cipher/BaseCipherResetTest.java | 177 ++++++++++++++++++ .../sshd/common/cipher/BaseCipherTest.java | 8 + .../sshd/common/cipher/OpenSshCipherTest.java | 156 +++++++++++++++ sshd-mina/pom.xml | 1 + sshd-netty/pom.xml | 1 + 12 files changed, 605 insertions(+), 13 deletions(-) create mode 100644 sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCBCCipher.java create mode 100644 sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCTRCipher.java create mode 100644 sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherResetTest.java create mode 100644 sshd-core/src/test/java/org/apache/sshd/common/cipher/OpenSshCipherTest.java diff --git a/CHANGES.md b/CHANGES.md index 4721dcc23..207d06b6b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -32,6 +32,8 @@ ## Bug Fixes +* [GH-455](https://github.com/apache/mina-sshd/issues/455) Fix `BaseCipher`: make sure all bytes are processed + ## New Features ## Behavioral changes and enhancements diff --git a/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCBCCipher.java b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCBCCipher.java new file mode 100644 index 000000000..cf593f328 --- /dev/null +++ b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCBCCipher.java @@ -0,0 +1,55 @@ +/* + * 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.cipher; + +import java.security.spec.AlgorithmParameterSpec; +import java.util.Arrays; + +import javax.crypto.spec.IvParameterSpec; + +public class BaseCBCCipher extends BaseCipher { + + private byte[] lastEncryptedBlock; + + public BaseCBCCipher(int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation, + int blkSize) { + super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize); + } + + @Override + public void update(byte[] input, int inputOffset, int inputLen) throws Exception { + if (mode == Mode.Decrypt) { + lastEncryptedBlock = Arrays.copyOfRange(input, inputOffset + inputLen - getCipherBlockSize(), + inputOffset + inputLen); + } + super.update(input, inputOffset, inputLen); + } + + @Override + protected AlgorithmParameterSpec determineNewParameters(byte[] processed, int offset, int length) { + // The IV is the last encrypted block + if (mode == Mode.Decrypt) { + byte[] result = lastEncryptedBlock; + lastEncryptedBlock = null; + return new IvParameterSpec(result); + } + return new IvParameterSpec(Arrays.copyOfRange(processed, offset + length - getCipherBlockSize(), offset + length)); + } +} diff --git a/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCTRCipher.java b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCTRCipher.java new file mode 100644 index 000000000..754bf7cdd --- /dev/null +++ b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCTRCipher.java @@ -0,0 +1,78 @@ +/* + * 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.cipher; + +import java.security.InvalidAlgorithmParameterException; +import java.security.InvalidKeyException; +import java.security.spec.AlgorithmParameterSpec; + +import javax.crypto.spec.IvParameterSpec; + +import org.apache.sshd.common.util.buffer.ByteArrayBuffer; + +public class BaseCTRCipher extends BaseCipher { + + private long blocksProcessed; + + public BaseCTRCipher(int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation, + int blkSize) { + super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize); + } + + @Override + public void update(byte[] input, int inputOffset, int inputLen) throws Exception { + blocksProcessed += inputLen / getCipherBlockSize(); + super.update(input, inputOffset, inputLen); + } + + @Override + protected void reInit(byte[] processed, int offset, int length) + throws InvalidKeyException, InvalidAlgorithmParameterException { + super.reInit(processed, offset, length); + blocksProcessed = 0; + } + + @Override + protected AlgorithmParameterSpec determineNewParameters(byte[] processed, int offset, int length) { + byte[] iv = getCipherInstance().getIV().clone(); + // Treat the IV as a counter and add blocksProcessed + ByteArrayBuffer buf = new ByteArrayBuffer(iv, iv.length - Long.BYTES, Long.BYTES); + long unsigned = buf.getLong(); + long highBitBefore = unsigned & ~Long.MAX_VALUE; + unsigned &= Long.MAX_VALUE; // Clear most significant bit + unsigned += blocksProcessed; + long highBitNow = unsigned & ~Long.MAX_VALUE; + unsigned = (unsigned & Long.MAX_VALUE) | (highBitBefore ^ highBitNow); + int carry = (int) ((highBitBefore & highBitNow) >>> (Long.SIZE - 1)); + addCarry(iv, iv.length - Long.BYTES, carry); + buf.wpos(iv.length - Long.BYTES); + buf.putLong(unsigned); + return new IvParameterSpec(iv); + } + + private void addCarry(byte[] iv, int length, int carry) { + int add = carry; + for (int i = length - 1; i >= 0; i--) { + int b = (iv[i] & 0xFF) + add; + iv[i] = (byte) b; + add = b >> Byte.SIZE; + } + } +} diff --git a/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCipher.java b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCipher.java index aac2a5ce3..d6e9df147 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCipher.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCipher.java @@ -18,6 +18,12 @@ */ package org.apache.sshd.common.cipher; +import java.security.GeneralSecurityException; +import java.security.InvalidAlgorithmParameterException; +import java.security.InvalidKeyException; +import java.security.spec.AlgorithmParameterSpec; + +import javax.crypto.SecretKey; import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; @@ -31,6 +37,17 @@ */ public class BaseCipher implements Cipher { + // For tests + interface CipherFactory { + javax.crypto.Cipher getCipher(String transformation) throws GeneralSecurityException; + } + + static CipherFactory factory = SecurityUtils::getCipher; + + static boolean alwaysReInit; + + protected Mode mode; + private javax.crypto.Cipher cipher; private final int ivsize; private final int authSize; @@ -40,6 +57,7 @@ public class BaseCipher implements Cipher { private final int blkSize; private final String transformation; private String s; + private SecretKey secretKey; public BaseCipher(int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation, int blkSize) { @@ -99,12 +117,14 @@ protected javax.crypto.Cipher getCipherInstance() { } protected javax.crypto.Cipher createCipherInstance(Mode mode, byte[] key, byte[] iv) throws Exception { - javax.crypto.Cipher instance = SecurityUtils.getCipher(getTransformation()); + javax.crypto.Cipher instance = factory.getCipher(getTransformation()); + this.mode = mode; + this.secretKey = new SecretKeySpec(key, getAlgorithm()); instance.init( Mode.Encrypt.equals(mode) ? javax.crypto.Cipher.ENCRYPT_MODE : javax.crypto.Cipher.DECRYPT_MODE, - new SecretKeySpec(key, getAlgorithm()), + secretKey, new IvParameterSpec(iv)); return instance; } @@ -119,7 +139,38 @@ protected byte[] initializeIVData(Mode mode, byte[] iv, int reqLen) { @Override public void update(byte[] input, int inputOffset, int inputLen) throws Exception { - cipher.update(input, inputOffset, inputLen, input, inputOffset); + try { + int stored = cipher.update(input, inputOffset, inputLen, input, inputOffset); + if (stored < inputLen || alwaysReInit) { + // Cipher.update() may buffer. We need all. Call doFinal and re-init the cipher. + // This works because in SSH inputLen is always a multiple of the cipher's block size. + stored += cipher.doFinal(input, inputOffset + stored); + // Now stored had better be inputLen + if (stored != inputLen) { + throw new GeneralSecurityException( + "Cipher.doFinal() did not return all bytes: " + stored + " != " + inputLen); + } + reInit(input, inputOffset, inputLen); + } + } catch (GeneralSecurityException e) { + // Add algorithm information + throw new GeneralSecurityException( + "BaseCipher.update() for " + getTransformation() + '/' + getKeySize() + " failed (" + mode + ')', e); + } + } + + protected void reInit(byte[] processed, int offset, int length) + throws InvalidKeyException, InvalidAlgorithmParameterException { + cipher.init(Mode.Encrypt.equals(mode) + ? javax.crypto.Cipher.ENCRYPT_MODE + : javax.crypto.Cipher.DECRYPT_MODE, + secretKey, + determineNewParameters(processed, offset, length)); + } + + protected AlgorithmParameterSpec determineNewParameters(byte[] processed, int offset, int length) { + // Default implementation; overridden as appropriate in subclasses + throw new UnsupportedOperationException(getClass() + " needs to override determineNewParameters()"); } @Override diff --git a/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseRC4Cipher.java b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseRC4Cipher.java index a8b1f14ac..039c0543f 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseRC4Cipher.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseRC4Cipher.java @@ -53,4 +53,9 @@ protected javax.crypto.Cipher createCipherInstance(Mode mode, byte[] key, byte[] return instance; } + + @Override + public void update(byte[] input, int inputOffset, int inputLen) throws Exception { + getCipherInstance().update(input, inputOffset, inputLen, input, inputOffset); + } } diff --git a/sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java index dd9213946..bdacbaa95 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java @@ -52,8 +52,20 @@ public Cipher create() { return new CipherNone(); } }, - aes128cbc(Constants.AES128_CBC, 16, 0, 16, "AES", 128, "AES/CBC/NoPadding", 16), - aes128ctr(Constants.AES128_CTR, 16, 0, 16, "AES", 128, "AES/CTR/NoPadding", 16), + aes128cbc(Constants.AES128_CBC, 16, 0, 16, "AES", 128, "AES/CBC/NoPadding", 16) { + @Override + public Cipher create() { + return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(), + getTransformation(), getCipherBlockSize()); + } + }, + aes128ctr(Constants.AES128_CTR, 16, 0, 16, "AES", 128, "AES/CTR/NoPadding", 16) { + @Override + public Cipher create() { + return new BaseCTRCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(), + getTransformation(), getCipherBlockSize()); + } + }, aes128gcm(Constants.AES128_GCM, 12, 16, 16, "AES", 128, "AES/GCM/NoPadding", 16) { @Override public Cipher create() { @@ -70,10 +82,34 @@ public Cipher create() { getKeySize(), getTransformation(), getCipherBlockSize()); } }, - aes192cbc(Constants.AES192_CBC, 16, 0, 24, "AES", 192, "AES/CBC/NoPadding", 16), - aes192ctr(Constants.AES192_CTR, 16, 0, 24, "AES", 192, "AES/CTR/NoPadding", 16), - aes256cbc(Constants.AES256_CBC, 16, 0, 32, "AES", 256, "AES/CBC/NoPadding", 16), - aes256ctr(Constants.AES256_CTR, 16, 0, 32, "AES", 256, "AES/CTR/NoPadding", 16), + aes192cbc(Constants.AES192_CBC, 16, 0, 24, "AES", 192, "AES/CBC/NoPadding", 16) { + @Override + public Cipher create() { + return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(), + getTransformation(), getCipherBlockSize()); + } + }, + aes192ctr(Constants.AES192_CTR, 16, 0, 24, "AES", 192, "AES/CTR/NoPadding", 16) { + @Override + public Cipher create() { + return new BaseCTRCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(), + getTransformation(), getCipherBlockSize()); + } + }, + aes256cbc(Constants.AES256_CBC, 16, 0, 32, "AES", 256, "AES/CBC/NoPadding", 16) { + @Override + public Cipher create() { + return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(), + getTransformation(), getCipherBlockSize()); + } + }, + aes256ctr(Constants.AES256_CTR, 16, 0, 32, "AES", 256, "AES/CTR/NoPadding", 16) { + @Override + public Cipher create() { + return new BaseCTRCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(), + getTransformation(), getCipherBlockSize()); + } + }, /** * @deprecated * @see SSHD-1004 @@ -101,7 +137,13 @@ public Cipher create() { * @see SSHD-1004 */ @Deprecated - blowfishcbc(Constants.BLOWFISH_CBC, 8, 0, 16, "Blowfish", 128, "Blowfish/CBC/NoPadding", 8), + blowfishcbc(Constants.BLOWFISH_CBC, 8, 0, 16, "Blowfish", 128, "Blowfish/CBC/NoPadding", 8) { + @Override + public Cipher create() { + return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(), + getTransformation(), getCipherBlockSize()); + } + }, cc20p1305_openssh(Constants.CC20P1305_OPENSSH, 8, 16, 64, "ChaCha", 256, "ChaCha", 8) { @Override public Cipher create() { @@ -113,7 +155,13 @@ public Cipher create() { * @see SSHD-1004 */ @Deprecated - tripledescbc(Constants.TRIPLE_DES_CBC, 8, 0, 24, "DESede", 192, "DESede/CBC/NoPadding", 8); + tripledescbc(Constants.TRIPLE_DES_CBC, 8, 0, 24, "DESede", 192, "DESede/CBC/NoPadding", 8) { + @Override + public Cipher create() { + return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(), + getTransformation(), getCipherBlockSize()); + } + }; public static final Set VALUES = Collections.unmodifiableSet(EnumSet.allOf(BuiltinCiphers.class)); diff --git a/sshd-common/src/main/java/org/apache/sshd/common/cipher/Cipher.java b/sshd-common/src/main/java/org/apache/sshd/common/cipher/Cipher.java index 09e5aaf97..ed8d5d230 100644 --- a/sshd-common/src/main/java/org/apache/sshd/common/cipher/Cipher.java +++ b/sshd-common/src/main/java/org/apache/sshd/common/cipher/Cipher.java @@ -45,6 +45,9 @@ enum Mode { /** * Performs in-place encryption or decryption on the given data. + *

+ * Note:{@code input.length} must be a multiple of the cipher's block size. + *

* * @param input The input/output bytes * @throws Exception If failed to execute @@ -59,7 +62,8 @@ default void update(byte[] input) throws Exception { * * @param input The input/output bytes * @param inputOffset The offset of the data in the data buffer - * @param inputLen The number of bytes to update - starting at the given offset + * @param inputLen The number of bytes to update, starting at the given offset; must be a multiple of the + * cipher's block size * @throws Exception If failed to execute */ void update(byte[] input, int inputOffset, int inputLen) throws Exception; @@ -89,11 +93,17 @@ default void updateAAD(byte[] data) throws Exception { * implicitly appended after the output ciphertext or implicitly verified after the input ciphertext. Header data * indicated by the {@code aadLen} parameter are authenticated but not encrypted/decrypted, while payload data * indicated by the {@code inputLen} parameter are authenticated and encrypted/decrypted. + *

+ * Note: on encryption the {@code input} must have enough space after {@code offset + aadLen + inputLength} + * to store the authentication tag. On decryption, the authentication tag is assumed to be in the {@code input} at + * that offset (i.e., after the payload data). + *

* * @param input The input/output bytes * @param offset The offset of the data in the input buffer * @param aadLen The number of bytes to use as additional authenticated data - starting at offset - * @param inputLen The number of bytes to update - starting at offset + aadLen + * @param inputLen The number of bytes to update, starting at offset + aadLen; must be a multiple of the cipher's + * block size * @throws Exception If failed to execute */ default void updateWithAAD(byte[] input, int offset, int aadLen, int inputLen) throws Exception { diff --git a/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherResetTest.java b/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherResetTest.java new file mode 100644 index 000000000..54772d168 --- /dev/null +++ b/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherResetTest.java @@ -0,0 +1,177 @@ +/* + * 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.cipher; + +import java.security.SecureRandom; +import java.security.Security; +import java.security.spec.AlgorithmParameterSpec; +import java.util.ArrayList; +import java.util.List; +import java.util.Random; + +import javax.crypto.SecretKey; +import javax.crypto.spec.IvParameterSpec; +import javax.crypto.spec.SecretKeySpec; + +import org.apache.sshd.common.cipher.Cipher.Mode; +import org.apache.sshd.common.util.security.SecurityUtils; +import org.apache.sshd.util.test.JUnitTestSupport; +import org.apache.sshd.util.test.NoIoTestCase; +import org.bouncycastle.jce.provider.BouncyCastleProvider; +import org.junit.After; +import org.junit.Before; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; + +@Category({ NoIoTestCase.class }) +@RunWith(Parameterized.class) +public class BaseCipherResetTest extends JUnitTestSupport { + + private static final Random RND = new SecureRandom(); + + private final String providerName; + + private final BuiltinCiphers builtIn; + + public BaseCipherResetTest(String providerName, BuiltinCiphers builtIn, String name) { + this.providerName = providerName; + this.builtIn = builtIn; + if ("BC".equals(providerName)) { + registerBouncyCastleProviderIfNecessary(); + } + } + + private static void registerBouncyCastleProviderIfNecessary() { + if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME) == null) { + Security.addProvider(new BouncyCastleProvider()); + } + } + + @Parameters(name = "{2} - {0}") + public static List getParameters() { + List items = new ArrayList<>(); + for (BuiltinCiphers c : BuiltinCiphers.values()) { + String name = c.getName(); + if (name.endsWith("-cbc") || name.endsWith("-ctr")) { + items.add(new Object[] { "SunJCE", c, c.getName() }); + items.add(new Object[] { "BC", c, c.getName() }); + } + } + return items; + } + + @Before + public void changeCipher() { + BaseCipher.factory = t -> javax.crypto.Cipher.getInstance(t, providerName); + BaseCipher.alwaysReInit = true; + } + + @After + public void resetCipher() { + BaseCipher.factory = SecurityUtils::getCipher; + BaseCipher.alwaysReInit = false; + } + + private void checkBuffer(byte[] data, int index, byte[] front, byte[] back) { + byte[] expected = front.clone(); + System.arraycopy(back, index, expected, index, back.length - index); + assertArrayEquals("Mismatched bytes at " + index, expected, data); + } + + @Test + public void testReset() throws Exception { + byte[] plaintext = new byte[builtIn.getCipherBlockSize() * 30]; + for (int i = 0; i < plaintext.length; i++) { + plaintext[i] = (byte) (' ' + i); + } + byte[] key = new byte[builtIn.getKdfSize()]; + RND.nextBytes(key); + byte[] iv = new byte[builtIn.getIVSize()]; + RND.nextBytes(iv); + // Set last 8 bytes of iv to 0xff so we can see that the overflow is handled correctly + for (int i = iv.length - 8; i < iv.length; i++) { + iv[i] = (byte) 0xff; + } + iv[iv.length - 1] = (byte) 0xf5; + // Now the upper 8 bytes correspond to -11. We process 30 blocks, so in CTR mode we should see that overflow is + // handled correctly. + SecretKey secretKey = new SecretKeySpec(key, builtIn.getAlgorithm()); + AlgorithmParameterSpec param = new IvParameterSpec(iv); + javax.crypto.Cipher cipher = javax.crypto.Cipher.getInstance(builtIn.getTransformation(), providerName); + cipher.init(javax.crypto.Cipher.ENCRYPT_MODE, secretKey, param); + byte[] encrypted = cipher.doFinal(plaintext); + assertEquals("Mismatched length", plaintext.length, encrypted.length); + Cipher sshCipher = builtIn.create(); + sshCipher.init(Mode.Encrypt, key, iv); + byte[] sshText = plaintext.clone(); + // Encrypt it all + sshCipher.update(sshText); + assertArrayEquals("Mismatched encrypted bytes", encrypted, sshText); + // Same, but encrypt block by block + sshCipher = builtIn.create(); + sshCipher.init(Mode.Encrypt, key, iv); + sshText = plaintext.clone(); + int blockSize = builtIn.getCipherBlockSize(); + for (int i = 0; i < sshText.length; i += blockSize) { + sshCipher.update(sshText, i, blockSize); + checkBuffer(sshText, i + blockSize, encrypted, plaintext); + } + assertArrayEquals("Mismatched encrypted bytes", encrypted, sshText); + // Same, but encrypt six times five blocks + sshCipher = builtIn.create(); + sshCipher.init(Mode.Encrypt, key, iv); + sshText = plaintext.clone(); + blockSize = builtIn.getCipherBlockSize() * 5; + for (int i = 0; i < sshText.length; i += blockSize) { + sshCipher.update(sshText, i, blockSize); + checkBuffer(sshText, i + blockSize, encrypted, plaintext); + } + assertArrayEquals("Mismatched encrypted bytes", encrypted, sshText); + // Decrypt in all three ways: should be equal to the original plaintext + cipher.init(javax.crypto.Cipher.DECRYPT_MODE, secretKey, param); + byte[] decrypted = cipher.doFinal(encrypted); + assertArrayEquals("Mismatched encrypted bytes", plaintext, decrypted); + sshCipher = builtIn.create(); + sshCipher.init(Mode.Decrypt, key, iv); + byte[] data = encrypted.clone(); + sshCipher.update(data); + assertArrayEquals("Mismatched encrypted bytes", plaintext, data); + sshCipher = builtIn.create(); + sshCipher.init(Mode.Decrypt, key, iv); + data = encrypted.clone(); + blockSize = builtIn.getCipherBlockSize(); + for (int i = 0; i < data.length; i += blockSize) { + sshCipher.update(data, i, blockSize); + checkBuffer(data, i + blockSize, plaintext, encrypted); + } + assertArrayEquals("Mismatched encrypted bytes", plaintext, data); + sshCipher = builtIn.create(); + sshCipher.init(Mode.Decrypt, key, iv); + data = encrypted.clone(); + blockSize = builtIn.getCipherBlockSize() * 5; + for (int i = 0; i < data.length; i += blockSize) { + sshCipher.update(data, i, blockSize); + checkBuffer(data, i + blockSize, plaintext, encrypted); + } + assertArrayEquals("Mismatched encrypted bytes", plaintext, data); + } +} diff --git a/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherTest.java b/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherTest.java index 68402acb6..fd5305ce4 100644 --- a/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherTest.java +++ b/sshd-common/src/test/java/org/apache/sshd/common/cipher/BaseCipherTest.java @@ -22,6 +22,7 @@ import java.nio.charset.StandardCharsets; import java.security.GeneralSecurityException; import java.security.InvalidKeyException; +import java.util.Arrays; import javax.crypto.spec.IvParameterSpec; import javax.crypto.spec.SecretKeySpec; @@ -103,6 +104,13 @@ protected void testEncryptDecrypt(NamedFactory factory) throws Exception String expected = getClass().getName() + "[" + facName + "]"; byte[] expBytes = expected.getBytes(StandardCharsets.UTF_8); + // All ciphers use no padding, so the input must be padded to a multiple of the block size + int length = expBytes.length; + int blockSize = enc.getCipherBlockSize(); + expBytes = Arrays.copyOf(expBytes, (length / blockSize + 1) * blockSize); + for (int i = length; i < expBytes.length; i++) { + expBytes[i] = (byte) i; + } byte[] workBuf = expBytes.clone(); // need to clone since the cipher works in-line enc.update(workBuf, 0, workBuf.length); diff --git a/sshd-core/src/test/java/org/apache/sshd/common/cipher/OpenSshCipherTest.java b/sshd-core/src/test/java/org/apache/sshd/common/cipher/OpenSshCipherTest.java new file mode 100644 index 000000000..1b9ae7d69 --- /dev/null +++ b/sshd-core/src/test/java/org/apache/sshd/common/cipher/OpenSshCipherTest.java @@ -0,0 +1,156 @@ +/* + * 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.cipher; + +import java.security.Security; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import org.apache.sshd.client.SshClient; +import org.apache.sshd.client.auth.pubkey.HostBoundPubKeyAuthTest; +import org.apache.sshd.client.future.AuthFuture; +import org.apache.sshd.client.session.ClientSession; +import org.apache.sshd.common.keyprovider.FileKeyPairProvider; +import org.apache.sshd.common.util.security.SecurityUtils; +import org.apache.sshd.util.test.BaseTestSupport; +import org.apache.sshd.util.test.CommonTestSupportUtils; +import org.apache.sshd.util.test.ContainerTestCase; +import org.bouncycastle.jce.provider.BouncyCastleProvider; +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.junit.runner.RunWith; +import org.junit.runners.Parameterized; +import org.junit.runners.Parameterized.Parameters; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.containers.output.Slf4jLogConsumer; +import org.testcontainers.containers.wait.strategy.Wait; +import org.testcontainers.images.builder.ImageFromDockerfile; +import org.testcontainers.utility.MountableFile; + +/** + * Test ciphers against OpenSSH. Force resetting ciphers every time to verify that they are res-initialized correctly. + * + * @author Apache MINA SSHD Project + */ +@RunWith(Parameterized.class) +@Category(ContainerTestCase.class) +public class OpenSshCipherTest extends BaseTestSupport { + + private static final Logger LOG = LoggerFactory.getLogger(HostBoundPubKeyAuthTest.class); + + // Re-use an already defined key + private static final String TEST_RESOURCES = "org/apache/sshd/common/kex/extensions/client"; + + @Rule + public GenericContainer sshdContainer = new GenericContainer<>(new ImageFromDockerfile() + .withDockerfileFromBuilder(builder -> builder.from("alpine:3.19") // + .run("apk --update add openssh-server") // Installs OpenSSH + // Enable deprecated ciphers + .run("echo 'Ciphers +aes128-cbc,aes192-cbc,aes256-cbc,3des-cbc' >> /etc/ssh/sshd_config") + .run("ssh-keygen -A") // Generate multiple host keys + .run("adduser -D bob") // Add a user + .run("echo 'bob:passwordBob' | chpasswd") // Give it a password to unlock the user + .run("mkdir -p /home/bob/.ssh") // Create the SSH config directory + .entryPoint("/entrypoint.sh") // Sets bob as owner of anything under /home/bob and launches sshd + .build())) // + .withCopyFileToContainer(MountableFile.forClasspathResource(TEST_RESOURCES + "/bob_key.pub"), + "/home/bob/.ssh/authorized_keys") + // entrypoint must be executable. Spotbugs doesn't like 0777, so use hex + .withCopyFileToContainer( + MountableFile.forClasspathResource(TEST_RESOURCES + "/entrypoint.sh", 0x1ff), + "/entrypoint.sh") + .waitingFor(Wait.forLogMessage(".*Server listening on :: port 22.*\\n", 1)).withExposedPorts(22) // + .withLogConsumer(new Slf4jLogConsumer(LOG)); + + private final String providerName; + + private final BuiltinCiphers builtIn; + + public OpenSshCipherTest(String providerName, BuiltinCiphers factory, String name) { + this.providerName = providerName; + this.builtIn = factory; + if ("BC".equals(providerName)) { + registerBouncyCastleProviderIfNecessary(); + } + } + + @Before + public void changeCipher() { + BaseCipher.factory = t -> javax.crypto.Cipher.getInstance(t, providerName); + BaseCipher.alwaysReInit = true; + } + + @After + public void resetCipher() { + BaseCipher.factory = SecurityUtils::getCipher; + BaseCipher.alwaysReInit = false; + } + + private static void registerBouncyCastleProviderIfNecessary() { + if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME) == null) { + Security.addProvider(new BouncyCastleProvider()); + } + } + + private static void addCipher(BuiltinCiphers cipherFactory, List items) { + items.add(new Object[] { "SunJCE", cipherFactory, cipherFactory.getName() }); + items.add(new Object[] { "BC", cipherFactory, cipherFactory.getName() }); + } + + @SuppressWarnings("deprecation") + @Parameters(name = "{2} - {0}") + public static List getParameters() { + List items = new ArrayList<>(); + addCipher(BuiltinCiphers.tripledescbc, items); + addCipher(BuiltinCiphers.aes128cbc, items); + addCipher(BuiltinCiphers.aes128ctr, items); + addCipher(BuiltinCiphers.aes128gcm, items); + addCipher(BuiltinCiphers.aes192cbc, items); + addCipher(BuiltinCiphers.aes192ctr, items); + addCipher(BuiltinCiphers.aes256cbc, items); + addCipher(BuiltinCiphers.aes256ctr, items); + addCipher(BuiltinCiphers.aes256gcm, items); + addCipher(BuiltinCiphers.cc20p1305_openssh, items); + return items; + } + + @Test + public void testConnection() throws Exception { + FileKeyPairProvider keyPairProvider = CommonTestSupportUtils.createTestKeyPairProvider(TEST_RESOURCES + "/bob_key"); + SshClient client = setupTestClient(); + client.setKeyIdentityProvider(keyPairProvider); + client.setCipherFactories(Collections.singletonList(builtIn)); + client.start(); + + Integer actualPort = sshdContainer.getMappedPort(22); + String actualHost = sshdContainer.getHost(); + try (ClientSession session = client.connect("bob", actualHost, actualPort).verify(CONNECT_TIMEOUT).getSession()) { + AuthFuture authed = session.auth().verify(AUTH_TIMEOUT); + assertTrue(authed.isDone() && authed.isSuccess()); + } finally { + client.stop(); + } + } +} diff --git a/sshd-mina/pom.xml b/sshd-mina/pom.xml index 0644a36a2..404eb4b03 100644 --- a/sshd-mina/pom.xml +++ b/sshd-mina/pom.xml @@ -123,6 +123,7 @@ **/ClientOpenSSHCertificatesTest.java **/SessionReKeyHostKeyExchangeTest.java **/HostBoundPubKeyAuthTest.java + **/OpenSshCipherTest.java **/PortForwardingWithOpenSshTest.java **/StrictKexInteroperabilityTest.java diff --git a/sshd-netty/pom.xml b/sshd-netty/pom.xml index 630df7e26..9d25f843f 100644 --- a/sshd-netty/pom.xml +++ b/sshd-netty/pom.xml @@ -142,6 +142,7 @@ **/ClientOpenSSHCertificatesTest.java **/SessionReKeyHostKeyExchangeTest.java **/HostBoundPubKeyAuthTest.java + **/OpenSshCipherTest.java **/PortForwardingWithOpenSshTest.java **/StrictKexInteroperabilityTest.java