-
Notifications
You must be signed in to change notification settings - Fork 25
feat(security): implement recipient guard and related tests for conversation key management #65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| using System.Security.Cryptography; | ||
| using Aetherphone.Core.Aethernet.Clients; | ||
| using Aetherphone.Core.Aethernet.Contracts; | ||
| using Aetherphone.Core.Crypto; | ||
| using Xunit; | ||
|
|
||
| namespace Aetherphone.Tests; | ||
|
|
||
| public sealed class ConversationKeyStoreSecurityTests | ||
| { | ||
| [Fact] | ||
| public async Task DmMemberRestrictionDropsInjectedThirdParty() | ||
| { | ||
| using var victim = CryptoBox.TryGenerateIdentity()!; | ||
| using var bob = CryptoBox.TryGenerateIdentity()!; | ||
| using var attacker = CryptoBox.TryGenerateIdentity()!; | ||
| var cek = CryptoBox.GenerateCek(); | ||
|
|
||
| var response = new ConversationKeysDto( | ||
| "velvet-thread", | ||
| 1, | ||
| new[] { new KeyWrapDto(1, CryptoBox.WrapCek(cek, CryptoBox.ExportPublicKey(victim))!, "victim", 1, 0) }, | ||
| new[] | ||
| { | ||
| new UserPublicKeyDto("victim", CryptoBox.ExportPublicKey(victim), 1), | ||
| new UserPublicKeyDto("bob", CryptoBox.ExportPublicKey(bob), 1), | ||
| new UserPublicKeyDto("evil", CryptoBox.ExportPublicKey(attacker), 1), | ||
| }, | ||
| Array.Empty<string>(), | ||
| Array.Empty<string>(), | ||
| new[] { "evil" }, | ||
| false); | ||
|
|
||
| var client = new FakeKeysClient { VelvetResponse = response }; | ||
| var store = new ConversationKeyStore(client, new FakeVault(victim)); | ||
| await store.EnsureVelvetKeysAsync("bob", "victim", CancellationToken.None); | ||
|
|
||
| Assert.DoesNotContain(client.VelvetWraps, wrap => wrap.RecipientUserId == "evil"); | ||
| } | ||
|
|
||
| [Fact] | ||
| public async Task GuardRosterRejectsInjectedRecipientInGroupChat() | ||
| { | ||
| using var victim = CryptoBox.TryGenerateIdentity()!; | ||
| using var attacker = CryptoBox.TryGenerateIdentity()!; | ||
| var cek = CryptoBox.GenerateCek(); | ||
|
|
||
| var response = new ConversationKeysDto( | ||
| "42", | ||
| 1, | ||
| new[] { new KeyWrapDto(1, CryptoBox.WrapCek(cek, CryptoBox.ExportPublicKey(victim))!, "victim", 1, 0) }, | ||
| new[] { new UserPublicKeyDto("evil", CryptoBox.ExportPublicKey(attacker), 1) }, | ||
| Array.Empty<string>(), | ||
| Array.Empty<string>(), | ||
| new[] { "evil" }, | ||
| false); | ||
|
|
||
| var guard = new PinnedRecipientGuard(); | ||
| guard.SetAuthorizedMembers("victim"); | ||
| var client = new FakeKeysClient { ChatResponse = response }; | ||
| var store = new ConversationKeyStore(client, new FakeVault(victim), guard); | ||
| await store.EnsureChatKeysAsync("42", CancellationToken.None); | ||
|
|
||
| Assert.Empty(client.ChatWraps); | ||
| } | ||
| } | ||
|
|
||
| internal sealed class FakeVault : IKeyVault | ||
| { | ||
| private readonly ECDiffieHellman privateKey; | ||
|
|
||
| public FakeVault(ECDiffieHellman privateKey) => this.privateKey = privateKey; | ||
|
|
||
| public event Action? Changed | ||
| { | ||
| add { } | ||
| remove { } | ||
| } | ||
|
|
||
| public KeyVaultState State => KeyVaultState.Unlocked; | ||
|
|
||
| public byte[]? UnwrapCek(string wrappedKey) => CryptoBox.UnwrapCek(wrappedKey, privateKey); | ||
| } | ||
|
|
||
| internal sealed class FakeKeysClient : IKeysClient | ||
| { | ||
| public ConversationKeysDto? ChatResponse { get; set; } | ||
|
|
||
| public ConversationKeysDto? VelvetResponse { get; set; } | ||
|
|
||
| public List<NewWrapDto> ChatWraps { get; } = new(); | ||
|
|
||
| public List<NewWrapDto> VelvetWraps { get; } = new(); | ||
|
|
||
| public Task<ConversationKeysDto?> ConversationKeysAsync(string conversationId, CancellationToken token) | ||
| => Task.FromResult(ChatResponse); | ||
|
|
||
| public Task<bool> AddConversationWrapsAsync(string conversationId, AddWrapsRequest request, CancellationToken token) | ||
| { | ||
| ChatWraps.AddRange(request.Wraps); | ||
| return Task.FromResult(true); | ||
| } | ||
|
|
||
| public Task<ConversationKeysDto?> VelvetThreadKeysAsync(string otherId, CancellationToken token) | ||
| => Task.FromResult(VelvetResponse); | ||
|
|
||
| public Task<bool> AddVelvetWrapsAsync(string otherId, AddWrapsRequest request, CancellationToken token) | ||
| { | ||
| VelvetWraps.AddRange(request.Wraps); | ||
| return Task.FromResult(true); | ||
| } | ||
|
|
||
| public Task<MyKeysDto?> PutMyKeysAsync(PutMyKeysRequest request, CancellationToken token) => Task.FromResult<MyKeysDto?>(null); | ||
|
|
||
| public Task<(MyKeysDto? Keys, int Status)> MyKeysAsync(CancellationToken token) => Task.FromResult<(MyKeysDto?, int)>((null, 0)); | ||
|
|
||
| public Task<PublicKeysDto?> PublicKeysAsync(string[] userIds, CancellationToken token) => Task.FromResult<PublicKeysDto?>(null); | ||
|
|
||
| public Task<MyConversationKeysDto?> MyConversationKeysAsync(CancellationToken token) => Task.FromResult<MyConversationKeysDto?>(null); | ||
|
|
||
| public Task<(bool Ok, int Status)> CreateConversationGenerationAsync(string conversationId, CreateGenerationRequest request, CancellationToken token) => Task.FromResult((false, 0)); | ||
|
|
||
| public Task<MyConversationKeysDto?> VelvetKeysAsync(CancellationToken token) => Task.FromResult<MyConversationKeysDto?>(null); | ||
|
|
||
| public Task<(bool Ok, int Status)> CreateVelvetGenerationAsync(string otherId, CreateGenerationRequest request, CancellationToken token) => Task.FromResult((false, 0)); | ||
|
|
||
| public Task<MyConversationKeysDto?> GramKeysAsync(CancellationToken token) => Task.FromResult<MyConversationKeysDto?>(null); | ||
|
|
||
| public Task<ConversationKeysDto?> GramThreadKeysAsync(string otherId, CancellationToken token) => Task.FromResult<ConversationKeysDto?>(null); | ||
|
|
||
| public Task<(bool Ok, int Status)> CreateGramGenerationAsync(string otherId, CreateGenerationRequest request, CancellationToken token) => Task.FromResult((false, 0)); | ||
|
|
||
| public Task<bool> AddGramWrapsAsync(string otherId, AddWrapsRequest request, CancellationToken token) => Task.FromResult(true); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| using Aetherphone.Core.Aethernet.Contracts; | ||
| using Aetherphone.Core.Crypto; | ||
| using Xunit; | ||
|
|
||
| namespace Aetherphone.Tests; | ||
|
|
||
| public sealed class PinnedRecipientGuardTests | ||
| { | ||
| [Fact] | ||
| public void AllowsUnknownRecipientWhenNoRosterConfigured() | ||
| { | ||
| var guard = new PinnedRecipientGuard(); | ||
| Assert.True(guard.IsAuthorized(new UserPublicKeyDto("bob", "key-a", 1))); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void RejectsRecipientNotOnConfiguredRoster() | ||
| { | ||
| var guard = new PinnedRecipientGuard(); | ||
| guard.SetAuthorizedMembers("alice"); | ||
| Assert.False(guard.IsAuthorized(new UserPublicKeyDto("bob", "key-a", 1))); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void RejectsSameVersionKeySubstitution() | ||
| { | ||
| var guard = new PinnedRecipientGuard(); | ||
| guard.Pin("bob", 1, "honest-key"); | ||
| Assert.False(guard.IsAuthorized(new UserPublicKeyDto("bob", "attacker-key", 1))); | ||
| } | ||
|
|
||
| [Fact] | ||
| public void AllowsHigherVersionKeyRotation() | ||
| { | ||
| var guard = new PinnedRecipientGuard(); | ||
| guard.Pin("bob", 1, "honest-key"); | ||
| Assert.True(guard.IsAuthorized(new UserPublicKeyDto("bob", "rotated-key", 2))); | ||
| } | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This asserts the bypass as intended behaviour: any key at a higher version is accepted. |
||
|
|
||
| [Fact] | ||
| public void AcceptsMatchingPinnedKey() | ||
| { | ||
| var guard = new PinnedRecipientGuard(); | ||
| var recipient = new UserPublicKeyDto("bob", "key-a", 1); | ||
| Assert.True(guard.IsAuthorized(recipient)); | ||
| Assert.True(guard.IsAuthorized(recipient)); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| using Aetherphone.Core.Aethernet.Contracts; | ||
|
|
||
| namespace Aetherphone.Core.Aethernet.Clients; | ||
|
|
||
| internal interface IKeysClient | ||
| { | ||
| Task<MyKeysDto?> PutMyKeysAsync(PutMyKeysRequest request, CancellationToken token); | ||
|
|
||
| Task<(MyKeysDto? Keys, int Status)> MyKeysAsync(CancellationToken token); | ||
|
|
||
| Task<PublicKeysDto?> PublicKeysAsync(string[] userIds, CancellationToken token); | ||
|
|
||
| Task<MyConversationKeysDto?> MyConversationKeysAsync(CancellationToken token); | ||
|
|
||
| Task<ConversationKeysDto?> ConversationKeysAsync(string conversationId, CancellationToken token); | ||
|
|
||
| Task<(bool Ok, int Status)> CreateConversationGenerationAsync(string conversationId, CreateGenerationRequest request, CancellationToken token); | ||
|
|
||
| Task<bool> AddConversationWrapsAsync(string conversationId, AddWrapsRequest request, CancellationToken token); | ||
|
|
||
| Task<MyConversationKeysDto?> VelvetKeysAsync(CancellationToken token); | ||
|
|
||
| Task<ConversationKeysDto?> VelvetThreadKeysAsync(string otherId, CancellationToken token); | ||
|
|
||
| Task<(bool Ok, int Status)> CreateVelvetGenerationAsync(string otherId, CreateGenerationRequest request, CancellationToken token); | ||
|
|
||
| Task<bool> AddVelvetWrapsAsync(string otherId, AddWrapsRequest request, CancellationToken token); | ||
|
|
||
| Task<MyConversationKeysDto?> GramKeysAsync(CancellationToken token); | ||
|
|
||
| Task<ConversationKeysDto?> GramThreadKeysAsync(string otherId, CancellationToken token); | ||
|
|
||
| Task<(bool Ok, int Status)> CreateGramGenerationAsync(string otherId, CreateGenerationRequest request, CancellationToken token); | ||
|
|
||
| Task<bool> AddGramWrapsAsync(string otherId, AddWrapsRequest request, CancellationToken token); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,15 +15,17 @@ internal sealed record ChatKeyStatus( | |
|
|
||
| internal sealed class ConversationKeyStore | ||
| { | ||
| private readonly KeysClient client; | ||
| private readonly KeyVault vault; | ||
| private readonly IKeysClient client; | ||
| private readonly IKeyVault vault; | ||
| private readonly IWrapRecipientGuard? guard; | ||
| private readonly ConcurrentDictionary<string, ConcurrentDictionary<int, byte[]>> keysByScope = new(StringComparer.Ordinal); | ||
| private readonly ConcurrentDictionary<string, int> currentGenerations = new(StringComparer.Ordinal); | ||
|
|
||
| public ConversationKeyStore(KeysClient client, KeyVault vault) | ||
| public ConversationKeyStore(IKeysClient client, IKeyVault vault, IWrapRecipientGuard? guard = null) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: an optional guard means the check is off by default, and |
||
| { | ||
| this.client = client; | ||
| this.vault = vault; | ||
| this.guard = guard; | ||
| vault.Changed += OnVaultChanged; | ||
| } | ||
|
|
||
|
|
@@ -129,6 +131,7 @@ public async Task<ChatKeyStatus> EnsureVelvetKeysAsync(string otherId, string my | |
| break; | ||
| } | ||
|
|
||
| keys = RestrictToMembers(keys, myUserId, otherId); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: 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 Worth noting Suggest: if |
||
| CacheWraps(scope, keys.CurrentGeneration, keys.MyWraps); | ||
|
|
||
| if (keys.CurrentGeneration == 0) | ||
|
|
@@ -273,6 +276,7 @@ public async Task<ChatKeyStatus> EnsureGramKeysAsync(string otherId, string myUs | |
| break; | ||
| } | ||
|
|
||
| keys = RestrictToMembers(keys, myUserId, otherId); | ||
| CacheWraps(scope, keys.CurrentGeneration, keys.MyWraps); | ||
|
|
||
| if (keys.CurrentGeneration == 0) | ||
|
|
@@ -534,12 +538,66 @@ private async Task FixWrapsAsync(string conversationId, string scope, Conversati | |
| } | ||
| } | ||
|
|
||
| private static NewWrapDto[]? BuildWraps(byte[] cek, IReadOnlyList<UserPublicKeyDto> recipients) | ||
| private static ConversationKeysDto RestrictToMembers(ConversationKeysDto keys, string first, string second) | ||
| { | ||
| var wraps = new NewWrapDto[recipients.Count]; | ||
| return keys with | ||
| { | ||
| MemberKeys = FilterKeys(keys.MemberKeys, first, second), | ||
| MembersWithoutKeys = FilterIds(keys.MembersWithoutKeys, first, second), | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocking: filtering 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. |
||
| StaleWrapUserIds = FilterIds(keys.StaleWrapUserIds, first, second), | ||
| MissingWrapUserIds = FilterIds(keys.MissingWrapUserIds, first, second), | ||
| }; | ||
| } | ||
|
|
||
| private static UserPublicKeyDto[] FilterKeys(UserPublicKeyDto[] items, string first, string second) | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Non-blocking: |
||
| { | ||
| var result = new List<UserPublicKeyDto>(items.Length); | ||
| for (var index = 0; index < items.Length; index++) | ||
| { | ||
| if (items[index].UserId == first || items[index].UserId == second) | ||
| { | ||
| result.Add(items[index]); | ||
| } | ||
| } | ||
|
|
||
| return result.ToArray(); | ||
| } | ||
|
|
||
| private static string[] FilterIds(string[] items, string first, string second) | ||
| { | ||
| var result = new List<string>(items.Length); | ||
| for (var index = 0; index < items.Length; index++) | ||
| { | ||
| if (items[index] == first || items[index] == second) | ||
| { | ||
| result.Add(items[index]); | ||
| } | ||
| } | ||
|
|
||
| return result.ToArray(); | ||
| } | ||
|
|
||
| private NewWrapDto[]? BuildWraps(byte[] cek, IReadOnlyList<UserPublicKeyDto> recipients) | ||
| { | ||
| var authorized = new List<UserPublicKeyDto>(recipients.Count); | ||
| for (var index = 0; index < recipients.Count; index++) | ||
| { | ||
| var recipient = recipients[index]; | ||
| if (guard is null || guard.IsAuthorized(recipient)) | ||
| { | ||
| authorized.Add(recipient); | ||
| } | ||
| } | ||
|
|
||
| if (authorized.Count == 0) | ||
| { | ||
| return null; | ||
| } | ||
|
|
||
| var wraps = new NewWrapDto[authorized.Count]; | ||
| for (var index = 0; index < authorized.Count; index++) | ||
| { | ||
| var recipient = authorized[index]; | ||
| var wrapped = CryptoBox.WrapCek(cek, recipient.PublicKey); | ||
| if (wrapped is null) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| namespace Aetherphone.Core.Crypto; | ||
|
|
||
| internal interface IKeyVault | ||
| { | ||
| event Action? Changed; | ||
|
|
||
| KeyVaultState State { get; } | ||
|
|
||
| byte[]? UnwrapCek(string wrappedKey); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| using Aetherphone.Core.Aethernet.Contracts; | ||
|
|
||
| namespace Aetherphone.Core.Crypto; | ||
|
|
||
| internal interface IWrapRecipientGuard | ||
| { | ||
| bool IsAuthorized(UserPublicKeyDto recipient); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| namespace Aetherphone.Core.Crypto; | ||
|
|
||
| internal enum KeyVaultState | ||
| { | ||
| Unavailable = 0, | ||
| Provisioning = 1, | ||
| Unlocked = 2, | ||
| Unsupported = 3, | ||
| Locked = 4, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.DoesNotContainagainst an empty list passes for "blocked it" and "nothing happened" alike.