From 8858c560af20039759ad1176bebd63e94c2bb353 Mon Sep 17 00:00:00 2001 From: evgeny Date: Wed, 27 Sep 2023 16:31:08 +0100 Subject: [PATCH] fix: create Cipher instance in place, do not store it in `ChannelOptions` 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 --- .../java/io/ably/lib/types/BaseMessage.java | 6 +- .../io/ably/lib/types/ChannelOptions.java | 52 ----------- .../main/java/io/ably/lib/util/Crypto.java | 89 ++++++++----------- .../lib/test/realtime/RealtimeCryptoTest.java | 7 +- .../java/io/ably/lib/util/CryptoTest.java | 15 ++-- 5 files changed, 53 insertions(+), 116 deletions(-) diff --git a/lib/src/main/java/io/ably/lib/types/BaseMessage.java b/lib/src/main/java/io/ably/lib/types/BaseMessage.java index c479169e7..e8045216f 100644 --- a/lib/src/main/java/io/ably/lib/types/BaseMessage.java +++ b/lib/src/main/java/io/ably/lib/types/BaseMessage.java @@ -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; @@ -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); } @@ -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(); } diff --git a/lib/src/main/java/io/ably/lib/types/ChannelOptions.java b/lib/src/main/java/io/ably/lib/types/ChannelOptions.java index a0377b705..6bfd0cb83 100644 --- a/lib/src/main/java/io/ably/lib/types/ChannelOptions.java +++ b/lib/src/main/java/io/ably/lib/types/ChannelOptions.java @@ -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; - } - /** * Deprecated. Use withCipherKey(byte[]) instead.

* Create ChannelOptions from the given cipher key. diff --git a/lib/src/main/java/io/ably/lib/util/Crypto.java b/lib/src/main/java/io/ably/lib/util/Crypto.java index 9e99e2433..d9d4798d8 100644 --- a/lib/src/main/java/io/ably/lib/util/Crypto.java +++ b/lib/src/main/java/io/ably/lib/util/Crypto.java @@ -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); @@ -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. @@ -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(); @@ -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 { @@ -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; } } @@ -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(); } } } diff --git a/lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoTest.java b/lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoTest.java index 3863bcefc..75d02ba61 100644 --- a/lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoTest.java +++ b/lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoTest.java @@ -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) { diff --git a/lib/src/test/java/io/ably/lib/util/CryptoTest.java b/lib/src/test/java/io/ably/lib/util/CryptoTest.java index 5aad520a9..063a438a7 100644 --- a/lib/src/test/java/io/ably/lib/util/CryptoTest.java +++ b/lib/src/test/java/io/ably/lib/util/CryptoTest.java @@ -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); @@ -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();