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 8858c56
Show file tree
Hide file tree
Showing 5 changed files with 53 additions and 116 deletions.
6 changes: 4 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,6 +8,7 @@
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.Log;
import io.ably.lib.util.Serialisation;
Expand Down Expand Up @@ -145,7 +146,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);
Crypto.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 +195,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
52 changes: 0 additions & 52 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 @@ -59,58 +59,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
89 changes: 37 additions & 52 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 @@ -239,13 +239,7 @@ public interface ChannelCipherSet {
* Internal; get an encrypting 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));
final CipherParams nonNullParams = checkCipherParams(cipherParams);

return new ChannelCipherSet() {
private final EncryptingChannelCipher encipher = new EncryptingCBCCipher(nonNullParams);
Expand All @@ -263,6 +257,30 @@ public DecryptingChannelCipher getDecipher() {
};
}

/**
* Internal; get an encrypting cipher instance based on the given channel options.
*/
public static EncryptingCBCCipher createChannelEncipher(final Object cipherParams) throws AblyException {
return new EncryptingCBCCipher(checkCipherParams(cipherParams));
}

/**
* Internal; get a decrypting cipher instance based on the given channel options.
*/
public static DecryptingCBCCipher createChannelDecipher(final Object cipherParams) throws AblyException {
return new DecryptingCBCCipher(checkCipherParams(cipherParams));
}

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));
}
}

/**
* Implements a CBC mode ChannelCipher.
* A single block of secure random data is provided for an initial IV.
Expand All @@ -277,7 +295,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 +310,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 +385,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 +406,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
7 changes: 4 additions & 3 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 @@ -805,12 +805,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
15 changes: 8 additions & 7 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 @@ -57,10 +57,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 +127,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 8858c56

Please sign in to comment.