Can be reviewed commit-by-commit.
This fixes another silly thing you can do with securejoinv3: show Bob a
QR code with auth token that is a broadcast channel secret of a known
channel, then never respond. Bob will decrypt messages from the channel
and drop them because they are sent by the "wrong" sender.
This can be avoided with domain separation, instead of
encrypting/decrypting securejoinv3 messages directly with auth token,
encrypt/decrypt them with `securejoin/<auth token>` as the secret or
even `securejoinv3/<alice's fingerprint>/<auth token>`. For existing
broadcast channels we cannot do this, but for securejoinv3 that is not
released yet this looks like an improvement that avoids at least this
problem.
Credits to link2xt for noticing the problem.
This also adds Alice's fingerprint to the auth tokens, which
was pretty easy to do. I find it hard to develop an intuition for
whether this is important, or whether we will be annoyed by it in the
future.
**Note:** This means that QR code scans will not work if one of the chat
partners uses a self-compiled core between c724e2981 and merging this PR
here. This is fine; we will just have to tell the other developers to
update their self-compiled cores.
The tests were originally generated with AI and then reworked.
Follow-up to https://github.com/chatmail/core/pull/7754 (c724e29)
This prevents the following attack:
/// Eve is subscribed to a channel and wants to know whether Alice is also subscribed to it.
/// To achieve this, Eve sends a message to Alice
/// encrypted with the symmetric secret of this broadcast channel.
///
/// If Alice sends an answer (or read receipt),
/// then Eve knows that Alice is in the broadcast channel.
///
/// A similar attack would be possible with auth tokens
/// that are also used to symmetrically encrypt messages.
///
/// To prevent this, a message that was unexpectedly
/// encrypted with a symmetric secret must be dropped.
encryption info needs a dedicated string for "Messages are end-to-end encrypted"
as the UI will add more infomation to the info messages,
smth. as "Tap for more information".
an alternative fix would have been to let the UI render the info-message
differently, but adding another string to core causes less friction.
If one of broadcast owner's devices didn't add a new subscriber for any reason, e.g. because of
missing SecureJoin messages, this device shall add "member added" messages when syncing the member
list from the `SetPgpContacts` message.
The test sometimes fails because of wrong message ordering for bob:
[...]
Waiting for the device of alice@example.org to reply… [NOTICED][INFO]
<Msg#2010🔒: (Contact#Contact#2001): hi [FRESH]
Msg#2008🔒: (Contact#Contact#2001): You joined the channel. [FRESH][INFO]
>Msg#2010🔒: (Contact#Contact#2001): hi [FRESH]
Msg#2011🔒: (Contact#Contact#2001): Member Me removed by alice@example.org. [FRESH][INFO]
This adds `SystemTime::shift(Duration::from_secs(1))` as a workaround.
Co-authored-by: Hocuri <hocuri@gmx.de>
dc_jsonrpc_init called Arc::from_raw on the account_manager pointer, which took ownership of the caller's refcount. When the local Arc dropped at the end of the function, the refcount was decremented, leaving the C side's pointer with a stolen refcount. This caused a use-after-free race between dc_accounts_unref and dc_jsonrpc_unref at shutdown.
Wrap in ManuallyDrop to prevent the implicit drop, keeping the caller's refcount intact.
Regression introduced in #7662.
This change is mainly to avoid exposing the write lock outside the pool module.
To avoid deadlocks, outside code should work only with the pooled connections
and use no more than one connection per thread.
With multiple transports there are multiple inbox loops on the same profile `Context`.
They tend to start running housekeeping at the same time, e.g. when deleting
a message with an attachment, and then `remove_unused_files()`
tries to remove the same files that are already deleted by another thread
and logs errors.
Don't use first-person form in placeholder texts,
as these can be misleading when broadcasted to group.
Additionally ensures that broadcasted system messages
are not localized to not leak locally-set language
to the group chat.
Fixes#7930
Signed-off-by: Jagoda Ślązak <jslazak@jslazak.com>
Don't depend on these 3 cleartext headers for the question whether we
download a message.
This PR will waste a bit of bandwidth for people who use the legacy
show_emails option; apart from that, there is no user-visible change
yet. It's a preparation for being able to remove these headers, in order
to further reduce unencrypted metadata.
Removing In-Reply-To and References will be easy; removing Chat-Version
must happen at least one release after the PR here is released, so that
people don't miss messages. Also, maybe some nerds depend on the
Chat-Version header for server-side filtering of messages, but we shall
have this discussion at some other time.
For the question whether a message should be moved, we do still depend
on them; this will be fixed with
https://github.com/chatmail/core/pull/7780.
When both this PR and #7780 are merged, we can stop requesting
Chat-Version header during prefetch.
I assume that the problem was that sometimes, alice2 or fiona doesn't
accept alice's smeared timestamp, because `calc_sort_timestamp()`
doesn't allow the timestamp of a received message to be in the future. I
tried this patch:
```diff
diff --cc src/chat.rs
index 9565437cf,9565437cf..a2e4f97d0
--- a/src/chat.rs
+++ b/src/chat.rs
@@@ -46,6 -46,6 +46,7 @@@ use crate::receive_imf::ReceivedMsg
use crate::smtp::{self, send_msg_to_smtp};
use crate::stock_str;
use crate::sync::{self, Sync::*, SyncData};
++use crate::timesmearing::MAX_SECONDS_TO_LEND_FROM_FUTURE;
use crate::tools::{
IsNoneOrEmpty, SystemTime, buf_compress, create_broadcast_secret, create_id,
create_outgoing_rfc724_mid, create_smeared_timestamp, create_smeared_timestamps, get_abs_path,
@@@ -1212,7 -1212,7 +1213,11 @@@ SELECT id, rfc724_mid, pre_rfc724_mid,
received: bool,
incoming: bool,
) -> Result<i64> {
-- let mut sort_timestamp = cmp::min(message_timestamp, smeared_time(context));
++ let mut sort_timestamp = cmp::min(
++ message_timestamp,
++ // Add MAX_SECONDS_TO_LEND_FROM_FUTURE in order to allow other senders to do timesmearing, too:
++ smeared_time(context) + MAX_SECONDS_TO_LEND_FROM_FUTURE,
++ );
let last_msg_time: Option<i64> = if always_sort_to_bottom {
// get newest message for this chat
```
...maybe this patch makes sense anyways, but you still get the problem
that the message sent by alice2 (i.e. the add-fiona message) will have
an earlier timestamp than the message sent by alice, because alice
already sent more messages, and therefore has more timesmearing-seconds.
It's unsure it makes sense to modify calc_sort_timestamp() this way because if some chat member has the clock in the future (even unintentionally), their fresh messages will be sorted to the bottom relatively to others' fresh messages. Maybe it's even better to limit the message timestamp ("Date") by the current system time there.
To really fix the problem, we could send a serial number together with the timestamp, that distinguishes two messages sent in the same second. But since we haven't gotten complaints about message ordering since some time, let's just leave things as they are.
Since all this timesmearing is a bit best-effort right now, I decided to
instead just make the test more relaxed.
fix https://github.com/chatmail/core/issues/7880
depends on #7754 (merged)
With this change, a securejoin message is just ignored if the contact
was deleted in the meantime; apparently the user is not interested in
the securejoin process anymore if they deleted the contact.
But other, parallel securejoin processes must not be affected; the test
also tests this.
This was initially done in the IMAP loop
to set is_chatmail for existing users.
They should all have the setting configured
by now unless they install some very old backup.
Setting during the configuration is needed
for Delta Chat Desktop because it caches the value
internally:
<https://github.com/deltachat/deltachat-desktop/issues/6068>
Close https://github.com/chatmail/core/issues/7396. Before reviewing,
you should read the issue description of
https://github.com/chatmail/core/issues/7396.
I recommend to review with hidden whitespace changes.
TODO:
- [x] Implement the new protocol
- [x] Make Rust tests pass
- [x] Make Python tests pass
- [x] Test it manually on a phone
- [x] Print the sent messages, and check that they look how they should:
[test_secure_join_group_with_mime_printed.txt](https://github.com/user-attachments/files/24800556/test_secure_join_group.txt)
- [x] Fix bug: If Alice has a second device, then Bob's chat won't be
shown yet on that second device. Also, Bob's contact isn't shown in her
contact list. As soon as either party writes something into the chat,
the that shows up and everything is fine. All of this is still a way
better UX than in WhatsApp, where Bob always has to write first 😂
Still, I should fix that.
- This is actually caused by a larger bug: AUTH tokens aren't synced if
there is no corresponding INVITE token.
- Fixed by 6b658a0e0
- [x] Either make a new `auth_tokens` table with a proper UNIQUE bound,
or put a UNIQUE bound on the `tokens` table
- [x] Benchmarking
- [x] TODOs in the code, maybe change naming of the new functions
- [x] Write test for interop with older DC (esp. that the original
securejoin runs if you remove the &v=3 param)
- [x] From a cryptography perspective, is it fine that vc-request is
encrypted with AUTH, rather than a separate secret (like INVITE)?
- [x] Make sure that QR codes without INVITE work, so that we can remove
it eventually
- [x] Self-review, and comment on some of my code changes to explain
what they do
- [x] ~~Maybe use a new table rather than reusing AUTH token.~~ See
https://github.com/chatmail/core/pull/7754#discussion_r2728544725
- [ ] Update documentation; I'll do that in a separate PR. All necessary
information is in the https://github.com/chatmail/core/issues/7396 issue
description
- [ ] Update tests and other code to use the new names (e.g.
`request-pubkey` rather than `request` and `pubkey` rather than
`auth-required`); I'll do that in a follow-up PR
**Backwards compatibility:**
Everything works seamlessly in my tests. If both devices are updated,
then the new protocol is used; otherwise, the old protocol is used. If
there is a not-yet-updated second device, it will correctly observe the
protocol, and mark the chat partner as verified.
Note that I removed the `Auto-Submitted: auto-replied` header from
securejoin messages. We don't need it ourselves, it's a cleartext header
that leaks too much information, and I can't see any reason to have it.
---------
Co-authored-by: iequidoo <117991069+iequidoo@users.noreply.github.com>
