fix: Don't decrease member add/remove timestamps if they aren't far away in the future

We shouldn't decrease `add_timestamp` and `remove_timestamp` in the `chats_contacts` table normally,
even if remote changes arrive reordered. This particularly makes sense for ad-hoc groups (see
`chat::update_chat_contacts_table()` in `apply_group_changes()`) and in case if we join an encrypted
group which we were a member of before (see `chat::add_to_chat_contacts_table()` call).

Still, limit already stored timestamps in case local clock was in the future and is set back
now. But our clock may be slow, so limit stored timestamps with a remote timestamp if it's
bigger.

NB: `receive_imf::update_chats_contacts_timestamps()` already only increases timestamps, but it's
used only for handling of the "Chat-Group-Member-Timestamps" header, i.e. for encrypted groups.

.github/workflows/ci.yml

@@ -62,7 +62,7 @@ jobs:
         with:
           show-progress: false
           persist-credentials: false
-      - uses: EmbarkStudios/cargo-deny-action@6c8f9facfa5047ec02d8485b6bf52b587b7777d1
+      - uses: EmbarkStudios/cargo-deny-action@a531616d8ce3b9177443e48a1159bc945a099823
         with:
           arguments: --workspace --all-features --locked
           command: check
@@ -146,7 +146,7 @@ jobs:
           cache-bin: false
 
       - name: Install nextest
-        uses: taiki-e/install-action@213ccc1a076163c093f914550b94feb90fab916d
+        uses: taiki-e/install-action@60ae4ce63c7aeb6e96d7f572c1ec7fafbb17ca80
         with:
           tool: nextest
 

Cargo.lock

@@ -391,6 +391,28 @@ version = "1.4.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "ace50bade8e6234aa140d9a2f552bbee1db4d353f69b8217bc503490fc1a9f26"
 
+[[package]]
+name = "aws-lc-rs"
+version = "1.17.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5ec2f1fc3ec205783a5da9a7e6c1509cc69dedf09a1949e412c1e18469326d00"
+dependencies = [
+ "aws-lc-sys",
+ "zeroize",
+]
+
+[[package]]
+name = "aws-lc-sys"
+version = "0.41.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "1a2f9779ce85b93ab6170dd940ad0169b5766ff848247aff13bb788b832fe3f4"
+dependencies = [
+ "cc",
+ "cmake",
+ "dunce",
+ "fs_extra",
+]
+
 [[package]]
 name = "backon"
 version = "1.5.0"
@@ -763,10 +785,13 @@ dependencies = [
 
 [[package]]
 name = "cc"
-version = "1.2.14"
+version = "1.2.63"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0c3d1b2e905a3a7b00a6141adb0e4c0bb941d11caf55349d863942a1cc44e3c9"
+checksum = "556e016178bb5662a08681bbe0f00f8e17631781a4dfc8c45e466e4b185ec27f"
 dependencies = [
+ "find-msvc-tools",
+ "jobserver",
+ "libc",
  "shlex",
 ]
 
@@ -920,6 +945,15 @@ dependencies = [
  "digest",
 ]
 
+[[package]]
+name = "cmake"
+version = "0.1.58"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "c0f78a02292a74a88ac736019ab962ece0bc380e3f977bf72e376c5d78ff0678"
+dependencies = [
+ "cc",
+]
+
 [[package]]
 name = "cobs"
 version = "0.2.3"
@@ -1676,7 +1710,7 @@ dependencies = [
  "libc",
  "option-ext",
  "redox_users",
- "windows-sys 0.61.1",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -1726,6 +1760,12 @@ dependencies = [
  "zeroize",
 ]
 
+[[package]]
+name = "dunce"
+version = "1.0.5"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "92773504d58c093f6de2459af4af33faa518c13451eb8f2b5698ed3d36e7c813"
+
 [[package]]
 name = "dyn-clone"
 version = "1.0.18"
@@ -2051,6 +2091,12 @@ dependencies = [
  "windows-sys 0.59.0",
 ]
 
+[[package]]
+name = "find-msvc-tools"
+version = "0.1.9"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582"
+
 [[package]]
 name = "fixedbitset"
 version = "0.5.7"
@@ -2108,6 +2154,12 @@ dependencies = [
 name = "format-flowed"
 version = "1.0.0"
 
+[[package]]
+name = "fs_extra"
+version = "1.3.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "42703706b716c37f96a77aea830392ad231f44c9e9a67872fa5548707e11b11c"
+
 [[package]]
 name = "funty"
 version = "2.0.0"
@@ -2681,7 +2733,7 @@ dependencies = [
  "hyper",
  "libc",
  "pin-project-lite",
- "socket2 0.6.3",
+ "socket2 0.5.9",
  "tokio",
  "tower-service",
  "tracing",
@@ -3234,6 +3286,16 @@ version = "1.0.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "b1a46d1a171d865aa5f83f92695765caa047a9b4cbae2cbf37dbd613a793fd4c"
 
+[[package]]
+name = "jobserver"
+version = "0.1.34"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "9afb3de4395d6b3e67a780b6de64b51c978ecf11cb9a462c66be7d4ca9039d33"
+dependencies = [
+ "getrandom 0.3.3",
+ "libc",
+]
+
 [[package]]
 name = "js-sys"
 version = "0.3.77"
@@ -3374,9 +3436,9 @@ dependencies = [
 
 [[package]]
 name = "log"
-version = "0.4.29"
+version = "0.4.31"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5e5032e24019045c762d3c0f28f5b6b8bbf38563a65908389bf7978758920897"
+checksum = "113b30b4cd05f7c06868fdb2854f66a7b9fece9a48425351cd532e810d74024f"
 
 [[package]]
 name = "loom"
@@ -3818,7 +3880,7 @@ version = "0.50.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "7957b9740744892f114936ab4a57b3f487491bbeafaf8083688b16841a4240e5"
 dependencies = [
- "windows-sys 0.61.1",
+ "windows-sys 0.59.0",
 ]
 
 [[package]]
@@ -4303,18 +4365,18 @@ dependencies = [
 
 [[package]]
 name = "pin-project"
-version = "1.1.11"
+version = "1.1.13"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f1749c7ed4bcaf4c3d0a3efc28538844fb29bcdd7d2b67b2be7e20ba861ff517"
+checksum = "2466b2336ed02bcdca6b294417127b90ec92038d1d5c4fbeac971a922e0e0924"
 dependencies = [
  "pin-project-internal",
 ]
 
 [[package]]
 name = "pin-project-internal"
-version = "1.1.11"
+version = "1.1.13"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d9b20ed30f105399776b9c883e68e536ef602a16ae6f596d2c473591d6ad64c6"
+checksum = "c96395f0a926bc13b1c17622aaddda1ecb55d49c8f1bf9777e4d877800a43f8b"
 dependencies = [
  "proc-macro2",
  "quote",
@@ -4735,9 +4797,9 @@ checksum = "a993555f31e5a609f617c12db6250dedcac1b0a85076912c436e6fc9b2c8e6a3"
 
 [[package]]
 name = "quick-xml"
-version = "0.40.1"
+version = "0.39.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2474bd2e5029e7ccb6abb2ba48cf2383a333851dedf495901544281590c7da7f"
+checksum = "958f21e8e7ceb5a1aa7fa87fab28e7c75976e0bfe7e23ff069e0a260f894067d"
 dependencies = [
  "memchr",
 ]
@@ -5219,7 +5281,7 @@ dependencies = [
  "errno",
  "libc",
  "linux-raw-sys 0.12.1",
- "windows-sys 0.61.1",
+ "windows-sys 0.52.0",
 ]
 
 [[package]]
@@ -5228,6 +5290,7 @@ version = "0.23.37"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "758025cb5fccfd3bc2fd74708fd4682be41d99e5dff73c377c0646c6012c73a4"
 dependencies = [
+ "aws-lc-rs",
  "log",
  "once_cell",
  "ring",
@@ -5273,6 +5336,7 @@ version = "0.103.13"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "61c429a8649f110dddef65e2a5ad240f747e85f7758a6bccc7e5777bd33f756e"
 dependencies = [
+ "aws-lc-rs",
  "ring",
  "rustls-pki-types",
  "untrusted",
@@ -5517,9 +5581,9 @@ dependencies = [
 
 [[package]]
 name = "serde_json"
-version = "1.0.149"
+version = "1.0.150"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "83fc039473c5595ace860d8c4fafa220ff474b3fc6bfdb4293327f1a37e94d86"
+checksum = "e8014e44b4736ed0538adeecded0fce2a272f22dc9578a7eb6b2d9993c74cfb9"
 dependencies = [
  "itoa",
  "memchr",
@@ -5695,9 +5759,9 @@ dependencies = [
 
 [[package]]
 name = "shlex"
-version = "1.3.0"
+version = "2.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64"
+checksum = "f8fadd59c855ef2080decdef8ff161eb6661b86933c9d82e5ba29dc602a55aba"
 
 [[package]]
 name = "signal-hook-registry"
@@ -6083,7 +6147,7 @@ dependencies = [
  "getrandom 0.3.3",
  "once_cell",
  "rustix 1.1.4",
- "windows-sys 0.61.1",
+ "windows-sys 0.52.0",
 ]
 
 [[package]]
@@ -6231,9 +6295,9 @@ checksum = "1f3ccbac311fea05f86f61904b462b55fb3df8837a366dfc601a0161d0532f20"
 
 [[package]]
 name = "tokio"
-version = "1.52.1"
+version = "1.52.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b67dee974fe86fd92cc45b7a95fdd2f99a36a6d7b0d431a231178d3d670bbcc6"
+checksum = "8fc7f01b389ac15039e4dc9531aa973a135d7a4135281b12d7c1bc79fd57fffe"
 dependencies = [
  "bytes",
  "libc",

Cargo.toml

@@ -101,7 +101,7 @@ tagger = "4.3.4"
 textwrap = "0.16.2"
 thiserror = { workspace = true }
 tokio-io-timeout = "1.2.1"
-tokio-rustls = { version = "0.26.2", default-features = false }
+tokio-rustls = { version = "0.26.2", default-features = false, features = ["aws-lc-rs", "tls12"] }
 tokio-stream = { version = "0.1.17", features = ["fs"] }
 astral-tokio-tar = { version = "0.6.2", default-features = false }
 tokio-util = { workspace = true }

src/chat.rs

@@ -3738,17 +3738,19 @@ pub(crate) async fn update_chat_contacts_table(
     id: ChatId,
     contacts: &BTreeSet<ContactId>,
 ) -> Result<()> {
+    // See add_to_chat_contacts_table() for reasoning.
+    let limit = cmp::max(time().saturating_add(TIMESTAMP_SENT_TOLERANCE), timestamp);
     context
         .sql
         .transaction(move |transaction| {
-            // Bump `remove_timestamp` to at least `now`
-            // even for members from `contacts`.
+            // Bump `remove_timestamp` even for members from `contacts`.
             // We add members from `contacts` back below.
             transaction.execute(
-                "UPDATE chats_contacts
-                 SET remove_timestamp=MAX(add_timestamp+1, ?)
+                "UPDATE chats_contacts SET
+                     add_timestamp=MIN(add_timestamp, ?1),
+                     remove_timestamp=MAX(MIN(remove_timestamp,?1), MIN(add_timestamp,?1)+1, ?)
                  WHERE chat_id=?",
-                (timestamp, id),
+                (limit, timestamp, id),
             )?;
 
             if !contacts.is_empty() {
@@ -3760,9 +3762,8 @@ pub(crate) async fn update_chat_contacts_table(
                 )?;
 
                 for contact_id in contacts {
-                    // We bumped `add_timestamp` for existing rows above,
-                    // so on conflict it is enough to set `add_timestamp = remove_timestamp`
-                    // and this guarantees that `add_timestamp` is no less than `timestamp`.
+                    // We bumped `remove_timestamp` for existing rows above,
+                    // so on conflict it is enough to set `add_timestamp = remove_timestamp`.
                     statement.execute((id, contact_id, timestamp))?;
                 }
             }
@@ -3779,17 +3780,24 @@ pub(crate) async fn add_to_chat_contacts_table(
     chat_id: ChatId,
     contact_ids: &[ContactId],
 ) -> Result<()> {
+    // Our clock may be slow, so limit stored timestamps with `timestamp` if it's bigger. This way
+    // we only cap remote timestamps if, in addition, remote changes arrive reordered or we do local
+    // changes. Also allow some tolerance, moreover, previous removals might lend time from the
+    // future.
+    let limit = cmp::max(time().saturating_add(TIMESTAMP_SENT_TOLERANCE), timestamp);
     context
         .sql
         .transaction(move |transaction| {
             let mut add_statement = transaction.prepare(
                 "INSERT INTO chats_contacts (chat_id, contact_id, add_timestamp) VALUES(?1, ?2, ?3)
                  ON CONFLICT (chat_id, contact_id)
-                 DO UPDATE SET add_timestamp=MAX(remove_timestamp, ?3)",
+                 DO UPDATE SET
+                     remove_timestamp=MIN(remove_timestamp, ?4),
+                     add_timestamp=MIN(MAX(add_timestamp,remove_timestamp,?3), ?4)",
             )?;
 
             for contact_id in contact_ids {
-                add_statement.execute((chat_id, contact_id, timestamp))?;
+                add_statement.execute((chat_id, contact_id, timestamp, limit))?;
             }
             Ok(())
         })
@@ -3800,26 +3808,34 @@ pub(crate) async fn add_to_chat_contacts_table(
 
 /// Removes a contact from the chat
 /// by updating the `remove_timestamp`.
+/// Returns whether the contact has been a chat member recently. If so, a removal message should be
+/// sent.
 pub(crate) async fn remove_from_chat_contacts_table(
     context: &Context,
     chat_id: ChatId,
     contact_id: ContactId,
-) -> Result<()> {
+) -> Result<bool> {
     let now = time();
-    context
+    // See add_to_chat_contacts_table() for reasoning.
+    let limit = now.saturating_add(TIMESTAMP_SENT_TOLERANCE);
+    let is_past_member = context
         .sql
         .execute(
-            "UPDATE chats_contacts
-             SET remove_timestamp=MAX(add_timestamp+1, ?)
+            "UPDATE chats_contacts SET
+                 add_timestamp=MIN(add_timestamp, ?1),
+                 remove_timestamp=MAX(MIN(remove_timestamp,?1), MIN(add_timestamp,?1)+1, ?)
              WHERE chat_id=? AND contact_id=?",
-            (now, chat_id, contact_id),
+            (limit, now, chat_id, contact_id),
         )
-        .await?;
-    Ok(())
+        .await?
+        > 0;
+    Ok(is_past_member)
 }
 
 /// Removes a contact from the chat
-/// without leaving a trace.
+/// without leaving a trace in the db.
+/// Returns whether the contact was removed, even if it was a past contact. If so, a removal message
+/// should be sent if the removal is issued by this device.
 ///
 /// Note that if we call this function,
 /// and then receive a message from another device
@@ -3829,17 +3845,17 @@ pub(crate) async fn remove_from_chat_contacts_table_without_trace(
     context: &Context,
     chat_id: ChatId,
     contact_id: ContactId,
-) -> Result<()> {
-    context
+) -> Result<bool> {
+    let removed = context
         .sql
         .execute(
             "DELETE FROM chats_contacts
             WHERE chat_id=? AND contact_id=?",
             (chat_id, contact_id),
         )
-        .await?;
-
-    Ok(())
+        .await?
+        > 0;
+    Ok(removed)
 }
 
 /// Adds a contact to the chat.
@@ -4159,10 +4175,13 @@ pub async fn remove_contact_from_chat(
 
     let mut sync = Nosync;
 
-    if chat.is_promoted() && chat.typ != Chattype::OutBroadcast {
-        remove_from_chat_contacts_table(context, chat_id, contact_id).await?;
+    let removed = if chat.is_promoted() && chat.typ != Chattype::OutBroadcast {
+        remove_from_chat_contacts_table(context, chat_id, contact_id).await?
     } else {
-        remove_from_chat_contacts_table_without_trace(context, chat_id, contact_id).await?;
+        remove_from_chat_contacts_table_without_trace(context, chat_id, contact_id).await?
+    };
+    if !removed {
+        return Ok(());
     }
 
     // We do not return an error if the contact does not exist in the database.

src/chat/chat_tests.rs

@@ -2800,6 +2800,30 @@ async fn test_can_send_group() -> Result<()> {
     Ok(())
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn test_cant_remove_nonmember() -> Result<()> {
+    let mut tcm = TestContextManager::new();
+    let alice = &tcm.alice().await;
+    let bob = &tcm.bob().await;
+    let charlie = &tcm.charlie().await;
+
+    let alice_broadcast_id = create_broadcast(alice, "Channel".to_string()).await?;
+    let qr = get_securejoin_qr(alice, Some(alice_broadcast_id))
+        .await
+        .unwrap();
+    tcm.exec_securejoin_qr(bob, alice, &qr).await;
+
+    let alice_charlie_id = alice.add_or_lookup_contact_id(charlie).await;
+    remove_contact_from_chat(alice, alice_broadcast_id, alice_charlie_id).await?;
+    assert!(alice.pop_sent_msg_opt(Duration::ZERO).await.is_none());
+    assert!(!remove_from_chat_contacts_table(alice, alice_broadcast_id, alice_charlie_id).await?);
+    assert!(
+        !remove_from_chat_contacts_table_without_trace(alice, alice_broadcast_id, alice_charlie_id)
+            .await?
+    );
+    Ok(())
+}
+
 /// Tests that in a broadcast channel,
 /// the recipients can't see the identity of their fellow recipients.
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]

src/net/tls.rs

@@ -126,9 +126,12 @@ pub async fn wrap_rustls<'a>(
     let root_cert_store =
         rustls::RootCertStore::from_iter(webpki_roots::TLS_SERVER_ROOTS.iter().cloned());
 
-    let mut config = rustls::ClientConfig::builder()
-        .with_root_certificates(root_cert_store)
-        .with_no_client_auth();
+    let mut config = rustls::ClientConfig::builder_with_provider(Arc::new(
+        rustls::crypto::aws_lc_rs::default_provider(),
+    ))
+    .with_safe_default_protocol_versions()?
+    .with_root_certificates(root_cert_store)
+    .with_no_client_auth();
     config.alpn_protocols = if alpn.is_empty() {
         vec![]
     } else {

src/net/tls/danger.rs

@@ -51,7 +51,7 @@ impl rustls::client::danger::ServerCertVerifier for CustomCertificateVerifier {
 
         let spki = parsed_certificate.subject_public_key_info();
 
-        let provider = rustls::crypto::ring::default_provider();
+        let provider = rustls::crypto::aws_lc_rs::default_provider();
 
         if let ServerName::DnsName(dns_name) = server_name
             && dns_name.as_ref().starts_with("_")
@@ -97,7 +97,7 @@ impl rustls::client::danger::ServerCertVerifier for CustomCertificateVerifier {
         cert: &CertificateDer<'_>,
         dss: &rustls::DigitallySignedStruct,
     ) -> Result<rustls::client::danger::HandshakeSignatureValid, rustls::Error> {
-        let provider = rustls::crypto::ring::default_provider();
+        let provider = rustls::crypto::aws_lc_rs::default_provider();
         let supported_schemes = &provider.signature_verification_algorithms;
         rustls::crypto::verify_tls12_signature(message, cert, dss, supported_schemes)
     }
@@ -108,13 +108,13 @@ impl rustls::client::danger::ServerCertVerifier for CustomCertificateVerifier {
         cert: &CertificateDer<'_>,
         dss: &rustls::DigitallySignedStruct,
     ) -> Result<rustls::client::danger::HandshakeSignatureValid, rustls::Error> {
-        let provider = rustls::crypto::ring::default_provider();
+        let provider = rustls::crypto::aws_lc_rs::default_provider();
         let supported_schemes = &provider.signature_verification_algorithms;
         rustls::crypto::verify_tls13_signature(message, cert, dss, supported_schemes)
     }
 
     fn supported_verify_schemes(&self) -> Vec<rustls::SignatureScheme> {
-        let provider = rustls::crypto::ring::default_provider();
+        let provider = rustls::crypto::aws_lc_rs::default_provider();
         provider
             .signature_verification_algorithms
             .supported_schemes()

src/receive_imf.rs

@@ -3790,13 +3790,17 @@ async fn apply_out_broadcast_changes(
         } else if from_id == ContactId::SELF
             && let Some(removed_id) = removed_id
         {
-            chat::remove_from_chat_contacts_table_without_trace(context, chat.id, removed_id)
-                .await?;
-
-            better_msg.get_or_insert(
-                stock_str::msg_del_member_local(context, removed_id, ContactId::SELF).await,
-            );
-            added_removed_id = Some(removed_id);
+            if chat::remove_from_chat_contacts_table_without_trace(context, chat.id, removed_id)
+                .await?
+            {
+                better_msg.get_or_insert(
+                    stock_str::msg_del_member_local(context, removed_id, ContactId::SELF).await,
+                );
+                added_removed_id = Some(removed_id);
+            } else {
+                info!(context, "No-op broadcast member removal message (TRASH).");
+                better_msg = Some("".to_string());
+            }
         }
     }
 
@@ -3870,17 +3874,20 @@ async fn apply_in_broadcast_changes(
         }
         chat::delete_broadcast_secret(context, chat.id).await?;
 
-        if from_id == ContactId::SELF {
+        let removed =
+            chat::remove_from_chat_contacts_table_without_trace(context, chat.id, ContactId::SELF)
+                .await?;
+        if !removed {
+            info!(context, "No-op broadcast SELF-removal message (TRASH).");
+            better_msg = Some("".to_string());
+        } else if from_id == ContactId::SELF {
             better_msg.get_or_insert(stock_str::msg_you_left_broadcast(context));
         } else {
             better_msg.get_or_insert(
                 stock_str::msg_del_member_local(context, ContactId::SELF, from_id).await,
             );
         }
-
-        chat::remove_from_chat_contacts_table_without_trace(context, chat.id, ContactId::SELF)
-            .await?;
-        send_event_chat_modified = true;
+        send_event_chat_modified |= removed;
     } else if !chat.is_self_in_chat(context).await? {
         chat::add_to_chat_contacts_table(
             context,