Skip to content

Commit

Permalink
GH-455: ensure BaseCipher.update() fulfills the contract
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
tomaswolf committed Feb 21, 2024
1 parent 2c62c72 commit ee382b0
Show file tree
Hide file tree
Showing 12 changed files with 605 additions and 13 deletions.
2 changes: 2 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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));
}
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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;
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
}
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand All @@ -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 <A HREF="https://issues.apache.org/jira/browse/SSHD-1004">SSHD-1004</A>
Expand Down Expand Up @@ -101,7 +137,13 @@ public Cipher create() {
* @see <A HREF="https://issues.apache.org/jira/browse/SSHD-1004">SSHD-1004</A>
*/
@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() {
Expand All @@ -113,7 +155,13 @@ public Cipher create() {
* @see <A HREF="https://issues.apache.org/jira/browse/SSHD-1004">SSHD-1004</A>
*/
@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<BuiltinCiphers> VALUES = Collections.unmodifiableSet(EnumSet.allOf(BuiltinCiphers.class));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@ enum Mode {

/**
* Performs in-place encryption or decryption on the given data.
* <p>
* <b>Note:</b>{@code input.length} must be a multiple of the cipher's block size.
* </p>
*
* @param input The input/output bytes
* @throws Exception If failed to execute
Expand All @@ -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;
Expand Down Expand Up @@ -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.
* <p>
* <b>Note:</b> 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).
* </p>
*
* @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 {
Expand Down
Loading

0 comments on commit ee382b0

Please sign in to comment.