From 738a7fd355b3ef0be5e4e15c487906ccf2332579 Mon Sep 17 00:00:00 2001 From: Thomas Vilte Date: Mon, 6 Jul 2026 13:08:26 -0300 Subject: [PATCH] feat(session): validate proposal sender and type --- session/mls.go | 54 +++++++++++ session/proposal_validation_test.go | 140 ++++++++++++++++++++++++---- session/sole_member_test.go | 13 ++- 3 files changed, 187 insertions(+), 20 deletions(-) diff --git a/session/mls.go b/session/mls.go index 6aca184..e15519e 100644 --- a/session/mls.go +++ b/session/mls.go @@ -387,6 +387,24 @@ func (s *session) processProposalBatchLocked(proposals []byte) (bool, error) { return acceptedAny, ErrEmptyProposal } + allowed, err := s.proposalAllowedByDAVELocked(msg) + if err != nil { + // fail closed: if we can't inspect the proposal safely, reject it. + s.logger.Warn("processProposalBatchLocked: rejecting proposal that failed inspection", + "error", err, + "index", i) + remaining = remaining[len(wire):] + + continue + } + if !allowed { + s.logger.Warn("processProposalBatchLocked: rejecting disallowed proposal sender/type", + "index", i) + remaining = remaining[len(wire):] + + continue + } + userID, ok, err := s.addProposalUserID(msg) if err != nil { // This is a Proposal we couldn't parse or extract an identity from — @@ -427,6 +445,42 @@ func (s *session) processProposalBatchLocked(proposals []byte) (bool, error) { return acceptedAny, nil } +func (s *session) proposalAllowedByDAVELocked(msg *framing.MLSMessage) (bool, error) { + if msg == nil { + return false, nil + } + + pubMsg, ok := msg.AsPublic() + if !ok || pubMsg == nil { + return false, nil + } + + if pubMsg.Content.Sender.Type != framing.SenderTypeExternal { + return false, nil + } + + if pubMsg.Content.ContentType() != framing.ContentTypeProposal { + return false, nil + } + + data, ok := pubMsg.Content.ProposalData() + if !ok { + return false, nil + } + + proposal, err := group.UnmarshalProposal(data) + if err != nil { + return false, err + } + + switch proposal.Type { + case group.ProposalTypeAdd, group.ProposalTypeRemove: + return true, nil + default: + return false, nil + } +} + func (s *session) addProposalUserID(msg *framing.MLSMessage) (godave.UserID, bool, error) { if msg == nil { return "", false, nil diff --git a/session/proposal_validation_test.go b/session/proposal_validation_test.go index 1ad5815..2c47d8b 100644 --- a/session/proposal_validation_test.go +++ b/session/proposal_validation_test.go @@ -2,31 +2,36 @@ package session import ( "context" + "crypto/ecdsa" "testing" "github.com/disgoorg/godave" "github.com/thomas-vilte/mls-go" "github.com/thomas-vilte/mls-go/ciphersuite" + "github.com/thomas-vilte/mls-go/framing" + "github.com/thomas-vilte/mls-go/group" + "github.com/thomas-vilte/mls-go/keypackages" memorystore "github.com/thomas-vilte/mls-go/storage/memory" ) -func setupActiveSoleMemberSession(t *testing.T) *session { +func setupActiveSoleMemberSessionWithKey(t *testing.T) (*session, *ecdsa.PrivateKey) { t.Helper() + pkg, extPriv := buildExternalSenderPackageWithKey(t) s := New(nil, "123456789", testCallbacks{}).(*session) s.SetChannelID(987654321) s.OnSelectProtocolAck(1) - s.OnDaveMLSExternalSenderPackage(buildExternalSenderPackage(t)) + s.OnDaveMLSExternalSenderPackage(pkg) s.OnDavePrepareTransition(0, 1) if s.groupID == nil || s.mlsClient == nil || s.activeEpoch == nil { t.Fatal("expected active sole-member MLS epoch to be initialized") } - return s + return s, extPriv } -func addProposalBytesForUser(t *testing.T, s *session, userID string) []byte { +func addProposalBytesForUser(t *testing.T, s *session, extPriv *ecdsa.PrivateKey, userID string) []byte { t.Helper() identity, err := userIDToIdentityBytes(godave.UserID(userID)) @@ -34,18 +39,32 @@ func addProposalBytesForUser(t *testing.T, s *session, userID string) []byte { t.Fatalf("userIDToIdentityBytes(%q): %v", userID, err) } - return addProposalBytesForIdentity(t, s, identity) + return addProposalBytesAsExternalSender(t, s, extPriv, identity) } -// addProposalBytesForIdentity is the same as addProposalBytesForUser but +// addProposalBytesAsExternalSender is the same as addProposalBytesForUser but // takes raw identity bytes directly, bypassing the 8-byte snowflake // encoding — used to build a credential that parses fine at the MLS level // but that identityBytesToUserID (which requires exactly 8 bytes) can't // turn into a UserID. -func addProposalBytesForIdentity(t *testing.T, s *session, identity []byte) []byte { +func addProposalBytesForIdentity(t *testing.T, s *session, extPriv *ecdsa.PrivateKey, identity []byte) []byte { + t.Helper() + + return addProposalBytesAsExternalSender(t, s, extPriv, identity) +} + +// addProposalBytesAsExternalSender builds an Add proposal signed by the +// external sender (the voice gateway), not by a group member. This mirrors +// how DAVE delivers proposals in reality (protocol.md:166: "Only Add and +// Remove proposals sent by the voice gateway external sender are allowed"). +// The bot's own ProposeAddMember signs as SenderTypeMember, which the +// #16 check now rejects; tests must generate proposals the way the gateway +// actually would. +func addProposalBytesAsExternalSender(t *testing.T, s *session, extPriv *ecdsa.PrivateKey, identity []byte) []byte { t.Helper() ctx := context.Background() + store := memorystore.NewStore() member, err := mls.NewClient(identity, ciphersuite.MLS128DHKEMP256, mls.WithStorage(store, store), @@ -55,20 +74,48 @@ func addProposalBytesForIdentity(t *testing.T, s *session, identity []byte) []by t.Fatalf("mls.NewClient(identity=%x): %v", identity, err) } - keyPackage, err := member.FreshKeyPackageBytes(ctx) + keyPackageBytes, err := member.FreshKeyPackageBytes(ctx) if err != nil { t.Fatalf("FreshKeyPackageBytes(identity=%x): %v", identity, err) } + keyPackage, err := keypackages.UnmarshalKeyPackage(keyPackageBytes) + if err != nil { + t.Fatalf("UnmarshalKeyPackage: %v", err) + } + + proposal := group.NewAddProposal(keyPackage) + proposalData := group.ProposalMarshal(proposal) + + epoch, err := s.mlsClient.client.Epoch(ctx, s.groupID) + if err != nil { + t.Fatalf("Epoch: %v", err) + } - proposalBytes, err := s.mlsClient.client.ProposeAddMember(ctx, s.groupID, keyPackage) + groupInfoBytes, err := s.mlsClient.client.GroupInfo(ctx, s.groupID) if err != nil { - t.Fatalf("ProposeAddMember(identity=%x): %v", identity, err) + t.Fatalf("GroupInfo: %v", err) } - if err := s.mlsClient.client.CancelPendingProposals(ctx, s.groupID); err != nil { - t.Fatalf("CancelPendingProposals(identity=%x): %v", identity, err) + info, err := group.UnmarshalGroupInfo(groupInfoBytes) + if err != nil { + t.Fatalf("UnmarshalGroupInfo: %v", err) + } + gc := info.GroupContext.Marshal() + + content := framing.FramedContent{ + GroupID: s.groupID, + Epoch: epoch, + Sender: framing.Sender{Type: framing.SenderTypeExternal, SenderIndex: 0}, + AuthenticatedData: []byte{}, + Body: framing.ProposalBody{Data: proposalData}, + } + + sigKey := ciphersuite.NewSignaturePrivateKey(extPriv) + pm, err := framing.NewPublicMessage(content, sigKey, gc, nil, ciphersuite.MLS128DHKEMP256) + if err != nil { + t.Fatalf("NewPublicMessage: %v", err) } - return proposalBytes + return framing.NewMLSMessagePublic(pm).Marshal() } func memberCount(t *testing.T, s *session) int { @@ -82,10 +129,23 @@ func memberCount(t *testing.T, s *session) int { return len(members) } +func mutateMLSMessage(t *testing.T, wire []byte, mutate func(*framing.MLSMessage)) []byte { + t.Helper() + + msg, err := framing.UnmarshalMLSMessage(wire) + if err != nil { + t.Fatalf("framing.UnmarshalMLSMessage: %v", err) + } + + mutate(msg) + + return msg.Marshal() +} + func TestOnDaveMLSProposals_AddValidation(t *testing.T) { t.Run("ignores unexpected add", func(t *testing.T) { - s := setupActiveSoleMemberSession(t) - proposalBytes := addProposalBytesForUser(t, s, "222222222") + s, extPriv := setupActiveSoleMemberSessionWithKey(t) + proposalBytes := addProposalBytesForUser(t, s, extPriv, "222222222") if got := memberCount(t, s); got != 1 { t.Fatalf("initial member count = %d, want 1", got) @@ -99,10 +159,10 @@ func TestOnDaveMLSProposals_AddValidation(t *testing.T) { }) t.Run("accepts expected add", func(t *testing.T) { - s := setupActiveSoleMemberSession(t) + s, extPriv := setupActiveSoleMemberSessionWithKey(t) const expectedUserID = "333333333" s.AddUser(expectedUserID) - proposalBytes := addProposalBytesForUser(t, s, expectedUserID) + proposalBytes := addProposalBytesForUser(t, s, extPriv, expectedUserID) if got := memberCount(t, s); got != 1 { t.Fatalf("initial member count = %d, want 1", got) @@ -120,8 +180,8 @@ func TestOnDaveMLSProposals_AddValidation(t *testing.T) { // expects. identityBytesToUserID fails on it, and the proposal must be // rejected rather than let through to ProcessPublicMessage unchecked. t.Run("rejects add whose identity fails inspection", func(t *testing.T) { - s := setupActiveSoleMemberSession(t) - proposalBytes := addProposalBytesForIdentity(t, s, []byte{0x01, 0x02, 0x03}) + s, extPriv := setupActiveSoleMemberSessionWithKey(t) + proposalBytes := addProposalBytesForIdentity(t, s, extPriv, []byte{0x01, 0x02, 0x03}) if got := memberCount(t, s); got != 1 { t.Fatalf("initial member count = %d, want 1", got) @@ -134,3 +194,45 @@ func TestOnDaveMLSProposals_AddValidation(t *testing.T) { } }) } + +func TestOnDaveMLSProposals_RejectsNewMemberProposalSender(t *testing.T) { + s, extPriv := setupActiveSoleMemberSessionWithKey(t) + proposalBytes := addProposalBytesForUser(t, s, extPriv, "222222222") + + mutated := mutateMLSMessage(t, proposalBytes, func(msg *framing.MLSMessage) { + pub, _ := msg.AsPublic() + pub.Content.Sender.Type = framing.SenderTypeNewMemberProposal + }) + + if got := memberCount(t, s); got != 1 { + t.Fatalf("initial member count = %d, want 1", got) + } + + s.OnDaveMLSProposals(buildProposalBatch(mutated)) + + if got := memberCount(t, s); got != 1 { + t.Fatalf("member count after disallowed sender = %d, want 1", got) + } +} + +func TestOnDaveMLSProposals_RejectsDisallowedProposalType(t *testing.T) { + s, extPriv := setupActiveSoleMemberSessionWithKey(t) + proposalBytes := addProposalBytesForUser(t, s, extPriv, "222222222") + + mutated := mutateMLSMessage(t, proposalBytes, func(msg *framing.MLSMessage) { + pub, _ := msg.AsPublic() + pub.Content.Body = framing.ProposalBody{ + Data: group.ProposalMarshal(group.NewPreSharedKeyProposal(1, []byte("psk"))), + } + }) + + if got := memberCount(t, s); got != 1 { + t.Fatalf("initial member count = %d, want 1", got) + } + + s.OnDaveMLSProposals(buildProposalBatch(mutated)) + + if got := memberCount(t, s); got != 1 { + t.Fatalf("member count after disallowed proposal type = %d, want 1", got) + } +} diff --git a/session/sole_member_test.go b/session/sole_member_test.go index 481e66d..b2ae2db 100644 --- a/session/sole_member_test.go +++ b/session/sole_member_test.go @@ -32,6 +32,17 @@ func writeVLBytes(b []byte) []byte { // the voice gateway sends it: VL(signature_public_key) || Credential_inline. func buildExternalSenderPackage(t *testing.T) []byte { t.Helper() + pkg, _ := buildExternalSenderPackageWithKey(t) + + return pkg +} + +// buildExternalSenderPackageWithKey is like buildExternalSenderPackage but also +// returns the ECDH private key so tests can sign proposals as the external +// sender (simulating the gateway, which is how DAVE delivers proposals in +// reality — not as a group member). +func buildExternalSenderPackageWithKey(t *testing.T) ([]byte, *ecdsa.PrivateKey) { + t.Helper() priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) if err != nil { @@ -44,7 +55,7 @@ func buildExternalSenderPackage(t *testing.T) []byte { cred := credentials.NewBasicCredentialFromUint64(1) - return append(writeVLBytes(ecdhPub.Bytes()), cred.Marshal()...) + return append(writeVLBytes(ecdhPub.Bytes()), cred.Marshal()...), priv } // TestSoleMemberReset_EncryptsWhileAlone reproduces the DAVE "Sole member reset":