From 934bd69bba275787aca9f4862cb1504431415b05 Mon Sep 17 00:00:00 2001 From: Jason Robinson Date: Sun, 6 Aug 2017 14:04:50 +0300 Subject: [PATCH] Verify payload and entity handle are the same Add proper checks to make sure Diaspora protocol payload handle and entity handle are the same. Even though we already verified the signature of the sender, we didn't ensure that the sender isn't trying to fake an entity authored by someone else. The Diaspora protocol functions `message_to_objects` and `element_to_objects` now require a new parameter, the payload sender handle. These functions should normally not be needed to be used directly. --- CHANGELOG.md | 5 ++ federation/entities/diaspora/mappers.py | 29 ++++++++-- federation/inbound.py | 2 +- .../tests/entities/diaspora/test_entities.py | 3 +- .../tests/entities/diaspora/test_mappers.py | 58 ++++++++++++------- federation/tests/fixtures/payloads.py | 2 +- 6 files changed, 71 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e0fb126..4b02ba8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## [unreleased] +### Security +* Add proper checks to make sure Diaspora protocol payload handle and entity handle are the same. Even though we already verified the signature of the sender, we didn't ensure that the sender isn't trying to fake an entity authored by someone else. + + The Diaspora protocol functions `message_to_objects` and `element_to_objects` now require a new parameter, the payload sender handle. These functions should normally not be needed to be used directly. + ### Changed * **Breaking change.** The high level `federation.outbound` functions `handle_send` and `handle_create_payload` signatures have been changed. This has been done to better represent the objects that are actually sent in and to add an optional `parent_user` object. diff --git a/federation/entities/diaspora/mappers.py b/federation/entities/diaspora/mappers.py index 6a1d4cb..2acb27a 100644 --- a/federation/entities/diaspora/mappers.py +++ b/federation/entities/diaspora/mappers.py @@ -52,12 +52,27 @@ def xml_children_as_dict(node): return dict((e.tag, e.text) for e in node) -def element_to_objects(element, sender_key_fetcher=None, user=None): +def check_sender_and_entity_handle_match(sender_handle, entity_handle): + """Ensure that sender and entity handles match. + + Basically we've already verified the sender is who they say when receiving the payload. However, the sender might + be trying to set another author in the payload itself, since Diaspora has the sender in both the payload headers + AND the object. We must ensure they're the same. + """ + if sender_handle != entity_handle: + logger.warning("sender_handle and entity_handle don't match, aborting! sender_handle: %s, entity_handle: %s", + sender_handle, entity_handle) + return False + return True + + +def element_to_objects(element, sender, sender_key_fetcher=None, user=None): """Transform an Element to a list of entities recursively. Possible child entities are added to each entity `_children` list. :param tree: Element + :param sender: Payload sender handle :param sender_key_fetcher: Function to fetch sender public key. If not given, key will always be fetched over network. The function should take sender handle as the only parameter. :param user: Optional receiving user object. If given, should have a `handle`. @@ -73,6 +88,8 @@ def element_to_objects(element, sender_key_fetcher=None, user=None): if hasattr(cls, "fill_extra_attributes"): transformed = cls.fill_extra_attributes(transformed) entity = cls(**transformed) + if not check_sender_and_entity_handle_match(sender, entity.handle): + return [] # Add protocol name entity._source_protocol = "diaspora" # Save element object to entity for possible later use @@ -99,7 +116,7 @@ def element_to_objects(element, sender_key_fetcher=None, user=None): return [] # Do child elements for child in element: - entity._children.extend(element_to_objects(child)) + entity._children.extend(element_to_objects(child, sender)) # Add to entities list entities.append(entity) if cls == DiasporaRequest: @@ -110,11 +127,13 @@ def element_to_objects(element, sender_key_fetcher=None, user=None): return entities -def message_to_objects(message, sender_key_fetcher=None, user=None): +def message_to_objects(message, sender, sender_key_fetcher=None, user=None): """Takes in a message extracted by a protocol and maps it to entities. :param message: XML payload :type message: str + :param sender: Payload sender handle + :type message: str :param sender_key_fetcher: Function to fetch sender public key. If not given, key will always be fetched over network. The function should take sender handle as the only parameter. :param user: Optional receiving user object. If given, should have a `handle`. @@ -123,12 +142,12 @@ def message_to_objects(message, sender_key_fetcher=None, user=None): doc = etree.fromstring(message) # Future Diaspora protocol version contains the element at top level if doc.tag in TAGS: - return element_to_objects(doc, sender_key_fetcher, user) + return element_to_objects(doc, sender, sender_key_fetcher, user) # Legacy Diaspora protocol wraps the element in , so find the right element for tag in TAGS: element = doc.find(".//%s" % tag) if element is not None: - return element_to_objects(element, sender_key_fetcher, user) + return element_to_objects(element, sender, sender_key_fetcher, user) return [] diff --git a/federation/inbound.py b/federation/inbound.py index 8c6edb3..d4a9bbe 100644 --- a/federation/inbound.py +++ b/federation/inbound.py @@ -48,7 +48,7 @@ def handle_receive(payload, user=None, sender_key_fetcher=None, skip_author_veri raise NoSuitableProtocolFoundError() mappers = importlib.import_module("federation.entities.%s.mappers" % found_protocol.PROTOCOL_NAME) - entities = mappers.message_to_objects(message, sender_key_fetcher, user) + entities = mappers.message_to_objects(message, sender, sender_key_fetcher, user) logger.debug("handle_receive: entities %s", entities) return sender, found_protocol.PROTOCOL_NAME, entities diff --git a/federation/tests/entities/diaspora/test_entities.py b/federation/tests/entities/diaspora/test_entities.py index 7e9bab4..2f8671d 100644 --- a/federation/tests/entities/diaspora/test_entities.py +++ b/federation/tests/entities/diaspora/test_entities.py @@ -147,7 +147,8 @@ class TestDiasporaRelayableMixin: @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures") def test_sign_with_parent(self, mock_validate): - entities = message_to_objects(DIASPORA_POST_COMMENT, sender_key_fetcher=Mock()) + entities = message_to_objects(DIASPORA_POST_COMMENT, "alice@alice.diaspora.example.org", + sender_key_fetcher=Mock()) entity = entities[0] entity.sign_with_parent(get_dummy_private_key()) assert entity.parent_signature == "UTIDiFZqjxfU6ssVlmjz2RwOD/WPmMTFv57qOm0BZvBhF8Ef49Ynse1c2XTtx3rs8DyRMn54" \ diff --git a/federation/tests/entities/diaspora/test_mappers.py b/federation/tests/entities/diaspora/test_mappers.py index 2c8c0ad..9313596 100644 --- a/federation/tests/entities/diaspora/test_mappers.py +++ b/federation/tests/entities/diaspora/test_mappers.py @@ -10,7 +10,8 @@ from federation.entities.base import ( from federation.entities.diaspora.entities import ( DiasporaPost, DiasporaComment, DiasporaLike, DiasporaRequest, DiasporaProfile, DiasporaRetraction, DiasporaContact) -from federation.entities.diaspora.mappers import message_to_objects, get_outbound_entity +from federation.entities.diaspora.mappers import ( + message_to_objects, get_outbound_entity, check_sender_and_entity_handle_match) from federation.tests.fixtures.payloads import ( DIASPORA_POST_SIMPLE, DIASPORA_POST_COMMENT, DIASPORA_POST_LIKE, DIASPORA_REQUEST, DIASPORA_PROFILE, DIASPORA_POST_INVALID, DIASPORA_RETRACTION, @@ -23,9 +24,9 @@ def mock_fill(attributes): return attributes -class TestDiasporaEntityMappersReceive(): +class TestDiasporaEntityMappersReceive: def test_message_to_objects_simple_post(self): - entities = message_to_objects(DIASPORA_POST_SIMPLE) + entities = message_to_objects(DIASPORA_POST_SIMPLE, "alice@alice.diaspora.example.org") assert len(entities) == 1 post = entities[0] assert isinstance(post, DiasporaPost) @@ -39,7 +40,7 @@ class TestDiasporaEntityMappersReceive(): def test_message_to_objects_post_legacy(self): # This is the previous XML schema used before renewal of protocol - entities = message_to_objects(DIASPORA_POST_LEGACY) + entities = message_to_objects(DIASPORA_POST_LEGACY, "alice@alice.diaspora.example.org") assert len(entities) == 1 post = entities[0] assert isinstance(post, DiasporaPost) @@ -52,12 +53,12 @@ class TestDiasporaEntityMappersReceive(): assert post.provider_display_name == "Socialhome" def test_message_to_objects_legact_timestamp(self): - entities = message_to_objects(DIASPORA_POST_LEGACY_TIMESTAMP) + entities = message_to_objects(DIASPORA_POST_LEGACY_TIMESTAMP, "alice@alice.diaspora.example.org") post = entities[0] assert post.created_at == datetime(2011, 7, 20, 1, 36, 7) def test_message_to_objects_post_with_photos(self): - entities = message_to_objects(DIASPORA_POST_WITH_PHOTOS) + entities = message_to_objects(DIASPORA_POST_WITH_PHOTOS, "alice@alice.diaspora.example.org") assert len(entities) == 1 post = entities[0] assert isinstance(post, DiasporaPost) @@ -75,7 +76,7 @@ class TestDiasporaEntityMappersReceive(): assert photo.public == False assert photo.created_at == datetime(2011, 7, 20, 1, 36, 7) - entities = message_to_objects(DIASPORA_POST_WITH_PHOTOS_2) + entities = message_to_objects(DIASPORA_POST_WITH_PHOTOS_2, "xxxxxxxxxxxxxxx@diasp.org") assert len(entities) == 1 post = entities[0] assert isinstance(post, DiasporaPost) @@ -84,7 +85,8 @@ class TestDiasporaEntityMappersReceive(): @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures") def test_message_to_objects_comment(self, mock_validate): - entities = message_to_objects(DIASPORA_POST_COMMENT, sender_key_fetcher=Mock()) + entities = message_to_objects(DIASPORA_POST_COMMENT, "alice@alice.diaspora.example.org", + sender_key_fetcher=Mock()) assert len(entities) == 1 comment = entities[0] assert isinstance(comment, DiasporaComment) @@ -102,7 +104,7 @@ class TestDiasporaEntityMappersReceive(): @patch("federation.entities.diaspora.mappers.DiasporaLike._validate_signatures") def test_message_to_objects_like(self, mock_validate): - entities = message_to_objects(DIASPORA_POST_LIKE, sender_key_fetcher=Mock()) + entities = message_to_objects(DIASPORA_POST_LIKE, "alice@alice.diaspora.example.org", sender_key_fetcher=Mock()) assert len(entities) == 1 like = entities[0] assert isinstance(like, DiasporaLike) @@ -119,7 +121,7 @@ class TestDiasporaEntityMappersReceive(): mock_validate.assert_called_once_with() def test_message_to_objects_request(self): - entities = message_to_objects(DIASPORA_REQUEST) + entities = message_to_objects(DIASPORA_REQUEST, "bob@example.com") assert len(entities) == 2 sharing = entities[0] assert isinstance(sharing, DiasporaRequest) @@ -136,7 +138,7 @@ class TestDiasporaEntityMappersReceive(): @patch("federation.entities.diaspora.entities.DiasporaProfile.fill_extra_attributes", new=mock_fill) def test_message_to_objects_profile(self): - entities = message_to_objects(DIASPORA_PROFILE) + entities = message_to_objects(DIASPORA_PROFILE, "bob@example.com") assert len(entities) == 1 profile = entities[0] assert profile.handle == "bob@example.com" @@ -155,11 +157,11 @@ class TestDiasporaEntityMappersReceive(): @patch("federation.entities.diaspora.entities.DiasporaProfile.fill_extra_attributes", new=mock_fill) def test_message_to_objects_profile_survives_empty_tag_string(self): - entities = message_to_objects(DIASPORA_PROFILE_EMPTY_TAGS) + entities = message_to_objects(DIASPORA_PROFILE_EMPTY_TAGS, "bob@example.com") assert len(entities) == 1 def test_message_to_objects_retraction(self): - entities = message_to_objects(DIASPORA_RETRACTION) + entities = message_to_objects(DIASPORA_RETRACTION, "bob@example.com") assert len(entities) == 1 entity = entities[0] assert isinstance(entity, Retraction) @@ -168,7 +170,8 @@ class TestDiasporaEntityMappersReceive(): assert entity.entity_type == "Post" def test_message_to_objects_retraction_legacy_request(self): - entities = message_to_objects(DIASPORA_LEGACY_REQUEST_RETRACTION, user=Mock(guid="swfeuihiwehuifhiwheiuf")) + entities = message_to_objects(DIASPORA_LEGACY_REQUEST_RETRACTION, "jaywink@iliketoast.net", + user=Mock(guid="swfeuihiwehuifhiwheiuf")) assert len(entities) == 1 entity = entities[0] assert isinstance(entity, Retraction) @@ -178,7 +181,7 @@ class TestDiasporaEntityMappersReceive(): assert entity._receiving_guid == "swfeuihiwehuifhiwheiuf" def test_message_to_objects_contact(self): - entities = message_to_objects(DIASPORA_CONTACT) + entities = message_to_objects(DIASPORA_CONTACT, "alice@example.com") assert len(entities) == 1 entity = entities[0] assert isinstance(entity, DiasporaContact) @@ -188,32 +191,42 @@ class TestDiasporaEntityMappersReceive(): @patch("federation.entities.diaspora.mappers.logger.error") def test_invalid_entity_logs_an_error(self, mock_logger): - entities = message_to_objects(DIASPORA_POST_INVALID) + entities = message_to_objects(DIASPORA_POST_INVALID, "alice@alice.diaspora.example.org") assert len(entities) == 0 assert mock_logger.called def test_adds_source_protocol_to_entity(self): - entities = message_to_objects(DIASPORA_POST_SIMPLE) + entities = message_to_objects(DIASPORA_POST_SIMPLE, "alice@alice.diaspora.example.org") assert entities[0]._source_protocol == "diaspora" @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures") def test_source_object(self, mock_validate): - entities = message_to_objects(DIASPORA_POST_COMMENT, sender_key_fetcher=Mock()) + entities = message_to_objects(DIASPORA_POST_COMMENT, "alice@alice.diaspora.example.org", + sender_key_fetcher=Mock()) entity = entities[0] assert entity._source_object == etree.tostring(etree.fromstring(DIASPORA_POST_COMMENT)) @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures") def test_element_to_objects_calls_sender_key_fetcher(self, mock_validate): mock_fetcher = Mock() - message_to_objects(DIASPORA_POST_COMMENT, mock_fetcher) + message_to_objects(DIASPORA_POST_COMMENT, "alice@alice.diaspora.example.org", mock_fetcher) mock_fetcher.assert_called_once_with("alice@alice.diaspora.example.org") @patch("federation.entities.diaspora.mappers.DiasporaComment._validate_signatures") @patch("federation.entities.diaspora.mappers.retrieve_and_parse_profile") def test_element_to_objects_calls_retrieve_remote_profile(self, mock_retrieve, mock_validate): - message_to_objects(DIASPORA_POST_COMMENT) + message_to_objects(DIASPORA_POST_COMMENT, "alice@alice.diaspora.example.org") mock_retrieve.assert_called_once_with("alice@alice.diaspora.example.org") + @patch("federation.entities.diaspora.mappers.check_sender_and_entity_handle_match") + def test_element_to_objects_verifies_handles_are_the_same(self, mock_check): + message_to_objects(DIASPORA_POST_SIMPLE, "bob@example.org") + mock_check.assert_called_once_with("bob@example.org", "alice@alice.diaspora.example.org") + + def test_element_to_objects_returns_no_entity_if_handles_are_different(self): + entities = message_to_objects(DIASPORA_POST_SIMPLE, "bob@example.org") + assert not entities + class TestGetOutboundEntity: def test_already_fine_entities_are_returned_as_is(self, private_key): @@ -279,3 +292,8 @@ class TestGetOutboundEntity: entity = Comment() entity.outbound_doc = "foobar" assert get_outbound_entity(entity, private_key) == entity + + +def test_check_sender_and_entity_handle_match(): + assert not check_sender_and_entity_handle_match("foo", "bar") + assert check_sender_and_entity_handle_match("foo", "foo") diff --git a/federation/tests/fixtures/payloads.py b/federation/tests/fixtures/payloads.py index 25bd80a..973247b 100644 --- a/federation/tests/fixtures/payloads.py +++ b/federation/tests/fixtures/payloads.py @@ -118,7 +118,7 @@ DIASPORA_POST_WITH_PHOTOS_2 = """ #foo #bar (fewfefewfwfewfwe) fjwjewiofjoiwjfiowefewew - xxxxxxxxxxxxxxxxx@diasp.org + xxxxxxxxxxxxxxx@diasp.org true 2017-06-10T14:41:28Z https://diasp.org/uploads/images/