This fixes a vulnerability where an attacker with a a malicious remote
server and a user on the local server can trick the local server into
signing arbitrary events. The attacker issue a remote leave as the local
user to a room on the malicious server. Without any validation of the
make_leave response, the local server would sign the attacker-controlled
event and pass it back to the malicious server with send_leave.
The join endpoints is also fixed in this commit, but is less useful for
exploitation because the local server replaces the "content" field
returned by the remote server. Remote invites are unaffected because we
already check that the event returned from /invite has the same event ID
as the event passed to it.
Both of these endpoints sign the received event so without the
validation a malicious server can use these endpoints to trick our
server into signing effectively arbitrary forged events from local
users.
Rebased from a continuwuity patch by nex. The create_join changes were
not present in the continuwuity version because these checks were
already present there.
Co-authored-by: Olivia Lee <olivia@computer.surgery>
<https://github.com/ruma/ruma/pull/2050>
We could just set None now, but the ruma changelog states that
> Servers are encouraged to keep sending it for compatibility with
> clients that required it.
The only change to the internal interface for this one is removing the
current_third_party_invite argument from state_res::auth_check. Ruma now
fetches the event using the fetch_state closure. This is convenient for
us, because we previously didn't bother to implement it.
The significant change is 26edd40a704040e7104161da81c9bae91b7ddcaa,
which removes the global compat feature, so that each compat feature
must now be enabled individually. We're using the slightly later
1387667d because it has a bugfix that ruma needs to compile.
There are a few ruma compat features that were not previously part of
the global compat feature:
- compat-arbitrary-length-ids
- compat-upload-signature
- compat-encrypted-stickers
I have not enabled these here, to avoid a behavior change.
Servers are required to preserve unknown properties in event content,
since they may be added by a future version of the spec. Round-tripping
through RoomRedactionEventContent results in dropping all unknown
properties.
Previously we were mashing everything together as RoomAccountDataEvent,
even the global events. This technically worked, because of the hidden
custom fields on the ruma event types, but it's confusing and easy to
mess up. Separate methods with appropriate types are preferable.
In preparation for adding some additional methods at the service level.
Dropping the tracing spans for the data methods, because two duplicate
spans here seems kinda excessive.
Previously we were only using trust-dns for resolving SRV records in
server discovery, and then for resolving the hostname from the SRV
record target if one exists. With the previous behavior, admins need to
ensure that both their system resolver and trust-dns are working
correctly in order for outgoing traffic to work reliably. This can be
confusing to debug, because it's not obvious to the admin if or when
each resolver are being used. Now, everything goes through trust-dns and
outgoing federation DNS should fail/succeed more predictably.
I also expect some performance improvement from having an in-process DNS
cache, but haven't taken measurements yet.
This used to be supported, as we explicitly call std::fs::create_dir_all
when initializing these, but it was broken in
b01b70fc20, which attempts to canonicalize
the paths to check for overlap before creating them.
Previously, read receipts would only be forwarded via federation
incidentally when some PDU was later sent to the destination server.
Trigger a send without any event to collect EDUs and get read receipts
out directly.
For example, if the database path is `/foo` and the media path is
`/foo/bar`, but `/foo/bar` is a symlink or hardlink to `/baz`, the
previous check would pass. The whole point of this check is to ensure
that the database and media data can't step on each other, so this check
is needed to deny that kind of situation as well.
It would probably be good to add a test for this behavior.
