From c3aca5fdd81cf16e11a47c68ad8b7006f8f4740c Mon Sep 17 00:00:00 2001 From: nerotol Date: Sat, 25 Jul 2026 20:42:13 -0700 Subject: [PATCH] feat(security): implement recipient guard and related tests for conversation key management --- .../ConversationKeyStoreSecurityTests.cs | 134 ++++++++++++++++++ .../PinnedRecipientGuardTests.cs | 48 +++++++ .../Core/Aethernet/Clients/IKeysClient.cs | 36 +++++ .../Core/Aethernet/Clients/KeysClient.cs | 2 +- .../Core/Crypto/ConversationKeyStore.cs | 68 ++++++++- src/Aetherphone/Core/Crypto/IKeyVault.cs | 10 ++ .../Core/Crypto/IWrapRecipientGuard.cs | 8 ++ src/Aetherphone/Core/Crypto/KeyVault.cs | 11 +- src/Aetherphone/Core/Crypto/KeyVaultState.cs | 10 ++ .../Core/Crypto/PinnedRecipientGuard.cs | 59 ++++++++ src/Aetherphone/Core/PhoneServices.cs | 2 +- 11 files changed, 371 insertions(+), 17 deletions(-) create mode 100644 src/Aetherphone.Tests/ConversationKeyStoreSecurityTests.cs create mode 100644 src/Aetherphone.Tests/PinnedRecipientGuardTests.cs create mode 100644 src/Aetherphone/Core/Aethernet/Clients/IKeysClient.cs create mode 100644 src/Aetherphone/Core/Crypto/IKeyVault.cs create mode 100644 src/Aetherphone/Core/Crypto/IWrapRecipientGuard.cs create mode 100644 src/Aetherphone/Core/Crypto/KeyVaultState.cs create mode 100644 src/Aetherphone/Core/Crypto/PinnedRecipientGuard.cs diff --git a/src/Aetherphone.Tests/ConversationKeyStoreSecurityTests.cs b/src/Aetherphone.Tests/ConversationKeyStoreSecurityTests.cs new file mode 100644 index 000000000..1e44c863c --- /dev/null +++ b/src/Aetherphone.Tests/ConversationKeyStoreSecurityTests.cs @@ -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(), + Array.Empty(), + 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(), + Array.Empty(), + 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 ChatWraps { get; } = new(); + + public List VelvetWraps { get; } = new(); + + public Task ConversationKeysAsync(string conversationId, CancellationToken token) + => Task.FromResult(ChatResponse); + + public Task AddConversationWrapsAsync(string conversationId, AddWrapsRequest request, CancellationToken token) + { + ChatWraps.AddRange(request.Wraps); + return Task.FromResult(true); + } + + public Task VelvetThreadKeysAsync(string otherId, CancellationToken token) + => Task.FromResult(VelvetResponse); + + public Task AddVelvetWrapsAsync(string otherId, AddWrapsRequest request, CancellationToken token) + { + VelvetWraps.AddRange(request.Wraps); + return Task.FromResult(true); + } + + public Task PutMyKeysAsync(PutMyKeysRequest request, CancellationToken token) => Task.FromResult(null); + + public Task<(MyKeysDto? Keys, int Status)> MyKeysAsync(CancellationToken token) => Task.FromResult<(MyKeysDto?, int)>((null, 0)); + + public Task PublicKeysAsync(string[] userIds, CancellationToken token) => Task.FromResult(null); + + public Task MyConversationKeysAsync(CancellationToken token) => Task.FromResult(null); + + public Task<(bool Ok, int Status)> CreateConversationGenerationAsync(string conversationId, CreateGenerationRequest request, CancellationToken token) => Task.FromResult((false, 0)); + + public Task VelvetKeysAsync(CancellationToken token) => Task.FromResult(null); + + public Task<(bool Ok, int Status)> CreateVelvetGenerationAsync(string otherId, CreateGenerationRequest request, CancellationToken token) => Task.FromResult((false, 0)); + + public Task GramKeysAsync(CancellationToken token) => Task.FromResult(null); + + public Task GramThreadKeysAsync(string otherId, CancellationToken token) => Task.FromResult(null); + + public Task<(bool Ok, int Status)> CreateGramGenerationAsync(string otherId, CreateGenerationRequest request, CancellationToken token) => Task.FromResult((false, 0)); + + public Task AddGramWrapsAsync(string otherId, AddWrapsRequest request, CancellationToken token) => Task.FromResult(true); +} diff --git a/src/Aetherphone.Tests/PinnedRecipientGuardTests.cs b/src/Aetherphone.Tests/PinnedRecipientGuardTests.cs new file mode 100644 index 000000000..bf03568a6 --- /dev/null +++ b/src/Aetherphone.Tests/PinnedRecipientGuardTests.cs @@ -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))); + } + + [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)); + } +} diff --git a/src/Aetherphone/Core/Aethernet/Clients/IKeysClient.cs b/src/Aetherphone/Core/Aethernet/Clients/IKeysClient.cs new file mode 100644 index 000000000..c682700eb --- /dev/null +++ b/src/Aetherphone/Core/Aethernet/Clients/IKeysClient.cs @@ -0,0 +1,36 @@ +using Aetherphone.Core.Aethernet.Contracts; + +namespace Aetherphone.Core.Aethernet.Clients; + +internal interface IKeysClient +{ + Task PutMyKeysAsync(PutMyKeysRequest request, CancellationToken token); + + Task<(MyKeysDto? Keys, int Status)> MyKeysAsync(CancellationToken token); + + Task PublicKeysAsync(string[] userIds, CancellationToken token); + + Task MyConversationKeysAsync(CancellationToken token); + + Task ConversationKeysAsync(string conversationId, CancellationToken token); + + Task<(bool Ok, int Status)> CreateConversationGenerationAsync(string conversationId, CreateGenerationRequest request, CancellationToken token); + + Task AddConversationWrapsAsync(string conversationId, AddWrapsRequest request, CancellationToken token); + + Task VelvetKeysAsync(CancellationToken token); + + Task VelvetThreadKeysAsync(string otherId, CancellationToken token); + + Task<(bool Ok, int Status)> CreateVelvetGenerationAsync(string otherId, CreateGenerationRequest request, CancellationToken token); + + Task AddVelvetWrapsAsync(string otherId, AddWrapsRequest request, CancellationToken token); + + Task GramKeysAsync(CancellationToken token); + + Task GramThreadKeysAsync(string otherId, CancellationToken token); + + Task<(bool Ok, int Status)> CreateGramGenerationAsync(string otherId, CreateGenerationRequest request, CancellationToken token); + + Task AddGramWrapsAsync(string otherId, AddWrapsRequest request, CancellationToken token); +} diff --git a/src/Aetherphone/Core/Aethernet/Clients/KeysClient.cs b/src/Aetherphone/Core/Aethernet/Clients/KeysClient.cs index d82028e12..0eda98a81 100644 --- a/src/Aetherphone/Core/Aethernet/Clients/KeysClient.cs +++ b/src/Aetherphone/Core/Aethernet/Clients/KeysClient.cs @@ -2,7 +2,7 @@ namespace Aetherphone.Core.Aethernet.Clients; -internal sealed class KeysClient +internal sealed class KeysClient : IKeysClient { private readonly AethernetTransport net; diff --git a/src/Aetherphone/Core/Crypto/ConversationKeyStore.cs b/src/Aetherphone/Core/Crypto/ConversationKeyStore.cs index 75b1e899e..69898a2e2 100644 --- a/src/Aetherphone/Core/Crypto/ConversationKeyStore.cs +++ b/src/Aetherphone/Core/Crypto/ConversationKeyStore.cs @@ -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> keysByScope = new(StringComparer.Ordinal); private readonly ConcurrentDictionary currentGenerations = new(StringComparer.Ordinal); - public ConversationKeyStore(KeysClient client, KeyVault vault) + public ConversationKeyStore(IKeysClient client, IKeyVault vault, IWrapRecipientGuard? guard = null) { this.client = client; this.vault = vault; + this.guard = guard; vault.Changed += OnVaultChanged; } @@ -129,6 +131,7 @@ public async Task EnsureVelvetKeysAsync(string otherId, string my break; } + keys = RestrictToMembers(keys, myUserId, otherId); CacheWraps(scope, keys.CurrentGeneration, keys.MyWraps); if (keys.CurrentGeneration == 0) @@ -273,6 +276,7 @@ public async Task 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 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), + StaleWrapUserIds = FilterIds(keys.StaleWrapUserIds, first, second), + MissingWrapUserIds = FilterIds(keys.MissingWrapUserIds, first, second), + }; + } + + private static UserPublicKeyDto[] FilterKeys(UserPublicKeyDto[] items, string first, string second) + { + var result = new List(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(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 recipients) + { + var authorized = new List(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) { diff --git a/src/Aetherphone/Core/Crypto/IKeyVault.cs b/src/Aetherphone/Core/Crypto/IKeyVault.cs new file mode 100644 index 000000000..70ec5b484 --- /dev/null +++ b/src/Aetherphone/Core/Crypto/IKeyVault.cs @@ -0,0 +1,10 @@ +namespace Aetherphone.Core.Crypto; + +internal interface IKeyVault +{ + event Action? Changed; + + KeyVaultState State { get; } + + byte[]? UnwrapCek(string wrappedKey); +} diff --git a/src/Aetherphone/Core/Crypto/IWrapRecipientGuard.cs b/src/Aetherphone/Core/Crypto/IWrapRecipientGuard.cs new file mode 100644 index 000000000..5e1da69dc --- /dev/null +++ b/src/Aetherphone/Core/Crypto/IWrapRecipientGuard.cs @@ -0,0 +1,8 @@ +using Aetherphone.Core.Aethernet.Contracts; + +namespace Aetherphone.Core.Crypto; + +internal interface IWrapRecipientGuard +{ + bool IsAuthorized(UserPublicKeyDto recipient); +} diff --git a/src/Aetherphone/Core/Crypto/KeyVault.cs b/src/Aetherphone/Core/Crypto/KeyVault.cs index d595f564f..e629c8898 100644 --- a/src/Aetherphone/Core/Crypto/KeyVault.cs +++ b/src/Aetherphone/Core/Crypto/KeyVault.cs @@ -6,16 +6,7 @@ namespace Aetherphone.Core.Crypto; -internal enum KeyVaultState -{ - Unavailable = 0, - Provisioning = 1, - Unlocked = 2, - Unsupported = 3, - Locked = 4, -} - -internal sealed class KeyVault : IDisposable +internal sealed class KeyVault : IDisposable, IKeyVault { private readonly Configuration configuration; private readonly AethernetSession session; diff --git a/src/Aetherphone/Core/Crypto/KeyVaultState.cs b/src/Aetherphone/Core/Crypto/KeyVaultState.cs new file mode 100644 index 000000000..fdf3d48da --- /dev/null +++ b/src/Aetherphone/Core/Crypto/KeyVaultState.cs @@ -0,0 +1,10 @@ +namespace Aetherphone.Core.Crypto; + +internal enum KeyVaultState +{ + Unavailable = 0, + Provisioning = 1, + Unlocked = 2, + Unsupported = 3, + Locked = 4, +} diff --git a/src/Aetherphone/Core/Crypto/PinnedRecipientGuard.cs b/src/Aetherphone/Core/Crypto/PinnedRecipientGuard.cs new file mode 100644 index 000000000..55f11de20 --- /dev/null +++ b/src/Aetherphone/Core/Crypto/PinnedRecipientGuard.cs @@ -0,0 +1,59 @@ +using System.Collections.Concurrent; +using Aetherphone.Core.Aethernet.Contracts; + +namespace Aetherphone.Core.Crypto; + +internal sealed class PinnedRecipientGuard : IWrapRecipientGuard +{ + private readonly ConcurrentDictionary authorizedMembers = new(StringComparer.Ordinal); + private readonly ConcurrentDictionary pins = new(StringComparer.Ordinal); + private readonly ConcurrentQueue<(string UserId, string Reason)> rejections = new(); + private volatile bool rosterConfigured; + + public IReadOnlyCollection<(string UserId, string Reason)> Rejections => rejections.ToArray(); + + public void SetAuthorizedMembers(params string[] userIds) + { + authorizedMembers.Clear(); + for (var index = 0; index < userIds.Length; index++) + { + authorizedMembers[userIds[index]] = 1; + } + + rosterConfigured = true; + } + + public void Pin(string userId, int keyVersion, string publicKey) + { + pins[userId] = (keyVersion, publicKey); + } + + public bool IsAuthorized(UserPublicKeyDto recipient) + { + if (rosterConfigured && !authorizedMembers.ContainsKey(recipient.UserId)) + { + rejections.Enqueue((recipient.UserId, "not an authorized member")); + return false; + } + + if (pins.TryGetValue(recipient.UserId, out var pinned)) + { + if (recipient.KeyVersion > pinned.Version) + { + pins[recipient.UserId] = (recipient.KeyVersion, recipient.PublicKey); + return true; + } + + if (!string.Equals(pinned.PublicKey, recipient.PublicKey, StringComparison.Ordinal)) + { + rejections.Enqueue((recipient.UserId, "key material changed")); + return false; + } + + return true; + } + + pins[recipient.UserId] = (recipient.KeyVersion, recipient.PublicKey); + return true; + } +} diff --git a/src/Aetherphone/Core/PhoneServices.cs b/src/Aetherphone/Core/PhoneServices.cs index 7f6289d36..80424ccc5 100644 --- a/src/Aetherphone/Core/PhoneServices.cs +++ b/src/Aetherphone/Core/PhoneServices.cs @@ -150,7 +150,7 @@ public static PhoneServices Build(Configuration configuration, IChatGui chatGui, var aethernet = new AethernetApi(http, aethernetSession); 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()); var marketIndex = new MarketItemIndex(dataManager); var market = new MarketboardService(http); var marketLauncher = new MarketLauncher();