Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[security] Check all involved IRIs during block checking #593

Merged
merged 21 commits into from
May 23, 2022

Conversation

tsmethurst
Copy link
Contributor

This PR updates the logic in the Blocked function of the federator to make more aggressive checks against accounts involved in the Activity in some way.

In short, we now make sure that:

  • the account receiving an Activity doesn't block an account either To'd, CC'ed, or RepliedTo
  • an account either To'd, CC'ed, or RepliedTo doesn't block the account that created an Activity

This means that in the following scenario --

  • local1 follows remote
  • local2 blocks remote
  • remote delivers an activity replyingTo/CCing local2, to the inbox of local1, who is in their Followers collection

-- the request will be refused because our instance knows that local2 blocks remote. So even though local1 technically can see the status, the instance won't process it because of the block. This makes it much less likely that local2 will bump into the status somehow, and see a message from someone who they blocked.

The PR also tightens up some of the domain block logic in the same way. Now, if an Activity is To'd, CC'ed, or RepliedTo someone from a blocked domain, the request will be refused, even if the inbox account follows the account who created the Activity.

In order to make these extra checks work without totally tanking the database doing lots of selects, some new lighter database functions have been added to select just the account ID of an account or status by its URI.

return cached.ID, nil
}

account := &gtsmodel.Account{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth doing this separate db query? Because in the case this account isn't cached the result will never be cached, increasing database access. Alternatively it would be just doing a.GetAccountByURI(...) and returning the ID from that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I also wasn't sure about these. The problem is that we really just need that one field, whereas a full-blown GetAccountByURI does a massive join with all the relations we have set on it. So this is a really cheap query, but it will indeed mean more queries in total. I'm not really sure which way ends up faster overall

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with cacheable functions 👍

return cached.AccountID, nil
}

status := &gtsmodel.Status{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, but using relevant status function

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replaced with cacheable functions 👍

@tsmethurst tsmethurst merged commit 469da93 into main May 23, 2022
@tsmethurst tsmethurst deleted the check_involved_iris_during_block branch May 23, 2022 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants