From e658f103692df05536217c149c4b1f63753a5225 Mon Sep 17 00:00:00 2001 From: Ryan Barrett Date: Fri, 6 Dec 2024 14:51:40 -0800 Subject: [PATCH] ActivityPub.inbox: start on same-domain check for actors and activity/object ids for https://github.com/snarfed/bridgy-fed/security/advisories/GHSA-37r7-jqmr-3472 --- activitypub.py | 47 +++++++++++++++++++++++------------- tests/test_activitypub.py | 50 +++++++++++++++++++++++++++++++++++---- 2 files changed, 75 insertions(+), 22 deletions(-) diff --git a/activitypub.py b/activitypub.py index b0964cf1..952e4c60 100644 --- a/activitypub.py +++ b/activitypub.py @@ -37,6 +37,7 @@ PRIMARY_DOMAIN, PROTOCOL_DOMAINS, redirect_wrap, + report_error, subdomain_wrap, unwrap, ) @@ -1070,31 +1071,19 @@ def inbox(protocol=None, id=None): # do we support this object type? # (this logic is duplicated in Protocol.check_supported) - object = as1.get_object(activity) + obj = as1.get_object(activity) if type := activity.get('type'): - inner_type = as1.object_type(object) or '' + inner_type = as1.object_type(obj) or '' if (type not in ActivityPub.SUPPORTED_AS2_TYPES or (type in as2.CRUD_VERBS and inner_type and inner_type not in ActivityPub.SUPPORTED_AS2_TYPES)): error(f"Bridgy Fed for ActivityPub doesn't support {type} {inner_type} yet: {json_dumps(activity, indent=2)}", status=204) - # are we already processing or done with this activity? - id = activity.get('id') - if id: - domain = util.domain_from_link(id) - if util.domain_or_parent_in(domain, web_opt_out_domains()): - logger.info(f'{domain} is opted out') - return '', 204 - - if memcache.get(activity_id_memcache_key(id)): - logger.info(f'Already seen {id}') - return '', 204 - - # check actor, signature, auth + # check actor, authz actor's domain against activity and object ids + # https://github.com/snarfed/bridgy-fed/security/advisories/GHSA-37r7-jqmr-3472 actor = as1.get_object(activity, 'actor') actor_id = actor.get('id') - logger.info(f'Got {type} {id} from {actor_id}') if ActivityPub.is_blocklisted(actor_id): error(f'Actor {actor_id} is blocklisted') @@ -1104,6 +1093,28 @@ def inbox(protocol=None, id=None): logger.info(f'{actor_domain} is opted out') return '', 204 + id = activity.get('id') + obj_id = obj.get('id') + if id and actor_domain != util.domain_from_link(id): + report_error('Auth: actor and activity on different domains', + user=f'actor {actor_id} activity {id}') + elif (type in as2.CRUD_VERBS and obj_id + and actor_domain != util.domain_from_link(obj_id)): + report_error('Auth: actor and object on different domains', + user=f'actor {actor_id} object {id}') + + # are we already processing or done with this activity? + if id: + domain = util.domain_from_link(id) + if util.domain_or_parent_in(domain, web_opt_out_domains()): + logger.info(f'{domain} is opted out') + return '', 204 + + if memcache.get(activity_id_memcache_key(id)): + logger.info(f'Already seen {id}') + return '', 204 + + # check signature, auth authed_as = ActivityPub.verify_signature(activity) authed_domain = util.domain_from_link(authed_as) @@ -1115,6 +1126,8 @@ def inbox(protocol=None, id=None): if authed_as != actor_id and activity.get('signature'): error(f"Ignoring LD Signature, sorry, we can't verify those yet. https://github.com/snarfed/bridgy-fed/issues/566", status=202) + logger.info(f'Got {type} {id} from {actor_id}') + if type == 'Follow': # rendered mf2 HTML proxy pages (in render.py) fall back to redirecting # to the follow's AS2 id field, but Mastodon's Accept ids are URLs that @@ -1128,7 +1141,7 @@ def inbox(protocol=None, id=None): activity.setdefault('url', f'{follower_url}#followed-{followee_url}') if not id: - id = f'{actor_id}#{type}-{object.get("id", "")}-{util.now().isoformat()}' + id = f'{actor_id}#{type}-{obj_id or ""}-{util.now().isoformat()}' # automatically bridge server aka instance actors # https://codeberg.org/fediverse/fep/src/branch/main/fep/d556/fep-d556.md diff --git a/tests/test_activitypub.py b/tests/test_activitypub.py index c589ac73..d35c6484 100644 --- a/tests/test_activitypub.py +++ b/tests/test_activitypub.py @@ -1696,13 +1696,9 @@ def test_delete_actor(self, *mocks): def test_delete_actor_not_fetchable(self, _, mock_get, ___): mock_get.return_value = requests_response(status=410) - with self.assertLogs() as logs: - got = self.post('/ap/sharedInbox', json=DELETE) - + got = self.post('/ap/sharedInbox', json=DELETE) self.assertEqual(202, got.status_code) self.assertTrue(Object.get_by_id(DELETE['object']).deleted) - # self.assertIn('Object/actor being deleted is also keyId', - # ' '.join(logs.output)) def test_delete_actor_empty_deleted_object(self, _, mock_get, ___): actor = self.make_user(DELETE['actor'], cls=ActivityPub) @@ -1880,6 +1876,50 @@ def test_inbox_existing_server_actor_adds_enabled_protocols( actor = ActivityPub.get_by_id('https://mas.to/actor') self.assertCountEqual(['ui', 'fake', 'other'], actor.enabled_protocols) + # https://github.com/snarfed/bridgy-fed/security/advisories/GHSA-37r7-jqmr-3472 + def test_inbox_actor_auth_check_activity_id_different_domain( + self, mock_head, mock_get, mock_post): + mock_get.side_effect = [ + self.as2_resp(ACTOR), + self.as2_resp(ACTOR), + self.as2_resp(NOTE), + ] + + with self.assertLogs() as logs: + got = self.post('/user.com/inbox', json={ + 'id': 'http://no.pe/like', + 'type': 'Like', + 'actor': 'https://mas.to/users/swentel', + 'object': 'https://mas.to/note/as2', + }) + + self.assertEqual(204, got.status_code) + self.assertIn('Auth: actor and activity on different domains', + ' '.join(logs.output)) + + # https://github.com/snarfed/bridgy-fed/security/advisories/GHSA-37r7-jqmr-3472 + def test_inbox_actor_auth_check_object_id_different_domain( + self, mock_head, mock_get, mock_post): + mock_get.side_effect = [ + self.as2_resp(ACTOR), + self.as2_resp(ACTOR), + ] + + with self.assertLogs() as logs: + got = self.post('/user.com/inbox', json={ + 'id': 'http://mas.to/create', + 'type': 'Create', + 'actor': 'https://mas.to/users/swentel', + 'object': { + **NOTE_OBJECT, + 'id': 'https://no.pe/note', + }, + }) + + self.assertEqual(204, got.status_code) + self.assertIn('Auth: actor and object on different domains', + ' '.join(logs.output)) + def test_followers_collection_unknown_user(self, *_): resp = self.client.get('/nope.com/followers') self.assertEqual(404, resp.status_code)