Never let a degraded RNG path succeed quietly

[kwsantiago]

Summary

• Audit of the app against the bug class in Block's COLDCARD predictable-RNG disclosure: a degraded cryptographic path that succeeds silently.
• The randomness itself came back clean, and that is the main result. Every draw in production Kotlin already goes through java.security.SecureRandom or UUID.randomUUID(): the NIP-55 permission-database key, the Keystore HMAC key, the PIN PBKDF2 salt, the recovery-screen hex, and the relay reconnect jitter. No kotlin.random, no java.util.Random, no Math.random(), no setSeed, no SHA1PRNG, and minSdk = 33 puts the app well past the Android versions with weak SecureRandom seeding.
• What the audit did find is the same failure mode one door over: AndroidKeystoreStorage had a single initCipherWithKey(key, mode, ivBase64) used for both directions, which would happily initialise an AES-GCM cipher for encryption with a caller-supplied IV. Reusing a GCM IV under one key destroys confidentiality and authenticity. (See the review round for a correction to how this PR originally described the runtime behaviour of that mistake on Keystore-backed keys.)

What changed

Every call site happened to pass null on the encrypt path, so nothing was broken. But "happened to" is the property this audit exists to remove. Encryption and decryption are now separate functions: initCipherForEncryption(key) takes no IV and lets the provider draw a fresh one, initCipherForDecryption(key, ivBase64) requires one. Passing an IV to the encrypt path is a compile error rather than a convention, and the shared error mapping (including the KeyPermanentlyInvalidatedException re-import message) is preserved in one helper.

scripts/check-rng-hygiene.sh pins three rules in CI: non-cryptographic generators in production code, SecureRandom weakened at the call site (setSeed, an explicitly requested SHA1PRNG), and any Cipher.init with three or more arguments that is not provably a decrypt (see the review round for why it is phrased that way and not in terms of ENCRYPT_MODE). It runs as a step in the existing build job alongside check-toolchain-pins.sh and the other guards, before the SDK/NDK/Rust setup, so it fails in seconds rather than after a toolchain install.

Review notes

The guard is written to survive the refactors that defeat a naive grep. It matches the RNG token rather than a receiver shape, because pinning "0123..".random() is bypassed by hoisting the alphabet into a val — which is the same code, one refactor away. It parses // and /* */ comments with real state tracking and blanks string literals, so prose naming a banned symbol is not a finding and a log message containing "Random" is not either. It joins wrapped calls into one statement, so a rustfmt-style four-line cipher.init(...) is judged whole instead of as four innocuous lines. And it makes two passes: rules about code shape see literals blanked, while the rule about an argument value (SHA1PRNG) sees them intact.

It also fails closed. A scanner error, a run outside a git work tree, or an empty file list is reported as a failure, never as a pass — a guard that prints "OK" when it scanned nothing is worse than no guard, because it gets trusted. Both cases are verified below.

Decision log

Hardest decision: whether to fix the cipher helper at all, given no call site was wrong. A require(mode != ENCRYPT_MODE || ivBase64 == null) would have been one line, but it defends at runtime against a mistake the type system can prevent outright, and it needs a test that cannot run without a device. Splitting the function costs a few more lines and makes the class of mistake unrepresentable.

Alternatives rejected:

• A separate rng-hygiene workflow, matching the two sibling repos. This repo already has a scripts/check-*.sh convention running inside the build job, and those steps sit before the heavy setup, so they already fail fast. Consistency won.
• Adding a docs/SECURITY.md. There is no such file in this repo and no security-doc convention to slot into; the invariant is documented where it is enforced (the guard header) and where it can be violated (the comment above the split functions).

Least confident about: rule 1 flags .random() on any receiver, so a future legitimate list.random() for UI flavour will trip it. That is a deliberate false-positive bias — the author either goes through SecureRandom or adds an // rng-hygiene: ok marker consciously — but it will annoy someone eventually.

What is not verified

The split cipher functions have no executing test. They are private, they need the Android Keystore, and the existing coverage for this class is instrumented (KeystoreEncryptedPrefsBindingTest), which needs a device or emulator this workspace does not have. The instrumented-tests CI job is the first place it actually runs. verifyKeepVersion was skipped locally because the keep path is a symlink to the main keep repo, which is currently on a different branch; CI checks out the pinned SHA itself and will verify it properly.

Test plan

• ./gradlew compileDebugKotlin testDebugUnitTest lintDebug (JDK 21): BUILD SUCCESSFUL, 171 unit tests passed, 0 failed, lint clean with 0 errors and 0 warnings
• scripts/check-rng-hygiene.sh exits 0 on the tree
• Guard verified by reintroduction (superseded by the fuller matrix in the review round below)
• Instrumented tests — not run locally, no device attached; CI's instrumented-tests job covers the Keystore path

Review round

A security review ran over the branch with the explicit job of falsifying the "randomness was already clean" claim. The claim held. It traced every RNG draw in all 109 production Kotlin files and every AES-GCM IV, confirmed the cipher split is behaviour-preserving call site by call site (including the legacy-IV migration path and the error-message mapping), and found no non-CSPRNG source, no reused IV, and no degraded-but-succeeding path.

The findings were in the guard, and two of them were blockers.

Rule 3 did not catch the shape this very PR removes. Checking out main's AndroidKeystoreStorage.kt into a scratch tree and running the guard printed RNG hygiene: OK. The rule required ENCRYPT_MODE and the spec constructor in the same statement, and the old cipher.init(mode, key, spec) had both hoisted into variables. Hoisting the spec, hoisting the mode, or passing a numeric opmode each bypassed it.

The rule is now structural instead of a token pair, and the reformulation is better than the original intent: the invariant never mentioned ENCRYPT at all. Cipher.init takes a parameter spec only when decrypting, so any init with three or more arguments has to be provably a decrypt. Argument counting is done at paren depth so nested calls do not inflate it. All four demonstrated bypasses are caught, including a straight revert of this PR's split.

The guard could print OK on a failing scan. preprocess's exit status was checked but scan/scan_with_strings were bare command substitutions, so an ERE the local awk rejects (the BSD-awk case the header itself raises) yielded an empty result that read as "no violations". Both now abort the run.

Also fixed, each verified by reintroduction:

• The strings-intact pass had no string awareness, so the MIME wildcard "*/*" at BackupRestoreScreen.kt:304 opened an unterminated block comment and hid the rest of that file from the SHA1PRNG rule. String state is tracked in both modes now; a SHA1PRNG appended to that specific file is detected.
• Rule 1 missed SplittableRandom, a fully-qualified java.util.concurrent.ThreadLocalRandom.current() (the leading . defeated the token anchor), and a seeded SecureRandom(byteArray) constructor.
• The opt-out marker waived rule 3. It no longer can: rule 3 reads a separate preprocessing pass where the marker is not honoured, because a catastrophic-and-silent failure mode should not have a // trust me escape hatch. The marker still works for rules 1 and 2, where UI jitter is a real use.
• app/src/debug/** was excluded from the scan despite shipping in debug builds. No such directory exists today, so this closed a future hole rather than a live one.
• The guard regexes now use bracket expressions rather than backslash escapes. gawk strips \( from an -v variable and then rejects the resulting unbalanced regex; that broke the sibling repo's guard in CI, where it correctly failed closed rather than passing.

One correction to this PR's own framing. The comment I wrote said an encrypt with a supplied IV "succeeds and the ciphertext looks fine". That is true of AES-GCM in general but probably not of this call path: Keystore keys are created with randomized encryption required by default, which is expected to reject a caller-supplied IV at init. I could not confirm the platform behaviour from this workspace, so the comment now says what is actually known: the platform backstop comes from another component, does not apply to a non-Keystore key, and is not visible at this call site. The split is still worth having for those reasons, not because the runtime would have stayed quiet.

Filed, not fixed here: the review found a pre-existing defect of exactly this PR's thesis in nip55/PermissionDatabase.kt:143-160. KeystoreEncryptedPrefs.getTypedValue swallows any decrypt exception and returns the default, so "unreadable" and "absent" are indistinguishable at that call site, and a transient Keystore fault makes it mint and store a new SQLCipher passphrase, orphaning the permissions database and the entire signing audit log. It logs nothing. That is a separate bug in a separate subsystem and belongs in its own change.

Test plan (updated)

• ./gradlew compileDebugKotlin testDebugUnitTest lintDebug (JDK 21): BUILD SUCCESSFUL, 171 unit tests passed, lint clean with 0 errors and 0 warnings; recompiled clean after the comment correction
• Guard: 15-case matrix. Flags a straight revert of this PR, a hoisted spec, a hoisted mode, a numeric opmode, an opt-out marker attempting to waive rule 3, kotlin.random, a hoisted-alphabet .random(), java.util.Random, Math.random(), SplittableRandom, fully-qualified ThreadLocalRandom, SecureRandom.setSeed, a seeded SecureRandom constructor, and an explicit SHA1PRNG provider
• Stays clean on: a decrypt with a supplied IV, a two-argument encrypt, an unseeded SecureRandom, an opt-out marker on rules 1 and 2, a comment naming banned symbols, and a string literal mentioning java.util.Random
• Guard fails closed at both stages: a stub awk exiting 2 during preprocessing and during scanning, and a run outside a git work tree
• SHA1PRNG appended to BackupRestoreScreen.kt (the file with the "*/*" literal) is now detected, confirming the string-state fix
• Instrumented tests — not run locally, no device attached; CI's instrumented-tests job covers the Keystore path

[coderabbitai]

Walkthrough

Changes

Cryptographic hygiene enforcement

Layer / File(s)	Summary
Separate Keystore cipher paths app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt	Metadata and share encryption now generate fresh AES-GCM IVs. Decryption decodes stored IVs. Cipher errors use centralized handling.
Production-source hygiene scanner scripts/check-rng-hygiene.sh	The new scanner detects non-cryptographic randomness, weakened SecureRandom, and caller-supplied AES-GCM encryption IVs. It fails closed on findings or scan errors.
CI hygiene enforcement .github/workflows/ci.yml	The build job runs the hygiene scanner before Android SDK setup.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• privkeyio/keep-android#4: Modifies AndroidKeystoreStorage.kt cipher initialization.
• privkeyio/keep-android#29: Changes share cipher handling and IV management.
• privkeyio/keep-android#90: Further hardens AndroidKeystoreStorage cipher handling.

Suggested reviewers: wksantiago

Poem

A rabbit checks each random spark,
And guards the cipher through the dark.
Fresh IVs hop into the stream,
Stored ones guide the decryption dream.
CI watches every trail,
Unsafe patterns cannot prevail.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: preventing degraded RNG and encryption paths from succeeding silently.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch rng-hygiene-guard

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/check-rng-hygiene.sh`:
- Around line 87-150: Update the quote-handling branch in the awk strip function
so entering a string always sets instr = 1, including when keepstrings is
enabled. Preserve string contents in keepstrings mode, while only emitting the
masked escaped-quote output when keepstrings is disabled, ensuring later comment
delimiters inside strings are ignored.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 951ec5fc-f8e4-45ff-9aed-116ad459f44f

📥 Commits

Reviewing files that changed from the base of the PR and between b60b88b and 92facf9.

📒 Files selected for processing (3)

• .github/workflows/ci.yml
• app/src/main/kotlin/io/privkey/keep/storage/AndroidKeystoreStorage.kt
• scripts/check-rng-hygiene.sh