Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

E2EE permission check too late when creating a room #15688

Closed
grantm opened this issue May 30, 2023 · 3 comments
Closed

E2EE permission check too late when creating a room #15688

grantm opened this issue May 30, 2023 · 3 comments
Labels
O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@grantm
Copy link
Contributor

grantm commented May 30, 2023

Description

For context: I have an in-house Synapse server on which I am disabling E2EE with default_power_level_content_override. Our configuration sets the power level required for sending the m.room.encryption event to 999.

This mostly works, except for the following.

If a user attempts to create a room with E2EE enabled, the normal server sequence includes these steps:

  1. create the room (in the DB)
  2. send the m.room.create event
  3. send the m.room.member event
  4. send the m.room.encryption event
  5. send the m.room.power_levels event

With the power levels overridden as described above, the API request is correctly rejected with a 403 and the server logs this message:

Denying new event <FrozenEventV3 event_id=$NGozDEtEjIc54LXI87iIio-CroITX-omswcsjmEORFE, type=m.room.encryption, state_key=, outlier=False> because 403: You don't have permission to post that to the room. user_level (100) < send_level (999)

Unfortunately, the exception for the 403 response is raised after the m.room.create event and before the m.room.power_levels event. Which means that the room is created, but the power level overrides are not applied. So the room will appear as "Empty Room" in the Element client and the user can complete the initialisation by manually setting the room name and enabling encryption.

I have made a proof of concept patch that checks the creator's intent (with respect to E2EE) earlier and raises the 403 error sooner so the user doesn't get ownership of a partially configured room. With this patch, the room_id does still get allocated in the rooms table, but I'm not sure it's worth the trouble to avoid that.

Any guidance on progressing this to a PR gratefully accepted.

Steps to reproduce

  • Use the following setting in homeserver.yaml to disallow users from enabling E2EE:
      default_power_level_content_override:
      private_chat:
        "events":
          "m.room.avatar": 50
          "m.room.canonical_alias": 50
          "m.room.encryption": 999
          "m.room.history_visibility": 100
          "m.room.name": 50
          "m.room.power_levels": 100
          "m.room.server_acl": 100
          "m.room.tombstone": 100
        "events_default": 0
      trusted_private_chat:
        "events":
          "m.room.avatar": 50
          "m.room.canonical_alias": 50
          "m.room.encryption": 999
          "m.room.history_visibility": 100
          "m.room.name": 50
          "m.room.power_levels": 100
          "m.room.server_acl": 100
          "m.room.tombstone": 100
        "events_default": 0
      public_chat:
        "events":
          "m.room.avatar": 50
          "m.room.canonical_alias": 50
          "m.room.encryption": 999
          "m.room.history_visibility": 100
          "m.room.name": 50
          "m.room.power_levels": 100
          "m.room.server_acl": 100
          "m.room.tombstone": 100
        "events_default": 0
    
  • Attempt to create a room with encryption enabled (here using curl, but could be done in client):
    curl -D - --header "Content-Type: application/json" \
      --header "Authorization: Bearer <token here>" \
      --request POST \
      --data '{"creation_content": {"m.federate": false},"name": "Secret Private Room","preset": "private_chat","initial_state": [{"type": "m.room.encryption","state_key": "","content": {"algorithm": "m.megolm.v1.aes-sha2"}}]}' \
      http://localhost:8008/_matrix/client/v3/createRoom
    
  • Confirm that the request was rejected with a 403 error.
  • Confirm that the room was created despite the error (e.g.: it will appear in the users room list in the client)
  • Confirm that the default_power_level_content_override values were not applied and in room settings the value for m.room.encryption is 100 rather than 999
  • Confirm that in a client, the user can enable encryption in the room

Homeserver

A private in-house server

Synapse Version

1.84.0rc1

Installation Method

pip (from PyPI)

Database

PostgreSQL

Workers

Single process

Platform

Server for these tests was a development environment running on a laptop running Ubuntu 20.04 with the homeserver process running directly in the host OS (not in a container)

Configuration

Yes, see default_power_level_content_override setting above.

Relevant log output

Denying new event <FrozenEventV3 event_id=$NGozDEtEjIc54LXI87iIio-CroITX-omswcsjmEORFE, type=m.room.encryption, state_key=, outlier=False> because 403: You don't have permission to post that to the room. user_level (100) < send_level (999)

Anything else that would be useful to know?

No response

@DMRobertson
Copy link
Contributor

Xref: #8895 tracks the "/createRoom is not atomic" problem.

@clokep
Copy link
Member

clokep commented May 30, 2023

Any guidance on progressing this to a PR gratefully accepted.

I think my guidance would mostly be -- create a PR and let's chat about it there. 😄

The changes look somewhat reasonable, but I think (in reference to what @DMRobertson posted) it might make sense to create a new function (_validate_room_config or something) that encapsulates the logic for ensuring the config makes sense. There's a bunch of checks we already do that could be added there too, I think.

This would get called before we create any events.

It still wouldn't fully solve #8895, because we don't (yet) create the entire room in a single database transaction, but it would solve this and maybe some other cases.

@clokep clokep added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. O-Uncommon Most users are unlikely to come across this or unexpected workflow labels May 30, 2023
@reivilibre
Copy link
Contributor

I believe this is fixed by #15695.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

No branches or pull requests

4 participants