Fixes #20 by protecting DestroyableSecretKey from unwanted mutations

`CipherSupplier.forMode()` will now work on a local copy of the key
cryptomator · Jun 24, 2021 · 8139787 · 8139787
1 parent d32856f
commit 8139787
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 36 deletions.
diff --git a/src/main/java/org/cryptomator/cryptolib/api/Masterkey.java b/src/main/java/org/cryptomator/cryptolib/api/Masterkey.java
@@ -32,6 +32,19 @@ public static Masterkey generate(SecureRandom csprng) {
 		}
 	}
 
+	public static Masterkey from(DestroyableSecretKey encKey, DestroyableSecretKey macKey) {
+		Preconditions.checkArgument(encKey.getEncoded().length == SUBKEY_LEN_BYTES, "Invalid key length of encKey");
+		Preconditions.checkArgument(macKey.getEncoded().length == SUBKEY_LEN_BYTES, "Invalid key length of macKey");
+		byte[] key = new byte[SUBKEY_LEN_BYTES + SUBKEY_LEN_BYTES];
+		try {
+			System.arraycopy(encKey.getEncoded(), 0, key, 0, SUBKEY_LEN_BYTES);
+			System.arraycopy(macKey.getEncoded(), 0, key, SUBKEY_LEN_BYTES, SUBKEY_LEN_BYTES);
+			return new Masterkey(key);
+		} finally {
+			Arrays.fill(key, (byte) 0x00);
+		}
+	}
+
 	@Override
 	public Masterkey clone() {
 		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
@@ -8,12 +8,11 @@
  *******************************************************************************/
 package org.cryptomator.cryptolib.common;
 
-import java.security.InvalidKeyException;
-import java.security.NoSuchAlgorithmException;
-
 import javax.crypto.Cipher;
 import javax.crypto.IllegalBlockSizeException;
 import javax.crypto.SecretKey;
+import java.security.InvalidKeyException;
+import java.security.NoSuchAlgorithmException;
 
 public class AesKeyWrap {
 
@@ -22,7 +21,7 @@ public class AesKeyWrap {
 	 * @param key Key to be wrapped
 	 * @return Wrapped key
 	 */
-	public static byte[] wrap(SecretKey kek, SecretKey key) {
+	public static byte[] wrap(DestroyableSecretKey kek, SecretKey key) {
 		try {
 			final Cipher cipher = CipherSupplier.RFC3394_KEYWRAP.forWrapping(kek);
 			return cipher.wrap(key);
@@ -38,15 +37,15 @@ public static byte[] wrap(SecretKey kek, SecretKey key) {
 	 * @return Unwrapped key
 	 * @throws InvalidKeyException If unwrapping failed (i.e. wrong kek)
 	 */
-	public static SecretKey unwrap(SecretKey kek, byte[] wrappedKey, String wrappedKeyAlgorithm) throws InvalidKeyException {
+	public static DestroyableSecretKey unwrap(DestroyableSecretKey kek, byte[] wrappedKey, String wrappedKeyAlgorithm) throws InvalidKeyException {
 		return unwrap(kek, wrappedKey, wrappedKeyAlgorithm, Cipher.SECRET_KEY);
 	}
 
 	// visible for testing
-	static SecretKey unwrap(SecretKey kek, byte[] wrappedKey, String wrappedKeyAlgorithm, int wrappedKeyType) throws InvalidKeyException {
+	static DestroyableSecretKey unwrap(DestroyableSecretKey kek, byte[] wrappedKey, String wrappedKeyAlgorithm, int wrappedKeyType) throws InvalidKeyException {
 		final Cipher cipher = CipherSupplier.RFC3394_KEYWRAP.forUnwrapping(kek);
 		try {
-			return (SecretKey) cipher.unwrap(wrappedKey, wrappedKeyAlgorithm, wrappedKeyType);
+			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
@@ -8,15 +8,13 @@
  *******************************************************************************/
 package org.cryptomator.cryptolib.common;
 
+import javax.crypto.Cipher;
+import javax.crypto.NoSuchPaddingException;
 import java.security.InvalidAlgorithmParameterException;
 import java.security.InvalidKeyException;
 import java.security.NoSuchAlgorithmException;
 import java.security.spec.AlgorithmParameterSpec;
 
-import javax.crypto.Cipher;
-import javax.crypto.NoSuchPaddingException;
-import javax.crypto.SecretKey;
-
 public final class CipherSupplier {
 
 	public static final CipherSupplier AES_CTR = new CipherSupplier("AES/CTR/NoPadding");
@@ -43,27 +41,27 @@ protected Cipher initialValue() {
 		}
 	}
 
-	public Cipher forEncryption(SecretKey key, AlgorithmParameterSpec params) {
+	public Cipher forEncryption(DestroyableSecretKey key, AlgorithmParameterSpec params) {
 		return forMode(Cipher.ENCRYPT_MODE, key, params);
 	}
 
-	public Cipher forDecryption(SecretKey key, AlgorithmParameterSpec params) {
+	public Cipher forDecryption(DestroyableSecretKey key, AlgorithmParameterSpec params) {
 		return forMode(Cipher.DECRYPT_MODE, key, params);
 	}
 
-	public Cipher forWrapping(SecretKey kek) {
+	public Cipher forWrapping(DestroyableSecretKey kek) {
 		return forMode(Cipher.WRAP_MODE, kek, null);
 	}
 
-	public Cipher forUnwrapping(SecretKey kek) {
+	public Cipher forUnwrapping(DestroyableSecretKey kek) {
 		return forMode(Cipher.UNWRAP_MODE, kek, null);
 	}
 
 	// visible for testing
-	Cipher forMode(int ciphermode, SecretKey key, AlgorithmParameterSpec params) {
+	Cipher forMode(int ciphermode, DestroyableSecretKey key, AlgorithmParameterSpec params) {
 		final Cipher cipher = threadLocal.get();
-		try {
-			cipher.init(ciphermode, key, params);
+		try (DestroyableSecretKey clone = key.clone()) {
+			cipher.init(ciphermode, clone, params); // use cloned key, as this may destroy key.getEncoded()
 			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
@@ -4,6 +4,7 @@
 
 import javax.crypto.SecretKey;
 import javax.security.auth.Destroyable;
+import java.security.Key;
 import java.security.MessageDigest;
 import java.security.SecureRandom;
 import java.util.Arrays;
@@ -66,7 +67,7 @@ public DestroyableSecretKey(byte[] key, int offset, int len, String algorithm) {
 	 * @param secretKey The secret key
 	 * @return Either the provided or a new key, depending on whether the provided key is already a DestroyableSecretKey
 	 */
-	public static DestroyableSecretKey from(SecretKey secretKey) {
+	public static DestroyableSecretKey from(Key secretKey) {
 		if (secretKey instanceof DestroyableSecretKey) {
 			return (DestroyableSecretKey) secretKey;
 		} else {

diff --git a/src/main/java/org/cryptomator/cryptolib/common/MasterkeyFileAccess.java b/src/main/java/org/cryptomator/cryptolib/common/MasterkeyFileAccess.java
@@ -139,20 +139,12 @@ Masterkey unlock(MasterkeyFile parsedFile, CharSequence passphrase) throws Inval
 		Preconditions.checkArgument(parsedFile.isValid(), "Invalid masterkey file");
 		Preconditions.checkNotNull(passphrase);
 
-		byte[] encKey = new byte[0], macKey = new byte[0], combined = new byte[0];
-		try (DestroyableSecretKey kek = scrypt(passphrase, parsedFile.scryptSalt, pepper, parsedFile.scryptCostParam, parsedFile.scryptBlockSize)) {
-			encKey = AesKeyWrap.unwrap(kek, parsedFile.encMasterKey, Masterkey.ENC_ALG).getEncoded();
-			macKey = AesKeyWrap.unwrap(kek, parsedFile.macMasterKey, Masterkey.MAC_ALG).getEncoded();
-			combined = new byte[encKey.length + macKey.length];
-			System.arraycopy(encKey, 0, combined, 0, encKey.length);
-			System.arraycopy(macKey, 0, combined, encKey.length, macKey.length);
-			return new Masterkey(combined);
+		try (DestroyableSecretKey kek = scrypt(passphrase, parsedFile.scryptSalt, pepper, parsedFile.scryptCostParam, parsedFile.scryptBlockSize);
+			 DestroyableSecretKey encKey = AesKeyWrap.unwrap(kek, parsedFile.encMasterKey, Masterkey.ENC_ALG);
+			 DestroyableSecretKey macKey = AesKeyWrap.unwrap(kek, parsedFile.macMasterKey, Masterkey.MAC_ALG)) {
+			return Masterkey.from(encKey, macKey);
 		} catch (InvalidKeyException e) {
 			throw new InvalidPassphraseException();
-		} finally {
-			Arrays.fill(encKey, (byte) 0x00);
-			Arrays.fill(macKey, (byte) 0x00);
-			Arrays.fill(combined, (byte) 0x00);
 		}
 	}
 

diff --git a/src/test/java/org/cryptomator/cryptolib/common/AesKeyWrapTest.java b/src/test/java/org/cryptomator/cryptolib/common/AesKeyWrapTest.java
@@ -20,7 +20,7 @@ public class AesKeyWrapTest {
 
 	@Test
 	public void wrapAndUnwrap() throws InvalidKeyException {
-		SecretKey kek = new SecretKeySpec(new byte[32], "AES");
+		DestroyableSecretKey kek = new DestroyableSecretKey(new byte[32], "AES");
 		SecretKey key = new SecretKeySpec(new byte[32], "AES");
 		byte[] wrapped = AesKeyWrap.wrap(kek, key);
 		SecretKey unwrapped = AesKeyWrap.unwrap(kek, wrapped, "AES");
@@ -29,7 +29,7 @@ public void wrapAndUnwrap() throws InvalidKeyException {
 
 	@Test
 	public void wrapWithInvalidKey() {
-		SecretKey kek = new SecretKeySpec(new byte[32], "AES");
+		DestroyableSecretKey kek = new DestroyableSecretKey(new byte[32], "AES");
 		SecretKey key = new SecretKeySpec(new byte[17], "AES");
 		Assertions.assertThrows(IllegalArgumentException.class, () -> {
 			AesKeyWrap.wrap(kek, key);
@@ -38,7 +38,7 @@ public void wrapWithInvalidKey() {
 
 	@Test
 	public void unwrapWithInvalidKey() {
-		SecretKey kek = new SecretKeySpec(new byte[32], "AES");
+		DestroyableSecretKey kek = new DestroyableSecretKey(new byte[32], "AES");
 		SecretKey key = new SecretKeySpec(new byte[32], "AES");
 		byte[] wrapped = AesKeyWrap.wrap(kek, key);
 		Assertions.assertThrows(IllegalArgumentException.class, () -> {

diff --git a/src/test/java/org/cryptomator/cryptolib/common/CipherSupplierTest.java b/src/test/java/org/cryptomator/cryptolib/common/CipherSupplierTest.java
@@ -31,7 +31,7 @@ public void testGetUnknownCipher() {
 	public void testGetCipherWithInvalidKey() {
 		CipherSupplier supplier = new CipherSupplier("AES/CBC/PKCS5Padding");
 		IllegalArgumentException exception = Assertions.assertThrows(IllegalArgumentException.class, () -> {
-			supplier.forMode(Cipher.ENCRYPT_MODE, new SecretKeySpec(new byte[13], "AES"), new IvParameterSpec(new byte[16]));
+			supplier.forMode(Cipher.ENCRYPT_MODE, new DestroyableSecretKey(new byte[13], "AES"), new IvParameterSpec(new byte[16]));
 		});
 		MatcherAssert.assertThat(exception.getMessage(), CoreMatchers.containsString("Invalid key"));
 	}
@@ -40,7 +40,7 @@ public void testGetCipherWithInvalidKey() {
 	public void testGetCipherWithInvalidAlgorithmParam() {
 		CipherSupplier supplier = new CipherSupplier("AES/CBC/PKCS5Padding");
 		IllegalArgumentException exception = Assertions.assertThrows(IllegalArgumentException.class, () -> {
-			supplier.forMode(Cipher.ENCRYPT_MODE, new SecretKeySpec(new byte[16], "AES"), new RC5ParameterSpec(1, 1, 8));
+			supplier.forMode(Cipher.ENCRYPT_MODE, new DestroyableSecretKey(new byte[16], "AES"), new RC5ParameterSpec(1, 1, 8));
 		});
 		MatcherAssert.assertThat(exception.getMessage(), CoreMatchers.containsString("Algorithm parameter not appropriate for"));
 	}

diff --git a/src/test/java/org/cryptomator/cryptolib/common/MasterkeyTest.java b/src/test/java/org/cryptomator/cryptolib/common/MasterkeyTest.java
@@ -34,6 +34,24 @@ public void testGenerate() {
 		Assertions.assertNotNull(masterkey);
 	}
 
+	@Test
+	public void testFrom() {
+		byte[] encKeyBytes = new byte[32];
+		byte[] macKeyBytes = new byte[32];
+		Arrays.fill(encKeyBytes, (byte) 0x55);
+		Arrays.fill(macKeyBytes, (byte) 0x77);
+		DestroyableSecretKey encKey = Mockito.mock(DestroyableSecretKey.class);
+		DestroyableSecretKey macKey = Mockito.mock(DestroyableSecretKey.class);
+		Mockito.when(encKey.getEncoded()).thenReturn(encKeyBytes);
+		Mockito.when(macKey.getEncoded()).thenReturn(macKeyBytes);
+
+		Masterkey masterkey = Masterkey.from(encKey, macKey);
+
+		Assertions.assertNotNull(masterkey);
+		Assertions.assertArrayEquals(encKeyBytes, masterkey.getEncKey().getEncoded());
+		Assertions.assertArrayEquals(macKeyBytes, masterkey.getMacKey().getEncoded());
+	}
+
 	@Test
 	public void testGetEncKey() {
 		SecretKey encKey = masterkey.getEncKey();