Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SSHD-506] Add support for RFC 5647 #132

Closed
wants to merge 20 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
* 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.Cipher;
import javax.crypto.SecretKey;
import javax.crypto.spec.SecretKeySpec;

import org.apache.sshd.common.util.security.SecurityUtils;

/**
* Base cipher for authenticated encryption with associated data implementations.
*
* @see <a href="https://tools.ietf.org/html/rfc5116">RFC 5116</a>
*/
public abstract class BaseAEADCipher extends BaseCipher {

private Mode mode;
private SecretKey secretKey;
private AlgorithmParameterSpec params;

public BaseAEADCipher(
int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
int blkSize) {
super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
}

@Override
protected byte[] initializeIVData(final Mode mode, final byte[] iv, final int reqLen) {
byte[] data = super.initializeIVData(mode, iv, reqLen);
params = initializeAlgorithmParameters(data);
return data;
}

@Override
protected byte[] initializeKeyData(final Mode mode, final byte[] key, final int reqLen) {
byte[] keyData = super.initializeKeyData(mode, key, reqLen);
secretKey = new SecretKeySpec(keyData, getAlgorithm());
return keyData;
}

@Override
protected Cipher createCipherInstance(final Mode mode, final byte[] key, final byte[] iv) throws Exception {
this.mode = mode;
Cipher instance = SecurityUtils.getCipher(getTransformation());
init(instance);
return instance;
}

/**
* Initializes this cipher using the given JDK cipher and the previously configured secret key and algorithm
* parameters.
*
* @param cipher the JDK cipher instance to initialize
* @throws InvalidKeyException if this cipher's secret key is incompatible with the given cipher
* @throws InvalidAlgorithmParameterException if this cipher's algorithm parameters are incompatible with the given
* cipher
*/
protected void init(Cipher cipher) throws InvalidKeyException, InvalidAlgorithmParameterException {
cipher.init(mode == Mode.Encrypt ? Cipher.ENCRYPT_MODE : Cipher.DECRYPT_MODE, secretKey, params);
}

/**
* Creates the specific algorithm parameters for the given input initialization vector.
*
* @param iv initialization vector provided at construction
* @return this cipher's parameters
*/
protected abstract AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv);

/**
* Returns the next algorithm parameters to use for encrypting or decrypting the next
* {@link #updateWithAAD(byte[], int, int, int, int)}} operation.
*
* @return the parameters for the next updateWithAAD operation
*/
protected abstract AlgorithmParameterSpec getNextAlgorithmParameters();

@Override
public void updateWithAAD(byte[] input, int aadOffset, int aadLen, int inputOffset, int inputLen) throws Exception {
Cipher cipher = getCipherInstance();
params = getNextAlgorithmParameters();
init(cipher);
cipher.updateAAD(input, aadOffset, aadLen);
cipher.doFinal(input, inputOffset, inputLen, input, inputOffset);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ public class BaseCipher implements Cipher {

private javax.crypto.Cipher cipher;
private final int ivsize;
private final int authSize;
private final int kdfSize;
private final String algorithm;
private final int keySize;
Expand All @@ -41,9 +42,10 @@ public class BaseCipher implements Cipher {
private String s;

public BaseCipher(
int ivsize, int kdfSize, String algorithm,
int ivsize, int authSize, int kdfSize, String algorithm,
int keySize, String transformation, int blkSize) {
this.ivsize = ivsize;
this.authSize = authSize;
this.kdfSize = kdfSize;
this.algorithm = ValidateUtils.checkNotNullAndNotEmpty(algorithm, "No algorithm");
this.keySize = keySize;
Expand Down Expand Up @@ -71,6 +73,11 @@ public int getIVSize() {
return ivsize;
}

@Override
public int getAuthenticationTagSize() {
return authSize;
}

@Override
public int getKdfSize() {
return kdfSize;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/*
* 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 javax.crypto.Cipher;
import javax.crypto.spec.GCMParameterSpec;

import org.apache.sshd.common.util.buffer.Buffer;
import org.apache.sshd.common.util.buffer.ByteArrayBuffer;

public class BaseGCMCipher extends BaseAEADCipher {

public BaseGCMCipher(
int ivsize, int authSize, int kdfSize, String algorithm, int keySize, String transformation,
int blkSize) {
super(ivsize, authSize, kdfSize, algorithm, keySize, transformation, blkSize);
}

@Override
protected AlgorithmParameterSpec initializeAlgorithmParameters(byte[] iv) {
Buffer buffer = new ByteArrayBuffer(iv);
buffer.rpos(Integer.BYTES);
long ic = buffer.getLong();
// decrement IV as it will be incremented back to initial value on first call to updateWithAAD
ic = (ic - 1) & 0x0ffffffffL;
buffer.wpos(Integer.BYTES);
buffer.putLong(ic);
return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, buffer.array());
}

@Override
protected AlgorithmParameterSpec getNextAlgorithmParameters() {
Cipher cipher = getCipherInstance();
Buffer iv = new ByteArrayBuffer(cipher.getIV());
iv.rpos(Integer.BYTES);
long ic = iv.getLong();
ic = (ic + 1) & 0x0ffffffffL;
iv.wpos(Integer.BYTES);
iv.putLong(ic);
return new GCMParameterSpec(getAuthenticationTagSize() * Byte.SIZE, iv.array());
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty expensive in memory - every time it is called (which is for every packet) a new byte[] (cipher.getIv() returns a clone), new Buffer and new GCMParameterSpec instance is created. Can we do something about it ?

Moreover, this code is exactly the same as initializeAlgorithmParameters except it does +1 instead of -1 - perhaps we could use an internal (protected) updateAlgorithmParameters(byte[] iv, long delta) and call it once with +1 and once with -1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can avoid creating the Buffer since all the code does is replace an 8-byte section of the IV at offset 4:

byte[] iv = cipher.getIV();
// Need to write decodeLong  but it should be similar to decodeInt in the same class
// can use the code of Buffer#getLong as base
long ic = KeyEntryResolver.decodeLong(iv, Integer.BYTES, iv.length - Integer.BYTES);
ic = (ic + delta) & 0x0ffffffffL;
// Need to write it - can use the code of Buffer#putLong as base
KeyEntryResolver.encodeLong(iv, ic, Integer.BYTES, iv.length - Integer.BYTES);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh, nice find. Yes, we just need to update the 8-byte section of the buffer. The update algorithm parameters API idea sounds good to me; it's more in line with how OpenSSH is structured, too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the GCMParameterSpec based on this link we might be able to execute an ugly hack via reflection API and manipulate the internal IV data directly instead of creating a new instance every time... will need to think about it some more - but only after this code is working.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I came up with a better idea: extend the class and don't clone the IV. Less safe, but doesn't continually create garbage.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've mostly optimized away all the garbage here. I do an initial IV clone when calling the public init method, but that's the only time. I don't see the IV being cloned further by the Cipher classes that use GCMParameterSpec.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I optimized it so much, both AES/GCM engines can't tell that the IV changes between invocations and throw an exception. Nice.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out how to get the engine to consider the IV as different, so I still have some clone calls happening, but much less waste than before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into the internal implementation of the cipher engine, and it makes its own clone regardless of what we do, so this seems to be about as optimal as I can get it without digging into Bouncycastle internals or something similar.


}
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ public class BaseRC4Cipher extends BaseCipher {
public static final int SKIP_SIZE = 1536;

public BaseRC4Cipher(int ivsize, int kdfSize, int keySize, int blkSize) {
super(ivsize, kdfSize, "ARCFOUR", keySize, "RC4", blkSize);
super(ivsize, 0, kdfSize, "ARCFOUR", keySize, "RC4", blkSize);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,39 +46,56 @@
* @author <a href="mailto:dev@mina.apache.org">Apache MINA SSHD Project</a>
*/
public enum BuiltinCiphers implements CipherFactory {
none(Constants.NONE, 0, 0, "None", 0, "None", 0) {
none(Constants.NONE, 0, 0, 0, "None", 0, "None", 0) {
@Override
public Cipher create() {
return new CipherNone();
}
},
aes128cbc(Constants.AES128_CBC, 16, 16, "AES", 128, "AES/CBC/NoPadding", 16),
aes128ctr(Constants.AES128_CTR, 16, 16, "AES", 128, "AES/CTR/NoPadding", 16),
aes192cbc(Constants.AES192_CBC, 16, 24, "AES", 192, "AES/CBC/NoPadding", 16),
aes192ctr(Constants.AES192_CTR, 16, 24, "AES", 192, "AES/CTR/NoPadding", 16),
aes256cbc(Constants.AES256_CBC, 16, 32, "AES", 256, "AES/CBC/NoPadding", 16),
aes256ctr(Constants.AES256_CTR, 16, 32, "AES", 256, "AES/CTR/NoPadding", 16),
arcfour128(Constants.ARCFOUR128, 8, 16, "ARCFOUR", 128, "RC4", 16) {
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),
aes128gcm(Constants.AES128_GCM, 12, 16, 16, "AES", 128, "AES/GCM/NoPadding", 16) {
@Override
public Cipher create() {
return new BaseGCMCipher(
getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(),
getKeySize(), getTransformation(), getCipherBlockSize());
}
},
aes256gcm(Constants.AES256_GCM, 12, 16, 32, "AES", 256, "AES/GCM/NoPadding", 16) {
@Override
public Cipher create() {
return new BaseGCMCipher(
getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(),
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),
arcfour128(Constants.ARCFOUR128, 8, 0, 16, "ARCFOUR", 128, "RC4", 16) {
@Override
public Cipher create() {
return new BaseRC4Cipher(getIVSize(), getKdfSize(), getKeySize(), getCipherBlockSize());
}
},
arcfour256(Constants.ARCFOUR256, 8, 32, "ARCFOUR", 256, "RC4", 32) {
arcfour256(Constants.ARCFOUR256, 8, 0, 32, "ARCFOUR", 256, "RC4", 32) {
@Override
public Cipher create() {
return new BaseRC4Cipher(getIVSize(), getKdfSize(), getKeySize(), getCipherBlockSize());
}
},
blowfishcbc(Constants.BLOWFISH_CBC, 8, 16, "Blowfish", 128, "Blowfish/CBC/NoPadding", 8),
tripledescbc(Constants.TRIPLE_DES_CBC, 8, 24, "DESede", 192, "DESede/CBC/NoPadding", 8);
blowfishcbc(Constants.BLOWFISH_CBC, 8, 0, 16, "Blowfish", 128, "Blowfish/CBC/NoPadding", 8),
tripledescbc(Constants.TRIPLE_DES_CBC, 8, 0, 24, "DESede", 192, "DESede/CBC/NoPadding", 8);

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

private static final Map<String, CipherFactory> EXTENSIONS = new TreeMap<>(String.CASE_INSENSITIVE_ORDER);

private final String factoryName;
private final int ivsize;
private final int authSize;
private final int kdfSize;
private final int keysize;
private final int blkSize;
Expand All @@ -87,10 +104,11 @@ public Cipher create() {
private final boolean supported;

BuiltinCiphers(
String factoryName, int ivsize, int kdfSize,
jvz marked this conversation as resolved.
Show resolved Hide resolved
String factoryName, int ivsize, int authSize, int kdfSize,
String algorithm, int keySize, String transformation, int blkSize) {
this.factoryName = factoryName;
this.ivsize = ivsize;
this.authSize = authSize;
this.kdfSize = kdfSize;
this.keysize = keySize;
this.algorithm = algorithm;
Expand Down Expand Up @@ -133,6 +151,11 @@ public int getIVSize() {
return ivsize;
}

@Override
public int getAuthenticationTagSize() {
return authSize;
}

@Override
public int getKdfSize() {
return kdfSize;
Expand All @@ -156,7 +179,7 @@ public String getTransformation() {
@Override
public Cipher create() {
return new BaseCipher(
getIVSize(), getKdfSize(), getAlgorithm(),
getIVSize(), getAuthenticationTagSize(), getKdfSize(), getAlgorithm(),
getKeySize(), getTransformation(), getCipherBlockSize());
}

Expand Down Expand Up @@ -319,10 +342,12 @@ public static final class Constants {

public static final String AES128_CBC = "aes128-cbc";
public static final String AES128_CTR = "aes128-ctr";
public static final String AES128_GCM = "aes128-gcm@openssh.com";
public static final String AES192_CBC = "aes192-cbc";
public static final String AES192_CTR = "aes192-ctr";
public static final String AES256_CBC = "aes256-cbc";
public static final String AES256_CTR = "aes256-ctr";
public static final String AES256_GCM = "aes256-gcm@openssh.com";
public static final String ARCFOUR128 = "arcfour128";
public static final String ARCFOUR256 = "arcfour256";
public static final String BLOWFISH_CBC = "blowfish-cbc";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,21 @@ default void update(byte[] input) throws Exception {
*/
void update(byte[] input, int inputOffset, int inputLen) throws Exception;

/**
* Performs in-place authenticated encryption or decrypting with additional data. Authentication tags are implicitly
* appended after the output ciphertext or implicitly verified after the input ciphertext.
*
* @param input The input/output bytes
* @param aadOffset The offset of the additional authenticated data in the input buffer
* @param aadLen The number of bytes to use as additional authenticated data - starting at aadOffset
* @param inputOffset The offset of the data in the data buffer
* @param inputLen The number of bytes to update - starting at inputOffset
* @throws Exception If failed to executed
*/
default void updateWithAAD(byte[] input, int aadOffset, int aadLen, int inputOffset, int inputLen) throws Exception {
throw new UnsupportedOperationException(getClass() + " does not support additional authenticated data");
}

/**
* @param xform The full cipher transformation - e.g., AES/CBC/NoPadding - never {@code null}/empty
* @param keyLength The required key length in bits - always positive
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@ public interface CipherInformation extends AlgorithmNameProvider, KeySizeIndicat
*/
int getIVSize();

/**
* @return Size of the authentication tag (AT) in bytes or 0 if this cipher does not support authentication
*/
default int getAuthenticationTagSize() {
return 0;
}

/**
* @return Size of block data used by the cipher (in bytes). For stream ciphers this value is (currently) used to
* indicate some average work buffer size to be used for the automatic re-keying mechanism described in
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ public class BaseBuilder<T extends AbstractFactoryManager, S extends BaseBuilder
BuiltinCiphers.aes128ctr,
BuiltinCiphers.aes192ctr,
BuiltinCiphers.aes256ctr,
BuiltinCiphers.aes128gcm,
BuiltinCiphers.aes256gcm,
BuiltinCiphers.arcfour256,
BuiltinCiphers.arcfour128,
BuiltinCiphers.aes128cbc,
Expand Down
Loading