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 <noreply@anthropic.com>

5 files changed, 120 insertions, 48 deletions

diff --git a/app/src/main/java/app/passwordstore/ui/setup/GitServerViewModel.kt b/app/src/main/java/app/passwordstore/ui/setup/GitServerViewModel.kt
index f2b8ca30..d549624e 100644
--- a/app/src/main/java/app/passwordstore/ui/setup/GitServerViewModel.kt
+++ b/app/src/main/java/app/passwordstore/ui/setup/GitServerViewModel.kt
@@ -83,13 +83,16 @@ constructor(
   /**
    * Empties the store folder at [root] for a clone, unless it is empty already. Never follows a
    * symlink: `deleteRecursively()` would, and a `store/link -> ../keys` link would take the PGP
-   * keys with it.
+   * keys with it. Returns null, or why it failed as a string resource: then the clone must not
+   * start.
    */
-  suspend fun clearStore(root: File) {
-    if (withContext(dispatcherProvider.io()) { cloneTarget(root) } == CloneTarget.Empty) return
+  suspend fun clearStore(root: File): Int? {
+    if (withContext(dispatcherProvider.io()) { cloneTarget(root) } == CloneTarget.Empty) return null
     progress = R.string.delete_directory_progress_text
-    withContext(dispatcherProvider.io()) { root.deleteWithoutFollowingLinks() }
+    if (!withContext(dispatcherProvider.io()) { root.deleteWithoutFollowingLinks() })
+      return R.string.git_clone_delete_failed
     progress = R.string.git_clone_progress
+    return null
   }
 
   /** Set while [finishClone]'s key check runs: the clone's [progress] stays up until it is done. */
diff --git a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt
index e6000206..ed7fcf37 100644
--- a/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt
+++ b/app/src/main/java/app/passwordstore/ui/setup/PgpKeysViewModel.kt
@@ -13,11 +13,13 @@ import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.setValue
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.viewModelScope
+import app.passwordstore.R
 import app.passwordstore.crypto.KeyUtils
 import app.passwordstore.crypto.PGPIdentifier
 import app.passwordstore.crypto.PGPKey
 import app.passwordstore.crypto.PGPKeyManager
 import app.passwordstore.crypto.errors.KeyAlreadyExistsException
+import app.passwordstore.crypto.errors.KeyDowngradeException
 import app.passwordstore.data.repo.PasswordRepository
 import app.passwordstore.util.coroutines.DispatcherProvider
 import com.github.michaelbull.result.onSuccess
@@ -126,18 +128,23 @@ constructor(
     }
   }
 
-  /** Reads the key file at [uri] and imports it. */
+  /** Reads the key file at [uri] (at most [MAX_KEY_BYTES]) and imports it. */
   fun import(uri: Uri, context: Context) {
     if (busy) return
     busy = true
     val resolver = context.applicationContext.contentResolver
+    val tooLarge = context.getString(R.string.pgp_key_import_too_large)
+    val downgrade = context.getString(R.string.pgp_key_import_downgrade)
     viewModelScope.launch {
       try {
         val bytes =
           withContext(dispatcherProvider.io()) {
-            resolver.openInputStream(uri)?.use { it.readBytes() }
+            resolver.openInputStream(uri)?.use {
+              readAtMost(it, MAX_KEY_BYTES) ?: throw IOException(tooLarge)
+            }
           }
-        if (bytes == null) importResult = KeyImport.Failed(null) else add(bytes, replace = false)
+        if (bytes == null) importResult = KeyImport.Failed(null)
+        else add(bytes, replace = false, downgrade)
       } catch (e: IOException) {
         importResult = KeyImport.Failed(e.message)
       } catch (e: SecurityException) {
@@ -168,22 +175,34 @@ constructor(
     importResult = null
   }
 
-  /** Imports [bytes]; they are zeroed afterwards unless the replace question keeps them. */
-  private suspend fun add(bytes: ByteArray, replace: Boolean) {
-    val result = keyManager.addKey(PGPKey(bytes), replace)
-    if (!replace && result.isErr && result.error is KeyAlreadyExistsException) {
-      pending = bytes
-      importResult = KeyImport.Exists
-      return
+  /**
+   * Imports [bytes]; they are zeroed afterwards, also on failure or cancellation, unless the
+   * replace question keeps them. [downgrade] is the message for a public key over a secret one.
+   */
+  private suspend fun add(bytes: ByteArray, replace: Boolean, downgrade: String? = null) {
+    var keep = false
+    try {
+      val result = keyManager.addKey(PGPKey(bytes), replace)
+      if (!replace && result.isErr && result.error is KeyAlreadyExistsException) {
+        // A question still open from an earlier pick is superseded: zero its bytes first.
+        dropPending()
+        pending = bytes
+        keep = true
+        importResult = KeyImport.Exists
+        return
+      }
+      // The ID is read before the bytes are zeroed: the returned key shares them.
+      importResult =
+        if (result.isOk) {
+          val id = withContext(dispatcherProvider.default()) { KeyUtils.tryGetId(result.value) }
+          KeyImport.Imported(id?.toString().orEmpty())
+        } else if (result.error is KeyDowngradeException) {
+          KeyImport.Failed(downgrade ?: result.error.message)
+        } else KeyImport.Failed(result.error.message)
+      if (result.isOk) updateKeySet()
+    } finally {
+      if (!keep) bytes.fill(0)
     }
-    // The ID is read before the bytes are zeroed: the returned key shares them.
-    importResult =
-      if (result.isOk) {
-        val id = withContext(dispatcherProvider.default()) { KeyUtils.tryGetId(result.value) }
-        KeyImport.Imported(id?.toString().orEmpty())
-      } else KeyImport.Failed(result.error.message)
-    bytes.fill(0)
-    if (result.isOk) updateKeySet()
   }
 
   /**
@@ -223,3 +242,6 @@ constructor(
     dropPending()
   }
 }
+
+/** The largest key file imported: far above any real key, small enough to hold in memory. */
+private const val MAX_KEY_BYTES = 1024 * 1024
diff --git a/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt b/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt
index 8c7b20a1..ade23aa2 100644
--- a/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt
+++ b/app/src/main/java/app/passwordstore/ui/setup/SetupModel.kt
@@ -9,8 +9,11 @@ import app.passwordstore.crypto.PGPIdentifier
 import app.passwordstore.util.settings.AuthMode
 import java.io.File
 import java.io.IOException
+import java.io.InputStream
+import java.nio.channels.Channels
 import java.nio.file.Files
 import java.nio.file.LinkOption
+import java.nio.file.StandardOpenOption
 import java.util.Locale
 
 /** Why a repository URL is questioned before it is saved, as the old server screen checked it. */
@@ -73,9 +76,17 @@ fun cloneTarget(dir: File): CloneTarget {
   }
 }
 
-/** The recipients named by a `.gpg-id` file's [text]; blank and unparsable lines are skipped. */
+/**
+ * The recipients named by a `.gpg-id` file's [text], read as pass reads it: a `#` starts a comment,
+ * and a trailing `!` (GnuPG's "this exact subkey") is dropped. Blank and unparsable lines are
+ * skipped.
+ */
 fun storeRecipients(text: String): List<PGPIdentifier> =
-  text.lines().map(String::trim).filter(String::isNotEmpty).mapNotNull(PGPIdentifier::fromString)
+  text
+    .lines()
+    .map { it.substringBefore('#').trim().removeSuffix("!") }
+    .filter(String::isNotEmpty)
+    .mapNotNull(PGPIdentifier::fromString)
 
 /**
  * A `.gpg-id` naming [keys]: each 16-hex long key ID in uppercase on a line of its own, the format
@@ -91,12 +102,34 @@ private const val MAX_GPG_ID_BYTES = 64 * 1024
  * than 64 KiB or unreadable. Blocking: call it off the main thread.
  */
 fun readGpgId(dir: File): String? {
-  val file = File(dir, ".gpg-id")
-  val path = file.toPath()
+  val path = File(dir, ".gpg-id").toPath()
+  if (!Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS)) return null
+  // Opened without following a link too: one swapped in after the check is refused.
+  val options = setOf(StandardOpenOption.READ, LinkOption.NOFOLLOW_LINKS)
   return try {
-    if (!Files.isRegularFile(path, LinkOption.NOFOLLOW_LINKS)) null
-    else if (Files.size(path) > MAX_GPG_ID_BYTES) null else file.readText()
+    Channels.newInputStream(Files.newByteChannel(path, options))
+      .use { readAtMost(it, MAX_GPG_ID_BYTES) }
+      ?.decodeToString()
   } catch (_: IOException) {
     null
   }
 }
+
+/**
+ * At most [limit] bytes of [input], or null when it holds more. Reads at most [limit] + 1 bytes,
+ * whatever the stream's size, and zeroes its buffer: the bytes may be a private key.
+ */
+fun readAtMost(input: InputStream, limit: Int): ByteArray? {
+  val buffer = ByteArray(limit + 1)
+  try {
+    var size = 0
+    while (size < buffer.size) {
+      val read = input.read(buffer, size, buffer.size - size)
+      if (read < 0) break
+      size += read
+    }
+    return if (size > limit) null else buffer.copyOf(size)
+  } finally {
+    buffer.fill(0)
+  }
+}
diff --git a/app/src/main/java/app/passwordstore/ui/setup/SshKeyViewModel.kt b/app/src/main/java/app/passwordstore/ui/setup/SshKeyViewModel.kt
index d70b06af..bc3e9474 100644
--- a/app/src/main/java/app/passwordstore/ui/setup/SshKeyViewModel.kt
+++ b/app/src/main/java/app/passwordstore/ui/setup/SshKeyViewModel.kt
@@ -17,10 +17,8 @@ import app.passwordstore.R
 import app.passwordstore.util.coroutines.DispatcherProvider
 import app.passwordstore.util.git.sshj.SshKey
 import dagger.hilt.android.lifecycle.HiltViewModel
-import java.io.IOException
-import java.security.GeneralSecurityException
-import java.security.ProviderException
 import javax.inject.Inject
+import kotlin.coroutines.cancellation.CancellationException
 import kotlinx.coroutines.launch
 import kotlinx.coroutines.withContext
 
@@ -83,11 +81,10 @@ class SshKeyViewModel @Inject constructor(private val dispatcherProvider: Dispat
         withContext(dispatcherProvider.io()) { SshKey.import(uri) }
         imported = true
         version++
-      } catch (e: IOException) {
-        failure = e.message.orEmpty()
-      } catch (e: IllegalArgumentException) {
-        failure = e.message.orEmpty()
-      } catch (e: SecurityException) {
+      } catch (e: CancellationException) {
+        throw e
+      } catch (e: Exception) {
+        // Any failure of a provider or of sshj, not only I/O: an uncaught one would crash the app.
         failure = e.message.orEmpty()
       } finally {
         busy = false
@@ -114,11 +111,10 @@ class SshKeyViewModel @Inject constructor(private val dispatcherProvider: Dispat
             SshKey.sshPublicKey.orEmpty()
           }
         version++
-      } catch (e: GeneralSecurityException) {
-        failure = e.message.orEmpty()
-      } catch (e: ProviderException) {
-        failure = e.message.orEmpty()
-      } catch (e: IOException) {
+      } catch (e: CancellationException) {
+        throw e
+      } catch (e: Exception) {
+        // Keystore and provider failures come as many exception types; none may crash the app.
         failure = e.message.orEmpty()
       } finally {
         busy = false
diff --git a/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt b/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt
index 8d5519a8..06cf5bc8 100644
--- a/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt
+++ b/app/src/main/java/app/passwordstore/util/git/sshj/SshKey.kt
@@ -29,6 +29,8 @@ import com.github.michaelbull.result.getOrElse
 import com.github.michaelbull.result.runCatching
 import java.io.File
 import java.io.IOException
+import java.nio.file.Files
+import java.nio.file.StandardCopyOption
 import java.security.KeyFactory
 import java.security.KeyPair
 import java.security.KeyPairGenerator
@@ -203,15 +205,20 @@ object SshKey {
   }
 
   private fun delete() {
-    androidKeystore.deleteEntry(KEYSTORE_ALIAS)
     if (privateKeyFile.isFile) {
       privateKeyFile.delete()
     }
+    deleteKeyExtras()
+    type = null
+  }
+
+  /** Everything of the current key but its private key file. */
+  private fun deleteKeyExtras() {
+    androidKeystore.deleteEntry(KEYSTORE_ALIAS)
     if (publicKeyFile.isFile) {
       publicKeyFile.delete()
     }
     context.getEncryptedGitPrefs().edit { remove(PreferenceKeys.SSH_KEY_LOCAL_PASSPHRASE) }
-    type = null
   }
 
   fun import(uri: Uri) {
@@ -241,12 +248,23 @@ object SshKey {
       )
 
     // At this point, we are reasonably confident that we have actually been provided a private
-    // key and delete the old key.
-    delete()
-    // Canonicalize line endings to '\n'.
-    privateKeyFile.writeText(lines.joinToString("\n"))
-
+    // key. It replaces the old one atomically (an I/O error keeps the old key), then the old key's
+    // Keystore entry, public key and passphrase go.
+    val temp = File(privateKeyFile.parentFile, ".${privateKeyFile.name}.tmp")
+    try {
+      // Canonicalize line endings to '\n'.
+      temp.writeText(lines.joinToString("\n"))
+      Files.move(
+        temp.toPath(),
+        privateKeyFile.toPath(),
+        StandardCopyOption.REPLACE_EXISTING,
+        StandardCopyOption.ATOMIC_MOVE,
+      )
+    } finally {
+      temp.delete()
+    }
     type = Type.Imported
+    deleteKeyExtras()
   }
 
   /** Deletes an Ed25519 key wrapped by androidx security-crypto, which can no longer be read. */