diff --git a/main/ui/src/main/java/org/cryptomator/ui/controllers/ChangePasswordController.java b/main/ui/src/main/java/org/cryptomator/ui/controllers/ChangePasswordController.java index 12a838139..2965d5dbd 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/controllers/ChangePasswordController.java +++ b/main/ui/src/main/java/org/cryptomator/ui/controllers/ChangePasswordController.java @@ -16,6 +16,7 @@ import java.util.Optional; import javax.inject.Inject; +import javafx.beans.Observable; import org.cryptomator.cryptolib.api.InvalidPassphraseException; import org.cryptomator.cryptolib.api.UnsupportedVaultFormatException; import org.cryptomator.ui.controls.SecPasswordField; @@ -48,7 +49,7 @@ public class ChangePasswordController implements ViewController { private final Application app; private final PasswordStrengthUtil strengthRater; private final Localization localization; - private final IntegerProperty passwordStrength = new SimpleIntegerProperty(); // 0-4 + private final IntegerProperty passwordStrength = new SimpleIntegerProperty(-1); // 0-4 private Optional listener = Optional.empty(); private Vault vault; @@ -100,11 +101,9 @@ public class ChangePasswordController implements ViewController { @Override public void initialize() { - BooleanBinding oldPasswordIsEmpty = oldPasswordField.textProperty().isEmpty(); - BooleanBinding newPasswordIsEmpty = newPasswordField.textProperty().isEmpty(); - BooleanBinding passwordsDiffer = newPasswordField.textProperty().isNotEqualTo(retypePasswordField.textProperty()); - changePasswordButton.disableProperty().bind(oldPasswordIsEmpty.or(newPasswordIsEmpty.or(passwordsDiffer))); - passwordStrength.bind(EasyBind.map(newPasswordField.textProperty(), strengthRater::computeRate)); + oldPasswordField.textProperty().addListener(this::passwordsChanged); + newPasswordField.textProperty().addListener(this::passwordsChanged); + retypePasswordField.textProperty().addListener(this::passwordsChanged); passwordStrengthLevel0.backgroundProperty().bind(EasyBind.combine(passwordStrength, new SimpleIntegerProperty(0), strengthRater::getBackgroundWithStrengthColor)); passwordStrengthLevel1.backgroundProperty().bind(EasyBind.combine(passwordStrength, new SimpleIntegerProperty(1), strengthRater::getBackgroundWithStrengthColor)); @@ -114,6 +113,14 @@ public class ChangePasswordController implements ViewController { passwordStrengthLabel.textProperty().bind(EasyBind.map(passwordStrength, strengthRater::getStrengthDescription)); } + private void passwordsChanged(Observable observable) { + boolean oldPasswordEmpty = oldPasswordField.getCharacters().length() == 0; + boolean newPasswordEmpty = newPasswordField.getCharacters().length() == 0; + boolean passwordsEqual = newPasswordField.getCharacters().equals(retypePasswordField.getCharacters()); + changePasswordButton.setDisable(oldPasswordEmpty || newPasswordEmpty || !passwordsEqual); + passwordStrength.set(strengthRater.computeRate(newPasswordField.getCharacters().toString())); + } + @Override public Parent getRoot() { return root; diff --git a/main/ui/src/main/java/org/cryptomator/ui/controllers/InitializeController.java b/main/ui/src/main/java/org/cryptomator/ui/controllers/InitializeController.java index 5c7c0dc5f..5e727d5f1 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/controllers/InitializeController.java +++ b/main/ui/src/main/java/org/cryptomator/ui/controllers/InitializeController.java @@ -16,6 +16,9 @@ import java.util.Optional; import javax.inject.Inject; +import javafx.beans.Observable; +import javafx.beans.property.IntegerProperty; +import javafx.beans.value.ObservableIntegerValue; import org.cryptomator.ui.controls.SecPasswordField; import org.cryptomator.ui.l10n.Localization; import org.cryptomator.ui.model.Vault; @@ -42,7 +45,7 @@ public class InitializeController implements ViewController { private final Localization localization; private final PasswordStrengthUtil strengthRater; - private ObservableValue passwordStrength; // 0-4 + private IntegerProperty passwordStrength = new SimpleIntegerProperty(-1); // strengths: 0-4 private Optional listener = Optional.empty(); private Vault vault; @@ -87,10 +90,8 @@ public class InitializeController implements ViewController { @Override public void initialize() { - BooleanBinding passwordIsEmpty = passwordField.textProperty().isEmpty(); - BooleanBinding passwordsDiffer = passwordField.textProperty().isNotEqualTo(retypePasswordField.textProperty()); - okButton.disableProperty().bind(passwordIsEmpty.or(passwordsDiffer)); - passwordStrength = EasyBind.map(passwordField.textProperty(), strengthRater::computeRate); + passwordField.textProperty().addListener(this::passwordsChanged); + retypePasswordField.textProperty().addListener(this::passwordsChanged); passwordStrengthLevel0.backgroundProperty().bind(EasyBind.combine(passwordStrength, new SimpleIntegerProperty(0), strengthRater::getBackgroundWithStrengthColor)); passwordStrengthLevel1.backgroundProperty().bind(EasyBind.combine(passwordStrength, new SimpleIntegerProperty(1), strengthRater::getBackgroundWithStrengthColor)); @@ -100,6 +101,13 @@ public class InitializeController implements ViewController { passwordStrengthLabel.textProperty().bind(EasyBind.map(passwordStrength, strengthRater::getStrengthDescription)); } + private void passwordsChanged(Observable observable) { + boolean passwordsEmpty = passwordField.getCharacters().length() == 0; + boolean passwordsEqual = passwordField.getCharacters().equals(retypePasswordField.getCharacters()); + okButton.setDisable(passwordsEmpty || !passwordsEqual); + passwordStrength.set(strengthRater.computeRate(passwordField.getCharacters().toString())); + } + @Override public Parent getRoot() { return root; diff --git a/main/ui/src/main/java/org/cryptomator/ui/controllers/UnlockController.java b/main/ui/src/main/java/org/cryptomator/ui/controllers/UnlockController.java index 2a2c0954c..c08409375 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/controllers/UnlockController.java +++ b/main/ui/src/main/java/org/cryptomator/ui/controllers/UnlockController.java @@ -235,7 +235,7 @@ public class UnlockController implements ViewController { char[] storedPw = keychainAccess.get().loadPassphrase(vault.getId()); if (storedPw != null) { savePassword.setSelected(true); - passwordField.setText(new String(storedPw)); + passwordField.setPassword(storedPw); passwordField.selectRange(storedPw.length, storedPw.length); Arrays.fill(storedPw, ' '); } diff --git a/main/ui/src/main/java/org/cryptomator/ui/controls/SecPasswordField.java b/main/ui/src/main/java/org/cryptomator/ui/controls/SecPasswordField.java index 19d01cfbd..c2774c345 100644 --- a/main/ui/src/main/java/org/cryptomator/ui/controls/SecPasswordField.java +++ b/main/ui/src/main/java/org/cryptomator/ui/controls/SecPasswordField.java @@ -2,25 +2,35 @@ * Copyright (c) 2014, 2017 Sebastian Stenzel * All rights reserved. * This program and the accompanying materials are made available under the terms of the accompanying LICENSE file. - * + * * Contributors: * Sebastian Stenzel - initial API and implementation ******************************************************************************/ package org.cryptomator.ui.controls; -import java.util.Arrays; - +import com.google.common.base.Strings; import javafx.scene.control.PasswordField; import javafx.scene.input.DragEvent; import javafx.scene.input.Dragboard; import javafx.scene.input.TransferMode; +import java.nio.CharBuffer; +import java.util.Arrays; + /** - * Compromise in security. While the text can be swiped, any access to the {@link #getText()} method will create a copy of the String in the heap. + * Patched PasswordField that doesn't create String copies of the password in memory. Instead the password is stored in a char[] that can be swiped. + * + * @implNote Since {@link #setText(String)} is final, we can not override its behaviour. For that reason you should not use the {@link #textProperty()} for anything else than display purposes. */ public class SecPasswordField extends PasswordField { private static final char SWIPE_CHAR = ' '; + private static final int INITIAL_BUFFER_SIZE = 50; + private static final int GROW_BUFFER_SIZE = 50; + private static final String PLACEHOLDER = "*"; + + private char[] content = new char[INITIAL_BUFFER_SIZE]; + private int length = 0; public SecPasswordField() { this.onDragOverProperty().set(this::handleDragOver); @@ -43,26 +53,54 @@ public class SecPasswordField extends PasswordField { event.consume(); } + @Override + public void replaceText(int start, int end, String text) { + int removed = end - start; + int added = text.length(); + this.length += added - removed; + growContentIfNeeded(); + text.getChars(0, text.length(), content, start); + + String placeholderString = Strings.repeat(PLACEHOLDER, text.length()); + super.replaceText(start, end, placeholderString); + } + + private void growContentIfNeeded() { + if (this.length > content.length) { + char[] newContent = new char[content.length + GROW_BUFFER_SIZE]; + System.arraycopy(content, 0, newContent, 0, content.length); + swipe(); + this.content = newContent; + } + } + /** - * {@link #getContent()} uses a StringBuilder, which in turn is backed by a char[]. - * The delete operation of AbstractStringBuilder closes the gap, that forms by deleting chars, by moving up the following chars. - *
- * Imagine the following example with pass being the password, x being the swipe char and ' being the offset of the char array: - *
    - *
  1. Append filling chars to the end of the password: passxxxx'
    2. - *
  2. Delete first 4 chars. Internal implementation will then copy subsequent chars to the position, where the deletion occured: xxxx'xxxx
    3. - *
  3. Delete first 4 chars again, as we appended 4 chars in step 1: 'xxxxxx
    4. - *
+ * Creates a CharSequence by wrapping the password characters. + * + * @return A character sequence backed by the SecPasswordField's buffer (not a copy). + * @implNote The CharSequence will not copy the backing char[]. + * Therefore any mutation to the SecPasswordField's content will mutate or eventually swipe the returned CharSequence. + * @see #swipe() + */ + @Override + public CharSequence getCharacters() { + return CharBuffer.wrap(content, 0, length); + } + + public void setPassword(char[] password) { + swipe(); + content = Arrays.copyOf(password, password.length); + length = password.length; + + String placeholderString = Strings.repeat(PLACEHOLDER, password.length); + setText(placeholderString); + } + + /** + * Destroys the stored password by overriding each character with a different character. */ public void swipe() { - final int pwLength = this.getContent().length(); - final char[] fillingChars = new char[pwLength]; - Arrays.fill(fillingChars, SWIPE_CHAR); - this.getContent().insert(pwLength, new String(fillingChars), false); - this.getContent().delete(0, pwLength, true); - this.getContent().delete(0, pwLength, true); - // previous text has now been overwritten. but we still need to update the text to trigger some property bindings: - this.clear(); + Arrays.fill(content, SWIPE_CHAR); } }