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.
This commit is contained in:
Alexander Bakker 2022-09-17 17:30:14 +02:00
parent 66b7fd38d6
commit 3bc10bd4b8
14 changed files with 159 additions and 24 deletions

View file

@ -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<VaultEntry> entries) {
VaultRepository vault;
try {

View file

@ -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++;
}
}

View file

@ -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<MainActivity> _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()));
}
}

View file

@ -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

View file

@ -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();
}

View file

@ -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();

View file

@ -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();

View file

@ -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();

View file

@ -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();

View file

@ -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);

View file

@ -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);

View file

@ -152,6 +152,7 @@
<string name="set_up_biometric">Set up biometric unlock</string>
<string name="copy">Copy</string>
<string name="edit">Edit</string>
<string name="error_all_caps">ERROR</string>
<string name="password">Password</string>
<string name="confirm_password">Confirm password</string>
<string name="show_password">Show password</string>

View file

@ -53,19 +53,19 @@ public class DatabaseImporterTest {
}
@Test
public void testImportPlainText() throws IOException, DatabaseImporterException {
public void testImportPlainText() throws IOException, DatabaseImporterException, OtpInfoException {
List<VaultEntry> entries = importPlain(GoogleAuthUriImporter.class, "plain.txt");
checkImportedEntries(entries);
}
@Test
public void testImportAegisPlain() throws IOException, DatabaseImporterException {
public void testImportAegisPlain() throws IOException, DatabaseImporterException, OtpInfoException {
List<VaultEntry> entries = importPlain(AegisImporter.class, "aegis_plain.json");
checkImportedEntries(entries);
}
@Test
public void testImportAegisEncrypted() throws IOException, DatabaseImporterException {
public void testImportAegisEncrypted() throws IOException, DatabaseImporterException, OtpInfoException {
List<VaultEntry> 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<VaultEntry> 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<VaultEntry> entries = importPlain(AndOtpImporter.class, "andotp_plain.json");
checkImportedEntries(entries);
}
@Test
public void testImportAndOTPEncrypted() throws IOException, DatabaseImporterException {
public void testImportAndOTPEncrypted() throws IOException, DatabaseImporterException, OtpInfoException {
List<VaultEntry> 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<VaultEntry> 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<VaultEntry> entries = importPlain(BitwardenImporter.class, "bitwarden.json");
checkImportedBitwardenEntries(entries);
}
@Test
public void testImportBitwardenCsv() throws IOException, DatabaseImporterException {
public void testImportBitwardenCsv() throws IOException, DatabaseImporterException, OtpInfoException {
List<VaultEntry> 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<VaultEntry> 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<VaultEntry> entries) {
private void checkImportedBitwardenEntries(List<VaultEntry> 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<VaultEntry> entries) {
private void checkImportedEntries(List<VaultEntry> 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());

View file

@ -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);
}
}