From 7b180ca9ba171c75e06e13aff3612338eeb4a9bb Mon Sep 17 00:00:00 2001 From: Sebastian Stenzel Date: Fri, 20 Aug 2021 09:55:08 +0200 Subject: [PATCH] partial revert of 8139787, fixes #23 --- pom.xml | 2 +- .../cryptomator/cryptolib/api/Masterkey.java | 2 +- .../cryptolib/common/AesKeyWrap.java | 8 ++++---- .../cryptolib/common/CipherSupplier.java | 16 +++++++++------- .../cryptolib/common/DestroyableSecretKey.java | 17 ++++++++++------- .../cryptolib/v1/FileContentCryptorImpl.java | 4 ++-- .../cryptolib/v2/FileContentCryptorImpl.java | 4 ++-- .../common/DestroyableSecretKeyTest.java | 18 +++++++++--------- .../cryptolib/common/MasterkeyTest.java | 10 +++++----- 9 files changed, 43 insertions(+), 38 deletions(-) diff --git a/pom.xml b/pom.xml index c57acf2..3288e18 100644 --- a/pom.xml +++ b/pom.xml @@ -2,7 +2,7 @@ 4.0.0 org.cryptomator cryptolib - 2.0.0 + 2.0.1 Cryptomator Crypto Library This library contains all cryptographic functions that are used by Cryptomator. https://github.com/cryptomator/cryptolib diff --git a/src/main/java/org/cryptomator/cryptolib/api/Masterkey.java b/src/main/java/org/cryptomator/cryptolib/api/Masterkey.java index 4e3d640..c0ec2e5 100644 --- a/src/main/java/org/cryptomator/cryptolib/api/Masterkey.java +++ b/src/main/java/org/cryptomator/cryptolib/api/Masterkey.java @@ -46,7 +46,7 @@ public static Masterkey from(DestroyableSecretKey encKey, DestroyableSecretKey m } @Override - public Masterkey clone() { + public Masterkey copy() { return new Masterkey(getEncoded()); } diff --git a/src/main/java/org/cryptomator/cryptolib/common/AesKeyWrap.java b/src/main/java/org/cryptomator/cryptolib/common/AesKeyWrap.java index 48f6c3b..0e9bcd9 100644 --- a/src/main/java/org/cryptomator/cryptolib/common/AesKeyWrap.java +++ b/src/main/java/org/cryptomator/cryptolib/common/AesKeyWrap.java @@ -22,8 +22,8 @@ public class AesKeyWrap { * @return Wrapped key */ public static byte[] wrap(DestroyableSecretKey kek, SecretKey key) { - try { - final Cipher cipher = CipherSupplier.RFC3394_KEYWRAP.forWrapping(kek); + try (DestroyableSecretKey kekCopy = kek.copy()) { + final Cipher cipher = CipherSupplier.RFC3394_KEYWRAP.forWrapping(kekCopy); return cipher.wrap(key); } catch (InvalidKeyException | IllegalBlockSizeException e) { throw new IllegalArgumentException("Unable to wrap key.", e); @@ -43,8 +43,8 @@ public static DestroyableSecretKey unwrap(DestroyableSecretKey kek, byte[] wrapp // visible for testing static DestroyableSecretKey unwrap(DestroyableSecretKey kek, byte[] wrappedKey, String wrappedKeyAlgorithm, int wrappedKeyType) throws InvalidKeyException { - final Cipher cipher = CipherSupplier.RFC3394_KEYWRAP.forUnwrapping(kek); - try { + try (DestroyableSecretKey kekCopy = kek.copy()) { + final Cipher cipher = CipherSupplier.RFC3394_KEYWRAP.forUnwrapping(kekCopy); return DestroyableSecretKey.from(cipher.unwrap(wrappedKey, wrappedKeyAlgorithm, wrappedKeyType)); } catch (NoSuchAlgorithmException e) { throw new IllegalArgumentException("Invalid algorithm: " + wrappedKeyAlgorithm, e); diff --git a/src/main/java/org/cryptomator/cryptolib/common/CipherSupplier.java b/src/main/java/org/cryptomator/cryptolib/common/CipherSupplier.java index 68eba32..edbc13e 100644 --- a/src/main/java/org/cryptomator/cryptolib/common/CipherSupplier.java +++ b/src/main/java/org/cryptomator/cryptolib/common/CipherSupplier.java @@ -10,10 +10,12 @@ import javax.crypto.Cipher; import javax.crypto.NoSuchPaddingException; +import javax.crypto.SecretKey; import java.security.InvalidAlgorithmParameterException; import java.security.InvalidKeyException; import java.security.NoSuchAlgorithmException; import java.security.spec.AlgorithmParameterSpec; +import java.util.function.Function; public final class CipherSupplier { @@ -41,27 +43,27 @@ protected Cipher initialValue() { } } - public Cipher forEncryption(DestroyableSecretKey key, AlgorithmParameterSpec params) { + public Cipher forEncryption(SecretKey key, AlgorithmParameterSpec params) { return forMode(Cipher.ENCRYPT_MODE, key, params); } - public Cipher forDecryption(DestroyableSecretKey key, AlgorithmParameterSpec params) { + public Cipher forDecryption(SecretKey key, AlgorithmParameterSpec params) { return forMode(Cipher.DECRYPT_MODE, key, params); } - public Cipher forWrapping(DestroyableSecretKey kek) { + public Cipher forWrapping(SecretKey kek) { return forMode(Cipher.WRAP_MODE, kek, null); } - public Cipher forUnwrapping(DestroyableSecretKey kek) { + public Cipher forUnwrapping(SecretKey kek) { return forMode(Cipher.UNWRAP_MODE, kek, null); } // visible for testing - Cipher forMode(int ciphermode, DestroyableSecretKey key, AlgorithmParameterSpec params) { + Cipher forMode(int ciphermode, SecretKey key, AlgorithmParameterSpec params) { final Cipher cipher = threadLocal.get(); - try (DestroyableSecretKey clone = key.clone()) { - cipher.init(ciphermode, clone, params); // use cloned key, as this may destroy key.getEncoded() + try { + cipher.init(ciphermode, key, params); return cipher; } catch (InvalidKeyException e) { throw new IllegalArgumentException("Invalid key.", e); diff --git a/src/main/java/org/cryptomator/cryptolib/common/DestroyableSecretKey.java b/src/main/java/org/cryptomator/cryptolib/common/DestroyableSecretKey.java index 1527c3b..18c1800 100644 --- a/src/main/java/org/cryptomator/cryptolib/common/DestroyableSecretKey.java +++ b/src/main/java/org/cryptomator/cryptolib/common/DestroyableSecretKey.java @@ -15,19 +15,19 @@ * actually implements {@link Destroyable}. *

* Furthermore, this implementation will not create copies when accessing {@link #getEncoded()}. - * Instead it implements {@link AutoCloseable} and {@link Cloneable} in an exception-free manner. To prevent mutation of the exposed key, + * Instead it implements {@link #copy} and {@link AutoCloseable} in an exception-free manner. To prevent mutation of the exposed key, * you would want to make sure to always work on scoped copies, such as in this example: * * 

- *     // clone "key" to protect it from unwanted modifications:
- *     try (DestroyableSecretKey k = key.clone()) {
+ *     // copy "key" to protect it from unwanted modifications:
+ *     try (DestroyableSecretKey k = key.copy()) {
  *         // use "k":
  *         Cipher cipher = Cipher.init(k, ...)
  *         cipher.doFinal(...)
  *     } // "k" will get destroyed here
  *
*/ -public class DestroyableSecretKey implements SecretKey, AutoCloseable, Cloneable { +public class DestroyableSecretKey implements SecretKey, AutoCloseable { private transient final byte[] key; private final String algorithm; @@ -109,7 +109,7 @@ public String getFormat() { * Returns the raw key bytes this instance wraps. *

* Important: Any change to the returned array will reflect in this key. Make sure to - * {@link #clone() make a local copy} if you can't rule out mutations. + * {@link #copy() make a local copy} if you can't rule out mutations. * * @return A byte array holding the secret key */ @@ -119,8 +119,11 @@ public byte[] getEncoded() { return key; } - @Override - public DestroyableSecretKey clone() { + /** + * Returns an independent copy of this key + * @return New copy of this + */ + public DestroyableSecretKey copy() { Preconditions.checkState(!destroyed, "Key has been destroyed"); return new DestroyableSecretKey(key, algorithm); // key will get copied by the constructor as per contract } diff --git a/src/main/java/org/cryptomator/cryptolib/v1/FileContentCryptorImpl.java b/src/main/java/org/cryptomator/cryptolib/v1/FileContentCryptorImpl.java index 0a68b32..cad0d0b 100644 --- a/src/main/java/org/cryptomator/cryptolib/v1/FileContentCryptorImpl.java +++ b/src/main/java/org/cryptomator/cryptolib/v1/FileContentCryptorImpl.java @@ -103,7 +103,7 @@ public void decryptChunk(ByteBuffer ciphertextChunk, ByteBuffer cleartextChunk, // visible for testing void encryptChunk(ByteBuffer cleartextChunk, ByteBuffer ciphertextChunk, long chunkNumber, byte[] headerNonce, DestroyableSecretKey fileKey) { - try (DestroyableSecretKey fk = fileKey.clone()) { + try (DestroyableSecretKey fk = fileKey.copy()) { // nonce: byte[] nonce = new byte[NONCE_SIZE]; random.nextBytes(nonce); @@ -131,7 +131,7 @@ void encryptChunk(ByteBuffer cleartextChunk, ByteBuffer ciphertextChunk, long ch void decryptChunk(ByteBuffer ciphertextChunk, ByteBuffer cleartextChunk, DestroyableSecretKey fileKey) { assert ciphertextChunk.remaining() >= NONCE_SIZE + MAC_SIZE; - try (DestroyableSecretKey fk = fileKey.clone()) { + try (DestroyableSecretKey fk = fileKey.copy()) { // nonce: final byte[] nonce = new byte[NONCE_SIZE]; final ByteBuffer chunkNonceBuf = ciphertextChunk.asReadOnlyBuffer(); diff --git a/src/main/java/org/cryptomator/cryptolib/v2/FileContentCryptorImpl.java b/src/main/java/org/cryptomator/cryptolib/v2/FileContentCryptorImpl.java index 25ec9c4..a03eea9 100644 --- a/src/main/java/org/cryptomator/cryptolib/v2/FileContentCryptorImpl.java +++ b/src/main/java/org/cryptomator/cryptolib/v2/FileContentCryptorImpl.java @@ -97,7 +97,7 @@ public void decryptChunk(ByteBuffer ciphertextChunk, ByteBuffer cleartextChunk, // visible for testing void encryptChunk(ByteBuffer cleartextChunk, ByteBuffer ciphertextChunk, long chunkNumber, byte[] headerNonce, DestroyableSecretKey fileKey) { - try (DestroyableSecretKey fk = fileKey.clone()) { + try (DestroyableSecretKey fk = fileKey.copy()) { // nonce: byte[] nonce = new byte[GCM_NONCE_SIZE]; random.nextBytes(nonce); @@ -121,7 +121,7 @@ void encryptChunk(ByteBuffer cleartextChunk, ByteBuffer ciphertextChunk, long ch void decryptChunk(ByteBuffer ciphertextChunk, ByteBuffer cleartextChunk, long chunkNumber, byte[] headerNonce, DestroyableSecretKey fileKey) throws AuthenticationFailedException { assert ciphertextChunk.remaining() >= GCM_NONCE_SIZE + GCM_TAG_SIZE; - try (DestroyableSecretKey fk = fileKey.clone()) { + try (DestroyableSecretKey fk = fileKey.copy()) { // nonce: final byte[] nonce = new byte[GCM_NONCE_SIZE]; final ByteBuffer chunkNonceBuf = ciphertextChunk.asReadOnlyBuffer(); diff --git a/src/test/java/org/cryptomator/cryptolib/common/DestroyableSecretKeyTest.java b/src/test/java/org/cryptomator/cryptolib/common/DestroyableSecretKeyTest.java index a1f099a..acbcf82 100644 --- a/src/test/java/org/cryptomator/cryptolib/common/DestroyableSecretKeyTest.java +++ b/src/test/java/org/cryptomator/cryptolib/common/DestroyableSecretKeyTest.java @@ -74,7 +74,7 @@ public void testConstructorCreatesLocalCopy() { } @Test - public void testConstructorClonesKey() { + public void testConstructorCopiesKey() { byte[] empty = new byte[32]; byte[] rawKey = new byte[32]; new Random(42).nextBytes(rawKey); @@ -135,12 +135,12 @@ public void testGetEncoded() { } @Test - @DisplayName("clone() returns equal copy") - public void testClone() { - DestroyableSecretKey clone = key.clone(); + @DisplayName("copy() returns equal copy") + public void testCopy() { + DestroyableSecretKey copy = key.copy(); - Assertions.assertEquals(key, clone); - Assertions.assertNotSame(key, clone); + Assertions.assertEquals(key, copy); + Assertions.assertNotSame(key, copy); } @Test @@ -194,9 +194,9 @@ public void testGetEncoded() { } @Test - @DisplayName("clone() throws IllegalStateException") - public void testClone() { - Assertions.assertThrows(IllegalStateException.class, key::clone); + @DisplayName("copy() throws IllegalStateException") + public void testCopy() { + Assertions.assertThrows(IllegalStateException.class, key::copy); } } diff --git a/src/test/java/org/cryptomator/cryptolib/common/MasterkeyTest.java b/src/test/java/org/cryptomator/cryptolib/common/MasterkeyTest.java index e865fca..4b0a2a0 100644 --- a/src/test/java/org/cryptomator/cryptolib/common/MasterkeyTest.java +++ b/src/test/java/org/cryptomator/cryptolib/common/MasterkeyTest.java @@ -67,16 +67,16 @@ public void testGetMacKey() { } @Test - public void testClone() { + public void testCopy() { byte[] raw = new byte[64]; Arrays.fill(raw, (byte) 0x55); Masterkey original = new Masterkey(raw); - Masterkey clone = original.clone(); + Masterkey copy = original.copy(); - Assertions.assertEquals(original, clone); - clone.destroy(); - Assertions.assertNotEquals(original, clone); + Assertions.assertEquals(original, copy); + copy.destroy(); + Assertions.assertNotEquals(original, copy); } } \ No newline at end of file