From febd39ad5cc69b2520739fb836bdcefb1b3e578b Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 3 Nov 2020 10:02:03 +0000 Subject: [PATCH 1/5] Add some typing --- mypy.ini | 1 + synapse/events/validator.py | 20 +++++++++++--------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/mypy.ini b/mypy.ini index 5e9f7b1259e4..3c9ea1d9ae7c 100644 --- a/mypy.ini +++ b/mypy.ini @@ -13,6 +13,7 @@ files = synapse/config, synapse/event_auth.py, synapse/events/builder.py, + synapse/events/validator.py, synapse/events/spamcheck.py, synapse/federation, synapse/handlers/_base.py, diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 5f9af8529be0..628d9690baae 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -13,20 +13,25 @@ # See the License for the specific language governing permissions and # limitations under the License. +from typing import Union + from synapse.api.constants import MAX_ALIAS_LENGTH, EventTypes, Membership from synapse.api.errors import Codes, SynapseError from synapse.api.room_versions import EventFormatVersions +from synapse.config.homeserver import HomeServerConfig +from synapse.events import EventBase +from synapse.events.builder import EventBuilder from synapse.events.utils import validate_canonicaljson from synapse.types import EventID, RoomID, UserID class EventValidator: - def validate_new(self, event, config): + def validate_new(self, event: EventBase, config: HomeServerConfig): """Validates the event has roughly the right format Args: - event (FrozenEvent): The event to validate. - config (Config): The homeserver's configuration. + event The event to validate. + config: The homeserver's configuration. """ self.validate_builder(event) @@ -76,12 +81,12 @@ def validate_new(self, event, config): if event.type == EventTypes.Retention: self._validate_retention(event) - def _validate_retention(self, event): + def _validate_retention(self, event: EventBase): """Checks that an event that defines the retention policy for a room respects the format enforced by the spec. Args: - event (FrozenEvent): The event to validate. + event: The event to validate. """ if not event.is_state(): raise SynapseError(code=400, msg="must be a state event") @@ -116,13 +121,10 @@ def _validate_retention(self, event): errcode=Codes.BAD_JSON, ) - def validate_builder(self, event): + def validate_builder(self, event: Union[EventBase, EventBuilder]): """Validates that the builder/event has roughly the right format. Only checks values that we expect a proto event to have, rather than all the fields an event would have - - Args: - event (EventBuilder|FrozenEvent) """ strings = ["room_id", "sender", "type"] From 784dc5a815cedea2c67cadd97a0ba6f8ba5286a4 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 3 Nov 2020 10:14:25 +0000 Subject: [PATCH 2/5] Block server acl on CS API that block local server --- synapse/events/validator.py | 7 +++++++ tests/handlers/test_message.py | 36 ++++++++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 628d9690baae..24a7987e4856 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -22,6 +22,7 @@ from synapse.events import EventBase from synapse.events.builder import EventBuilder from synapse.events.utils import validate_canonicaljson +from synapse.federation.federation_server import server_matches_acl_event from synapse.types import EventID, RoomID, UserID @@ -81,6 +82,12 @@ def validate_new(self, event: EventBase, config: HomeServerConfig): if event.type == EventTypes.Retention: self._validate_retention(event) + if event.type == EventTypes.ServerACL: + if not server_matches_acl_event(config.server_name, event): + raise SynapseError( + 400, "Can't create an ACL event that denies the local server" + ) + def _validate_retention(self, event: EventBase): """Checks that an event that defines the retention policy for a room respects the format enforced by the spec. diff --git a/tests/handlers/test_message.py b/tests/handlers/test_message.py index 9f6f21a6e202..d2d0354526b1 100644 --- a/tests/handlers/test_message.py +++ b/tests/handlers/test_message.py @@ -154,3 +154,39 @@ def test_duplicated_txn_id_one_call(self): # Check that we've deduplicated the events. self.assertEqual(len(events), 2) self.assertEqual(events[0].event_id, events[1].event_id) + + +class ServerAclValidationTestCase(unittest.HomeserverTestCase): + servlets = [ + admin.register_servlets, + login.register_servlets, + room.register_servlets, + ] + + def prepare(self, reactor, clock, hs): + self.user_id = self.register_user("tester", "foobar") + self.access_token = self.login("tester", "foobar") + self.room_id = self.helper.create_room_as(self.user_id, tok=self.access_token) + + def test_allow_server_acl(self): + """Test that sending an ACL that blocks everyone but ourselves work. + """ + + self.helper.send_state( + self.room_id, + EventTypes.ServerACL, + body={"allow": [self.hs.hostname]}, + tok=self.access_token, + expect_code=200, + ) + + def test_deny_server_acl_block_outselves(self): + """Test that sending an ACL that blocks ourselves does not work. + """ + self.helper.send_state( + self.room_id, + EventTypes.ServerACL, + body={}, + tok=self.access_token, + expect_code=400, + ) From b0667ddfd117808a8c8e409ab61190fac2a74a0f Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 3 Nov 2020 10:20:17 +0000 Subject: [PATCH 3/5] Block redactions of server acl on CS API --- synapse/handlers/message.py | 3 +++ tests/handlers/test_message.py | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py index fb0a04e9a7ab..b45ec397bdfc 100644 --- a/synapse/handlers/message.py +++ b/synapse/handlers/message.py @@ -1159,6 +1159,9 @@ def is_inviter_member_event(e): if original_event.room_id != event.room_id: raise SynapseError(400, "Cannot redact event from a different room") + if original_event.type == EventTypes.ServerACL: + raise AuthError(403, "Redacting server ACL events is not permitted") + prev_state_ids = await context.get_prev_state_ids() auth_events_ids = self.auth.compute_auth_events( event, prev_state_ids, for_verification=True diff --git a/tests/handlers/test_message.py b/tests/handlers/test_message.py index d2d0354526b1..93b61456ab50 100644 --- a/tests/handlers/test_message.py +++ b/tests/handlers/test_message.py @@ -190,3 +190,24 @@ def test_deny_server_acl_block_outselves(self): tok=self.access_token, expect_code=400, ) + + def test_deny_redact_server_acl(self): + """Test that attempting to redact an ACL is blocked + """ + + body = self.helper.send_state( + self.room_id, + EventTypes.ServerACL, + body={"allow": [self.hs.hostname]}, + tok=self.access_token, + expect_code=200, + ) + event_id = body["event_id"] + + # Redaction of event should fail. + path = "/_matrix/client/r0/rooms/%s/redact/%s" % (self.room_id, event_id) + request, channel = self.make_request( + "POST", path, content={}, access_token=self.access_token + ) + self.render(request) + self.assertEqual(int(channel.result["code"]), 403) From b0594c7059d079509647d48f6e76e5cb640a7a33 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 3 Nov 2020 10:22:16 +0000 Subject: [PATCH 4/5] Newsfile --- changelog.d/8708.misc | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/8708.misc diff --git a/changelog.d/8708.misc b/changelog.d/8708.misc new file mode 100644 index 000000000000..1d31ed0f2a2b --- /dev/null +++ b/changelog.d/8708.misc @@ -0,0 +1 @@ +Block attempts by clients to sender server ACLs, or redactions of server ACLs, that would result in the local server being blocked from the room. From 8f85440b24649acc4e8a30bb14fe55ff4ab185aa Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 3 Nov 2020 10:41:10 +0000 Subject: [PATCH 5/5] WORDS WORDS WORDS Co-authored-by: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com> --- changelog.d/8708.misc | 2 +- synapse/events/validator.py | 2 +- tests/handlers/test_message.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/changelog.d/8708.misc b/changelog.d/8708.misc index 1d31ed0f2a2b..be679fb0f8d2 100644 --- a/changelog.d/8708.misc +++ b/changelog.d/8708.misc @@ -1 +1 @@ -Block attempts by clients to sender server ACLs, or redactions of server ACLs, that would result in the local server being blocked from the room. +Block attempts by clients to send server ACLs, or redactions of server ACLs, that would result in the local server being blocked from the room. diff --git a/synapse/events/validator.py b/synapse/events/validator.py index 24a7987e4856..f8f3b1a31e0b 100644 --- a/synapse/events/validator.py +++ b/synapse/events/validator.py @@ -31,7 +31,7 @@ def validate_new(self, event: EventBase, config: HomeServerConfig): """Validates the event has roughly the right format Args: - event The event to validate. + event: The event to validate. config: The homeserver's configuration. """ self.validate_builder(event) diff --git a/tests/handlers/test_message.py b/tests/handlers/test_message.py index 455528e5b4d5..8b57081cbe70 100644 --- a/tests/handlers/test_message.py +++ b/tests/handlers/test_message.py @@ -169,7 +169,7 @@ def prepare(self, reactor, clock, hs): self.room_id = self.helper.create_room_as(self.user_id, tok=self.access_token) def test_allow_server_acl(self): - """Test that sending an ACL that blocks everyone but ourselves work. + """Test that sending an ACL that blocks everyone but ourselves works. """ self.helper.send_state( @@ -192,7 +192,7 @@ def test_deny_server_acl_block_outselves(self): ) def test_deny_redact_server_acl(self): - """Test that attempting to redact an ACL is blocked + """Test that attempting to redact an ACL is blocked. """ body = self.helper.send_state(