From 87738477be87afb639509f5ebdf5da979ba0bd04 Mon Sep 17 00:00:00 2001 From: Harsh Shandilya Date: Sun, 14 Apr 2024 22:50:59 +0530 Subject: [PATCH] fix: special-case AEAD failure Fixes #2974 Fixes #2963 Fixes #2921 Fixes #2924 Fixes #2653 Fixes #2461 Fixes #2586 Fixes #2179 --- .../ui/crypto/DecryptActivity.kt | 40 ++++++++++++------ .../ui/dialogs/BasicBottomSheet.kt | 20 +++++++-- app/src/main/res/values/strings.xml | 2 + .../crypto/errors/CryptoException.kt | 7 ++- .../crypto/PGPainlessCryptoHandler.kt | 9 ++++ .../crypto/PGPainlessCryptoHandlerTest.kt | 18 ++++++++ .../app/passwordstore/crypto/TestUtils.kt | 3 ++ .../src/test/resources/aead_encrypted_file | Bin 0 -> 354 bytes 8 files changed, 83 insertions(+), 16 deletions(-) create mode 100644 crypto/pgpainless/src/test/resources/aead_encrypted_file diff --git a/app/src/main/java/app/passwordstore/ui/crypto/DecryptActivity.kt b/app/src/main/java/app/passwordstore/ui/crypto/DecryptActivity.kt index 895ac50d..d36c1a9f 100644 --- a/app/src/main/java/app/passwordstore/ui/crypto/DecryptActivity.kt +++ b/app/src/main/java/app/passwordstore/ui/crypto/DecryptActivity.kt @@ -13,11 +13,14 @@ import androidx.fragment.app.setFragmentResultListener import androidx.lifecycle.lifecycleScope import app.passwordstore.R import app.passwordstore.crypto.PGPIdentifier +import app.passwordstore.crypto.errors.CryptoHandlerException +import app.passwordstore.crypto.errors.NonStandardAEAD import app.passwordstore.data.crypto.PGPPassphraseCache import app.passwordstore.data.passfile.PasswordEntry import app.passwordstore.data.password.FieldItem import app.passwordstore.databinding.DecryptLayoutBinding import app.passwordstore.ui.adapters.FieldItemAdapter +import app.passwordstore.ui.dialogs.BasicBottomSheet import app.passwordstore.util.auth.BiometricAuthenticator import app.passwordstore.util.auth.BiometricAuthenticator.Result as BiometricResult import app.passwordstore.util.extensions.getString @@ -27,7 +30,8 @@ import app.passwordstore.util.features.Feature.EnablePGPPassphraseCache import app.passwordstore.util.features.Features import app.passwordstore.util.settings.Constants import app.passwordstore.util.settings.PreferenceKeys -import com.github.michaelbull.result.runCatching +import com.github.michaelbull.result.Result +import com.github.michaelbull.result.map import dagger.hilt.android.AndroidEntryPoint import java.io.ByteArrayOutputStream import java.io.File @@ -198,7 +202,7 @@ class DecryptActivity : BasePGPActivity() { passphraseCache.cachePassphrase( this@DecryptActivity, gpgIdentifiers.first(), - passphrase + passphrase, ) } } @@ -221,24 +225,36 @@ class DecryptActivity : BasePGPActivity() { onSuccess() } else { logcat(ERROR) { result.error.stackTraceToString() } - decrypt(isError = true, authResult = authResult) + when (result.error) { + is NonStandardAEAD -> { + BasicBottomSheet.Builder(this) + .setTitle(getString(R.string.aead_detect_title)) + .setMessage(getString(R.string.aead_detect_message, result.error.message)) + .setPositiveButtonClickListener(getString(R.string.dialog_ok)) { + setResult(RESULT_CANCELED) + finish() + } + .setOnDismissListener { + setResult(RESULT_CANCELED) + finish() + } + .build() + .show(supportFragmentManager, "AEAD_INFO_SHEET") + } + else -> decrypt(isError = true, authResult = authResult) + } } } private suspend fun decryptPGPStream( passphrase: String, gpgIdentifiers: List, - ) = runCatching { + ): Result { val message = withContext(dispatcherProvider.io()) { File(fullPath).readBytes().inputStream() } val outputStream = ByteArrayOutputStream() - val result = - repository.decrypt( - passphrase, - gpgIdentifiers, - message, - outputStream, - ) - if (result.isOk) outputStream else throw result.error + return repository.decrypt(passphrase, gpgIdentifiers, message, outputStream).map { + outputStream + } } private suspend fun createPasswordUI(entry: PasswordEntry) = diff --git a/app/src/main/java/app/passwordstore/ui/dialogs/BasicBottomSheet.kt b/app/src/main/java/app/passwordstore/ui/dialogs/BasicBottomSheet.kt index c35985cb..11c23311 100644 --- a/app/src/main/java/app/passwordstore/ui/dialogs/BasicBottomSheet.kt +++ b/app/src/main/java/app/passwordstore/ui/dialogs/BasicBottomSheet.kt @@ -6,6 +6,7 @@ package app.passwordstore.ui.dialogs import android.content.Context +import android.content.DialogInterface.OnDismissListener import android.os.Bundle import android.view.LayoutInflater import android.view.View @@ -33,6 +34,7 @@ private constructor( val negativeButtonLabel: String?, val positiveButtonClickListener: View.OnClickListener?, val negativeButtonClickListener: View.OnClickListener?, + val onDismissListener: OnDismissListener?, ) : BottomSheetDialogFragment() { private val binding by viewBinding(BasicBottomSheetBinding::bind) @@ -94,6 +96,9 @@ private constructor( dismiss() } } + if (onDismissListener != null) { + dialog.setOnDismissListener(onDismissListener) + } } } ) @@ -112,6 +117,7 @@ private constructor( private var negativeButtonLabel: String? = null private var positiveButtonClickListener: View.OnClickListener? = null private var negativeButtonClickListener: View.OnClickListener? = null + private var onDismissListener: OnDismissListener? = null fun setTitleRes(@StringRes titleRes: Int): Builder { this.title = context.resources.getString(titleRes) @@ -135,7 +141,7 @@ private constructor( fun setPositiveButtonClickListener( buttonLabel: String? = null, - listener: View.OnClickListener + listener: View.OnClickListener, ): Builder { this.positiveButtonClickListener = listener this.positiveButtonLabel = buttonLabel @@ -144,13 +150,20 @@ private constructor( fun setNegativeButtonClickListener( buttonLabel: String? = null, - listener: View.OnClickListener + listener: View.OnClickListener, ): Builder { this.negativeButtonClickListener = listener this.negativeButtonLabel = buttonLabel return this } + fun setOnDismissListener( + onDismissListener: OnDismissListener, + ): Builder { + this.onDismissListener = onDismissListener + return this + } + fun build(): BasicBottomSheet { require(message != null) { "Message needs to be set" } return BasicBottomSheet( @@ -159,7 +172,8 @@ private constructor( positiveButtonLabel, negativeButtonLabel, positiveButtonClickListener, - negativeButtonClickListener + negativeButtonClickListener, + onDismissListener, ) } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 23e2e6e5..fa46776c 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -377,4 +377,6 @@ No keys imported There are no PGP keys imported in the app yet, press the button below to pick a key file Unlock passphrase cache + AEAD encryption detected + %1$s, see https://passwordstore.app/fix-aead for more information diff --git a/crypto/common/src/main/kotlin/app/passwordstore/crypto/errors/CryptoException.kt b/crypto/common/src/main/kotlin/app/passwordstore/crypto/errors/CryptoException.kt index eb64541e..c2d3ca4d 100644 --- a/crypto/common/src/main/kotlin/app/passwordstore/crypto/errors/CryptoException.kt +++ b/crypto/common/src/main/kotlin/app/passwordstore/crypto/errors/CryptoException.kt @@ -41,8 +41,13 @@ public sealed class CryptoHandlerException(message: String? = null, cause: Throw /** The passphrase provided for decryption was incorrect. */ public class IncorrectPassphraseException(cause: Throwable) : CryptoHandlerException(null, cause) +/** The encrypted material is using an incompatible variant of PGP's AEAD standard. */ +public class NonStandardAEAD(cause: Throwable) : + CryptoHandlerException("GnuPG's AEAD implementation is non-standard and unsupported", cause) + /** No keys were passed to the encrypt/decrypt operation. */ public data object NoKeysProvidedException : CryptoHandlerException(null, null) /** An unexpected error that cannot be mapped to a known type. */ -public class UnknownError(cause: Throwable) : CryptoHandlerException(null, cause) +public class UnknownError(cause: Throwable, message: String? = null) : + CryptoHandlerException(message, cause) diff --git a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPainlessCryptoHandler.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPainlessCryptoHandler.kt index a7087acf..92fbfa64 100644 --- a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPainlessCryptoHandler.kt +++ b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPainlessCryptoHandler.kt @@ -8,6 +8,7 @@ package app.passwordstore.crypto import app.passwordstore.crypto.errors.CryptoHandlerException import app.passwordstore.crypto.errors.IncorrectPassphraseException import app.passwordstore.crypto.errors.NoKeysProvidedException +import app.passwordstore.crypto.errors.NonStandardAEAD import app.passwordstore.crypto.errors.UnknownError import com.github.michaelbull.result.Result import com.github.michaelbull.result.mapError @@ -24,6 +25,7 @@ import org.pgpainless.PGPainless import org.pgpainless.decryption_verification.ConsumerOptions import org.pgpainless.encryption_signing.EncryptionOptions import org.pgpainless.encryption_signing.ProducerOptions +import org.pgpainless.exception.MessageNotIntegrityProtectedException import org.pgpainless.exception.WrongPassphraseException import org.pgpainless.key.protection.SecretKeyRingProtector import org.pgpainless.util.Passphrase @@ -75,6 +77,13 @@ public class PGPainlessCryptoHandler @Inject constructor() : when (error) { is WrongPassphraseException -> IncorrectPassphraseException(error) is CryptoHandlerException -> error + is MessageNotIntegrityProtectedException -> { + if (error.message?.contains("Symmetrically Encrypted Data") == true) { + NonStandardAEAD(error) + } else { + UnknownError(error) + } + } else -> UnknownError(error) } } diff --git a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPainlessCryptoHandlerTest.kt b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPainlessCryptoHandlerTest.kt index 4ec4b7fa..5de2bf4f 100644 --- a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPainlessCryptoHandlerTest.kt +++ b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPainlessCryptoHandlerTest.kt @@ -9,6 +9,7 @@ package app.passwordstore.crypto import app.passwordstore.crypto.CryptoConstants.KEY_PASSPHRASE import app.passwordstore.crypto.CryptoConstants.PLAIN_TEXT import app.passwordstore.crypto.errors.IncorrectPassphraseException +import app.passwordstore.crypto.errors.NonStandardAEAD import com.github.michaelbull.result.getError import com.google.testing.junit.testparameterinjector.TestParameter import com.google.testing.junit.testparameterinjector.TestParameterInjector @@ -137,6 +138,23 @@ class PGPainlessCryptoHandlerTest { } } + @Test + fun aeadEncryptedMaterialIsSurfacedProperly() { + val secKey = PGPKey(TestUtils.getAEADSecretKey()) + val plaintextStream = ByteArrayOutputStream() + val ciphertextStream = TestUtils.getAEADEncryptedFile().inputStream() + val res = + cryptoHandler.decrypt( + listOf(secKey), + "Password", + ciphertextStream, + plaintextStream, + PGPDecryptOptions.Builder().build(), + ) + assertTrue(res.isErr) + assertIs(res.error, message = "${res.error.cause}") + } + @Test fun canHandleFiltersFormats() { assertFalse { cryptoHandler.canHandle("example.com") } diff --git a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/TestUtils.kt b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/TestUtils.kt index 90b98ac9..56c8c1d8 100644 --- a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/TestUtils.kt +++ b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/TestUtils.kt @@ -21,6 +21,9 @@ object TestUtils { fun getAEADSecretKey() = this::class.java.classLoader.getResource("aead_sec").readBytes() + fun getAEADEncryptedFile() = + this::class.java.classLoader.getResource("aead_encrypted_file").readBytes() + enum class AllKeys(val keyMaterial: ByteArray) { ARMORED_SEC(getArmoredSecretKey()), ARMORED_PUB(getArmoredPublicKey()), diff --git a/crypto/pgpainless/src/test/resources/aead_encrypted_file b/crypto/pgpainless/src/test/resources/aead_encrypted_file new file mode 100644 index 0000000000000000000000000000000000000000..d8547bdbd33172486dd7f02839d9222144e0207c GIT binary patch literal 354 zcmV-o0iFJYUISdnhVi7S5f2gp2S717-0TPis(CK~F9u0AV5&1fpAiSAB`rN`Xb4=e zg=R1av2O2|x0`TsmVSDlIGhsHHjYNJFTm+~lVf&7cY@54>8!E)gKE8OfXEZB1w5zJ zz&`;A0uTyzRPpg3I0_g~phQ^jm&k!1Auxwlqjtr9XyfIDc*HZoMF;4v)RlO1Q=nOs zJM>-(d>T|Yi1Ywd(wi|)Gm+I!P{#5_0?6#vNb)s$p@uS{bTfX{;Av5=SbNEkzTYon zw^;BSs0W&$9qKA&JtqR{EI@KM345{YkXFVhhxYuq3R2QTmds};93IKKH=(5H3(cnF z<+=MjIncAvNxrviI{}WM&kItQo*f6Nds1=RN9n5%7wM|FA5+UE@;P4|$piFzh{^7e z1_e2qt6$axnS%J0SfW~$Zp2aO2+`S>(X;z?4|`kT7Qf{`mOf$$=<}-B%>QlWK8W?H AL;wH) literal 0 HcmV?d00001