From 3bc10bd4b84201c575363aef4f4d5df7091d37d2 Mon Sep 17 00:00:00 2001 From: Alexander Bakker Date: Sat, 17 Sep 2022 17:30:14 +0200 Subject: [PATCH] Disallow importing empty secrets Fixes #975 And display "ERROR" for any existing bad entries. This is kind of ugly, but it's better than crashing, and there are probably very few (or zero) users who have bad entries anyway. --- .../com/beemdevelopment/aegis/AegisTest.java | 4 ++ .../aegis/BackupExportTest.java | 7 ++- .../aegis/EmptySecretTest.java | 54 +++++++++++++++++++ .../aegis/otp/GoogleAuthInfo.java | 7 +++ .../aegis/otp/GoogleAuthInfoException.java | 4 +- .../beemdevelopment/aegis/otp/HotpInfo.java | 4 +- .../beemdevelopment/aegis/otp/OtpInfo.java | 8 ++- .../beemdevelopment/aegis/otp/SteamInfo.java | 4 +- .../beemdevelopment/aegis/otp/TotpInfo.java | 4 +- .../aegis/ui/MainActivity.java | 11 +++- .../aegis/ui/views/EntryHolder.java | 16 ++++-- app/src/main/res/values/strings.xml | 1 + .../aegis/importers/DatabaseImporterTest.java | 28 +++++----- .../aegis/otp/GoogleAuthInfoTest.java | 31 +++++++++++ 14 files changed, 159 insertions(+), 24 deletions(-) create mode 100644 app/src/androidTest/java/com/beemdevelopment/aegis/EmptySecretTest.java create mode 100644 app/src/test/java/com/beemdevelopment/aegis/otp/GoogleAuthInfoTest.java diff --git a/app/src/androidTest/java/com/beemdevelopment/aegis/AegisTest.java b/app/src/androidTest/java/com/beemdevelopment/aegis/AegisTest.java index f2a59ed8..e3ec9d48 100644 --- a/app/src/androidTest/java/com/beemdevelopment/aegis/AegisTest.java +++ b/app/src/androidTest/java/com/beemdevelopment/aegis/AegisTest.java @@ -86,6 +86,10 @@ public abstract class AegisTest { return initVault(null, VaultEntries.get()); } + protected VaultRepository initEmptyPlainVault() { + return initVault(null, null); + } + private VaultRepository initVault(@Nullable VaultFileCredentials creds, @Nullable List entries) { VaultRepository vault; try { diff --git a/app/src/androidTest/java/com/beemdevelopment/aegis/BackupExportTest.java b/app/src/androidTest/java/com/beemdevelopment/aegis/BackupExportTest.java index 3188db56..04babe67 100644 --- a/app/src/androidTest/java/com/beemdevelopment/aegis/BackupExportTest.java +++ b/app/src/androidTest/java/com/beemdevelopment/aegis/BackupExportTest.java @@ -37,6 +37,7 @@ import com.beemdevelopment.aegis.encoding.Hex; import com.beemdevelopment.aegis.importers.DatabaseImporter; import com.beemdevelopment.aegis.importers.DatabaseImporterException; import com.beemdevelopment.aegis.importers.GoogleAuthUriImporter; +import com.beemdevelopment.aegis.otp.OtpInfoException; import com.beemdevelopment.aegis.rules.ScreenshotTestRule; import com.beemdevelopment.aegis.ui.PreferencesActivity; import com.beemdevelopment.aegis.util.IOUtils; @@ -300,7 +301,11 @@ public class BackupExportTest extends AegisTest { VaultEntry vector = vectors.get(i); String message = String.format("Entries are not equivalent: (%s) (%s)", vector.toJson().toString(), entry.toJson().toString()); assertTrue(message, vector.equivalates(entry)); - assertEquals(message, vector.getInfo().getOtp(), entry.getInfo().getOtp()); + try { + assertEquals(message, vector.getInfo().getOtp(), entry.getInfo().getOtp()); + } catch (OtpInfoException e) { + throw new RuntimeException("Unable to generate OTP", e); + } i++; } } diff --git a/app/src/androidTest/java/com/beemdevelopment/aegis/EmptySecretTest.java b/app/src/androidTest/java/com/beemdevelopment/aegis/EmptySecretTest.java new file mode 100644 index 00000000..47c53234 --- /dev/null +++ b/app/src/androidTest/java/com/beemdevelopment/aegis/EmptySecretTest.java @@ -0,0 +1,54 @@ +package com.beemdevelopment.aegis; + +import static androidx.test.espresso.Espresso.onView; +import static androidx.test.espresso.action.ViewActions.click; +import static androidx.test.espresso.matcher.ViewMatchers.hasDescendant; +import static androidx.test.espresso.matcher.ViewMatchers.withId; +import static androidx.test.espresso.matcher.ViewMatchers.withText; + +import androidx.test.core.app.ActivityScenario; +import androidx.test.espresso.contrib.RecyclerViewActions; +import androidx.test.ext.junit.rules.ActivityScenarioRule; +import androidx.test.ext.junit.runners.AndroidJUnit4; +import androidx.test.filters.SmallTest; + +import com.beemdevelopment.aegis.otp.OtpInfoException; +import com.beemdevelopment.aegis.otp.TotpInfo; +import com.beemdevelopment.aegis.rules.ScreenshotTestRule; +import com.beemdevelopment.aegis.ui.MainActivity; +import com.beemdevelopment.aegis.vault.VaultEntry; + +import org.junit.After; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.RuleChain; +import org.junit.rules.TestRule; +import org.junit.runner.RunWith; + +import dagger.hilt.android.testing.HiltAndroidTest; + +@RunWith(AndroidJUnit4.class) +@HiltAndroidTest +@SmallTest +public class EmptySecretTest extends AegisTest { + private ActivityScenario _scenario; + + @Before + public void before() throws OtpInfoException { + initEmptyPlainVault(); + _vaultManager.getVault().addEntry(new VaultEntry(new TotpInfo(new byte[0]))); + + _scenario = ActivityScenario.launch(MainActivity.class); + } + + @After + public void after() { + _scenario.close(); + } + + @Test + public void testVaultEntryEmptySecret() { + onView(withId(R.id.rvKeyProfiles)).perform(RecyclerViewActions.actionOnItem(hasDescendant(withText(R.string.error_all_caps)), click())); + } +} diff --git a/app/src/main/java/com/beemdevelopment/aegis/otp/GoogleAuthInfo.java b/app/src/main/java/com/beemdevelopment/aegis/otp/GoogleAuthInfo.java index cbf43193..f41f155c 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/otp/GoogleAuthInfo.java +++ b/app/src/main/java/com/beemdevelopment/aegis/otp/GoogleAuthInfo.java @@ -59,6 +59,9 @@ public class GoogleAuthInfo implements Transferable, Serializable { } catch (EncodingException e) { throw new GoogleAuthInfoException(uri, "Bad secret", e); } + if (secret.length == 0) { + throw new GoogleAuthInfoException(uri, "Secret is empty"); + } OtpInfo info; String issuer = ""; @@ -233,6 +236,10 @@ public class GoogleAuthInfo implements Transferable, Serializable { } byte[] secret = params.getSecret().toByteArray(); + if (secret.length == 0) { + throw new GoogleAuthInfoException(uri, "Secret is empty"); + } + switch (params.getType()) { case OTP_TYPE_UNSPECIFIED: // intentional fallthrough diff --git a/app/src/main/java/com/beemdevelopment/aegis/otp/GoogleAuthInfoException.java b/app/src/main/java/com/beemdevelopment/aegis/otp/GoogleAuthInfoException.java index c26cd3f5..c912a5d5 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/otp/GoogleAuthInfoException.java +++ b/app/src/main/java/com/beemdevelopment/aegis/otp/GoogleAuthInfoException.java @@ -30,7 +30,9 @@ public class GoogleAuthInfoException extends Exception { @Override public String getMessage() { Throwable cause = getCause(); - if (cause == null) { + if (cause == null + || this == cause + || (super.getMessage() != null && super.getMessage().equals(cause.getMessage()))) { return super.getMessage(); } diff --git a/app/src/main/java/com/beemdevelopment/aegis/otp/HotpInfo.java b/app/src/main/java/com/beemdevelopment/aegis/otp/HotpInfo.java index 2d4a0a91..7fc91fca 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/otp/HotpInfo.java +++ b/app/src/main/java/com/beemdevelopment/aegis/otp/HotpInfo.java @@ -30,7 +30,9 @@ public class HotpInfo extends OtpInfo { } @Override - public String getOtp() { + public String getOtp() throws OtpInfoException { + checkSecret(); + try { OTP otp = HOTP.generateOTP(getSecret(), getAlgorithm(true), getDigits(), getCounter()); return otp.toString(); diff --git a/app/src/main/java/com/beemdevelopment/aegis/otp/OtpInfo.java b/app/src/main/java/com/beemdevelopment/aegis/otp/OtpInfo.java index 85d7973b..89b2cba4 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/otp/OtpInfo.java +++ b/app/src/main/java/com/beemdevelopment/aegis/otp/OtpInfo.java @@ -28,7 +28,13 @@ public abstract class OtpInfo implements Serializable { setDigits(digits); } - public abstract String getOtp(); + public abstract String getOtp() throws OtpInfoException; + + protected void checkSecret() throws OtpInfoException { + if (getSecret().length == 0) { + throw new OtpInfoException("Secret is empty"); + } + } public abstract String getTypeId(); diff --git a/app/src/main/java/com/beemdevelopment/aegis/otp/SteamInfo.java b/app/src/main/java/com/beemdevelopment/aegis/otp/SteamInfo.java index a475f403..6d345c98 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/otp/SteamInfo.java +++ b/app/src/main/java/com/beemdevelopment/aegis/otp/SteamInfo.java @@ -20,7 +20,9 @@ public class SteamInfo extends TotpInfo { } @Override - public String getOtp() { + public String getOtp() throws OtpInfoException { + checkSecret(); + try { OTP otp = TOTP.generateOTP(getSecret(), getAlgorithm(true), getDigits(), getPeriod()); return otp.toSteamString(); diff --git a/app/src/main/java/com/beemdevelopment/aegis/otp/TotpInfo.java b/app/src/main/java/com/beemdevelopment/aegis/otp/TotpInfo.java index a27e3671..5e7320f4 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/otp/TotpInfo.java +++ b/app/src/main/java/com/beemdevelopment/aegis/otp/TotpInfo.java @@ -26,7 +26,9 @@ public class TotpInfo extends OtpInfo { } @Override - public String getOtp() { + public String getOtp() throws OtpInfoException { + checkSecret(); + try { OTP otp = TOTP.generateOTP(getSecret(), getAlgorithm(true), getDigits(), getPeriod()); return otp.toString(); diff --git a/app/src/main/java/com/beemdevelopment/aegis/ui/MainActivity.java b/app/src/main/java/com/beemdevelopment/aegis/ui/MainActivity.java index 09d50070..9dfe5053 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/ui/MainActivity.java +++ b/app/src/main/java/com/beemdevelopment/aegis/ui/MainActivity.java @@ -41,6 +41,8 @@ import com.beemdevelopment.aegis.helpers.FabScrollHelper; import com.beemdevelopment.aegis.helpers.PermissionHelper; import com.beemdevelopment.aegis.otp.GoogleAuthInfo; import com.beemdevelopment.aegis.otp.GoogleAuthInfoException; +import com.beemdevelopment.aegis.otp.OtpInfo; +import com.beemdevelopment.aegis.otp.OtpInfoException; import com.beemdevelopment.aegis.ui.dialogs.Dialogs; import com.beemdevelopment.aegis.ui.fragments.preferences.BackupsPreferencesFragment; import com.beemdevelopment.aegis.ui.fragments.preferences.PreferencesFragment; @@ -931,8 +933,15 @@ public class MainActivity extends AegisActivity implements EntryListView.Listene @SuppressLint("InlinedApi") private void copyEntryCode(VaultEntry entry) { + String otp; + try { + otp = entry.getInfo().getOtp(); + } catch (OtpInfoException e) { + return; + } + ClipboardManager clipboard = (ClipboardManager) getSystemService(Context.CLIPBOARD_SERVICE); - ClipData clip = ClipData.newPlainText("text/plain", entry.getInfo().getOtp()); + ClipData clip = ClipData.newPlainText("text/plain", otp); if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { PersistableBundle extras = new PersistableBundle(); extras.putBoolean(ClipDescription.EXTRA_IS_SENSITIVE, true); diff --git a/app/src/main/java/com/beemdevelopment/aegis/ui/views/EntryHolder.java b/app/src/main/java/com/beemdevelopment/aegis/ui/views/EntryHolder.java index 04013d64..0ca82e7c 100644 --- a/app/src/main/java/com/beemdevelopment/aegis/ui/views/EntryHolder.java +++ b/app/src/main/java/com/beemdevelopment/aegis/ui/views/EntryHolder.java @@ -20,6 +20,7 @@ import com.beemdevelopment.aegis.helpers.ThemeHelper; import com.beemdevelopment.aegis.helpers.UiRefresher; import com.beemdevelopment.aegis.otp.HotpInfo; import com.beemdevelopment.aegis.otp.OtpInfo; +import com.beemdevelopment.aegis.otp.OtpInfoException; import com.beemdevelopment.aegis.otp.SteamInfo; import com.beemdevelopment.aegis.otp.TotpInfo; import com.beemdevelopment.aegis.otp.YandexInfo; @@ -238,9 +239,18 @@ public class EntryHolder extends RecyclerView.ViewHolder { private void updateCode() { OtpInfo info = _entry.getInfo(); - String otp = info.getOtp(); - if (!(info instanceof SteamInfo || info instanceof YandexInfo)) { - otp = formatCode(otp); + // In previous versions of Aegis, it was possible to import entries with an empty + // secret. Attempting to generate OTP's for such entries would result in a crash. + // In case we encounter an old entry that has this issue, we display "ERROR" as + // the OTP, instead of crashing. + String otp; + try { + otp = info.getOtp(); + if (!(info instanceof SteamInfo || info instanceof YandexInfo)) { + otp = formatCode(otp); + } + } catch (OtpInfoException e) { + otp = _view.getResources().getString(R.string.error_all_caps); } _profileCode.setText(otp); diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index ed3b120d..f3506d2f 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -152,6 +152,7 @@ Set up biometric unlock Copy Edit + ERROR Password Confirm password Show password diff --git a/app/src/test/java/com/beemdevelopment/aegis/importers/DatabaseImporterTest.java b/app/src/test/java/com/beemdevelopment/aegis/importers/DatabaseImporterTest.java index c6e8701b..d10fac8e 100644 --- a/app/src/test/java/com/beemdevelopment/aegis/importers/DatabaseImporterTest.java +++ b/app/src/test/java/com/beemdevelopment/aegis/importers/DatabaseImporterTest.java @@ -53,19 +53,19 @@ public class DatabaseImporterTest { } @Test - public void testImportPlainText() throws IOException, DatabaseImporterException { + public void testImportPlainText() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importPlain(GoogleAuthUriImporter.class, "plain.txt"); checkImportedEntries(entries); } @Test - public void testImportAegisPlain() throws IOException, DatabaseImporterException { + public void testImportAegisPlain() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importPlain(AegisImporter.class, "aegis_plain.json"); checkImportedEntries(entries); } @Test - public void testImportAegisEncrypted() throws IOException, DatabaseImporterException { + public void testImportAegisEncrypted() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importEncrypted(AegisImporter.class, "aegis_encrypted.json", encryptedState -> { final char[] password = "test".toCharArray(); return ((AegisImporter.EncryptedState) encryptedState).decrypt(password); @@ -88,7 +88,7 @@ public class DatabaseImporterTest { } @Test - public void testImportWinAuth() throws IOException, DatabaseImporterException { + public void testImportWinAuth() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importPlain(WinAuthImporter.class, "plain.txt"); for (VaultEntry entry : entries) { VaultEntry entryVector = getEntryVectorBySecret(entry.getInfo().getSecret()); @@ -99,13 +99,13 @@ public class DatabaseImporterTest { } @Test - public void testImportAndOTP() throws IOException, DatabaseImporterException { + public void testImportAndOTP() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importPlain(AndOtpImporter.class, "andotp_plain.json"); checkImportedEntries(entries); } @Test - public void testImportAndOTPEncrypted() throws IOException, DatabaseImporterException { + public void testImportAndOTPEncrypted() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importEncrypted(AndOtpImporter.class, "andotp_encrypted.bin", encryptedState -> { final char[] password = "test".toCharArray(); return ((AndOtpImporter.EncryptedState) encryptedState).decryptNewFormat(password); @@ -115,7 +115,7 @@ public class DatabaseImporterTest { } @Test - public void testImportAndOTPEncryptedOld() throws IOException, DatabaseImporterException { + public void testImportAndOTPEncryptedOld() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importEncrypted(AndOtpImporter.class, "andotp_encrypted_old.bin", encryptedState -> { final char[] password = "test".toCharArray(); return ((AndOtpImporter.EncryptedState) encryptedState).decryptOldFormat(password); @@ -162,13 +162,13 @@ public class DatabaseImporterTest { } @Test - public void testImportBitwardenJson() throws IOException, DatabaseImporterException { + public void testImportBitwardenJson() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importPlain(BitwardenImporter.class, "bitwarden.json"); checkImportedBitwardenEntries(entries); } @Test - public void testImportBitwardenCsv() throws IOException, DatabaseImporterException { + public void testImportBitwardenCsv() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importPlain(BitwardenImporter.class, "bitwarden.csv"); checkImportedBitwardenEntries(entries); } @@ -218,7 +218,7 @@ public class DatabaseImporterTest { } @Test - public void testImportSteam() throws IOException, DatabaseImporterException { + public void testImportSteam() throws IOException, DatabaseImporterException, OtpInfoException { List entries = importPlain(SteamImporter.class, "steam.json"); for (VaultEntry entry : entries) { VaultEntry entryVector = getEntryVectorBySecret(entry.getInfo().getSecret()); @@ -352,7 +352,7 @@ public class DatabaseImporterTest { } } - private void checkImportedBitwardenEntries(List entries) { + private void checkImportedBitwardenEntries(List entries) throws OtpInfoException { byte[] secret, vectorSecret; for (VaultEntry entry : entries) { if(entry.getInfo().getTypeId().equals(SteamInfo.ID)) { @@ -365,18 +365,18 @@ public class DatabaseImporterTest { } } - private void checkImportedEntries(List entries) { + private void checkImportedEntries(List entries) throws OtpInfoException { for (VaultEntry entry : entries) { checkImportedEntry(entry); } } - private void checkImportedEntry(VaultEntry entry) { + private void checkImportedEntry(VaultEntry entry) throws OtpInfoException { VaultEntry entryVector = getEntryVectorBySecret(entry.getInfo().getSecret()); checkImportedEntry(entryVector, entry); } - private void checkImportedEntry(VaultEntry entryVector, VaultEntry entry) { + private void checkImportedEntry(VaultEntry entryVector, VaultEntry entry) throws OtpInfoException { String message = String.format("Entries are not equivalent: (%s) (%s)", entryVector.toJson().toString(), entry.toJson().toString()); assertTrue(message, entryVector.equivalates(entry)); assertEquals(message, entryVector.getInfo().getOtp(), entry.getInfo().getOtp()); diff --git a/app/src/test/java/com/beemdevelopment/aegis/otp/GoogleAuthInfoTest.java b/app/src/test/java/com/beemdevelopment/aegis/otp/GoogleAuthInfoTest.java new file mode 100644 index 00000000..fd010317 --- /dev/null +++ b/app/src/test/java/com/beemdevelopment/aegis/otp/GoogleAuthInfoTest.java @@ -0,0 +1,31 @@ +package com.beemdevelopment.aegis.otp; + +import static org.junit.Assert.assertThrows; + +import android.os.Build; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +@Config(sdk = { Build.VERSION_CODES.S_V2 }) +@RunWith(RobolectricTestRunner.class) +public class GoogleAuthInfoTest { + @Test + public void testGoogleAuthInfoEmptySecret() throws GoogleAuthInfoException { + String uri = "otpauth://totp/test:test?secret=%s&algo=SHA1&digits=6&period=30"; + GoogleAuthInfo.parseUri(String.format(uri, "AA")); + assertThrows(GoogleAuthInfoException.class, () -> GoogleAuthInfo.parseUri(String.format(uri, ""))); + } + + @Test + public void testOtpInfoEmptySecret() throws OtpInfoException { + OtpInfo info = new TotpInfo(new byte[0]); + assertThrows(OtpInfoException.class, info::getOtp); + info = new HotpInfo(new byte[0]); + assertThrows(OtpInfoException.class, info::getOtp); + info = new SteamInfo(new byte[0]); + assertThrows(OtpInfoException.class, info::getOtp); + } +}