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

MSC2260: Update the auth rules for m.room.aliases events #2260

Closed
wants to merge 9 commits into from

Conversation

richvdh
Copy link
Member

@richvdh richvdh commented Aug 29, 2019

@richvdh richvdh added the proposal A matrix spec change proposal label Aug 29, 2019
@turt2live
Copy link
Member

(if this is a WIP, please tag it in the title. If it's not, please add proposal-in-review as a label)

Co-Authored-By: Matthew Hodgson <matthew@matrix.org>
@richvdh
Copy link
Member Author

richvdh commented Aug 29, 2019

actually, it's clear that this is still WIP

@richvdh richvdh changed the title MSC2260: Update the auth rules for m.room.aliases events WIP: MSC2260: Update the auth rules for m.room.aliases events Aug 29, 2019
@richvdh richvdh changed the title WIP: MSC2260: Update the auth rules for m.room.aliases events [WIP] MSC2260: Update the auth rules for m.room.aliases events Aug 29, 2019
rules](https://matrix.org/docs/spec/rooms/v1#authorization-rules). `m.room.aliases`
would instead be authorised following the normal rules for state events.

This would mean that only room moderators could add entries to the
Copy link
Member

Choose a reason for hiding this comment

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

I think we should still let users contribute entries to the event, because otherwise they will have to tell the moderator out-of-band to add their room. As you say, it also increases the risk that when the alias is removed from the directory in question, the event won't be updated if a normal user can't update it.

If a room admin deliberately wants to lock down who can advertise aliases to just be mods/ops, then they can do that too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should still let users contribute entries to the event,

I think this is different to what you said at https://gist.github.com/ara4n/0ec423e7c321acbb53eed04ceb4dd7df#gistcomment-3012860 though. Have you changed your mind or is one of us misunderstanding?

If we continue to allow regular users to update the aliases event without regard to power levels, then we still have an abuse problem that is hard to control.

Or do you mean that aliases events should be subject to PLs, but for public rooms, we should emit a PL event that lowers the required level for aliases to 0? In which case, I think this makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Have you changed your mind or is one of us misunderstanding?

I think i am misunderstanding. My assumption was that the special cases for 4a and 4b still take effect (meaning that you can't publish m.room.aliases on behalf of remote servers). However, on 4c, normal PL rules take effect - so it's possible to let anyone in the room advertise an alias via m.room.aliases, or alternatively put a PL threshold and only let ops/admins do so. My hunch would be to allow anyone to set one by default (so default to PL threshold of 0 for that state event).

Copy link
Member Author

Choose a reason for hiding this comment

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

right, so:

Or do you mean that aliases events should be subject to PLs, but for public rooms, we should emit a PL event that lowers the required level for aliases to 0? In which case, I think this makes sense.

this?

Copy link
Member

Choose a reason for hiding this comment

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

yes, or just define the default PL for m.room.aliases to be 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, or just define the default PL for m.room.aliases to be 0.

that seems an unnecessary complication.

I've updated the MSC to say that we will lower the PL for m.room.aliases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is having default PL 0 for this really a good idea? I don't know if I am making a wild assumption here but I'm not sure how many everyday users will examine the power levels when creating a new room unless they have specific reason to.

This would therefore still leave lots of rooms owned by inexperienced users open to drive by-style attacks where someone just joins the room, adds a whole bunch of evil aliases and leaves again (possibly some hours before anyone with a suitable PL even notices).

"Defaults reveal the soul" and all that.

Perhaps allowing room admins the ability to redact malicious `aliases` events
is sufficient? Given that a malicious user could immediately publish a new
`aliases` event (even if they have been banned from the room), it seems like
that would be ineffective.
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, in that scenario we still need the ability for the room admin to gag randoms from adding remote aliases.

Copy link
Member

Choose a reason for hiding this comment

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

...or require the user to be in the room in order to set a m.room.alias, so we can stop abuse by banning them, which you'd get if we fell through to the default auth rules.

@ara4n
Copy link
Member

ara4n commented Sep 1, 2019

another use case for this for trying to identify random rooms sitting on your server; the presence of an m.room.aliases event gives you more of a clue (programmatically) as to what they might be.

@ara4n
Copy link
Member

ara4n commented Sep 2, 2019

thanks for updating it - we've converged on the same proposal, hopefully for the best.

@richvdh
Copy link
Member Author

richvdh commented Sep 11, 2019

I think it's fair to say this is no longer WIP. There's a slight question mark over we do https://github.com/matrix-org/matrix-doc/issues/2259 first, which would make it work better, but increases the scope of the project. Otherwise I think Matthew and I are aligned on this proposal now.

@richvdh richvdh changed the title [WIP] MSC2260: Update the auth rules for m.room.aliases events MSC2260: Update the auth rules for m.room.aliases events Sep 11, 2019
@richvdh
Copy link
Member Author

richvdh commented Jan 30, 2020

This MSC is now implemented in synapse develop, in the org.matrix.msc2260 room version. I invite people to try it out.

One note: I haven't done anything to deal with the "delayed alias update" problem, as per https://github.com/matrix-org/matrix-doc/blob/rav/proposal/change_aliases_auth_rules/proposals/2260-change-aliases-auth-rules.md#potential-issues (#2). So:

  • Eve, who does not have the relevant PL to update the aliases event, adds hundreds of offensive aliases to the directory.
  • Later, Bob adds a new alias, and doesn't notice all of Eve's spam. This produces a new m.room.aliases event.
  • Everybody else sees "Bob added the aliases #i-hate-puppies, #i-hate-kittens, etc".

I guess I could try the "keep track of which aliases should be in the room state" solution.

@richvdh
Copy link
Member Author

richvdh commented Feb 3, 2020

well, time to propose FCP I guess:

@mscbot fcp merge

@mscbot
Copy link
Collaborator

mscbot commented Feb 3, 2020

Team member @richvdh has proposed to merge this. The next step is review by the rest of the tagged people:

Once at least 75% of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@anoadragon453 anoadragon453 added the proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. label Feb 3, 2020
@richvdh
Copy link
Member Author

richvdh commented Feb 9, 2020

It seems that we are reconsidering this approach and will try something else instead.

@mscbot fcp cancel

@turt2live turt2live removed the proposed-final-comment-period Currently awaiting signoff of a majority of team members in order to enter the final comment period. label Feb 9, 2020
@richvdh
Copy link
Member Author

richvdh commented Feb 10, 2020

Having implemented this, we found the following:

The core assumption was that everybody would still be able to add aliases to the list in m.room.aliases. This felt like something of an open door to spam.

Whilst this MSC opens the possibility of increasing the PL required to send an m.room.aliases event, it feels like a second-class citizen which doesn't work well. As an example: Riot would allow you to add an alias, and would show it in the "local aliases" list, but as soon as you reloaded the room settings, the new one would disappear.

We've had a bit of a rethink, and have an alternative approach, which I shall write it up as a new MSC.

babolivier added a commit to matrix-org/synapse that referenced this pull request Feb 12, 2020
Synapse 1.10.0 (2020-02-12)
===========================

**WARNING to client developers**: As of this release Synapse validates `client_secret` parameters in the Client-Server API as per the spec. See [\#6766](#6766) for details.

Updates to the Docker image
---------------------------

- Update the docker images to Alpine Linux 3.11. ([\#6897](#6897))

Synapse 1.10.0rc5 (2020-02-11)
==============================

Bugfixes
--------

- Fix the filtering introduced in 1.10.0rc3 to also apply to the state blocks returned by `/sync`. ([\#6884](#6884))

Synapse 1.10.0rc4 (2020-02-11)
==============================

This release candidate was built incorrectly and is superceded by 1.10.0rc5.

Synapse 1.10.0rc3 (2020-02-10)
==============================

Features
--------

- Filter out `m.room.aliases` from the CS API to mitigate abuse while a better solution is specced. ([\#6878](#6878))

Internal Changes
----------------

- Fix continuous integration failures with old versions of `pip`, which were introduced by a release of the `zipp` library. ([\#6880](#6880))

Synapse 1.10.0rc2 (2020-02-06)
==============================

Bugfixes
--------

- Fix an issue with cross-signing where device signatures were not sent to remote servers. ([\#6844](#6844))
- Fix to the unknown remote device detection which was introduced in 1.10.rc1. ([\#6848](#6848))

Internal Changes
----------------

- Detect unexpected sender keys on remote encrypted events and resync device lists. ([\#6850](#6850))

Synapse 1.10.0rc1 (2020-01-31)
==============================

Features
--------

- Add experimental support for updated authorization rules for aliases events, from [MSC2260](matrix-org/matrix-spec-proposals#2260). ([\#6787](#6787), [\#6790](#6790), [\#6794](#6794))

Bugfixes
--------

- Warn if postgres database has a non-C locale, as that can cause issues when upgrading locales (e.g. due to upgrading OS). ([\#6734](#6734))
- Minor fixes to `PUT /_synapse/admin/v2/users` admin api. ([\#6761](#6761))
- Validate `client_secret` parameter using the regex provided by the Client-Server API, temporarily allowing `:` characters for older clients. The `:` character will be removed in a future release. ([\#6767](#6767))
- Fix persisting redaction events that have been redacted (or otherwise don't have a redacts key). ([\#6771](#6771))
- Fix outbound federation request metrics. ([\#6795](#6795))
- Fix bug where querying a remote user's device keys that weren't cached resulted in only returning a single device. ([\#6796](#6796))
- Fix race in federation sender worker that delayed sending of device updates. ([\#6799](#6799), [\#6800](#6800))
- Fix bug where Synapse didn't invalidate cache of remote users' devices when Synapse left a room. ([\#6801](#6801))
- Fix waking up other workers when remote server is detected to have come back online. ([\#6811](#6811))

Improved Documentation
----------------------

- Clarify documentation related to `user_dir` and `federation_reader` workers. ([\#6775](#6775))

Internal Changes
----------------

- Record room versions in the `rooms` table. ([\#6729](#6729), [\#6788](#6788), [\#6810](#6810))
- Propagate cache invalidates from workers to other workers. ([\#6748](#6748))
- Remove some unnecessary admin handler abstraction methods. ([\#6751](#6751))
- Add some debugging for media storage providers. ([\#6757](#6757))
- Detect unknown remote devices and mark cache as stale. ([\#6776](#6776), [\#6819](#6819))
- Attempt to resync remote users' devices when detected as stale. ([\#6786](#6786))
- Delete current state from the database when server leaves a room. ([\#6792](#6792))
- When a client asks for a remote user's device keys check if the local cache for that user has been marked as potentially stale. ([\#6797](#6797))
- Add background update to clean out left rooms from current state. ([\#6802](#6802), [\#6816](#6816))
- Refactoring work in preparation for changing the event redaction algorithm. ([\#6803](#6803), [\#6805](#6805), [\#6806](#6806), [\#6807](#6807), [\#6820](#6820))
@turt2live
Copy link
Member

@richvdh are we doubling down on #2432 instead? If so, we might want to consider just closing this one as obsolete.

@turt2live turt2live removed their request for review March 5, 2020 21:45
@richvdh richvdh added obsolete A proposal which has been overtaken by other proposals and removed proposal-in-review labels Mar 6, 2020
@richvdh richvdh closed this Mar 6, 2020
richvdh added a commit to matrix-org/synapse that referenced this pull request Mar 23, 2020
Synapse 1.12.0 (2020-03-23)
===========================

No significant changes since 1.12.0rc1.

Debian packages and Docker images are rebuilt using the latest versions of
dependency libraries, including Twisted 20.3.0. **Please see security advisory
below**.

Security advisory
-----------------

Synapse may be vulnerable to request-smuggling attacks when it is used with a
reverse-proxy. The vulnerabilties are fixed in Twisted 20.3.0, and are
described in
[CVE-2020-10108](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10108)
and
[CVE-2020-10109](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-10109).
For a good introduction to this class of request-smuggling attacks, see
https://portswigger.net/research/http-desync-attacks-request-smuggling-reborn.

We are not aware of these vulnerabilities being exploited in the wild, and
do not believe that they are exploitable with current versions of any reverse
proxies. Nevertheless, we recommend that all Synapse administrators ensure that
they have the latest versions of the Twisted library to ensure that their
installation remains secure.

* Administrators using the [`matrix.org` Docker
  image](https://hub.docker.com/r/matrixdotorg/synapse/) or the [Debian/Ubuntu
  packages from
  `matrix.org`](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#matrixorg-packages)
  should ensure that they have version 1.12.0 installed: these images include
  Twisted 20.3.0.
* Administrators who have [installed Synapse from
  source](https://github.com/matrix-org/synapse/blob/master/INSTALL.md#installing-from-source)
  should upgrade Twisted within their virtualenv by running:
  ```sh
  <path_to_virtualenv>/bin/pip install 'Twisted>=20.3.0'
  ```
* Administrators who have installed Synapse from distribution packages should
  consult the information from their distributions.

The `matrix.org` Synapse instance was not vulnerable to these vulnerabilities.

Advance notice of change to the default `git` branch for Synapse
----------------------------------------------------------------

Currently, the default `git` branch for Synapse is `master`, which tracks the
latest release.

After the release of Synapse 1.13.0, we intend to change this default to
`develop`, which is the development tip. This is more consistent with common
practice and modern `git` usage.

Although we try to keep `develop` in a stable state, there may be occasions
where regressions creep in. Developers and distributors who have scripts which
run builds using the default branch of `Synapse` should therefore consider
pinning their scripts to `master`.

Synapse 1.12.0rc1 (2020-03-19)
==============================

Features
--------

- Changes related to room alias management ([MSC2432](matrix-org/matrix-spec-proposals#2432)):
  - Publishing/removing a room from the room directory now requires the user to have a power level capable of modifying the canonical alias, instead of the room aliases. ([\#6965](#6965))
  - Validate the `alt_aliases` property of canonical alias events. ([\#6971](#6971))
  - Users with a power level sufficient to modify the canonical alias of a room can now delete room aliases. ([\#6986](#6986))
  - Implement updated authorization rules and redaction rules for aliases events, from [MSC2261](matrix-org/matrix-spec-proposals#2261) and [MSC2432](matrix-org/matrix-spec-proposals#2432). ([\#7037](#7037))
  - Stop sending m.room.aliases events during room creation and upgrade. ([\#6941](#6941))
  - Synapse no longer uses room alias events to calculate room names for push notifications. ([\#6966](#6966))
  - The room list endpoint no longer returns a list of aliases. ([\#6970](#6970))
  - Remove special handling of aliases events from [MSC2260](matrix-org/matrix-spec-proposals#2260) added in v1.10.0rc1. ([\#7034](#7034))
- Expose the `synctl`, `hash_password` and `generate_config` commands in the snapcraft package. Contributed by @devec0. ([\#6315](#6315))
- Check that server_name is correctly set before running database updates. ([\#6982](#6982))
- Break down monthly active users by `appservice_id` and emit via Prometheus. ([\#7030](#7030))
- Render a configurable and comprehensible error page if something goes wrong during the SAML2 authentication process. ([\#7058](#7058), [\#7067](#7067))
- Add an optional parameter to control whether other sessions are logged out when a user's password is modified. ([\#7085](#7085))
- Add prometheus metrics for the number of active pushers. ([\#7103](#7103), [\#7106](#7106))
- Improve performance when making HTTPS requests to sygnal, sydent, etc, by sharing the SSL context object between connections. ([\#7094](#7094))

Bugfixes
--------

- When a user's profile is updated via the admin API, also generate a displayname/avatar update for that user in each room. ([\#6572](#6572))
- Fix a couple of bugs in email configuration handling. ([\#6962](#6962))
- Fix an issue affecting worker-based deployments where replication would stop working, necessitating a full restart, after joining a large room. ([\#6967](#6967))
- Fix `duplicate key` error which was logged when rejoining a room over federation. ([\#6968](#6968))
- Prevent user from setting 'deactivated' to anything other than a bool on the v2 PUT /users Admin API. ([\#6990](#6990))
- Fix py35-old CI by using native tox package. ([\#7018](#7018))
- Fix a bug causing `org.matrix.dummy_event` to be included in responses from `/sync`. ([\#7035](#7035))
- Fix a bug that renders UTF-8 text files incorrectly when loaded from media. Contributed by @TheStranjer. ([\#7044](#7044))
- Fix a bug that would cause Synapse to respond with an error about event visibility if a client tried to request the state of a room at a given token. ([\#7066](#7066))
- Repair a data-corruption issue which was introduced in Synapse 1.10, and fixed in Synapse 1.11, and which could cause `/sync` to return with 404 errors about missing events and unknown rooms. ([\#7070](#7070))
- Fix a bug causing account validity renewal emails to be sent even if the feature is turned off in some cases. ([\#7074](#7074))

Improved Documentation
----------------------

- Updated CentOS8 install instructions. Contributed by Richard Kellner. ([\#6925](#6925))
- Fix `POSTGRES_INITDB_ARGS` in the `contrib/docker/docker-compose.yml` example docker-compose configuration. ([\#6984](#6984))
- Change date in [INSTALL.md](./INSTALL.md#tls-certificates) for last date of getting TLS certificates to November 2019. ([\#7015](#7015))
- Document that the fallback auth endpoints must be routed to the same worker node as the register endpoints. ([\#7048](#7048))

Deprecations and Removals
-------------------------

- Remove the unused query_auth federation endpoint per [MSC2451](matrix-org/matrix-spec-proposals#2451). ([\#7026](#7026))

Internal Changes
----------------

- Add type hints to `logging/context.py`. ([\#6309](#6309))
- Add some clarifications to `README.md` in the database schema directory. ([\#6615](#6615))
- Refactoring work in preparation for changing the event redaction algorithm. ([\#6874](#6874), [\#6875](#6875), [\#6983](#6983), [\#7003](#7003))
- Improve performance of v2 state resolution for large rooms. ([\#6952](#6952), [\#7095](#7095))
- Reduce time spent doing GC, by freezing objects on startup. ([\#6953](#6953))
- Minor perfermance fixes to `get_auth_chain_ids`. ([\#6954](#6954))
- Don't record remote cross-signing keys in the `devices` table. ([\#6956](#6956))
- Use flake8-comprehensions to enforce good hygiene of list/set/dict comprehensions. ([\#6957](#6957))
- Merge worker apps together. ([\#6964](#6964), [\#7002](#7002), [\#7055](#7055), [\#7104](#7104))
- Remove redundant `store_room` call from `FederationHandler._process_received_pdu`. ([\#6979](#6979))
- Update warning for incorrect database collation/ctype to include link to documentation. ([\#6985](#6985))
- Add some type annotations to the database storage classes. ([\#6987](#6987))
- Port `synapse.handlers.presence` to async/await. ([\#6991](#6991), [\#7019](#7019))
- Add some type annotations to the federation base & client classes. ([\#6995](#6995))
- Port `synapse.rest.keys` to async/await. ([\#7020](#7020))
- Add a type check to `is_verified` when processing room keys. ([\#7045](#7045))
- Add type annotations and comments to the auth handler. ([\#7063](#7063))
@turt2live turt2live added the kind:core MSC which is critical to the protocol's success label Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:core MSC which is critical to the protocol's success obsolete A proposal which has been overtaken by other proposals proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants