Replace CharBuffer and char[] instances by destroyable Passphrase

src/main/java/org/cryptomator/common/Passphrase.java

@@ -0,0 +1,115 @@
+package org.cryptomator.common;
+
+import org.cryptomator.cryptolib.common.MessageDigestSupplier;
+
+import javax.security.auth.Destroyable;
+import java.nio.ByteBuffer;
+import java.util.Arrays;
+
+/**
+ * A destroyable CharSequence.
+ */
+public class Passphrase implements Destroyable, CharSequence {
+
+	private final char[] data;
+	private final int offset;
+	private final int length;
+	private boolean destroyed;
+
+	/**
+	 * Wraps (doesn't copy) the given data.
+	 *
+	 * @param data The wrapped data. Any changes to this will be reflected in this passphrase
+	 */
+	public Passphrase(char[] data) {
+		this(data, 0, data.length);
+	}
+
+	/**
+	 * Wraps (doesn't copy) a subarray of the given data.
+	 *
+	 * @param data The wrapped data. Any changes to this will be reflected in this passphrase
+	 * @param offset The subarray offset, i.e. the first character of this passphrase
+	 * @param length The subarray length, i.e. the length of this passphrase
+	 */
+	public Passphrase(char[] data, int offset, int length) {
+		if (offset < 0 || length < 0 || offset + length > data.length) {
+			throw new IndexOutOfBoundsException("[%1$d %1$d + %2$d[ not within [0, %3$d[".formatted(offset, length, data.length));
+		}
+		this.data = data;
+		this.offset = offset;
+		this.length = length;
+	}
+
+	public static Passphrase copyOf(CharSequence cs) {
+		char[] result = new char[cs.length()];
+		for (int i = 0; i < cs.length(); i++) {
+			result[i] = cs.charAt(i);
+		}
+		return new Passphrase(result);
+	}
+
+	@Override
+	public boolean equals(Object o) {
+		if (this == o) return true;
+		if (o == null || getClass() != o.getClass()) return false;
+		Passphrase that = (Passphrase) o;
+		// time-constant comparison
+		int diff = 0;
+		for (int i = 0; i < length; i++) {
+			diff |= charAt(i) ^ that.charAt(i);
+		}
+		return diff == 0;
+	}
+
+	@Override
+	public int hashCode() {
+		// TODO: do we really need to a secure hashcode? toString leaks the pw anyway
+		var md = MessageDigestSupplier.SHA256.get();
+		ByteBuffer buf = ByteBuffer.allocate(Character.BYTES * length);
+		for (int i = 0; i < length; i++) {
+			char c = charAt(i);
+			buf.putChar(i * Character.BYTES, c);
+		}
+		buf.flip();
+		md.update(buf);
+		return Arrays.hashCode(md.digest());
+	}
+
+	@Override
+	public String toString() {
+		return new String(data, offset, length);
+	}
+
+	@Override
+	public int length() {
+		return length;
+	}
+
+	@Override
+	public char charAt(int index) {
+		if (index < 0 || index >= length) {
+			throw new IndexOutOfBoundsException("%d not within [0, %d[".formatted(index, length));
+		}
+		return data[offset + index];
+	}
+
+	@Override
+	public Passphrase subSequence(int start, int end) {
+		if (start < 0 || end < 0 || end > length || start > end) {
+			throw new IndexOutOfBoundsException("[%d, %d[ not within [0, %d[".formatted(start, end, length));
+		}
+		return new Passphrase(Arrays.copyOfRange(data, offset + start, offset + end));
+	}
+
+	@Override
+	public boolean isDestroyed() {
+		return destroyed;
+	}
+
+	@Override
+	public void destroy() {
+		Arrays.fill(data, offset, offset + length, '\0');
+		destroyed = true;
+	}
+}

src/main/java/org/cryptomator/ui/controls/NiceSecurePasswordField.java

@@ -1,5 +1,7 @@
 package org.cryptomator.ui.controls;
 
+import org.cryptomator.common.Passphrase;
+
 import javafx.beans.Observable;
 import javafx.beans.binding.Bindings;
 import javafx.beans.property.StringProperty;
@@ -82,7 +84,7 @@ public class NiceSecurePasswordField extends StackPane {
 		return passwordField.textProperty();
 	}
 
-	public CharSequence getCharacters() {
+	public Passphrase getCharacters() {
 		return passwordField.getCharacters();
 	}
 

src/main/java/org/cryptomator/ui/controls/SecurePasswordField.java

@@ -9,6 +9,7 @@
 package org.cryptomator.ui.controls;
 
 import com.google.common.base.Strings;
+import org.cryptomator.common.Passphrase;
 
 import javafx.application.Platform;
 import javafx.beans.NamedArg;
@@ -28,7 +29,6 @@ import javafx.scene.input.KeyCodeCombination;
 import javafx.scene.input.KeyCombination;
 import javafx.scene.input.KeyEvent;
 import javafx.scene.input.TransferMode;
-import java.nio.CharBuffer;
 import java.text.Normalizer;
 import java.text.Normalizer.Form;
 import java.util.Arrays;
@@ -203,8 +203,8 @@ public class SecurePasswordField extends TextField {
 	 * @see #wipe()
 	 */
 	@Override
-	public CharSequence getCharacters() {
-		return CharBuffer.wrap(content, 0, length);
+	public Passphrase getCharacters() {
+		return new Passphrase(content, 0, length);
 	}
 
 	/**

src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java

@@ -13,6 +13,7 @@ import org.cryptomator.integrations.keychain.KeychainAccessException;
 import org.cryptomator.ui.common.Animations;
 import org.cryptomator.ui.common.FxmlFile;
 import org.cryptomator.ui.common.FxmlScene;
+import org.cryptomator.common.Passphrase;
 import org.cryptomator.ui.common.UserInteractionLock;
 import org.cryptomator.ui.keyloading.KeyLoading;
 import org.cryptomator.ui.keyloading.KeyLoadingStrategy;
@@ -26,10 +27,8 @@ import javafx.stage.Stage;
 import javafx.stage.Window;
 import java.io.IOException;
 import java.net.URI;
-import java.nio.CharBuffer;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.Arrays;
 import java.util.Optional;
 import java.util.concurrent.CancellationException;
 import java.util.concurrent.ExecutionException;
@@ -49,7 +48,7 @@ public class MasterkeyFileLoadingStrategy implements KeyLoadingStrategy {
 	private final AtomicReference<Path> filePath;
 	private final KeychainManager keychain;
 
-	private char[] passphrase;
+	private Passphrase passphrase;
 	private boolean savePassphrase;
 	private boolean wrongPassphrase;
 
@@ -63,7 +62,7 @@ public class MasterkeyFileLoadingStrategy implements KeyLoadingStrategy {
 		this.masterkeyFileProvisionLock = masterkeyFileProvisionLock;
 		this.filePath = filePath;
 		this.keychain = keychain;
-		this.passphrase = savedPassphrase.orElse(null);
+		this.passphrase = savedPassphrase.map(Passphrase::new).orElse(null);
 		this.savePassphrase = savedPassphrase.isPresent();
 	}
 
@@ -78,7 +77,7 @@ public class MasterkeyFileLoadingStrategy implements KeyLoadingStrategy {
 			if (passphrase == null) {
 				askForPassphrase();
 			}
-			var masterkey = masterkeyFileAccess.load(filePath, CharBuffer.wrap(passphrase));
+			var masterkey = masterkeyFileAccess.load(filePath, passphrase);
 			//backup
 			if (filePath.startsWith(vault.getPath())) {
 				try {
@@ -100,7 +99,7 @@ public class MasterkeyFileLoadingStrategy implements KeyLoadingStrategy {
 	public boolean recoverFromException(MasterkeyLoadingFailedException exception) {
 		if (exception instanceof InvalidPassphraseException) {
 			this.wrongPassphrase = true;
-			Arrays.fill(passphrase, '\0');
+			passphrase.destroy();
 			this.passphrase = null;
 			return true; // reattempting key load
 		} else {
@@ -113,13 +112,15 @@ public class MasterkeyFileLoadingStrategy implements KeyLoadingStrategy {
 		if (unlockedSuccessfully && savePassphrase) {
 			savePasswordToSystemkeychain(passphrase);
 		}
-		Arrays.fill(passphrase, '\0');
+		if (passphrase != null) {
+			passphrase.destroy();
+		}
 	}
 
-	private void savePasswordToSystemkeychain(char[] passphrase) {
+	private void savePasswordToSystemkeychain(Passphrase passphrase) {
 		if (keychain.isSupported()) {
 			try {
-				keychain.storePassphrase(vault.getId(), vault.getDisplayName(), CharBuffer.wrap(passphrase));
+				keychain.storePassphrase(vault.getId(), vault.getDisplayName(), passphrase);
 			} catch (KeychainAccessException e) {
 				LOG.error("Failed to store passphrase in system keychain.", e);
 			}

src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryComponent.java

@@ -3,15 +3,11 @@ package org.cryptomator.ui.keyloading.masterkeyfile;
 import dagger.BindsInstance;
 import dagger.Subcomponent;
 import org.cryptomator.common.Nullable;
-import org.cryptomator.ui.common.Animations;
+import org.cryptomator.common.Passphrase;
 
 import javax.inject.Named;
 import javafx.scene.Scene;
-import javafx.stage.Stage;
-import javafx.stage.Window;
-import java.util.concurrent.CancellationException;
 import java.util.concurrent.CompletableFuture;
-import java.util.concurrent.ExecutionException;
 
 @PassphraseEntryScoped
 @Subcomponent(modules = {PassphraseEntryModule.class})
@@ -27,7 +23,7 @@ public interface PassphraseEntryComponent {
 	interface Builder {
 
 		@BindsInstance
-		PassphraseEntryComponent.Builder savedPassword(@Nullable @Named("savedPassword") char[] savedPassword);
+		PassphraseEntryComponent.Builder savedPassword(@Nullable @Named("savedPassword") Passphrase savedPassword);
 
 		PassphraseEntryComponent build();
 	}

src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryController.java

@@ -4,6 +4,7 @@ import org.cryptomator.common.Nullable;
 import org.cryptomator.common.keychain.KeychainManager;
 import org.cryptomator.common.vaults.Vault;
 import org.cryptomator.ui.common.FxController;
+import org.cryptomator.common.Passphrase;
 import org.cryptomator.ui.common.WeakBindings;
 import org.cryptomator.ui.controls.NiceSecurePasswordField;
 import org.cryptomator.ui.forgetPassword.ForgetPasswordComponent;
@@ -44,7 +45,7 @@ public class PassphraseEntryController implements FxController {
 	private final Stage window;
 	private final Vault vault;
 	private final CompletableFuture<PassphraseEntryResult> result;
-	private final char[] savedPassword;
+	private final Passphrase savedPassword;
 	private final ForgetPasswordComponent.Builder forgetPassword;
 	private final KeychainManager keychain;
 	private final StringBinding vaultName;
@@ -63,7 +64,7 @@ public class PassphraseEntryController implements FxController {
 	public Animation unlockAnimation;
 
 	@Inject
-	public PassphraseEntryController(@KeyLoading Stage window, @KeyLoading Vault vault, CompletableFuture<PassphraseEntryResult> result, @Nullable @Named("savedPassword") char[] savedPassword, ForgetPasswordComponent.Builder forgetPassword, KeychainManager keychain) {
+	public PassphraseEntryController(@KeyLoading Stage window, @KeyLoading Vault vault, CompletableFuture<PassphraseEntryResult> result, @Nullable @Named("savedPassword") Passphrase savedPassword, ForgetPasswordComponent.Builder forgetPassword, KeychainManager keychain) {
 		this.window = window;
 		this.vault = vault;
 		this.result = result;
@@ -137,10 +138,7 @@ public class PassphraseEntryController implements FxController {
 		LOG.trace("UnlockController.unlock()");
 		unlockInProgress.set(true);
 		CharSequence pwFieldContents = passwordField.getCharacters();
-		char[] pw = new char[pwFieldContents.length()];
-		for (int i = 0; i < pwFieldContents.length(); i++) {
-			pw[i] = pwFieldContents.charAt(i);
-		}
+		Passphrase pw = Passphrase.copyOf(pwFieldContents);
 		result.complete(new PassphraseEntryResult(pw, savePasswordCheckbox.isSelected()));
 		startUnlockAnimation();
 	}

src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryResult.java

@@ -1,6 +1,8 @@
 package org.cryptomator.ui.keyloading.masterkeyfile;
 
+import org.cryptomator.common.Passphrase;
+
 // TODO needs to be public due to Dagger -.-
-public record PassphraseEntryResult(char[] passphrase, boolean savePassphrase) {
+public record PassphraseEntryResult(Passphrase passphrase, boolean savePassphrase) {
 
 }

src/test/java/org/cryptomator/common/PassphraseTest.java

@@ -0,0 +1,130 @@
+package org.cryptomator.common;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+class PassphraseTest {
+
+	@ParameterizedTest
+	@CsvSource(value = {
+			"-1, 0",
+			"0, -1",
+			"0, 10",
+			"10, 0",
+			"10, 10"
+	})
+	public void testInvalidConstructorArgs(int offset, int length) {
+		char[] data = "test".toCharArray();
+		Assertions.assertThrows(IndexOutOfBoundsException.class, () -> {
+			new Passphrase(data, offset, length);
+		});
+	}
+
+	@ParameterizedTest
+	@CsvSource(value = {
+			"0, 4",
+			"0, 0",
+			"0, 1",
+			"1, 1",
+			"2, 2"
+	})
+	public void testValidConstructorArgs(int offset, int length) {
+		char[] data = "test".toCharArray();
+		var pw = new Passphrase(data, offset, length);
+		Assertions.assertEquals(length, pw.length());
+		Assertions.assertEquals("test".substring(offset, offset + length), pw.toString());
+	}
+
+	@Test
+	public void testToString() {
+		Assertions.assertEquals("test", Passphrase.copyOf("test").toString());
+	}
+
+	@Nested
+	class WithInstances {
+
+		private Passphrase pw1;
+		private Passphrase pw2;
+
+		@BeforeEach
+		public void setup() {
+			char[] foo = "test test".toCharArray();
+			pw1 = new Passphrase(foo, 5, 4);
+			pw2 = Passphrase.copyOf("test");
+		}
+
+		@Test
+		public void testEquals() {
+			Assertions.assertEquals(pw1, pw2);
+			Assertions.assertEquals("test", pw1.toString());
+			Assertions.assertEquals("test", pw2.toString());
+		}
+
+		@Test
+		public void testHashcode() {
+			Assertions.assertEquals(pw1.hashCode(), pw2.hashCode());
+		}
+
+		@Test
+		public void testLength() {
+			Assertions.assertEquals(4, pw1.length());
+			Assertions.assertEquals(4, pw2.length());
+		}
+
+		@Test
+		public void testCharAt() {
+			Assertions.assertEquals('s', pw1.charAt(2));
+		}
+
+		@ParameterizedTest
+		@ValueSource(ints = {-1, 4, 5})
+		public void testInvalidCharAt(int idx) {
+			Assertions.assertThrows(IndexOutOfBoundsException.class, () -> pw1.charAt(idx));
+		}
+
+		@ParameterizedTest
+		@ValueSource(ints = {0, 1, 2, 3})
+		public void testValidCharAt(int idx) {
+			Assertions.assertEquals("test".charAt(idx), pw1.charAt(idx));
+		}
+
+		@ParameterizedTest
+		@CsvSource(value = {
+				"-1, 0",
+				"0, -1",
+				"-1, -1",
+				"0, 5",
+				"3, 2"
+		})
+		public void testInvalidSubSequence(int start, int end) {
+			Assertions.assertThrows(IndexOutOfBoundsException.class, () -> pw1.subSequence(start, end));
+		}
+
+		@ParameterizedTest
+		@CsvSource(value = {
+				"0, 4",
+				"1, 4",
+				"0, 2",
+				"2, 4",
+				"4, 4",
+		})
+		public void testValidSubSequence(int start, int end) {
+			Assertions.assertEquals("test".substring(start, end), pw1.subSequence(start, end).toString());
+		}
+
+		@Test
+		public void testDestroy() {
+			pw2.destroy();
+			Assertions.assertFalse(pw1.isDestroyed());
+			Assertions.assertTrue(pw2.isDestroyed());
+			Assertions.assertNotEquals(pw1, pw2);
+		}
+
+	}
+
+}