Skip to content

Commit bc44e0b

Browse files
tomaswolftomaswolf
committed
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.
1 parent 6c45997 commit bc44e0b

File tree

12 files changed

+604
-13
lines changed

12 files changed

+604
-13
lines changed

‎CHANGES.md

+1
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030

3131
## Bug Fixes
3232

33+
* [GH-455](https://github.com/apache/mina-sshd/issues/455) Fix `BaseCipher`: make sure all bytes are processed
3334
* [GH-458](https://github.com/apache/mina-sshd/issues/458) Singleton thread pool for kex message handler flushing
3435

3536
* [SSHD-1338](https://issues.apache.org/jira/browse/SSHD-1338) Restore binary compatibility with 2.9.2

‎sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCBCCipher.java

+55
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.sshd.common.cipher;
21+
22+
import java.security.spec.AlgorithmParameterSpec;
23+
import java.util.Arrays;
24+
25+
import javax.crypto.spec.IvParameterSpec;
26+
27+
public class BaseCBCCipher extends BaseCipher {
28+
29+
private byte[] lastEncryptedBlock;
30+
31+
public BaseCBCCipher(int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
32+
int blkSize) {
33+
super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
34+
}
35+
36+
@Override
37+
public void update(byte[] input, int inputOffset, int inputLen) throws Exception {
38+
if (mode == Mode.Decrypt) {
39+
lastEncryptedBlock = Arrays.copyOfRange(input, inputOffset + inputLen - getCipherBlockSize(),
40+
inputOffset + inputLen);
41+
}
42+
super.update(input, inputOffset, inputLen);
43+
}
44+
45+
@Override
46+
protected AlgorithmParameterSpec determineNewParameters(byte[] processed, int offset, int length) {
47+
// The IV is the last encrypted block
48+
if (mode == Mode.Decrypt) {
49+
byte[] result = lastEncryptedBlock;
50+
lastEncryptedBlock = null;
51+
return new IvParameterSpec(result);
52+
}
53+
return new IvParameterSpec(Arrays.copyOfRange(processed, offset + length - getCipherBlockSize(), offset + length));
54+
}
55+
}

‎sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCTRCipher.java

+78
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,78 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.sshd.common.cipher;
21+
22+
import java.security.InvalidAlgorithmParameterException;
23+
import java.security.InvalidKeyException;
24+
import java.security.spec.AlgorithmParameterSpec;
25+
26+
import javax.crypto.spec.IvParameterSpec;
27+
28+
import org.apache.sshd.common.util.buffer.ByteArrayBuffer;
29+
30+
public class BaseCTRCipher extends BaseCipher {
31+
32+
private long blocksProcessed;
33+
34+
public BaseCTRCipher(int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
35+
int blkSize) {
36+
super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
37+
}
38+
39+
@Override
40+
public void update(byte[] input, int inputOffset, int inputLen) throws Exception {
41+
blocksProcessed += inputLen / getCipherBlockSize();
42+
super.update(input, inputOffset, inputLen);
43+
}
44+
45+
@Override
46+
protected void reInit(byte[] processed, int offset, int length)
47+
throws InvalidKeyException, InvalidAlgorithmParameterException {
48+
super.reInit(processed, offset, length);
49+
blocksProcessed = 0;
50+
}
51+
52+
@Override
53+
protected AlgorithmParameterSpec determineNewParameters(byte[] processed, int offset, int length) {
54+
byte[] iv = getCipherInstance().getIV().clone();
55+
// Treat the IV as a counter and add blocksProcessed
56+
ByteArrayBuffer buf = new ByteArrayBuffer(iv, iv.length - Long.BYTES, Long.BYTES);
57+
long unsigned = buf.getLong();
58+
long highBitBefore = unsigned & ~Long.MAX_VALUE;
59+
unsigned &= Long.MAX_VALUE; // Clear most significant bit
60+
unsigned += blocksProcessed;
61+
long highBitNow = unsigned & ~Long.MAX_VALUE;
62+
unsigned = (unsigned & Long.MAX_VALUE) | (highBitBefore ^ highBitNow);
63+
int carry = (int) ((highBitBefore & highBitNow) >>> (Long.SIZE - 1));
64+
addCarry(iv, iv.length - Long.BYTES, carry);
65+
buf.wpos(iv.length - Long.BYTES);
66+
buf.putLong(unsigned);
67+
return new IvParameterSpec(iv);
68+
}
69+
70+
private void addCarry(byte[] iv, int length, int carry) {
71+
int add = carry;
72+
for (int i = length - 1; i >= 0; i--) {
73+
int b = (iv[i] & 0xFF) + add;
74+
iv[i] = (byte) b;
75+
add = b >> Byte.SIZE;
76+
}
77+
}
78+
}

‎sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseCipher.java

+54-3
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,12 @@
1818
*/
1919
package org.apache.sshd.common.cipher;
2020

21+
import java.security.GeneralSecurityException;
22+
import java.security.InvalidAlgorithmParameterException;
23+
import java.security.InvalidKeyException;
24+
import java.security.spec.AlgorithmParameterSpec;
25+
26+
import javax.crypto.SecretKey;
2127
import javax.crypto.spec.IvParameterSpec;
2228
import javax.crypto.spec.SecretKeySpec;
2329

@@ -31,6 +37,17 @@
3137
*/
3238
public class BaseCipher implements Cipher {
3339

40+
// For tests
41+
interface CipherFactory {
42+
javax.crypto.Cipher getCipher(String transformation) throws GeneralSecurityException;
43+
}
44+
45+
static CipherFactory factory = SecurityUtils::getCipher;
46+
47+
static boolean alwaysReInit;
48+
49+
protected Mode mode;
50+
3451
private javax.crypto.Cipher cipher;
3552
private final int ivsize;
3653
private final int authSize;
@@ -40,6 +57,7 @@ public class BaseCipher implements Cipher {
4057
private final int blkSize;
4158
private final String transformation;
4259
private String s;
60+
private SecretKey secretKey;
4361

4462
public BaseCipher(int ivsize, int authSize, int kdfSize, String algorithm,
4563
int keySize, String transformation, int blkSize) {
@@ -99,12 +117,14 @@ protected javax.crypto.Cipher getCipherInstance() {
99117
}
100118

101119
protected javax.crypto.Cipher createCipherInstance(Mode mode, byte[] key, byte[] iv) throws Exception {
102-
javax.crypto.Cipher instance = SecurityUtils.getCipher(getTransformation());
120+
javax.crypto.Cipher instance = factory.getCipher(getTransformation());
121+
this.mode = mode;
122+
this.secretKey = new SecretKeySpec(key, getAlgorithm());
103123
instance.init(
104124
Mode.Encrypt.equals(mode)
105125
? javax.crypto.Cipher.ENCRYPT_MODE
106126
: javax.crypto.Cipher.DECRYPT_MODE,
107-
new SecretKeySpec(key, getAlgorithm()),
127+
secretKey,
108128
new IvParameterSpec(iv));
109129
return instance;
110130
}
@@ -119,7 +139,38 @@ protected byte[] initializeIVData(Mode mode, byte[] iv, int reqLen) {
119139

120140
@Override
121141
public void update(byte[] input, int inputOffset, int inputLen) throws Exception {
122-
cipher.update(input, inputOffset, inputLen, input, inputOffset);
142+
try {
143+
int stored = cipher.update(input, inputOffset, inputLen, input, inputOffset);
144+
if (stored < inputLen || alwaysReInit) {
145+
// Cipher.update() may buffer. We need all. Call doFinal and re-init the cipher.
146+
// This works because in SSH inputLen is always a multiple of the cipher's block size.
147+
stored += cipher.doFinal(input, inputOffset + stored);
148+
// Now stored had better be inputLen
149+
if (stored != inputLen) {
150+
throw new GeneralSecurityException(
151+
"Cipher.doFinal() did not return all bytes: " + stored + " != " + inputLen);
152+
}
153+
reInit(input, inputOffset, inputLen);
154+
}
155+
} catch (GeneralSecurityException e) {
156+
// Add algorithm information
157+
throw new GeneralSecurityException(
158+
"BaseCipher.update() for " + getTransformation() + '/' + getKeySize() + " failed (" + mode + ')', e);
159+
}
160+
}
161+
162+
protected void reInit(byte[] processed, int offset, int length)
163+
throws InvalidKeyException, InvalidAlgorithmParameterException {
164+
cipher.init(Mode.Encrypt.equals(mode)
165+
? javax.crypto.Cipher.ENCRYPT_MODE
166+
: javax.crypto.Cipher.DECRYPT_MODE,
167+
secretKey,
168+
determineNewParameters(processed, offset, length));
169+
}
170+
171+
protected AlgorithmParameterSpec determineNewParameters(byte[] processed, int offset, int length) {
172+
// Default implementation; overridden as appropriate in subclasses
173+
throw new UnsupportedOperationException(getClass() + " needs to override determineNewParameters()");
123174
}
124175

125176
@Override

‎sshd-common/src/main/java/org/apache/sshd/common/cipher/BaseRC4Cipher.java

+5
Original file line numberDiff line numberDiff line change
@@ -53,4 +53,9 @@ protected javax.crypto.Cipher createCipherInstance(Mode mode, byte[] key, byte[]
5353

5454
return instance;
5555
}
56+
57+
@Override
58+
public void update(byte[] input, int inputOffset, int inputLen) throws Exception {
59+
getCipherInstance().update(input, inputOffset, inputLen, input, inputOffset);
60+
}
5661
}

‎sshd-common/src/main/java/org/apache/sshd/common/cipher/BuiltinCiphers.java

+56-8
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,20 @@ public Cipher create() {
5252
return new CipherNone();
5353
}
5454
},
55-
aes128cbc(Constants.AES128_CBC, 16, 0, 16, "AES", 128, "AES/CBC/NoPadding", 16),
56-
aes128ctr(Constants.AES128_CTR, 16, 0, 16, "AES", 128, "AES/CTR/NoPadding", 16),
55+
aes128cbc(Constants.AES128_CBC, 16, 0, 16, "AES", 128, "AES/CBC/NoPadding", 16) {
56+
@Override
57+
public Cipher create() {
58+
return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(),
59+
getTransformation(), getCipherBlockSize());
60+
}
61+
},
62+
aes128ctr(Constants.AES128_CTR, 16, 0, 16, "AES", 128, "AES/CTR/NoPadding", 16) {
63+
@Override
64+
public Cipher create() {
65+
return new BaseCTRCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(),
66+
getTransformation(), getCipherBlockSize());
67+
}
68+
},
5769
aes128gcm(Constants.AES128_GCM, 12, 16, 16, "AES", 128, "AES/GCM/NoPadding", 16) {
5870
@Override
5971
public Cipher create() {
@@ -70,10 +82,34 @@ public Cipher create() {
7082
getKeySize(), getTransformation(), getCipherBlockSize());
7183
}
7284
},
73-
aes192cbc(Constants.AES192_CBC, 16, 0, 24, "AES", 192, "AES/CBC/NoPadding", 16),
74-
aes192ctr(Constants.AES192_CTR, 16, 0, 24, "AES", 192, "AES/CTR/NoPadding", 16),
75-
aes256cbc(Constants.AES256_CBC, 16, 0, 32, "AES", 256, "AES/CBC/NoPadding", 16),
76-
aes256ctr(Constants.AES256_CTR, 16, 0, 32, "AES", 256, "AES/CTR/NoPadding", 16),
85+
aes192cbc(Constants.AES192_CBC, 16, 0, 24, "AES", 192, "AES/CBC/NoPadding", 16) {
86+
@Override
87+
public Cipher create() {
88+
return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(),
89+
getTransformation(), getCipherBlockSize());
90+
}
91+
},
92+
aes192ctr(Constants.AES192_CTR, 16, 0, 24, "AES", 192, "AES/CTR/NoPadding", 16) {
93+
@Override
94+
public Cipher create() {
95+
return new BaseCTRCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(),
96+
getTransformation(), getCipherBlockSize());
97+
}
98+
},
99+
aes256cbc(Constants.AES256_CBC, 16, 0, 32, "AES", 256, "AES/CBC/NoPadding", 16) {
100+
@Override
101+
public Cipher create() {
102+
return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(),
103+
getTransformation(), getCipherBlockSize());
104+
}
105+
},
106+
aes256ctr(Constants.AES256_CTR, 16, 0, 32, "AES", 256, "AES/CTR/NoPadding", 16) {
107+
@Override
108+
public Cipher create() {
109+
return new BaseCTRCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(),
110+
getTransformation(), getCipherBlockSize());
111+
}
112+
},
77113
/**
78114
* @deprecated
79115
* @see <A HREF="https://issues.apache.org/jira/browse/SSHD-1004">SSHD-1004</A>
@@ -101,7 +137,13 @@ public Cipher create() {
101137
* @see <A HREF="https://issues.apache.org/jira/browse/SSHD-1004">SSHD-1004</A>
102138
*/
103139
@Deprecated
104-
blowfishcbc(Constants.BLOWFISH_CBC, 8, 0, 16, "Blowfish", 128, "Blowfish/CBC/NoPadding", 8),
140+
blowfishcbc(Constants.BLOWFISH_CBC, 8, 0, 16, "Blowfish", 128, "Blowfish/CBC/NoPadding", 8) {
141+
@Override
142+
public Cipher create() {
143+
return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(),
144+
getTransformation(), getCipherBlockSize());
145+
}
146+
},
105147
cc20p1305_openssh(Constants.CC20P1305_OPENSSH, 8, 16, 64, "ChaCha", 256, "ChaCha", 8) {
106148
@Override
107149
public Cipher create() {
@@ -113,7 +155,13 @@ public Cipher create() {
113155
* @see <A HREF="https://issues.apache.org/jira/browse/SSHD-1004">SSHD-1004</A>
114156
*/
115157
@Deprecated
116-
tripledescbc(Constants.TRIPLE_DES_CBC, 8, 0, 24, "DESede", 192, "DESede/CBC/NoPadding", 8);
158+
tripledescbc(Constants.TRIPLE_DES_CBC, 8, 0, 24, "DESede", 192, "DESede/CBC/NoPadding", 8) {
159+
@Override
160+
public Cipher create() {
161+
return new BaseCBCCipher(getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(), getKeySize(),
162+
getTransformation(), getCipherBlockSize());
163+
}
164+
};
117165

118166
public static final Set<BuiltinCiphers> VALUES = Collections.unmodifiableSet(EnumSet.allOf(BuiltinCiphers.class));
119167

‎sshd-common/src/main/java/org/apache/sshd/common/cipher/Cipher.java

+12-2
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ enum Mode {
4545

4646
/**
4747
* Performs in-place encryption or decryption on the given data.
48+
* <p>
49+
* <b>Note:</b>{@code input.length} must be a multiple of the cipher's block size.
50+
* </p>
4851
*
4952
* @param input The input/output bytes
5053
* @throws Exception If failed to execute
@@ -59,7 +62,8 @@ default void update(byte[] input) throws Exception {
5962
*
6063
* @param input The input/output bytes
6164
* @param inputOffset The offset of the data in the data buffer
62-
* @param inputLen The number of bytes to update - starting at the given offset
65+
* @param inputLen The number of bytes to update, starting at the given offset; must be a multiple of the
66+
* cipher's block size
6367
* @throws Exception If failed to execute
6468
*/
6569
void update(byte[] input, int inputOffset, int inputLen) throws Exception;
@@ -89,11 +93,17 @@ default void updateAAD(byte[] data) throws Exception {
8993
* implicitly appended after the output ciphertext or implicitly verified after the input ciphertext. Header data
9094
* indicated by the {@code aadLen} parameter are authenticated but not encrypted/decrypted, while payload data
9195
* indicated by the {@code inputLen} parameter are authenticated and encrypted/decrypted.
96+
* <p>
97+
* <b>Note:</b> on encryption the {@code input} must have enough space after {@code offset + aadLen + inputLength}
98+
* to store the authentication tag. On decryption, the authentication tag is assumed to be in the {@code input} at
99+
* that offset (i.e., after the payload data).
100+
* </p>
92101
*
93102
* @param input The input/output bytes
94103
* @param offset The offset of the data in the input buffer
95104
* @param aadLen The number of bytes to use as additional authenticated data - starting at offset
96-
* @param inputLen The number of bytes to update - starting at offset + aadLen
105+
* @param inputLen The number of bytes to update, starting at offset + aadLen; must be a multiple of the cipher's
106+
* block size
97107
* @throws Exception If failed to execute
98108
*/
99109
default void updateWithAAD(byte[] input, int offset, int aadLen, int inputLen) throws Exception {

0 commit comments

Comments
 (0)