Skip to content

Commit

Permalink
[bugfix] Ensure activities sender always = activities actor (#2608)
Browse files Browse the repository at this point in the history
  • Loading branch information
tsmethurst committed Feb 6, 2024
1 parent ccecf5a commit f5314c0
Show file tree
Hide file tree
Showing 6 changed files with 147 additions and 15 deletions.
11 changes: 11 additions & 0 deletions internal/federation/authenticate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
24 changes: 19 additions & 5 deletions internal/federation/federatingdb/accept.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
Expand All @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
18 changes: 17 additions & 1 deletion internal/federation/federatingdb/announce.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package federatingdb

import (
"context"
"net/url"
"slices"

"codeberg.org/gruf/go-logger/v2/level"
"github.com/superseriousbusiness/activity/streams/vocab"
Expand All @@ -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)
Expand Down
56 changes: 56 additions & 0 deletions internal/federation/federatingdb/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
24 changes: 19 additions & 5 deletions internal/federation/federatingdb/reject.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
Expand All @@ -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)
Expand All @@ -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)
Expand Down
29 changes: 25 additions & 4 deletions internal/federation/federatingdb/undo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
}
Expand All @@ -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)
}
}
Expand All @@ -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 {
Expand All @@ -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)
Expand All @@ -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 {
Expand All @@ -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.
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
Expand Down

0 comments on commit f5314c0

Please sign in to comment.