This fixes a vulnerability where an attacker with 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 and knock endpoints are also fixed in this commit, but are 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.
Co-authored-by: timedout <git@nexy7574.co.uk>
Co-authored-by: Jade Ellis <jade@ellis.link>
Co-authored-by: Ginger <ginger@gingershaped.computer>
This ensures that the tokenization algorithm will remain in sync between
querying, indexing, and deindexing. The existing code had slightly
different behavior for querying, because it did not discard words with
>50 bytes. This was inconsequential, because >50 byte tokens are never
present in the index.
Signed-off-by: strawberry <strawberry@puppygock.gay>
The previous code would drop some events entirely if any events between
`skip` and `skip + limit` were not visible to the user. This would cause
the set of events skipped by the `skip(skip)` method to extend past
`skip` in the raw result set, because `skip(skip)` was being called
*after* filtering out invisible events.
This bug will become much more severe with a full filtering
implementation, because it will be more likely for events to be filtered
out. Currently, it is only possible to trigger with rooms that have
history visibility set to "invited" or "joined".
Signed-off-by: strawberry <strawberry@puppygock.gay>
The previous code would fail to return next_batch if any of the events
in the window were not visible to the user. It would also return an
unnecessary next_batch when no more results are available if the total
number of results is exactly `skip + limit`.
This bug will become much more severe with a full filtering
implementation, because we will be more likely to trigger it by
filtering out events in a search call. Currently, it is only possible to
trigger with rooms that have history visibility set to "invited" or
"joined".
Signed-off-by: strawberry <strawberry@puppygock.gay>
The original implementation of this was really weird, so I restructed it
a lot while debugging, and am just gonna leave the restructured version.
Root cause of the segfault seems to be that upstream nixpkgs liburing
derivation is generating both static and dynamic libraries, causing
rocksdb to statically link liburing in a dynamic build, pulling in some
allocator stuff at the same time. I created a PR[1] to fix this upstream,
but it probably won't be available on nixos-unstable for quite a while,
so we can also patch it locally.
[1]: https://github.com/NixOS/nixpkgs/pull/314945
I talked to somebody yesterday in #conduwuit:puppygock.gay that was using
this output in their system config. The dynamically-linked jemalloc build
is quite fragile, and is not tested by anything else in CI. We want to
make sure we don't break it again in the future.
This failed to inherit the fix from bec507d739
because the crane package's buildInputs become propagatedBuildInputs in
a static stdenv, but become normal buildInputs in a dynamic stdenv. Since
we were only pulling propagatedBuildInputs into the devshell, dynamically
linked devshells did not include the rust-jemalloc-sys package. This
causes tikv-jemalloc-sys to build it's own static jemalloc package, and
we end up loading libc before jemalloc at runtime.
It turns out that this was actually fixed by
bec507d739 and
857ac42aac, but we didn't identify it at the
time. Notably, the `dynamic` devshell is still broken.
Previously we were relying on NIX_CFLAGS_COMPILE, but this is not being
set in static devshells. A cleaner solution for complement would likely
be to build the tests in their own nix derivation instead of building
them in the devshell, but this change unblocks CI for now.
This is causing build failures on Mac:
> In file included from /tmp/nix-build-rocksdb-static-aarch64-apple-darwin-9.1.1.drv-0/source/memory/memory_allocator.cc:8:
> In file included from /tmp/nix-build-rocksdb-static-aarch64-apple-darwin-9.1.1.drv-0/source/memory/jemalloc_nodump_allocator.h:11:
> /tmp/nix-build-rocksdb-static-aarch64-apple-darwin-9.1.1.drv-0/source/port/jemalloc_helper.h:63:36: warning: unknown attribute '_rjem_malloc' ignored [-Wunknown-attributes]
> mallocx(size_t, int) JEMALLOC_ATTR(malloc) JEMALLOC_ALLOC_SIZE(1)
> ^~~~~~
> /nix/store/3bix0kzy670dyhhizri3dwb1qfj3sdpa-jemalloc-static-aarch64-apple-darwin-5.3.0/include/jemalloc/jemalloc.h:412:18: note: expanded from macro 'malloc'
> # define malloc je_malloc
> ^~~~~~~~~
> /nix/store/3bix0kzy670dyhhizri3dwb1qfj3sdpa-jemalloc-static-aarch64-apple-darwin-5.3.0/include/jemalloc/jemalloc.h:75:21: note: expanded from macro 'je_malloc'
> # define je_malloc _rjem_malloc
> ^~~~~~~~~~~~
> /nix/store/3bix0kzy670dyhhizri3dwb1qfj3sdpa-jemalloc-static-aarch64-apple-darwin-5.3.0/include/jemalloc/jemalloc.h:183:43: note: expanded from macro 'JEMALLOC_ATTR'
> # define JEMALLOC_ATTR(s) __attribute__((s))
Full build log at <https://girlboss.ceo/~strawberry/pb/ygJ3>. This is
likely fixable with patches to rocksdb, but not worth it since darwin is
only a dev platform.
CI is running `cargo build --all-features`, so we should be passing all
the features to nix as well.
The only thing this currently affects is the jemalloc_prof feature, but if
we add any non-default features that affect nix in the future they should
also be handled correctly now.
Dynamically-linked jemalloc doesn't work due to link-order issues, and we
want CI to be testing a static binary anyway since that's what we're
publishing in releases.
Some of the features affect nix dependencies, so we need to have a
full feature list available when constructing the nix derivation. This
incidentally fixes the bug where we weren't enabling jemalloc on rocksdb
in CI/devshells, because jemalloc is now a default feature. It does not
fix the more general class of that issue, where CI is performing an
`--all-features` build in a nix devshell built for default-features.
I am now passing `--no-default-features` to cargo, and having it use our
unified feature list rather than duplicating the unification inside cargo.
Without setting JEMALLOC_OVERRIDE, we end up linking to two different
jemalloc builds. Once dynamically, as a transitive dependency through
rocksdb, and a second time to the static jemalloc that tikv-jemalloc-sys
builds.
Previously, we were returning redundant member count updates or encrypted
device updates from the /sync endpoint in some cases. The extra member
count updates are spec-compliant, but unnecessary, while the extra
encrypted device updates violate the spec.
The refactor necessary to fix this bug is also necessary to support
filtering on state events in sync.
Details:
Joined room incremental sync needs to examine state events for four
purposes:
1. determining whether we need to return an update to room member counts
2. determining the set of left/joined devices for encrypted rooms
(returned in `device_lists`)
3. returning state events to the client (in `rooms.joined.*.state`)
4. tracking which member events we have sent to the client, so they can
be omitted on future requests when lazy-loading is enabled.
The state events that we need to examine for the first two cases is member
events in the delta between `since` and the end of `timeline`. For the
second two cases, we need the delta between `since` and the start of
`timeline`, plus contextual member events for any senders that occur in
`timeline`. The second list is subject to filtering, while the first is
not.
Before this change, we were using the same set of state events that we are
returning to the client (cases 3/4) to do the analysis for cases 1/2.
In a compliant implementation, this would result in us missing some
relevant member events in 1/2 in addition to seeing redundant member
events. In current conduwuit this is not the case because the set of
events that we return to the client is always a superset of the set that
is needed for cases 1/2. This is because we don't support filtering, and
we have an existing bug[1] where we are returning the delta between
`since` and the end of `timeline` rather than the start.
[1]: https://github.com/girlbossceo/conduwuit/issues/361
Fixing this is necessary to implement filtering because otherwise
we would start missing some member events for member count or encrypted
device updates if the relevant member events are rejected by the filter.
This would be much worse than our current behavior.
This cache can serve invalid responses, and has an extremely low hit
rate.
It serves invalid responses because because it's only keyed off
the `since` parameter, but many of the other request parameters also
affect the response or it's side effects. This will become worse once we
implement filtering, because there will be a wider space of parameters
with different responses. This problem is fixable, but not worth it
because of the low hit rate.
The low hit rate is because normal clients will always issue the next
sync request with `since` set to the `prev_batch` value of the previous
response. The only time we expect to see multiple requests with the same
`since` is when the response is empty, but we don't cache empty
responses.
This was confirmed experimentally by logging cache hits and misses over
15 minutes with a wide variety of clients. This test was run on
matrix.computer.surgery, which has only a few active users, but a
large volume of sync traffic from many rooms. Over the test period, we
had 3 hits and 5309 misses. All hits occurred in the first minute, so I
suspect that they had something to do with client recovery from an
offline state. The clients that were connected during the test are:
- element web
- schildichat web
- iamb
- gomuks
- nheko
- fractal
- fluffychat web
- fluffychat android
- cinny web
- element android
- element X android
Fixes: #336
Hardcoding the output path to something in CWD is a pain if you're running
conduwuit through systemd or similar. Also made the error message when
it's unable to create the output file a little more friendly.
Previously, enabling the `tracing_flame` or `allow_jaeger` options would
prevent any logs from being written to stdout. In addition, enabling the
`allow_jaeger` option would inhibit the `tracing_flame` option.
Now that we have a way to use separate tracing filters with different
layers, we can enable all three at the same time without issues.
This commit also prevents the `debug log_level` command from modifying
the `tracing-flame` filter. This was supported previously, but I don't
think it's something that you would ever want to do intentionally. Now
that we have both the normal log filter and the `tracing-flame` filter
enabled at the same time, we want to `debug log_level` to only modify the
normal filter.
This turned out to be quite hairy, mostly because we need to apply the
config's log level filter to the actual logs (stdout and, optionally
sentry), but do not want to filter out the tokio tracing events needed by
the console_subscriber. I hit several edge cases in tracing getting
this to work, and we now depend on a git version of tracing with a
backported patch :(
Benjamin Lee
