feat(security): implement recipient guard and related tests for conversation key management - #65
Conversation
…rsation key management
XeldarAlz
left a comment
XeldarAlz
left a comment
There was a problem hiding this comment.
Built the branch and ran the suite locally: clean Release build, all tests green. CI hasn't run on this PR, so that's the only build evidence.
The IKeysClient / IKeyVault split is good and worth keeping regardless of what happens to the guard.
Requesting changes on the guard itself. The short version:
- The roster never activates in production. Nothing calls
SetAuthorizedMembers, so the group-chat protection the tests demonstrate doesn't exist outside the tests. - The pin check is bypassed by
KeyVersion + 1, which the server controls. - Two of the changes are fail-open, which is the wrong direction for a security patch.
Details inline, each one on the line it applies to.
One design point that cuts across all of it: IsAuthorized(UserPublicKeyDto) has no conversation dimension, one guard instance is shared across every chat/Velvet/Gram thread, and SetAuthorizedMembers clears and replaces. So the roster can't simply be wired up later without one conversation's members rejecting another's. That needs rethinking before the roster idea can work at all.
Separately: the guard only checks who we wrap for, never the key material we accept. There's a broader gap on that side that this PR doesn't cover and that I'd rather not spell out in a public thread. I'll follow up with you directly. It doesn't change the review below, but it does mean this shouldn't be treated as closing the issue you raised.
Happy to take this in two pieces: the Velvet/Gram two-party filter with the fixes below, and a separate tracked issue for the deeper item with you involved.
| var keyVault = new KeyVault(configuration, aethernetSession, aethernet.Keys); | ||
| var peerKeys = new PeerKeyDirectory(configuration, aethernet.Keys); | ||
| var conversationKeys = new ConversationKeyStore(aethernet.Keys, keyVault); | ||
| var conversationKeys = new ConversationKeyStore(aethernet.Keys, keyVault, new PinnedRecipientGuard()); |
There was a problem hiding this comment.
Blocking: nothing ever calls SetAuthorizedMembers, so rosterConfigured stays false for the life of the process and the roster check is inert here.
I re-ran your group-chat test with this exact wiring and the injected recipient still receives a usable wrap. GuardRosterRejectsInjectedRecipientInGroupChat passes only because the test itself configures a roster.
|
|
||
| public bool IsAuthorized(UserPublicKeyDto recipient) | ||
| { | ||
| if (rosterConfigured && !authorizedMembers.ContainsKey(recipient.UserId)) |
There was a problem hiding this comment.
Blocking: with no roster configured this short-circuits to authorized for every recipient. See PhoneServices.cs:153.
|
|
||
| if (pins.TryGetValue(recipient.UserId, out var pinned)) | ||
| { | ||
| if (recipient.KeyVersion > pinned.Version) |
There was a problem hiding this comment.
Blocking: KeyVersion arrives in the same server response as PublicKey, so an attacker sends version + 1 and this accepts any key and re-pins it as truth.
Verified: pin bob@v1, then bob@v2 carrying an attacker key is accepted. The same-version check costs one integer to bypass.
Detecting substitution needs something out of band. PeerKeyDirectory already persists KnownPeerKeyVersions and raises a rotation notice, and encryption.safetyChanged is already wired through to the thread UI. Extending that is a better home than a second in-memory pin store that resets every plugin load.
| var guard = new PinnedRecipientGuard(); | ||
| guard.Pin("bob", 1, "honest-key"); | ||
| Assert.True(guard.IsAuthorized(new UserPublicKeyDto("bob", "rotated-key", 2))); | ||
| } |
There was a problem hiding this comment.
This asserts the bypass as intended behaviour: any key at a higher version is accepted.
| break; | ||
| } | ||
|
|
||
| keys = RestrictToMembers(keys, myUserId, otherId); |
There was a problem hiding this comment.
Blocking: myUserId is session.CurrentUser?.Id ?? string.Empty (ChatThreadStoreBase.cs:219), and nothing checks it's actually present in MemberKeys.
If it's ever empty or from a different id space, I get filtered out of my own member list, generation 1 is created wrapped for the peer only, and the in-memory CEK keeps CanEncrypt returning true. Verified: with an empty id the created generation contains exactly one wrap, for the peer, none for me. Messages send fine until the next launch, then that thread is permanently undecryptable.
Worth noting Pair(myUserId, otherId) previously used these ids only as a cache key, where a mismatch cost a cache miss. This makes the same assumption load-bearing for who gets keys.
Suggest: if myUserId isn't in MemberKeys, skip the filter and return the response unchanged.
| return keys with | ||
| { | ||
| MemberKeys = FilterKeys(keys.MemberKeys, first, second), | ||
| MembersWithoutKeys = FilterIds(keys.MembersWithoutKeys, first, second), |
There was a problem hiding this comment.
Blocking: filtering MembersWithoutKeys deletes the signal the gen-0 path aborts on (MembersWithoutKeys.Length > 0 at line 139).
If the server reports a third participant with no keys, the client now mints generation 1 instead of refusing. A security change shouldn't remove an abort condition.
| private readonly ConcurrentDictionary<string, int> currentGenerations = new(StringComparer.Ordinal); | ||
|
|
||
| public ConversationKeyStore(KeysClient client, KeyVault vault) | ||
| public ConversationKeyStore(IKeysClient client, IKeyVault vault, IWrapRecipientGuard? guard = null) |
There was a problem hiding this comment.
Non-blocking: an optional guard means the check is off by default, and BuildWraps treats null as authorize-everything. Require the dependency and pass a no-op implementation so a new call site can't silently lose it.
| private readonly ConcurrentQueue<(string UserId, string Reason)> rejections = new(); | ||
| private volatile bool rosterConfigured; | ||
|
|
||
| public IReadOnlyCollection<(string UserId, string Reason)> Rejections => rejections.ToArray(); |
There was a problem hiding this comment.
Non-blocking: Rejections is never read anywhere and the queue is unbounded, so a hostile server grows it for the life of the process. Drop it, or log at the point of rejection.
| }; | ||
| } | ||
|
|
||
| private static UserPublicKeyDto[] FilterKeys(UserPublicKeyDto[] items, string first, string second) |
There was a problem hiding this comment.
Non-blocking: FilterKeys and FilterIds are the same function twice, and each allocates a List plus a ToArray on a path that runs on every thread open and every key-status refresh. In the honest case nothing is filtered, so return the input array unchanged when the count is unchanged.
| var store = new ConversationKeyStore(client, new FakeVault(victim)); | ||
| await store.EnsureVelvetKeysAsync("bob", "victim", CancellationToken.None); | ||
|
|
||
| Assert.DoesNotContain(client.VelvetWraps, wrap => wrap.RecipientUserId == "evil"); |
There was a problem hiding this comment.
Non-blocking: no positive control. Nothing asserts a legitimate stale peer still gets re-wrapped after the filter, which is the regression this change could plausibly cause.
I traced it and the test does fail if the filter is removed, so it's valid, just weak. Assert.DoesNotContain against an empty list passes for "blocked it" and "nothing happened" alike.
2 participants
What
Add a guard that runs before the client wraps a conversation key, so it only wraps for recipients it can vouch for.
PinnedRecipientGuardrefuses a wrap when a member's key material changes at the same or lower version, and (when a roster is configured) when the recipient isn't a known member. Velvet and Gram 1:1 threads are additionally restricted to the two known participants. I also splitKeysClient/KeyVaultbehindIKeysClient/IKeyVaultso the flow is testable.Why
The client wrapped the conversation key for whatever recipients the server named in the
/keysresponse, without checking they belonged to the conversation or that their key material hadn't changed. A malicious or compromised server could inject a recipient, or swap a member's key at the same version, and have the client hand it a usable wrap, defeating E2E for that thread. A single stale-wrap response can even re-wrap every cached generation.This is client-side defense-in-depth. The server still has to enforce membership when it hands out wraps; that is the authoritative fix and it lives in the backend, not here.
Closes #
How to test
dotnet build Aetherphone.sln -c Releaseanddotnet test. The newPinnedRecipientGuardTestsandConversationKeyStoreSecurityTestscover the guard and the DM restriction: an injected recipient is refused, a same-version key swap is refused, a higher-version rotation is still accepted, a group member off the roster is refused, and a 1:1 DM drops an injected third party./phoneand send messages in Messages, Velvet, and Aethergram DMs. Normal encrypted messaging is unaffected. The guard only changes behavior when the server names an illegitimate recipient, so honest conversations behave exactly as before.Checklist
dotnet build -c ReleasepassesWindows/Components/widgets, nowhatcomments (core change, no UI touched; no comments added)