From fcbd5999f28f287507844b55f2bee82e714620c8 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 3 Nov 2026 13:57:05 +0100 Subject: fix(app): harden the setup key imports - PGP key import reads at most 1 MiB (bounded read, buffer zeroed) and refuses larger files with a new message. - SshKeyViewModel catches any Exception from import and generation (CancellationException rethrown) instead of a short list, so an unexpected provider or sshj error no longer crashes the app. - Pending PGP key bytes are zeroed in a finally block, also on cancellation during addKey after a replace, and an earlier pending pick is zeroed before a new one takes its place. - PGPKeyManager never replaces a stored secret key with a public-only key of the same ID, with or without replace (KeyDowngradeException); the setup ViewModel shows a localized message for it. Unit test added. - storeRecipients strips '#' comments and a trailing '!' as pass reads .gpg-id; tests added. - GitServerViewModel.clearStore returns the failure when the store folder cannot be emptied, so the clone does not start; the plan's Task 3 call site reports it. - readGpgId opens the file with NOFOLLOW_LINKS and reads at most 64 KiB + 1 bytes, refusing more. - SshKey.import writes the new private key to a temp file and moves it over atomically before dropping the old key's Keystore entry, public key and passphrase, so an I/O error keeps the old key. - KeyUtils.isSecretKey notes (ponytail) that GnuPG stub and smartcard keyrings count as secret. Co-Authored-By: Claude Opus 5.5 --- .../kotlin/app/passwordstore/crypto/errors/CryptoException.kt | 4 ++++ .../src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt | 2 ++ .../src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt | 4 ++++ .../test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt | 11 +++++++++++ 4 files changed, 21 insertions(+) (limited to 'crypto') 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 af6145d0..9589ceec 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 @@ -39,6 +39,10 @@ public class KeyNotFoundException(keyId: String) : public class KeyAlreadyExistsException(keyId: String) : KeyManagerException("Pre-existing key was found for $keyId") +/** Attempting to replace a stored secret key with a public-only key of the same `keyId`. */ +public class KeyDowngradeException(keyId: String) : + KeyManagerException("A secret key is stored for $keyId; a public key cannot replace it") + /** Sealed exception types for [app.passwordstore.crypto.CryptoHandler]. */ public sealed class CryptoHandlerException(message: String? = null, cause: Throwable? = null) : CryptoException(message, cause) diff --git a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt index 311e9049..9acd0489 100644 --- a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt +++ b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/KeyUtils.kt @@ -41,6 +41,8 @@ public object KeyUtils { } /** True when [key] parses as a secret key ring, the only kind that can decrypt. */ + // ponytail: GnuPG stub and smartcard keyrings (no usable secret material) count as secret too. + // Upgrade path: check for a secret subkey usable for decryption. public fun isSecretKey(key: PGPKey): Boolean = tryParseKeyring(key) is PGPSecretKeyRing /** diff --git a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt index 6f852af4..ef58ad19 100644 --- a/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt +++ b/crypto/pgpainless/src/main/kotlin/app/passwordstore/crypto/PGPKeyManager.kt @@ -14,6 +14,7 @@ import app.passwordstore.crypto.errors.InvalidKeyException import app.passwordstore.crypto.errors.KeyAlreadyExistsException import app.passwordstore.crypto.errors.KeyDeletionFailedException import app.passwordstore.crypto.errors.KeyDirectoryUnavailableException +import app.passwordstore.crypto.errors.KeyDowngradeException import app.passwordstore.crypto.errors.KeyNotFoundException import app.passwordstore.crypto.errors.NoKeysAvailableException import app.passwordstore.crypto.errors.UnusableKeyException @@ -59,6 +60,9 @@ constructor(filesDir: String, private val dispatcher: CoroutineDispatcher) : keyFile.writeBytes(keyBytes) return@runSuspendCatching key } + // Never with replace either: it would delete the only copy of the private key. + existingKeyRing is PGPSecretKeyRing && incomingKeyRing is PGPPublicKeyRing -> + throw KeyDowngradeException(tryGetId(key)?.toString() ?: "Failed to retrieve key ID") } // Check for replace flag first and if it is false, throw an error if (!replace) diff --git a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt index 0525c28a..81a666f4 100644 --- a/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt +++ b/crypto/pgpainless/src/test/kotlin/app/passwordstore/crypto/PGPKeyManagerTest.kt @@ -9,6 +9,7 @@ import app.passwordstore.crypto.KeyUtils.tryGetId import app.passwordstore.crypto.PGPIdentifier.KeyId import app.passwordstore.crypto.PGPIdentifier.UserId import app.passwordstore.crypto.errors.KeyAlreadyExistsException +import app.passwordstore.crypto.errors.KeyDowngradeException import app.passwordstore.crypto.errors.KeyNotFoundException import app.passwordstore.crypto.errors.NoKeysAvailableException import app.passwordstore.crypto.errors.UnusableKeyException @@ -176,6 +177,16 @@ class PGPKeyManagerTest { assertTrue(keyManager.addKey(publicKey).isErr) } + @Test + fun replaceSecretKeyWithPublicKeyIsRefusedEvenWithReplace() = + runTest(dispatcher) { + keyManager.addKey(secretKey).unwrap() + assertIs(keyManager.addKey(publicKey, replace = true).unwrapError()) + assertIs(keyManager.addKey(publicKey, replace = false).unwrapError()) + // The private key is still the stored one. + assertContentEquals(secretKey.contents, keyManager.getAllKeys().unwrap().single().contents) + } + @Test fun replacePublicKeyWithSecretKey() = runTest(dispatcher) { -- cgit v1.2.3