Repository navigation
android: treat a file that can no longer be decrypted as absent - #160
Merged
Merged
Conversation
A stored file whose keystore key is gone failed every read for good. Auto Backup and device-to-device transfer carry the app's files to a new phone but never keystore keys, and `getOrCreateSecretKey` then minted a fresh key, so every later read failed the GCM tag check. With `authenticationRequired: false` nothing ever clears that: no prompt, no lockout, just an error on every read until the app's data is cleared. `cipherForDecrypt` now checks the key exists before asking for a cipher; a file without one is deleted and reads as absent. A file that fails the tag check with its key present — a key minted over a restored file by an earlier version, or a torn write — is deleted the same way on read. Either way the next write starts clean. Writes now go to `<name>.partial` and are renamed over the file, so a process killed mid-write leaves the old file or the new one. Verified on an API 34 emulator: two new integration tests (a file put back after its key was removed; a file whose tag was flipped) fail on main with "Signature/MAC verification failed" on every read and pass with this change; the suite is 9 of 9. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…aside The previous commit deleted a file whenever KeyStore.containsAlias said its key was missing. On Android 12+ containsAlias answers false for every keystore failure, not only "not found" — the daemon restarting, a busy backend, a locked user — so a hiccup on a read after boot would have deleted a stored secret for good, on authenticated stores too, before any prompt. KeyStore.getKey throws for those and returns null only when the keystore positively reports no such key, so the decision is now made on that, and reading never creates a key: a key minted on the decrypt path was what made a restored file fail its tag check forever. A file judged unreadable — no key, a failed tag check, or too short to hold an IV and a tag — is now renamed to `.unreadable` instead of deleted, so a wrong verdict costs a rename and not the data; `delete` removes it. Below Android 12 nothing on the API surface can tell a lost binder from a missing key, which is the strongest reason for the rename. Writes are synced before the rename, so a power loss cannot leave an empty file under the real name, and a failed write removes its partial file. An empty or short file, what a power loss left before, now reads as absent instead of throwing on every read. The IV read closes its stream and throws IOException on a short read instead of asserting. Found in review, by reading keystore2's AndroidKeyStoreSpi and AndroidKeyStoreCipherSpiBase. Verified on an API 36 emulator: the integration suite, now 10 tests with one for a short file and the keyless test asserting the `.unreadable` file and its removal by delete, passes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A file moved aside as unreadable stayed behind when delete was called on a store with no file, because the handler only deleted when the main file existed. It now also deletes when a partial or set-aside file is there; the answer is still deleteFile's, true only when a stored value was deleted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A stored file whose keystore key is gone fails every read, permanently. Auto Backup and device-to-device transfer carry an app's files to a new phone but never its keystore keys. The decrypt path then minted a fresh key, and every later read failed the GCM tag check, with nothing to clear it on a store without authentication.
KeyStore.getKey, which returns null only when the keystore positively reports no such key and throws for every other failure.containsAliaswould not do: it answers false for every keystore failure, such as the daemon restarting.<name>.v2.txt.unreadable, not deleted. That covers no key, a failed tag check, or a file too short to hold an IV and a tag. A wrong verdict costs a rename, not the data, anddeleteremoves the moved-aside file.<name>.v2.txt.partial, are synced, then renamed over the file, so neither a killed process nor a power loss can leave a corrupt or empty file.Bumps to 6.0.0-dev.7.
Verified on Android emulators:
mainwithSignature/MAC verification failedon every read..unreadablefile and its removal.The review that found the
containsAliasproblem worked from keystore2'sAndroidKeyStoreSpiandAndroidKeyStoreCipherSpiBasesource.🤖 Generated with Claude Code