Bladeren bron

Updated SecPasswordField, which will now no longer store passwords in Strings but rather in char[].

Affects #409
Sebastian Stenzel 6 jaren geleden
bovenliggende
commit
ef5eabdb79

+ 13 - 6
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<ChangePasswordListener> 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;

+ 13 - 5
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<Integer> passwordStrength; // 0-4
+	private IntegerProperty passwordStrength = new SimpleIntegerProperty(-1); // strengths: 0-4
 	private Optional<InitializationListener> 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;

+ 1 - 1
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, ' ');
 			}

+ 59 - 21
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;
+		}
+	}
+
+	/**
+	 * 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);
+	}
+
 	/**
-	 * {@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.
-	 * <br/>
-	 * Imagine the following example with <code>pass</code> being the password, <code>x</code> being the swipe char and <code>'</code> being the offset of the char array:
-	 * <ol>
-	 * <li>Append filling chars to the end of the password: <code>passxxxx'</code></li>
-	 * <li>Delete first 4 chars. Internal implementation will then copy subsequent chars to the position, where the deletion occured: <code>xxxx'xxxx</code></li>
-	 * <li>Delete first 4 chars again, as we appended 4 chars in step 1: <code>'xxxxxx</code></li>
-	 * </ol>
+	 * 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);
 	}
 
 }