Wipe keys before dispatching decrypt-failure events - #39
Draft
robbinjanssen wants to merge 1 commit into
Draft
Conversation
The Laravel events expose their SecureMessage as a public readonly property, so listeners (and anything they serialise the event to, such as a queued listener writing to Redis) can read it. Most decrypt-failure paths hand over a wiped instance, because the DecryptException constructor wipes the keys when it is given the secure message. Three paths throw without it and left the decrypted key material on the dispatched instance: - a missing storage key file (database key + verification code present); - malformed stored ciphertext (all key parts present); - a missing file blob (database key + verification code present). This violates the split-key promise that the key parts never co-locate. Wipe the secure message in the catch block, before any event is dispatched, regardless of whether the exception carried it. The already-wiped paths are unaffected (wiping is idempotent). Regression tests assert the dispatched event carries no key material on both null paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
robbinjanssen
added
the
bugfix
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
2 participants
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.
Summary
Fixes a key-hygiene defect found during a security review of the v2 line: decrypt-failure events could hand unwiped key material to listeners.
The Laravel events expose their
SecureMessagevia apublic readonlyproperty (added in the v2.0 modernization), so any listener — and anything a queued listener serializes the event to (Redis, database, an error tracker) — can read it. The library's wipe contract only runs insideDecryptException::__constructwhen the exception is given a secure message. Three failure paths throw without one and therefore dispatched an instance that still held decrypted keys:fromString()hardening) — all key parts present, i.e. the full 32-byte key reconstructable, co-located with the (still valid) encrypted content. This dispatch path is new to v2.0: on v1 malformed input was an uncaught PHP error, so no event fired.This breaks the core promise that the three key parts never co-locate.
Fix
Wipe the secure message in
Laravel\Factory::decryptMessage()'s catch block, before any event is dispatched, regardless of whether the exception carried it. Wiping is idempotent, so the paths that were already wiped (wrong code, expiry, hit-point limit) are unaffected. One chokepoint covers all present and future failure paths.Verification
composer test: 51 tests, 333 assertions, green.SecureMessagehas no database key, storage key, meta key or verification code. Confirmed they fail without the fix (2 failures) and pass with it.composer analyse(PHPStan level 6) and php-cs-fixer: clean.Branch/merge notes
Branched off
feature/file-messages(v2.1) and targets it, so the fix covers both the v2.0 event paths and the v2.1 file-blob path. Merge order: #37 (v2.0) → #38 (v2.1) → this. GitHub retargets automatically as each base merges.🤖 Generated with Claude Code