Skip to content

Commit

Permalink
fix: create Cipher instance in place, do not store it in `ChannelOpti…
Browse files Browse the repository at this point in the history 
…ons`

Cipher instances was cached per `ChannelOptions` that probably increased performance a little, but it was causing `ConcurrentModificationException`. Now we create cipher instance during encryption/decryption process to avoid this
  • Loading branch information
@ttypic
ttypic committed Sep 27, 2023
1 parent a38b026 commit da5406f
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 163 deletions.
7 changes: 5 additions & 2 deletions lib/src/main/java/io/ably/lib/types/BaseMessage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
import com.google.gson.JsonParseException;
import com.google.gson.JsonPrimitive;
import io.ably.lib.util.Base64Coder;
import io.ably.lib.util.Crypto;
import io.ably.lib.util.Crypto.EncryptingChannelCipher;
import io.ably.lib.util.Crypto.DecryptingChannelCipher;
import io.ably.lib.util.Log;
import io.ably.lib.util.Serialisation;
import org.msgpack.core.MessageFormat;
Expand Down Expand Up @@ -145,7 +147,8 @@ public void decode(ChannelOptions opts, DecodingContext context) throws Message
case "cipher":
if(opts != null && opts.encrypted) {
try {
data = opts.getCipherSet().getDecipher().decrypt((byte[]) data);
DecryptingChannelCipher cipher = Crypto.createChannelDecipher(opts);
data = cipher.decrypt((byte[]) data);
} catch(AblyException e) {
throw MessageDecodeException.fromDescription(e.errorInfo.message);
}
Expand Down Expand Up @@ -193,7 +196,7 @@ public void encode(ChannelOptions opts) throws AblyException {
}
}
if (opts != null && opts.encrypted) {
EncryptingChannelCipher cipher = opts.getCipherSet().getEncipher();
EncryptingChannelCipher cipher = Crypto.createChannelEncipher(opts);
data = cipher.encrypt((byte[]) data);
encoding = ((encoding == null) ? "" : encoding + "/") + "cipher+" + cipher.getAlgorithm();
}
Expand Down
56 changes: 0 additions & 56 deletions lib/src/main/java/io/ably/lib/types/ChannelOptions.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,6 @@

import io.ably.lib.util.Base64Coder;
import io.ably.lib.util.Crypto;
import io.ably.lib.util.Crypto.ChannelCipher;
import io.ably.lib.util.Crypto.ChannelCipherSet;

/**
* Passes additional properties to a {@link io.ably.lib.rest.Channel} or {@link io.ably.lib.realtime.Channel} object,
Expand All @@ -27,8 +25,6 @@ public class ChannelOptions {
*/
public ChannelMode[] modes;

private ChannelCipherSet cipherSet;

/**
* Requests encryption for this channel when not null,
* and specifies encryption-related parameters (such as algorithm, chaining mode, key length and key).
Expand Down Expand Up @@ -59,58 +55,6 @@ public int getModeFlags() {
return flags;
}

/**
* Returns a wrapper around the cipher set to be used for this channel. This wrapper is only available in this API
* to support customers who may have been using it in their applications with version 1.2.10 or before.
*
* @deprecated Since version 1.2.11, this method (which was only ever intended for internal use within this library
* has been replaced by {@link #getCipherSet()}. It will be removed in the future.
*/
@Deprecated
public ChannelCipher getCipher() throws AblyException {
return new ChannelCipher() {
@Override
public byte[] encrypt(byte[] plaintext) throws AblyException {
return getCipherSet().getEncipher().encrypt(plaintext);
}

@Override
public byte[] decrypt(byte[] ciphertext) throws AblyException {
return getCipherSet().getDecipher().decrypt(ciphertext);
}

@Override
public String getAlgorithm() {
try {
return getCipherSet().getEncipher().getAlgorithm();
} catch (final AblyException e) {
throw new IllegalStateException("Unexpected exception when using legacy crypto cipher interface.", e);
}
}
};
}

/**
* Internal; this method is not intended for use by application developers. It may be changed or removed in future.
*
* Returns the cipher set to be used for encrypting and decrypting data on a channel, given the current state of
* this instance. On the first call to this method a new cipher set instance is created, with subsequent callers to
* this method being returned that same cipher set instance. This method is safe to be called from any thread.
*
* @apiNote Once this method has been called then the cipher set is fixed based on the value of the
* {@link #cipherParams} field at that time. If that field is then mutated, the cipher set will not be updated.
* This is not great API design and we should fix this under https://github.com/ably/ably-java/issues/745
*/
public synchronized ChannelCipherSet getCipherSet() throws AblyException {
if (!encrypted) {
throw new IllegalStateException("ChannelOptions encrypted field value is false.");
}
if (null == cipherSet) {
cipherSet = Crypto.createChannelCipherSet(cipherParams);
}
return cipherSet;
}

/**
* <b>Deprecated. Use withCipherKey(byte[]) instead.</b><br><br>
* Create ChannelOptions from the given cipher key.
Expand Down
117 changes: 27 additions & 90 deletions lib/src/main/java/io/ably/lib/util/Crypto.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import java.security.SecureRandom;
import java.util.ConcurrentModificationException;
import java.util.Locale;
import java.util.concurrent.Semaphore;

import javax.crypto.BadPaddingException;
import javax.crypto.Cipher;
Expand Down Expand Up @@ -176,22 +175,6 @@ public static byte[] generateRandomKey() {
return generateRandomKey(DEFAULT_KEYLENGTH);
}

/**
* Interface for a ChannelCipher instance that may be associated with a Channel.
*
* The operational methods implemented by channel cipher instances (encrypt and decrypt) are not designed to be
* safe to be called from any thread.
*
* @deprecated Since version 1.2.11, this interface (which was only ever intended for internal use within this
* library) has been replaced by {@link ChannelCipherSet}. It will be removed in the future.
*/
@Deprecated
public interface ChannelCipher {
byte[] encrypt(byte[] plaintext) throws AblyException;
byte[] decrypt(byte[] ciphertext) throws AblyException;
String getAlgorithm();
}

/**
* Internal; a cipher used to encrypt plaintext to ciphertext, for a channel.
*/
Expand Down Expand Up @@ -227,40 +210,27 @@ public interface DecryptingChannelCipher {
}

/**
* Internal; a matching encipher and decipher pair, where both are guaranteed to have been configured with the same
* {@link CipherParams} as each other.
* Internal; get an encrypting cipher instance based on the given channel options.
*/
public interface ChannelCipherSet {
EncryptingChannelCipher getEncipher();
DecryptingChannelCipher getDecipher();
public static EncryptingChannelCipher createChannelEncipher(final Object cipherParams) throws AblyException {
return new EncryptingCBCCipher(checkCipherParams(cipherParams));
}

/**
* Internal; get an encrypting cipher instance based on the given channel options.
* Internal; get a decrypting cipher instance based on the given channel options.
*/
public static ChannelCipherSet createChannelCipherSet(final Object cipherParams) throws AblyException {
final CipherParams nonNullParams;
if (null == cipherParams)
nonNullParams = Crypto.getDefaultParams();
else if (cipherParams instanceof CipherParams)
nonNullParams = (CipherParams)cipherParams;
else
throw AblyException.fromErrorInfo(new ErrorInfo("ChannelOptions not supported", 400, 40000));

return new ChannelCipherSet() {
private final EncryptingChannelCipher encipher = new EncryptingCBCCipher(nonNullParams);
private final DecryptingChannelCipher decipher = new DecryptingCBCCipher(nonNullParams);

@Override
public EncryptingChannelCipher getEncipher() {
return encipher;
}
public static DecryptingChannelCipher createChannelDecipher(final Object cipherParams) throws AblyException {
return new DecryptingCBCCipher(checkCipherParams(cipherParams));
}

@Override
public DecryptingChannelCipher getDecipher() {
return decipher;
}
};
private static CipherParams checkCipherParams(final Object cipherParams) throws AblyException {
if (null == cipherParams) {
return Crypto.getDefaultParams();
} else if (cipherParams instanceof CipherParams) {
return (CipherParams) cipherParams;
} else {
throw AblyException.fromErrorInfo(new ErrorInfo("ChannelOptions not supported", 400, 40000));
}
}

/**
Expand All @@ -277,7 +247,6 @@ private static class CBCCipher {
protected final Cipher cipher;
protected final int blockLength;
protected final String algorithm;
private final Semaphore semaphore = new Semaphore(1);

protected CBCCipher(final CipherParams params) throws AblyException {
final String cipherAlgorithm = params.getAlgorithm();
Expand All @@ -293,28 +262,6 @@ protected CBCCipher(final CipherParams params) throws AblyException {
throw AblyException.fromThrowable(e);
}
}

/**
* Subclasses must call this method before performing any work that uses the {@link #cipher} or otherwise
* mutates the state of this instance.
*
* TODO: under https://github.com/ably/ably-java/issues/747 we can then:
* - remove the need for the {@link #releaseOperationalPermit()} method, and
* - make this method return an AutoCloseable implementation that releases the semaphore.
*/
protected void acquireOperationalPermit() {
if (!semaphore.tryAcquire()) {
throw new ConcurrentModificationException("ChannelCipher instances are not designed to be operated from multiple threads simultaneously.");
}
}

/**
* Subclasses must call this method after performing any work that uses the {@link #cipher} or otherwise
* mutates the state of this instance.
*/
protected void releaseOperationalPermit() {
semaphore.release();
}
}

private static class EncryptingCBCCipher extends CBCCipher implements EncryptingChannelCipher {
Expand Down Expand Up @@ -390,23 +337,17 @@ private byte[] getNextIv() {
public byte[] encrypt(byte[] plaintext) {
if (plaintext == null) return null;

acquireOperationalPermit();
try {
final int plaintextLength = plaintext.length;
final int paddedLength = getPaddedLength(plaintextLength);
final byte[] cipherIn = new byte[paddedLength];
final byte[] ciphertext = new byte[paddedLength + blockLength];
final int padding = paddedLength - plaintextLength;
System.arraycopy(plaintext, 0, cipherIn, 0, plaintextLength);
System.arraycopy(pkcs5Padding[padding], 0, cipherIn, plaintextLength, padding);
System.arraycopy(getNextIv(), 0, ciphertext, 0, blockLength);
final byte[] cipherOut = cipher.update(cipherIn);
System.arraycopy(cipherOut, 0, ciphertext, blockLength, paddedLength);
return ciphertext;
} finally {
// TODO: under https://github.com/ably/ably-java/issues/747 we will remove this call.
releaseOperationalPermit();
}
final int plaintextLength = plaintext.length;
final int paddedLength = getPaddedLength(plaintextLength);
final byte[] cipherIn = new byte[paddedLength];
final byte[] ciphertext = new byte[paddedLength + blockLength];
final int padding = paddedLength - plaintextLength;
System.arraycopy(plaintext, 0, cipherIn, 0, plaintextLength);
System.arraycopy(pkcs5Padding[padding], 0, cipherIn, plaintextLength, padding);
System.arraycopy(getNextIv(), 0, ciphertext, 0, blockLength);
final byte[] cipherOut = cipher.update(cipherIn);
System.arraycopy(cipherOut, 0, ciphertext, blockLength, paddedLength);
return ciphertext;
}
}

Expand All @@ -417,17 +358,13 @@ private static class DecryptingCBCCipher extends CBCCipher implements Decrypting

@Override
public byte[] decrypt(byte[] ciphertext) throws AblyException {
if(ciphertext == null) return null;
if (ciphertext == null) return null;

acquireOperationalPermit();
try {
cipher.init(Cipher.DECRYPT_MODE, keySpec, new IvParameterSpec(ciphertext, 0, blockLength));
return cipher.doFinal(ciphertext, blockLength, ciphertext.length - blockLength);
} catch (InvalidAlgorithmParameterException | IllegalBlockSizeException | BadPaddingException | InvalidKeyException e) {
throw AblyException.fromThrowable(e);
} finally {
// TODO: under https://github.com/ably/ably-java/issues/747 we will remove this call.
releaseOperationalPermit();
}
}
}
Expand Down
17 changes: 10 additions & 7 deletions lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@
import io.ably.lib.types.ClientOptions;
import io.ably.lib.types.ErrorInfo;
import io.ably.lib.util.Crypto;
import io.ably.lib.util.Crypto.ChannelCipherSet;
import io.ably.lib.util.Crypto.EncryptingChannelCipher;
import io.ably.lib.util.Crypto.DecryptingChannelCipher;
import io.ably.lib.util.Crypto.CipherParams;

public class RealtimeCryptoTest extends ParameterizedTest {
Expand Down Expand Up @@ -805,12 +806,13 @@ public void channel_options_with_cipher_key() {
@Test
public void encodeDecodeVariableSizesWithAES256CBC() throws NoSuchAlgorithmException, AblyException {
final CipherParams params = Crypto.getParams("aes", generateNonce(32), generateNonce(16));
final ChannelCipherSet cipherSet = Crypto.createChannelCipherSet(params);
final Crypto.EncryptingChannelCipher encipher = Crypto.createChannelEncipher(params);
final Crypto.DecryptingChannelCipher decipher = Crypto.createChannelDecipher(params);
for (int i=1; i<1000; i++) {
final int size = RANDOM.nextInt(2000) + 1;
final byte[] message = generateNonce(size);
final byte[] encrypted = cipherSet.getEncipher().encrypt(message);
final byte[] decrypted = cipherSet.getDecipher().decrypt(encrypted);
final byte[] encrypted = encipher.encrypt(message);
final byte[] decrypted = decipher.decrypt(encrypted);
try {
assertArrayEquals(message, decrypted);
} catch (final AssertionError e) {
Expand Down Expand Up @@ -1066,12 +1068,13 @@ public void decodeAppleLibrarySequences() throws NoSuchAlgorithmException, AblyE
// We have to create a new ChannelCipher for each message we encode because
// cipher instances only use the IV we've supplied via CipherParams for the
// encryption of the very first message.
final ChannelCipherSet cipherSet = Crypto.createChannelCipherSet(params);
final EncryptingChannelCipher encipher = Crypto.createChannelEncipher(params);
final DecryptingChannelCipher decipher = Crypto.createChannelDecipher(params);

final byte[] appleMessage = hexStringToByteArray(entry.getKey());
final byte[] appleEncrypted = hexStringToByteArray(entry.getValue());
final byte[] encrypted = cipherSet.getEncipher().encrypt(appleMessage);
final byte[] decrypted = cipherSet.getDecipher().decrypt(appleEncrypted);
final byte[] encrypted = encipher.encrypt(appleMessage);
final byte[] decrypted = decipher.decrypt(appleEncrypted);

try {
assertArrayEquals(appleMessage, decrypted);
Expand Down
16 changes: 8 additions & 8 deletions lib/src/test/java/io/ably/lib/util/CryptoTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import com.google.gson.stream.JsonWriter;

import io.ably.lib.types.AblyException;
import io.ably.lib.util.Crypto.ChannelCipherSet;
import io.ably.lib.util.Crypto.CipherParams;
import io.ably.lib.util.Crypto.EncryptingChannelCipher;
import io.ably.lib.util.CryptoMessageTest.FixtureSet;
Expand Down Expand Up @@ -57,10 +56,10 @@ public void cipher_params() throws AblyException, NoSuchAlgorithmException {
);

byte[] plaintext = {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
EncryptingChannelCipher channelCipher1 = Crypto.createChannelCipherSet(params1).getEncipher();
EncryptingChannelCipher channelCipher2 = Crypto.createChannelCipherSet(params2).getEncipher();
EncryptingChannelCipher channelCipher3 = Crypto.createChannelCipherSet(params3).getEncipher();
EncryptingChannelCipher channelCipher4 = Crypto.createChannelCipherSet(params4).getEncipher();
EncryptingChannelCipher channelCipher1 = Crypto.createChannelEncipher(params1);
EncryptingChannelCipher channelCipher2 = Crypto.createChannelEncipher(params2);
EncryptingChannelCipher channelCipher3 = Crypto.createChannelEncipher(params3);
EncryptingChannelCipher channelCipher4 = Crypto.createChannelEncipher(params4);

byte[] ciphertext1 = channelCipher1.encrypt(plaintext);
byte[] ciphertext2 = channelCipher2.encrypt(plaintext);
Expand Down Expand Up @@ -127,18 +126,19 @@ public void encryptAndDecrypt() throws NoSuchAlgorithmException, AblyException,
for (int i=1; i<=maxLength; i++) {
// We need to create a new ChannelCipher for each message we encode,
// so that our IV gets used (being start of CBC chain).
final ChannelCipherSet cipherSet = Crypto.createChannelCipherSet(params);
final EncryptingChannelCipher encipher = Crypto.createChannelEncipher(params);
final Crypto.DecryptingChannelCipher decipher = Crypto.createChannelDecipher(params);

// Encrypt i bytes from the start of the message data.
final byte[] encoded = Arrays.copyOfRange(message, 0, i);
final byte[] encrypted = cipherSet.getEncipher().encrypt(encoded);
final byte[] encrypted = encipher.encrypt(encoded);

// Add encryption result to results in format ready for fixture.
writeResult(writer, "byte 1 to " + i, encoded, encrypted, fixtureSet.cipherName);

// Decrypt the encrypted data and verify the result is the same as what
// we submitted for encryption.
final byte[] verify = cipherSet.getDecipher().decrypt(encrypted);
final byte[] verify = decipher.decrypt(encrypted);
assertArrayEquals(verify, encoded);
}
writer.endArray();
Expand Down

0 comments on commit da5406f

Please sign in to comment.