diff --git a/session/commit_validation_test.go b/session/commit_validation_test.go new file mode 100644 index 0000000..7084289 --- /dev/null +++ b/session/commit_validation_test.go @@ -0,0 +1,136 @@ +package session + +import ( + "context" + "errors" + "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" +) + +// freshKeyPackageBytesForIdentity builds a fresh MLS key package for the given +// raw identity bytes. Two calls with the same identity produce packages with +// the SAME credential identity but DIFFERENT keys — used to force a group with +// two members sharing a user ID. +func freshKeyPackageBytesForIdentity(t *testing.T, identity []byte) []byte { + t.Helper() + + store := memorystore.NewStore() + member, err := mls.NewClient(identity, ciphersuite.MLS128DHKEMP256, + mls.WithStorage(store, store), + mls.WithCacheStrategy(mls.CacheNone), + ) + if err != nil { + t.Fatalf("mls.NewClient(identity=%x): %v", identity, err) + } + kp, err := member.FreshKeyPackageBytes(context.Background()) + if err != nil { + t.Fatalf("FreshKeyPackageBytes(identity=%x): %v", identity, err) + } + + return kp +} + +// TestValidateCommitProposalRefs_AcceptsReferenceCommit is the false-positive +// guard: a real reference-based DAVE commit (what mls-go produces) must pass, +// otherwise every foreign commit would be rejected in production. +func TestValidateCommitProposalRefs_AcceptsReferenceCommit(t *testing.T) { + s, extPriv := setupActiveSoleMemberSessionWithKey(t) + const expectedUserID = "333333333" + s.AddUser(expectedUserID) + proposalBytes := addProposalBytesForUser(t, s, extPriv, expectedUserID) + + // Drives commitProposalsLocked, which stores our real reference-based commit. + s.OnDaveMLSProposals(buildProposalBatch(proposalBytes)) + if len(s.pendingCommitBytes) == 0 { + t.Fatal("expected a real commit to have been produced") + } + + if err := validateCommitProposalRefs(s.pendingCommitBytes); err != nil { + t.Fatalf("real reference-based commit must pass, got: %v", err) + } +} + +// TestValidateCommitProposalRefs_RejectsInlineProposal takes a real member +// commit and rewrites its body to embed a proposal inline (ProposalOrRef type +// 1) instead of a cached reference, then confirms it is rejected per +// protocol.md:314. Starting from a real commit keeps the membership tag present +// so the message still parses (validateCommitProposalRefs is structural — it +// verifies neither the signature nor the tag). +func TestValidateCommitProposalRefs_RejectsInlineProposal(t *testing.T) { + s, extPriv := setupActiveSoleMemberSessionWithKey(t) + const expectedUserID = "333333333" + s.AddUser(expectedUserID) + proposalBytes := addProposalBytesForUser(t, s, extPriv, expectedUserID) + s.OnDaveMLSProposals(buildProposalBatch(proposalBytes)) + if len(s.pendingCommitBytes) == 0 { + t.Fatal("expected a real commit to have been produced") + } + + // An inline Add proposal to graft into the commit body. + kpBytes := freshKeyPackageBytesForIdentity(t, mustIdentity(t, "222222222")) + kp, err := keypackages.UnmarshalKeyPackage(kpBytes) + if err != nil { + t.Fatalf("UnmarshalKeyPackage: %v", err) + } + inlineCommit := &group.Commit{ + Proposals: []group.ProposalOrRef{{Proposal: group.NewAddProposal(kp)}}, + } + + mutated := mutateMLSMessage(t, s.pendingCommitBytes, func(msg *framing.MLSMessage) { + pub, ok := msg.AsPublic() + if !ok { + t.Fatal("real commit is not a public message") + } + pub.Content.Body = framing.CommitBody{Data: inlineCommit.Marshal()} + }) + + if err := validateCommitProposalRefs(mutated); !errors.Is(err, ErrInlineProposalInCommit) { + t.Fatalf("expected ErrInlineProposalInCommit, got: %v", err) + } +} + +// TestRebuildEpochState_RejectsDuplicateIdentity forces a group with two members +// sharing the same user ID (different keys, same credential identity) — which +// mls-go allows since it only enforces key uniqueness — and confirms +// rebuildEpochStateLocked rejects it per protocol.md:315. +func TestRebuildEpochState_RejectsDuplicateIdentity(t *testing.T) { + s, _ := setupActiveSoleMemberSessionWithKey(t) + ctx := context.Background() + + identity := mustIdentity(t, "222222222") + kpA := freshKeyPackageBytesForIdentity(t, identity) + kpB := freshKeyPackageBytesForIdentity(t, identity) + + // Two add proposals for the same identity, different keys. + if _, err := s.mlsClient.client.ProposeAddMember(ctx, s.groupID, kpA); err != nil { + t.Fatalf("ProposeAddMember(A): %v", err) + } + if _, err := s.mlsClient.client.ProposeAddMember(ctx, s.groupID, kpB); err != nil { + t.Fatalf("ProposeAddMember(B): %v", err) + } + if _, _, err := s.mlsClient.client.CommitPendingProposals(ctx, s.groupID); err != nil { + t.Fatalf("CommitPendingProposals: %v", err) + } + + // The group now has two members with user ID 222222222. + if _, err := s.rebuildEpochStateLocked(s.groupID); !errors.Is(err, ErrDuplicateGroupIdentity) { + t.Fatalf("expected ErrDuplicateGroupIdentity, got: %v", err) + } +} + +func mustIdentity(t *testing.T, userID string) []byte { + t.Helper() + id, err := userIDToIdentityBytes(godave.UserID(userID)) + if err != nil { + t.Fatalf("userIDToIdentityBytes(%q): %v", userID, err) + } + + return id +} diff --git a/session/errors.go b/session/errors.go index 3000978..b2a903f 100644 --- a/session/errors.go +++ b/session/errors.go @@ -21,4 +21,6 @@ var ( ErrNoCodecForSSRC = errors.New("session: no codec assigned for ssrc") ErrProposalsTooMany = errors.New("session: too many proposal refs in revoke batch") ErrNoActiveEpoch = errors.New("session: no active E2EE epoch") + ErrInlineProposalInCommit = errors.New("session: commit contains inline proposals (DAVE requires only references)") + ErrDuplicateGroupIdentity = errors.New("session: duplicate user ID in group after commit") ) diff --git a/session/mls.go b/session/mls.go index 3ee2668..e5634db 100644 --- a/session/mls.go +++ b/session/mls.go @@ -261,6 +261,12 @@ func (s *session) processCommitLocked(commit []byte) error { return ErrNoActiveGroup } + // DAVE Client Commit Validity (protocol.md:314): reject commits that + // embed proposals inline instead of referencing cached ones. + if err := validateCommitProposalRefs(commit); err != nil { + return err + } + if err := s.mlsClient.client.ProcessCommit(context.Background(), s.groupID, commit); err != nil { return fmt.Errorf("process commit: %w", err) } @@ -276,6 +282,39 @@ func (s *session) processCommitLocked(commit []byte) error { return nil } +// validateCommitProposalRefs checks that a serialized commit contains only +// proposal references (type 2), never inline proposals (type 1). DAVE +// protocol.md:314 requires that commits reference previously cached proposals +// rather than embedding them inline. +func validateCommitProposalRefs(commit []byte) error { + msg, err := framing.UnmarshalMLSMessage(commit) + if err != nil { + return fmt.Errorf("parse commit message: %w", err) + } + + pub, ok := msg.AsPublic() + if !ok || pub == nil { + return fmt.Errorf("%w: commit is not a public message", ErrInlineProposalInCommit) + } + data, ok := pub.Content.CommitData() + if !ok || len(data) == 0 { + return fmt.Errorf("%w: no commit data", ErrInlineProposalInCommit) + } + + c, err := group.UnmarshalCommit(data) + if err != nil { + return fmt.Errorf("parse commit body: %w", err) + } + + for i, por := range c.Proposals { + if por.Proposal != nil { + return fmt.Errorf("%w proposal index %d", ErrInlineProposalInCommit, i) + } + } + + return nil +} + // readTLSVectorLength decodes an MLS variable-length integer (RFC 9000 §16 encoding // used by mlspp for TLS vector length prefixes). // @@ -516,6 +555,8 @@ func (s *session) rebuildEpochStateLocked(groupID []byte) (*epochState, error) { return nil, fmt.Errorf("list members: %w", err) } + seen := make(map[godave.UserID]struct{}, len(members)) + epochID, err := s.mlsClient.client.Epoch(context.Background(), groupID) if err != nil { return nil, fmt.Errorf("read epoch: %w", err) @@ -534,6 +575,11 @@ func (s *session) rebuildEpochStateLocked(groupID []byte) (*epochState, error) { return nil, fmt.Errorf("decode member identity: %w", err) } + if _, dup := seen[memberUserID]; dup { + return nil, fmt.Errorf("%w: %s", ErrDuplicateGroupIdentity, memberUserID) + } + seen[memberUserID] = struct{}{} + baseSecret, err := mediakeys.DeriveSenderBaseSecret(exporter, senderID) if err != nil { return nil, fmt.Errorf("derive sender base secret for %s: %w", memberUserID, err)