diff --git a/src/main/java/org/cryptomator/common/Passphrase.java b/src/main/java/org/cryptomator/common/Passphrase.java new file mode 100644 index 000000000..d57252507 --- /dev/null +++ b/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; + } +} diff --git a/src/main/java/org/cryptomator/ui/controls/NiceSecurePasswordField.java b/src/main/java/org/cryptomator/ui/controls/NiceSecurePasswordField.java index 4a4e43fff..4d09707b9 100644 --- a/src/main/java/org/cryptomator/ui/controls/NiceSecurePasswordField.java +++ b/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(); } diff --git a/src/main/java/org/cryptomator/ui/controls/SecurePasswordField.java b/src/main/java/org/cryptomator/ui/controls/SecurePasswordField.java index 0290f512d..66df79394 100644 --- a/src/main/java/org/cryptomator/ui/controls/SecurePasswordField.java +++ b/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); } /** diff --git a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java index 3a5c7548b..a22903973 100644 --- a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/MasterkeyFileLoadingStrategy.java +++ b/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 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); } diff --git a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryComponent.java b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryComponent.java index 637ffd5c6..5e072efd0 100644 --- a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryComponent.java +++ b/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(); } diff --git a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryController.java b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryController.java index d048cb776..35b1b1903 100644 --- a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryController.java +++ b/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 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 result, @Nullable @Named("savedPassword") char[] savedPassword, ForgetPasswordComponent.Builder forgetPassword, KeychainManager keychain) { + public PassphraseEntryController(@KeyLoading Stage window, @KeyLoading Vault vault, CompletableFuture 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(); } diff --git a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryResult.java b/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryResult.java index 2c55c203e..f26184d9d 100644 --- a/src/main/java/org/cryptomator/ui/keyloading/masterkeyfile/PassphraseEntryResult.java +++ b/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) { } diff --git a/src/test/java/org/cryptomator/common/PassphraseTest.java b/src/test/java/org/cryptomator/common/PassphraseTest.java new file mode 100644 index 000000000..7bd71beb2 --- /dev/null +++ b/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); + } + + } + +} \ No newline at end of file