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

join_authorised_via_users_server is not protected from redactions #3373

Closed
tulir opened this issue Sep 1, 2021 · 10 comments · Fixed by #3375
Closed

join_authorised_via_users_server is not protected from redactions #3373

tulir opened this issue Sep 1, 2021 · 10 comments · Fixed by #3375
Assignees
Labels
room-spec Something to do with the room version specifications wart A point where the protocol is inconsistent or inelegant

Comments

@tulir
Copy link
Member

tulir commented Sep 1, 2021

Room v8 added join_authorised_via_users_server to m.room.member events to help with authorizing restricted room join events. However, the field isn't protected from redactions, which means the join event will suddenly become invalid (i.e. not pass event auth rules) if it's redacted. At least with synapse, existing servers will still see the member in the room, but any new servers will reject the event and any messages that the user sends.

@tulir
Copy link
Member Author

tulir commented Sep 1, 2021

It appears that sending a new join event might partially "fix" it: servers that joined after a new member event was sent (which is not redacted, but doesn't have a join_authorised_via_users_server field) seem to reject the first one, but accept the second one (maybe because it's a join -> join transition).

note: I haven't looked into what actually happened inside synapse, so "reject" means "user can't see event"

@clokep
Copy link
Contributor

clokep commented Sep 1, 2021

It appears that sending a new join event might partially "fix" it: servers that joined after a new member event was sent (which is not redacted, but doesn't have a join_authorised_via_users_server field) seem to reject the first one, but accept the second one (maybe because it's a join -> join transition).

Only an initial join needs that field, subsequent joins do not need to include it (since join -> join is an allowed transition, always).

@tulir
Copy link
Member Author

tulir commented Sep 1, 2021

Well yes, but the initial join was not valid anymore when the new member event was sent. New servers accept the last join event even though they reject the earlier redacted one.

@turt2live
Copy link
Member

This feels like a small enough oversight where we can patch it into the v8 spec when that gets written. Some rooms might be affected, but considering it's not even in the spec yet I'm not too worried about them (the fix also seems to be to leave and get rejoined).

@turt2live turt2live added room-spec Something to do with the room version specifications spec-omission implemented but not currently specified labels Sep 1, 2021
@uhoreg
Copy link
Member

uhoreg commented Sep 1, 2021

I don't think we can just patch it in because event IDs are based on the redaction algorithm, so if we change the redaction algorithm, servers will disagree about the event IDs for those events.

@turt2live
Copy link
Member

oh, good point.

@turt2live turt2live added wart A point where the protocol is inconsistent or inelegant and removed spec-omission implemented but not currently specified labels Sep 1, 2021
@clokep
Copy link
Contributor

clokep commented Sep 2, 2021

I did some testing with this:

  1. Create a public space and a v8 room on server A as alice.
  2. Update the join rules of the room to restricted to members of the space.
  3. Join the space as bob.
  4. Join the room as bob (via being a member of the space).
  5. Redact bob's join event.
  6. Alice and bob should still be able to send messages, etc.
  7. Join the space as charlie on server B.
  8. Join the room as charlie.
  9. Note that charlie only sees alice and charlie in the room and rejects all new messages from bob.
  10. Alice and bob see three members and all messages sent.

So this effectively split brains the room. Going through the above with a patch to avoid redacting the join_authorised_via_users_server field everything works as expected.

Unfortunately I think the only recourse here is to abandon the v8 room version and fix it in another one, unless someone has a more clever solution!

@richvdh
Copy link
Member

richvdh commented Sep 2, 2021

for links, join_authorised_via_users_server was added to the auth rules in MSC3083. This should have been caught there, but clearly wasn't.

@clokep clokep self-assigned this Sep 2, 2021
@clokep
Copy link
Contributor

clokep commented Sep 2, 2021

See #3375 for a fix to this. 😢

@turt2live
Copy link
Member

For links: v8 was introduced by #3289

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
room-spec Something to do with the room version specifications wart A point where the protocol is inconsistent or inelegant
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants