Proposal for ACLing servers from rooms

[ara4n]

Documentation: https://docs.google.com/document/d/1aiuROf1__7ZFkJvDdAZQfBNxyzjYd-ijiRAcHJYqJCM/edit#heading=h.t1ebd56v2ae6
Author: @richvdh, @ara4n

This proposal is treated as a vulnerability mitigation, and as such was written and reviewed by the core spec team and is now being made public in sync with a fix being made available in Synapse 0.32.0rc1 and the forthcoming urgent Synapse 0.32.0 upgrade.

[ara4n]

Responding to the comment over at #1308 (comment) from @maxidor:

Disclaimer: Also posting this on behalf of other community members

Who? To be clear: anyone in the community is very very welcome to voice their concerns, and it will make it a lot easier for feedback this to be taken seriously if it's not anonymous, or all being filtered through you (especially given the conflict of interest given your own fork, etc).

So today this happened: #1383

• Related code has been made available to prod systems before the issue was even created [1] [[2]] (https://matrix.to/#/!qBFNwucQebGPQldAnq:matrix.org/$153080098462388OadfZ:matrix.org)

This was a race of a few hours; I'm not sure why it matters?

• Reviewed by the whole spec team
• Skipped involving relevant projects in the ecosystem, making the actual review rate at best 50% (synapse, dendrite, construct, mxhsd) and at worst 33% (synapse, dendrite, construct, mxhsd, ruma, plasma)

So last time I checked mxhsd "isn't a thing in Matrix any more", and construct is known to actively attack the federation. It boils down to a question of trust; if Ruma/Plasma/Transform etc had any federation code and their authors weren't forking or attacking the protocol we would have discussed this with them.

• Skipped community review overall, possibly sending the message the community is not important

...or that we are trying to protect the community from attack whilst we solve the remaining S2S flaws.

• Straight to passed review (!)

...because it was reviewed under embargo by the core team.

• And yet, doesn't actually achieve anything as naively bypassed or state reseted in various ways.

As far as we're aware it can only be bypassed or state reseted maliciously by a colluding or noncompliant server (which would then be also banned, in the event of having to use this). If you've found an actual flaw in the proposal please spell it out clearly before we ship the release rather than bragging about undisclosed vulns.

• Took the community 30 sec to see how flawed it was and, when down to protecting against actual attacks, useless. [1] [2] [3]

Just because 3 people say "i don't understand how this helps much" or "i don't think this should have been handled as a security fix" doesn't make it true. If you'd like to reason it through please feel free.

And the best part of it all might be those quotes from the doc for a security fix, which is... interesting:

... room admins will need to keep the server_acl event updated, and block any servers not updated to handle it.

This is meant as a quick and temporary solution until there is a new version of the room protocol that natively supports server bans.

I don't follow your point here. This is a security feature which is needed to protect the network whilst we finish off fixing state resets (which is progressing well, fwiw). It feels pretty obvious to me that we would develop and ship this under embargo in order to get it out asap to protect the network.

If we had to pick one proposal that could have really used the review process, it was this one.

It's not too late; if you can spell out the bugs in this one (and better yet suggest an alternative) then please feel free.

[ara4n]

Just responding to the three points you mentioned with permalinks:

you can bypass it, you can state reset it, and it hasn't been debated... just urgh

• You can only bypass it by relaying traffic by another malicious server (or one which doesn't uphold server_acls). So yes, this solution assumes that such servers would have to also be banned to allow the ACL to stick.
• You can't reset it because to perform a reset a malicious server has to send sculpted events into the room, which would be grounds to then ban that server. If a non-malicious state reset happened to reset it, then (until state resets are fixed), you'd just have to put it back again.
• It has been extensively debated by the core spec team.

this patch ... DOESN'T ACTUALLY DO ANYTHING
It's trivial to get servers to pull an event if i'm blocked from pushing them

• Yes, if a server sends a message which references a 'bad event' from a banned server, then bad events can leak into the DAG (as the proposal makes clear in the 'Potential Problems' section). However, the only way such a server would have got that event in the first place is if a non-compliant server let it into the DAG. So for ACLs to work, all servers in the room have to honour them.

Either that or I'm too stupid to understand the attack here, in which case please do explain it clearly.

I can see the use for per-room federation whitelists, but other than that I have to agree with Jason and Max: this wasn't really a security fix or otherwise worthy of skipping the process and it doesn't even help much

@maunium: you're right that it's probably best described as a security feature rather than a security fix. It was honestly a borderline decision on whether to design & implement this under embargo, but given we want to be able to deploy it as rapidly as possible in the event of abuse, we wanted to make sure we had something ready to go rather than having multiple days where such a feature is on the horizon but not actually deployable. In terms of whether "it doesn't even help much", do the explanations above make sense? Or are we missing something, in which case please let us know before we ship this.

[richvdh]

this has been in production for a couple of weeks, so I guess it's now pr-missing

[turt2live]

Something that came up again is how are servers meant to tell other servers that they are no longer ACL'd? As far as I can tell, if you ACL a server it'll ignore the updated ACL event that unbans itself.