From 87eab2799619528e1a50d133fc02e032146740e7 Mon Sep 17 00:00:00 2001 From: Alan Evans Date: Mon, 4 May 2020 17:50:38 -0300 Subject: [PATCH] Prevent the creation of 'weak' PINs. Simple checks to prevent the same number, or sequentially increasing/decreasing PINs. e.g. 1111, 1234, 54321, etc. --- .../lock/v2/CreateKbsPinFragment.java | 35 ++++++++- .../lock/v2/CreateKbsPinViewModel.java | 20 ++++- app/src/main/res/values/strings.xml | 1 + .../v2/PinValidityChecker_validity_Test.java | 38 ++++++++++ .../v2/testdata/PinValidityVector.java | 27 +++++++ .../data/kbs_pin_validity_vectors.json | 62 ++++++++++++++++ .../internal/registrationpin/PinHasher.java | 27 +------ .../internal/registrationpin/PinString.java | 32 ++++++++ .../registrationpin/PinValidityChecker.java | 73 +++++++++++++++++++ 9 files changed, 287 insertions(+), 28 deletions(-) create mode 100644 app/src/test/java/org/thoughtcrime/securesms/registration/v2/PinValidityChecker_validity_Test.java create mode 100644 app/src/test/java/org/thoughtcrime/securesms/registration/v2/testdata/PinValidityVector.java create mode 100644 app/src/test/resources/data/kbs_pin_validity_vectors.json create mode 100644 libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinString.java create mode 100644 libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinValidityChecker.java diff --git a/app/src/main/java/org/thoughtcrime/securesms/lock/v2/CreateKbsPinFragment.java b/app/src/main/java/org/thoughtcrime/securesms/lock/v2/CreateKbsPinFragment.java index e662e3388..6ed2b8535 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/lock/v2/CreateKbsPinFragment.java +++ b/app/src/main/java/org/thoughtcrime/securesms/lock/v2/CreateKbsPinFragment.java @@ -1,15 +1,20 @@ package org.thoughtcrime.securesms.lock.v2; -import android.view.View; +import android.view.animation.Animation; +import android.view.animation.TranslateAnimation; +import android.widget.EditText; +import android.widget.Toast; import androidx.annotation.NonNull; import androidx.annotation.PluralsRes; import androidx.autofill.HintConstants; +import androidx.core.content.ContextCompat; import androidx.core.view.ViewCompat; import androidx.lifecycle.ViewModelProviders; import androidx.navigation.Navigation; import org.thoughtcrime.securesms.R; +import org.thoughtcrime.securesms.util.SpanUtil; public class CreateKbsPinFragment extends BaseKbsPinFragment { @@ -47,6 +52,15 @@ public class CreateKbsPinFragment extends BaseKbsPinFragment onConfirmPin(e.getUserEntry(), e.getKeyboard(), args.getIsPinChange())); + viewModel.getErrorEvents().observe(getViewLifecycleOwner(), e -> { + if (e == CreateKbsPinViewModel.PinErrorEvent.WEAK_PIN) { + getLabel().setText(SpanUtil.color(ContextCompat.getColor(requireContext(), R.color.red), + getString(R.string.CreateKbsPinFragment__choose_a_stronger_pin))); + shake(getInput(), () -> getInput().getText().clear()); + } else { + throw new AssertionError("Unexpected PIN error!"); + } + }); viewModel.getKeyboard().observe(getViewLifecycleOwner(), k -> { getLabel().setText(getLabelText(k)); getInput().getText().clear(); @@ -76,4 +90,23 @@ public class CreateKbsPinFragment extends BaseKbsPinFragment userEntry = new MutableLiveData<>(KbsPin.EMPTY); private final MutableLiveData keyboard = new MutableLiveData<>(PinKeyboardType.NUMERIC); private final SingleLiveEvent events = new SingleLiveEvent<>(); + private final SingleLiveEvent errors = new SingleLiveEvent<>(); @Override public LiveData getUserEntry() { @@ -27,6 +31,8 @@ public final class CreateKbsPinViewModel extends ViewModel implements BaseKbsPin LiveData getNavigationEvents() { return events; } + LiveData getErrorEvents() { return errors; } + @Override @MainThread public void setUserEntry(String userEntry) { @@ -42,8 +48,14 @@ public final class CreateKbsPinViewModel extends ViewModel implements BaseKbsPin @Override @MainThread public void confirm() { - events.setValue(new NavigationEvent(Preconditions.checkNotNull(this.getUserEntry().getValue()), - Preconditions.checkNotNull(this.getKeyboard().getValue()))); + KbsPin pin = Preconditions.checkNotNull(this.getUserEntry().getValue()); + PinKeyboardType keyboard = Preconditions.checkNotNull(this.getKeyboard().getValue()); + + if (PinValidityChecker.valid(pin.toString())) { + events.setValue(new NavigationEvent(pin, keyboard)); + } else { + errors.setValue(PinErrorEvent.WEAK_PIN); + } } static final class NavigationEvent { @@ -63,4 +75,8 @@ public final class CreateKbsPinViewModel extends ViewModel implements BaseKbsPin return keyboard; } } + + enum PinErrorEvent { + WEAK_PIN + } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index e8ebe486f..7ff30141a 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -1930,6 +1930,7 @@ You can change your PIN as long as this device is registered. Create your PIN PINs keep information stored with Signal encrypted so only you can access it. Your profile, settings, and contacts will restore when you reinstall Signal. + Choose a stronger PIN PINs don\'t match. Try again. diff --git a/app/src/test/java/org/thoughtcrime/securesms/registration/v2/PinValidityChecker_validity_Test.java b/app/src/test/java/org/thoughtcrime/securesms/registration/v2/PinValidityChecker_validity_Test.java new file mode 100644 index 000000000..01f4cd19c --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/registration/v2/PinValidityChecker_validity_Test.java @@ -0,0 +1,38 @@ +package org.thoughtcrime.securesms.registration.v2; + +import org.junit.Test; +import org.thoughtcrime.securesms.registration.v2.testdata.PinValidityVector; +import org.thoughtcrime.securesms.util.Util; +import org.whispersystems.signalservice.internal.registrationpin.PinValidityChecker; +import org.whispersystems.signalservice.internal.util.JsonUtil; + +import java.io.IOException; +import java.io.InputStream; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +public final class PinValidityChecker_validity_Test { + + @Test + public void vectors_valid() throws IOException { + for (PinValidityVector vector : getKbsPinValidityTestVectorList()) { + boolean valid = PinValidityChecker.valid(vector.getPin()); + + assertEquals(String.format("%s [%s]", vector.getName(), vector.getPin()), + vector.isValid(), + valid); + } + } + + private static PinValidityVector[] getKbsPinValidityTestVectorList() throws IOException { + try (InputStream resourceAsStream = ClassLoader.getSystemClassLoader().getResourceAsStream("data/kbs_pin_validity_vectors.json")) { + + PinValidityVector[] data = JsonUtil.fromJson(Util.readFullyAsString(resourceAsStream), PinValidityVector[].class); + + assertTrue(data.length > 0); + + return data; + } + } +} diff --git a/app/src/test/java/org/thoughtcrime/securesms/registration/v2/testdata/PinValidityVector.java b/app/src/test/java/org/thoughtcrime/securesms/registration/v2/testdata/PinValidityVector.java new file mode 100644 index 000000000..099e055b7 --- /dev/null +++ b/app/src/test/java/org/thoughtcrime/securesms/registration/v2/testdata/PinValidityVector.java @@ -0,0 +1,27 @@ +package org.thoughtcrime.securesms.registration.v2.testdata; + +import com.fasterxml.jackson.annotation.JsonProperty; + +public class PinValidityVector { + + @JsonProperty("name") + private String name; + + @JsonProperty("pin") + private String pin; + + @JsonProperty("valid") + private boolean valid; + + public String getName() { + return name; + } + + public String getPin() { + return pin; + } + + public boolean isValid() { + return valid; + } +} \ No newline at end of file diff --git a/app/src/test/resources/data/kbs_pin_validity_vectors.json b/app/src/test/resources/data/kbs_pin_validity_vectors.json new file mode 100644 index 000000000..9305b9abf --- /dev/null +++ b/app/src/test/resources/data/kbs_pin_validity_vectors.json @@ -0,0 +1,62 @@ +[ + { + "name": "Empty", + "pin": "", + "valid": false + }, + { + "name": "Alpha", + "pin": "abcd", + "valid": true + }, + { + "name": "Sequential", + "pin": "1234", + "valid": false + }, + { + "name": "Non-sequential", + "pin": "6485", + "valid": true + }, + { + "name": "Sequential descending", + "pin": "43210", + "valid": false + }, + { + "name": "Sequential with space", + "pin": "1234 ", + "valid": false + }, + { + "name": "Non-sequential with space", + "pin": "1236 ", + "valid": true + }, + { + "name": "Sequential Non-arabic digits", + "pin": "١٢٣٤٥", + "valid": false + }, + { + "name": "Sequential descending Non-arabic digits", + "pin": "٥٤٣٢١", + "valid": false + }, + { + "name": "Non-sequential Non-arabic digits", + "pin": "١٢٣٥٤", + "valid": true + }, + { + "name": "All digits the same", + "pin": "9999", + "valid": false + }, + { + "name": "All Non-arabic digits the same", + "pin": "٢٢٢٢", + "valid": false + } +] \ No newline at end of file diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinHasher.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinHasher.java index 808df43e0..e81c519bb 100644 --- a/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinHasher.java +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinHasher.java @@ -10,8 +10,8 @@ public final class PinHasher { public static byte[] normalize(String pin) { pin = pin.trim(); - if (allNumeric(pin)) { - pin = new String(toArabic(pin)); + if (PinString.allNumeric(pin)) { + pin = PinString.toArabic(pin); } pin = Normalizer.normalize(pin, Normalizer.Form.NFKD); @@ -26,27 +26,4 @@ public final class PinHasher { public interface Argon2 { byte[] hash(byte[] password); } - - private static boolean allNumeric(CharSequence pin) { - for (int i = 0; i < pin.length(); i++) { - if (!Character.isDigit(pin.charAt(i))) return false; - } - return true; - } - - /** - * Converts a string of not necessarily Arabic numerals to Arabic 0..9 characters. - */ - private static char[] toArabic(CharSequence numerals) { - int length = numerals.length(); - char[] arabic = new char[length]; - - for (int i = 0; i < length; i++) { - int digit = Character.digit(numerals.charAt(i), 10); - - arabic[i] = (char) ('0' + digit); - } - - return arabic; - } } diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinString.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinString.java new file mode 100644 index 000000000..1b45460c3 --- /dev/null +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinString.java @@ -0,0 +1,32 @@ +package org.whispersystems.signalservice.internal.registrationpin; + +import org.whispersystems.signalservice.api.kbs.HashedPin; + +import java.nio.charset.StandardCharsets; +import java.text.Normalizer; + +final class PinString { + + static boolean allNumeric(CharSequence pin) { + for (int i = 0; i < pin.length(); i++) { + if (!Character.isDigit(pin.charAt(i))) return false; + } + return true; + } + + /** + * Converts a string of not necessarily Arabic numerals to Arabic 0..9 characters. + */ + static String toArabic(CharSequence numerals) { + int length = numerals.length(); + char[] arabic = new char[length]; + + for (int i = 0; i < length; i++) { + int digit = Character.digit(numerals.charAt(i), 10); + + arabic[i] = (char) ('0' + digit); + } + + return new String(arabic); + } +} diff --git a/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinValidityChecker.java b/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinValidityChecker.java new file mode 100644 index 000000000..eae9c51ff --- /dev/null +++ b/libsignal/service/src/main/java/org/whispersystems/signalservice/internal/registrationpin/PinValidityChecker.java @@ -0,0 +1,73 @@ +package org.whispersystems.signalservice.internal.registrationpin; + +public final class PinValidityChecker { + + public static boolean valid(String pin) { + pin = pin.trim(); + + if (pin.isEmpty()) { + return false; + } + + if (PinString.allNumeric(pin)) { + pin = PinString.toArabic(pin); + + return !sequential(pin) && + !sequential(reverse(pin)) && + !allTheSame(pin); + } else { + return true; + } + } + + private static String reverse(String string) { + char[] chars = string.toCharArray(); + + for (int i = 0; i < chars.length / 2; i++) { + char temp = chars[i]; + chars[i] = chars[chars.length - i - 1]; + chars[chars.length - i - 1] = temp; + } + + return new String(chars); + } + + private static boolean sequential(String pin) { + int length = pin.length(); + + if (length == 0) { + return false; + } + + char c = pin.charAt(0); + + for (int i = 1; i < length; i++) { + char n = pin.charAt(i); + if (n != c + 1) { + return false; + } + c = n; + } + + return true; + } + + private static boolean allTheSame(String pin) { + int length = pin.length(); + + if (length == 0) { + return false; + } + + char c = pin.charAt(0); + + for (int i = 1; i < length; i++) { + char n = pin.charAt(i); + if (n != c) { + return false; + } + } + + return true; + } +}