From f5314c00680ab0f9129765f5f67ef65a547847e3 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Tue, 6 Feb 2024 12:59:37 +0100 Subject: [PATCH] [bugfix] Ensure activities sender always = activities actor (#2608) --- internal/federation/authenticate.go | 11 ++++ internal/federation/federatingdb/accept.go | 24 +++++++-- internal/federation/federatingdb/announce.go | 18 ++++++- internal/federation/federatingdb/create.go | 56 ++++++++++++++++++++ internal/federation/federatingdb/reject.go | 24 +++++++-- internal/federation/federatingdb/undo.go | 29 ++++++++-- 6 files changed, 147 insertions(+), 15 deletions(-) diff --git a/internal/federation/authenticate.go b/internal/federation/authenticate.go index fe611af8c..ddde5dab2 100644 --- a/internal/federation/authenticate.go +++ b/internal/federation/authenticate.go @@ -214,6 +214,17 @@ func (f *Federator) AuthenticateFederatedRequest(ctx context.Context, requestedU err := gtserror.Newf("error dereferencing account %s: %w", pubKeyAuth.OwnerURI, err) return nil, gtserror.NewErrorInternalError(err) } + + // Catch a possible (but very rare) race condition where + // we've fetched a key, then fetched the Actor who owns the + // key, but the Key of the Actor has changed in the meantime. + if !pubKeyAuth.Owner.PublicKey.Equal(pubKeyAuth.FetchedPubKey) { + err := gtserror.Newf( + "key mismatch: fetched key %s does not match pubkey of fetched Actor %s", + pubKeyID, pubKeyAuth.Owner.URI, + ) + return nil, gtserror.NewErrorUnauthorized(err) + } } if !pubKeyAuth.Owner.SuspendedAt.IsZero() { diff --git a/internal/federation/federatingdb/accept.go b/internal/federation/federatingdb/accept.go index 38b6b9300..69fb6d67b 100644 --- a/internal/federation/federatingdb/accept.go +++ b/internal/federation/federatingdb/accept.go @@ -41,7 +41,7 @@ func (f *federatingDB) Accept(ctx context.Context, accept vocab.ActivityStreamsA l.Debug("entering Accept") } - receivingAccount, _, internal := extractFromCtx(ctx) + receivingAccount, requestingAccount, internal := extractFromCtx(ctx) if internal { return nil // Already processed. } @@ -63,9 +63,16 @@ func (f *federatingDB) Accept(ctx context.Context, accept vocab.ActivityStreamsA return fmt.Errorf("ACCEPT: error converting asfollow to gtsfollow: %s", err) } - // make sure the addressee of the original follow is the same as whatever inbox this landed in + // Make sure the creator of the original follow + // is the same as whatever inbox this landed in. if gtsFollow.AccountID != receivingAccount.ID { - return errors.New("ACCEPT: follow object account and inbox account were not the same") + return errors.New("ACCEPT: follow account and inbox account were not the same") + } + + // Make sure the target of the original follow + // is the same as the account making the request. + if gtsFollow.TargetAccountID != requestingAccount.ID { + return errors.New("ACCEPT: follow target account and requesting account were not the same") } follow, err := f.state.DB.AcceptFollowRequest(ctx, gtsFollow.AccountID, gtsFollow.TargetAccountID) @@ -103,9 +110,16 @@ func (f *federatingDB) Accept(ctx context.Context, accept vocab.ActivityStreamsA return fmt.Errorf("ACCEPT: couldn't get follow request with id %s from the database: %s", iriStr, err) } - // make sure the addressee of the original follow is the same as whatever inbox this landed in + // Make sure the creator of the original follow + // is the same as whatever inbox this landed in. if followReq.AccountID != receivingAccount.ID { - return errors.New("ACCEPT: follow object account and inbox account were not the same") + return errors.New("ACCEPT: follow account and inbox account were not the same") + } + + // Make sure the target of the original follow + // is the same as the account making the request. + if followReq.TargetAccountID != requestingAccount.ID { + return errors.New("ACCEPT: follow target account and requesting account were not the same") } follow, err := f.state.DB.AcceptFollowRequest(ctx, followReq.AccountID, followReq.TargetAccountID) diff --git a/internal/federation/federatingdb/announce.go b/internal/federation/federatingdb/announce.go index 5a5b80bc1..b1bd51659 100644 --- a/internal/federation/federatingdb/announce.go +++ b/internal/federation/federatingdb/announce.go @@ -19,6 +19,8 @@ package federatingdb import ( "context" + "net/url" + "slices" "codeberg.org/gruf/go-logger/v2/level" "github.com/superseriousbusiness/activity/streams/vocab" @@ -39,11 +41,25 @@ func (f *federatingDB) Announce(ctx context.Context, announce vocab.ActivityStre l.Debug("entering Announce") } - receivingAccount, _, internal := extractFromCtx(ctx) + receivingAccount, requestingAccount, internal := extractFromCtx(ctx) if internal { return nil // Already processed. } + // Ensure requestingAccount is among + // the Actors doing the Announce. + // + // We don't support Announce forwards. + actorIRIs := ap.GetActorIRIs(announce) + if !slices.ContainsFunc(actorIRIs, func(actorIRI *url.URL) bool { + return actorIRI.String() == requestingAccount.URI + }) { + return gtserror.Newf( + "requestingAccount %s was not among Announce Actors", + requestingAccount.URI, + ) + } + boost, isNew, err := f.converter.ASAnnounceToStatus(ctx, announce) if err != nil { return gtserror.Newf("error converting announce to boost: %w", err) diff --git a/internal/federation/federatingdb/create.go b/internal/federation/federatingdb/create.go index 3a8d8f0ac..65953ba5a 100644 --- a/internal/federation/federatingdb/create.go +++ b/internal/federation/federatingdb/create.go @@ -24,6 +24,7 @@ import ( "strings" "codeberg.org/gruf/go-logger/v2/level" + "github.com/miekg/dns" "github.com/superseriousbusiness/activity/streams/vocab" "github.com/superseriousbusiness/gotosocial/internal/ap" "github.com/superseriousbusiness/gotosocial/internal/config" @@ -103,6 +104,20 @@ func (f *federatingDB) activityBlock(ctx context.Context, asType vocab.Type, rec return fmt.Errorf("activityBlock: could not convert Block to gts model block") } + if block.AccountID != requestingAccount.ID { + return fmt.Errorf( + "activityBlock: requestingAccount %s is not Block actor account %s", + requestingAccount.URI, block.Account.URI, + ) + } + + if block.TargetAccountID != receiving.ID { + return fmt.Errorf( + "activityBlock: inbox account %s is not Block object account %s", + receiving.URI, block.TargetAccount.URI, + ) + } + block.ID = id.NewULID() if err := f.state.DB.PutBlock(ctx, block); err != nil { @@ -420,6 +435,20 @@ func (f *federatingDB) activityFollow(ctx context.Context, asType vocab.Type, re return fmt.Errorf("activityFollow: could not convert Follow to follow request: %s", err) } + if followRequest.AccountID != requestingAccount.ID { + return fmt.Errorf( + "activityFollow: requestingAccount %s is not Follow actor account %s", + requestingAccount.URI, followRequest.Account.URI, + ) + } + + if followRequest.TargetAccountID != receivingAccount.ID { + return fmt.Errorf( + "activityFollow: inbox account %s is not Follow object account %s", + receivingAccount.URI, followRequest.TargetAccount.URI, + ) + } + followRequest.ID = id.NewULID() if err := f.state.DB.PutFollowRequest(ctx, followRequest); err != nil { @@ -451,6 +480,13 @@ func (f *federatingDB) activityLike(ctx context.Context, asType vocab.Type, rece return fmt.Errorf("activityLike: could not convert Like to fave: %w", err) } + if fave.AccountID != requestingAccount.ID { + return fmt.Errorf( + "activityLike: requestingAccount %s is not Like actor account %s", + requestingAccount.URI, fave.Account.URI, + ) + } + fave.ID = id.NewULID() if err := f.state.DB.PutStatusFave(ctx, fave); err != nil { @@ -488,6 +524,26 @@ func (f *federatingDB) activityFlag(ctx context.Context, asType vocab.Type, rece return fmt.Errorf("activityFlag: could not convert Flag to report: %w", err) } + // Requesting account must have at + // least two domains from the right + // in common with reporting account. + if dns.CompareDomainName( + requestingAccount.Domain, + report.Account.Domain, + ) < 2 { + return fmt.Errorf( + "activityFlag: requesting account %s does not share a domain with Flag Actor account %s", + requestingAccount.URI, report.Account.URI, + ) + } + + if report.TargetAccountID != receivingAccount.ID { + return fmt.Errorf( + "activityFlag: inbox account %s is not Flag object account %s", + receivingAccount.URI, report.TargetAccount.URI, + ) + } + report.ID = id.NewULID() if err := f.state.DB.PutReport(ctx, report); err != nil { diff --git a/internal/federation/federatingdb/reject.go b/internal/federation/federatingdb/reject.go index e02db18e0..ca0adbe1f 100644 --- a/internal/federation/federatingdb/reject.go +++ b/internal/federation/federatingdb/reject.go @@ -40,7 +40,7 @@ func (f *federatingDB) Reject(ctx context.Context, reject vocab.ActivityStreamsR l.Debug("entering Reject") } - receivingAccount, _, internal := extractFromCtx(ctx) + receivingAccount, requestingAccount, internal := extractFromCtx(ctx) if internal { return nil // Already processed. } @@ -62,9 +62,16 @@ func (f *federatingDB) Reject(ctx context.Context, reject vocab.ActivityStreamsR return fmt.Errorf("Reject: couldn't get follow request with id %s from the database: %s", rejectedObjectIRI.String(), err) } - // make sure the addressee of the original follow is the same as whatever inbox this landed in + // Make sure the creator of the original follow + // is the same as whatever inbox this landed in. if followReq.AccountID != receivingAccount.ID { - return errors.New("Reject: follow object account and inbox account were not the same") + return errors.New("Reject: follow account and inbox account were not the same") + } + + // Make sure the target of the original follow + // is the same as the account making the request. + if followReq.TargetAccountID != requestingAccount.ID { + return errors.New("Reject: follow target account and requesting account were not the same") } return f.state.DB.RejectFollowRequest(ctx, followReq.AccountID, followReq.TargetAccountID) @@ -90,9 +97,16 @@ func (f *federatingDB) Reject(ctx context.Context, reject vocab.ActivityStreamsR return fmt.Errorf("Reject: error converting asfollow to gtsfollow: %s", err) } - // make sure the addressee of the original follow is the same as whatever inbox this landed in + // Make sure the creator of the original follow + // is the same as whatever inbox this landed in. if gtsFollow.AccountID != receivingAccount.ID { - return errors.New("Reject: follow object account and inbox account were not the same") + return errors.New("Reject: follow account and inbox account were not the same") + } + + // Make sure the target of the original follow + // is the same as the account making the request. + if gtsFollow.TargetAccountID != requestingAccount.ID { + return errors.New("Reject: follow target account and requesting account were not the same") } return f.state.DB.RejectFollowRequest(ctx, gtsFollow.AccountID, gtsFollow.TargetAccountID) diff --git a/internal/federation/federatingdb/undo.go b/internal/federation/federatingdb/undo.go index a7a0f077a..6b70a4260 100644 --- a/internal/federation/federatingdb/undo.go +++ b/internal/federation/federatingdb/undo.go @@ -44,7 +44,7 @@ func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo) l.Debug("entering Undo") } - receivingAccount, _, internal := extractFromCtx(ctx) + receivingAccount, requestingAccount, internal := extractFromCtx(ctx) if internal { return nil // Already processed. } @@ -61,18 +61,18 @@ func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo) switch objType.GetTypeName() { case ap.ActivityFollow: - if err := f.undoFollow(ctx, receivingAccount, undo, objType); err != nil { + if err := f.undoFollow(ctx, receivingAccount, requestingAccount, undo, objType); err != nil { errs.Appendf("error undoing follow: %w", err) } case ap.ActivityLike: - if err := f.undoLike(ctx, receivingAccount, undo, objType); err != nil { + if err := f.undoLike(ctx, receivingAccount, requestingAccount, undo, objType); err != nil { errs.Appendf("error undoing like: %w", err) } case ap.ActivityAnnounce: // TODO: actually handle this ! log.Warn(ctx, "skipped undo announce") case ap.ActivityBlock: - if err := f.undoBlock(ctx, receivingAccount, undo, objType); err != nil { + if err := f.undoBlock(ctx, receivingAccount, requestingAccount, undo, objType); err != nil { errs.Appendf("error undoing block: %w", err) } } @@ -84,6 +84,7 @@ func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo) func (f *federatingDB) undoFollow( ctx context.Context, receivingAccount *gtsmodel.Account, + requestingAccount *gtsmodel.Account, undo vocab.ActivityStreamsUndo, t vocab.Type, ) error { @@ -109,6 +110,12 @@ func (f *federatingDB) undoFollow( return nil } + // Ensure requester is follow origin. + if follow.AccountID != requestingAccount.ID { + // Ignore this Activity. + return nil + } + // Delete any existing follow with this URI. if err := f.state.DB.DeleteFollowByURI(ctx, follow.URI); err != nil && !errors.Is(err, db.ErrNoEntries) { return fmt.Errorf("undoFollow: db error removing follow: %w", err) @@ -126,6 +133,7 @@ func (f *federatingDB) undoFollow( func (f *federatingDB) undoLike( ctx context.Context, receivingAccount *gtsmodel.Account, + requestingAccount *gtsmodel.Account, undo vocab.ActivityStreamsUndo, t vocab.Type, ) error { @@ -151,6 +159,12 @@ func (f *federatingDB) undoLike( return nil } + // Ensure requester is fave origin. + if fave.AccountID != requestingAccount.ID { + // Ignore this Activity. + return nil + } + // Ignore URI on Likes, since we often get multiple Likes // with the same target and account ID, but differing URIs. // Instead, we'll select using account and target status. @@ -179,6 +193,7 @@ func (f *federatingDB) undoLike( func (f *federatingDB) undoBlock( ctx context.Context, receivingAccount *gtsmodel.Account, + requestingAccount *gtsmodel.Account, undo vocab.ActivityStreamsUndo, t vocab.Type, ) error { @@ -204,6 +219,12 @@ func (f *federatingDB) undoBlock( return nil } + // Ensure requester is block origin. + if block.AccountID != requestingAccount.ID { + // Ignore this Activity. + return nil + } + // Delete any existing BLOCK if err := f.state.DB.DeleteBlockByURI(ctx, block.URI); err != nil && !errors.Is(err, db.ErrNoEntries) { return fmt.Errorf("undoBlock: db error removing block: %w", err)