From 3bd1b7ad9e0400c078ab703d3598942199383978 Mon Sep 17 00:00:00 2001 From: TroyHernandez Date: Wed, 5 Aug 2026 16:40:14 -0500 Subject: [PATCH 1/2] Do not send an encrypted event no one can read Three ways an encrypted send could report success while reaching nobody. All the same shape: a failure or an absence read as an answer. mx_send_encrypted() filtered out its own device by device_id alone. Matrix device ids are scoped to a user, not globally unique, so every other account whose device happened to share the name was silently dropped -- two bots both called BOT, and the only recipient disappears while the send still returns an event id. The self-filter compares (user_id, device_id) now. Devices are skipped when their keys or one-time keys do not verify, and that stays: one bad device must not make a room unusable. Skipping all of them is different. The room key then reaches nobody, mx_crypto_encrypt_for_devices() builds the event for an empty recipient list without complaint, and the caller gets an event id for a message every recipient will fail to decrypt. Reaching zero aborts. A room with no other devices at all is not that case -- it is a room of one, and sending to it is fine, which is tested so the guard cannot swallow it. And /keys/query and /keys/claim both answer 200 with a `failures` map when a server could not be reached, returning whatever they did manage. Reading only the successful half made an unreachable homeserver indistinguishable from a user with no devices. Both helpers gain `strict`: warn by default, error under it. mx_send_encrypted() asks for strict, because encrypting to a user whose devices could not be listed is exactly how a message ends up readable by nobody. The decrypt path keeps the default, where an unlisted device means an unverified sender rather than a lost message. Found by auditing the two calls I had named as the next place to look after the same pattern turned up four times in chat.api's adapter. 148 assertions, three mutations, all caught. R CMD check clean but for the days-since-last-update NOTE. --- DESCRIPTION | 2 +- NEWS.md | 29 +++++ R/profile.R | 2 +- R/transport.R | 70 +++++++++++- inst/tinytest/test_transport.R | 160 +++++++++++++++++++++++++++ man/mx_client_configure.Rd | 12 +- man/mx_client_load.Rd | 8 +- man/mx_crypto_claim_otks.Rd | 9 +- man/mx_crypto_encrypt_event.Rd | 9 +- man/mx_crypto_encrypt_for_devices.Rd | 13 ++- man/mx_crypto_handle_to_device.Rd | 10 +- man/mx_crypto_known_devices.Rd | 12 +- man/mx_crypto_process_sync.Rd | 10 +- man/mx_crypto_room_key_payload.Rd | 15 ++- man/mx_extract_reaction_verdict.Rd | 10 +- man/mx_resolve_room.Rd | 10 +- man/mx_send_encrypted.Rd | 12 +- man/mx_send_media.Rd | 14 ++- man/mx_send_table.Rd | 11 +- man/mx_send_text.Rd | 12 +- man/mx_sync_update.Rd | 10 +- 21 files changed, 398 insertions(+), 42 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 10a332a..6eef878 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Package: mx.client Type: Package Title: Stateful Matrix Client Helpers -Version: 0.2.0 +Version: 0.2.0.1 Date: 2026-08-04 Authors@R: c( person("Troy", "Hernandez", role = c("aut", "cre"), diff --git a/NEWS.md b/NEWS.md index 65fd8b7..6cf3f07 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,32 @@ +# mx.client 0.2.0.1 + +## Security + +* **HIGH**: `mx_send_encrypted()` filtered its own device by `device_id` + alone. Matrix device ids are scoped to a user, not globally unique, so + every other account whose device happened to share the name was + dropped from the recipient list -- two bots both called `BOT`, and the + only recipient disappears while the send still returns an event id. + The self-filter now compares `(user_id, device_id)`. + +* **HIGH**: an encrypted send with no usable recipients posted the + `m.room.encrypted` event anyway. Devices are skipped when their keys + or one-time keys do not verify, which is deliberate, but skipping all + of them means the room key reaches nobody and the caller is handed an + event id for a message every recipient will fail to decrypt. Reaching + zero now aborts. A room with no other devices at all is unaffected: + that is a room of one, and sending to it is fine. + +* **HIGH**: `/keys/query` and `/keys/claim` answer 200 with a `failures` + map when a server could not be reached, returning whatever they did + manage. Both were read as though the successful half were the whole + answer, so an unreachable homeserver was indistinguishable from a user + with no devices. `mx_crypto_known_devices()` and + `mx_crypto_claim_otks()` gain `strict`: they warn by default and error + under it, and `mx_send_encrypted()` asks for strict, because + encrypting to a user whose devices could not be listed is how a + message ends up readable by nobody. + # mx.client 0.2.0 ## Security diff --git a/R/profile.R b/R/profile.R index dfa7039..e0e1b79 100644 --- a/R/profile.R +++ b/R/profile.R @@ -24,7 +24,7 @@ #' @export mx_set_displayname <- function(client, name, save = TRUE) { if (!is.character(name) || length(name) != 1L || is.na(name) || - !nzchar(name)) { + !nzchar(name)) { stop("name must be a single non-empty string", call. = FALSE) } mx_with_relogin(client, function(cl) { diff --git a/R/transport.R b/R/transport.R index 96f1774..0378d98 100644 --- a/R/transport.R +++ b/R/transport.R @@ -60,8 +60,17 @@ mx_crypto_publish_keys <- function(client, account, store_dir, n_otks = 50L) { #' fails is dropped with a warning naming it, and the remaining devices #' are still returned: one bad device must not make a room unusable. #' +#' \code{/keys/query} answers 200 with a \code{failures} map when it could +#' not reach a server, and returns whatever it did manage. That is not the +#' same as a user having no devices, so it is never silent: by default it +#' warns, and with \code{strict = TRUE} it is an error. Encrypting to a +#' user whose devices could not be listed is how a message ends up +#' readable by nobody, so the send path asks for strict. +#' #' @param client Matrix client config. #' @param user_ids Character vector of Matrix user ids. +#' @param strict Logical. Treat a \code{failures} map as an error rather +#' than a warning. #' @return List of verified devices, each \code{list(user_id, device_id, #' curve25519, ed25519)}. #' @examples @@ -69,14 +78,35 @@ mx_crypto_publish_keys <- function(client, account, store_dir, n_otks = 50L) { #' mx_crypto_known_devices(client, "@bob:example.org") #' } #' @export -mx_crypto_known_devices <- function(client, user_ids) { +mx_crypto_known_devices <- function(client, user_ids, strict = FALSE) { mx_require_crypto() s <- mx_client_session(client) query <- stats::setNames(rep(list(list()), length(user_ids)), user_ids) resp <- mx.api::mx_keys_query(s, device_keys = query) + mx_crypto_report_failures(resp$failures, "/keys/query", strict) mx_crypto_verify_device_map(resp$device_keys) } +# A server we could not reach is not a user with no devices. +# +# Both /keys/query and /keys/claim answer 200 with a `failures` map and +# whatever they did manage beside it. Read only the successful half and an +# unreachable homeserver looks exactly like an empty one, which on the +# send path means encrypting to nobody and reporting success. +mx_crypto_report_failures <- function(failures, what, strict = FALSE) { + if (!length(failures)) { + return(invisible(FALSE)) + } + msg <- paste0("mx.client: ", what, " could not reach ", + paste(names(failures), collapse = ", "), + "; devices there are unknown, not absent") + if (isTRUE(strict)) { + stop(msg, call. = FALSE) + } + warning(msg, call. = FALSE) + invisible(TRUE) +} + # Verify every device in a parsed /keys/query device_keys map. # # Split out from the HTTP call so the verification logic is testable @@ -118,8 +148,14 @@ mx_crypto_verify_device_map <- function(device_keys_map) { #' (that is, from \code{mx_crypto_known_devices()}). A key that fails is #' dropped with a warning and its device comes back with no \code{otk}. #' +#' \code{/keys/claim} carries the same \code{failures} map as +#' \code{/keys/query}, and it is handled the same way: warned about by +#' default, an error under \code{strict}. +#' #' @param client Matrix client config. #' @param devices List of devices from \code{mx_crypto_known_devices()}. +#' @param strict Logical. Treat a \code{failures} map as an error rather +#' than a warning. #' @return The devices with an \code{otk} field added where one was #' claimed and verified. #' @examples @@ -127,7 +163,7 @@ mx_crypto_verify_device_map <- function(device_keys_map) { #' devs <- mx_crypto_claim_otks(client, mx_crypto_known_devices(client, uid)) #' } #' @export -mx_crypto_claim_otks <- function(client, devices) { +mx_crypto_claim_otks <- function(client, devices, strict = FALSE) { mx_require_crypto() if (!length(devices)) { return(devices) @@ -139,6 +175,7 @@ mx_crypto_claim_otks <- function(client, devices) { stats::setNames(list("signed_curve25519"), d$device_id)) } resp <- mx.api::mx_keys_claim(s, one_time_keys = req) + mx_crypto_report_failures(resp$failures, "/keys/claim", strict) mx_crypto_verify_claimed_otks(devices, resp$one_time_keys %||% list()) } @@ -215,15 +252,24 @@ mx_send_encrypted <- function(client, account, sessions, room_id, content, sender_curve <- mx.crypto::mxc_account_identity_keys(account)$curve25519 if (is.null(recipients)) { - devs <- mx_crypto_known_devices(client, member_ids) - # skip our own device; claim OTKs only where no Olm session exists + # strict: a server that could not be reached leaves its users' + # devices unknown, and an unknown device is not an absent one. Read + # the successful half of a partial response and a federation blip + # becomes a message nobody can read. + devs <- mx_crypto_known_devices(client, member_ids, strict = TRUE) + # Skip our own device, and only our own. Device ids are scoped to a + # user, not globally unique, so comparing device_id alone dropped + # every other account whose device happened to share the name -- + # two bots both called BOT, and the one recipient vanishes while + # the send still reports success. devs <- Filter(function(d) { - !identical(d$device_id, client$device_id) && + !(identical(d$user_id, client$user_id) && + identical(d$device_id, client$device_id)) && !is.null(d$curve25519) && nzchar(d$curve25519) }, devs) need <- Filter(function(d) is.null(sessions$olm[[d$curve25519]]), devs) have <- Filter(function(d) !is.null(sessions$olm[[d$curve25519]]), devs) - claimed <- mx_crypto_claim_otks(client, need) + claimed <- mx_crypto_claim_otks(client, need, strict = TRUE) # A device whose one-time key failed verification has no usable otk. # Drop it rather than letting encrypt_for_devices abort the send: # the remaining devices should still get the message. @@ -235,6 +281,18 @@ mx_send_encrypted <- function(client, account, sessions, room_id, content, call. = FALSE) } recipients <- c(usable, have) + # Skipping some devices is the policy above. Skipping all of them + # is not: the room key reaches nobody, the m.room.encrypted event + # goes out anyway, and the caller is handed an event id for a + # message every recipient will fail to decrypt. A room with no + # other devices at all is different -- that is a room of one, and + # sending to it is fine. + if (length(devs) && !length(recipients)) { + stop("mx.client: every device in ", room_id, " was skipped (", + length(devs), " found, none usable). Refusing to send an ", + "encrypted event whose room key would reach nobody.", + call. = FALSE) + } } out <- mx_crypto_encrypt_for_devices(account, sessions, room_id, diff --git a/inst/tinytest/test_transport.R b/inst/tinytest/test_transport.R index c869b86..f19bd9d 100644 --- a/inst/tinytest/test_transport.R +++ b/inst/tinytest/test_transport.R @@ -109,3 +109,163 @@ expect_null(res[[1]]$otk) res <- mx.client:::mx_crypto_verify_claimed_otks(list(device), list()) expect_equal(length(res), 1L) expect_null(res[[1]]$otk) + +# ---- A partial response is not an empty one ---- +# /keys/query and /keys/claim both answer 200 with a `failures` map and +# whatever they did manage. Reading only the successful half makes an +# unreachable homeserver look like a user with no devices, which on the +# send path means encrypting to nobody and reporting success. +rf <- mx.client:::mx_crypto_report_failures +expect_false(rf(NULL, "/keys/query")) +expect_false(rf(list(), "/keys/query")) +expect_warning(rf(list(`ex.org` = list()), "/keys/query"), + "could not reach ex.org") +expect_error(rf(list(`ex.org` = list()), "/keys/query", strict = TRUE), + "could not reach ex.org") +# Every unreachable server is named, not just the first. +expect_error(rf(list(a = list(), b = list()), "/keys/claim", strict = TRUE), + "a, b") +# Both call sites take the flag, so the send path can ask for strict +# while the decrypt path stays best-effort. +expect_true("strict" %in% names(formals(mx_crypto_known_devices))) +expect_true("strict" %in% names(formals(mx_crypto_claim_otks))) + +# ---- Device ids are scoped to a user ---- +# mx_send_encrypted() filters out its own device. Comparing device_id +# alone dropped every other account whose device happened to share the +# name: two bots both called BOT, and the only recipient disappears while +# the send still returns an event id. +local({ + me <- list(user_id = "@alice:example.org", device_id = "BOT", + server = "https://ex.invalid", token = "t") + bob <- mx.crypto::mxc_account_new() + bob_idk <- mx.crypto::mxc_account_identity_keys(bob) + bob_dk <- mx_crypto_device_keys(bob, "@bob:example.org", "BOT") + alice_dk <- mx_crypto_device_keys(alice, "@alice:example.org", "BOT") + resp <- list(device_keys = list( + `@bob:example.org` = stats::setNames(list(bob_dk), "BOT"), + `@alice:example.org` = stats::setNames(list(alice_dk), "BOT"))) + + seen <- NULL + o1 <- mx.api::mx_keys_query + assignInNamespace("mx_keys_query", function(...) resp, ns = "mx.api") + o2 <- mx.api::mx_keys_claim + assignInNamespace("mx_keys_claim", function(session, one_time_keys) { + seen <<- names(one_time_keys) + list(one_time_keys = list()) + }, ns = "mx.api") + on.exit({ + assignInNamespace("mx_keys_query", o1, ns = "mx.api") + assignInNamespace("mx_keys_claim", o2, ns = "mx.api") + }) + + acct <- mx.crypto::mxc_account_new() + store <- tempfile("sendstore") + # Bob's BOT survives the self-filter, so a key is claimed for him and + # the send gets that far. Alice's own BOT is the only one dropped. + expect_error(mx_send_encrypted(me, acct, mx_crypto_sessions_new(), + "!r:ex", list(msgtype = "m.text", + body = "hi"), store, + member_ids = c("@bob:example.org", + "@alice:example.org")), + "was skipped") + expect_identical(seen, "@bob:example.org") + unlink(store, recursive = TRUE) +}) + +# ---- Every device skipped is not a room of one ---- +# Skipping some devices is the documented policy. Skipping all of them +# means the room key reaches nobody, and posting the event anyway hands +# the caller an event id for a message every recipient will fail to +# decrypt. +local({ + me <- list(user_id = "@alice:example.org", device_id = "ALICEDEV", + server = "https://ex.invalid", token = "t") + bob <- mx.crypto::mxc_account_new() + bob_dk <- mx_crypto_device_keys(bob, "@bob:example.org", "BOBDEV") + posted <- 0L + o1 <- mx.api::mx_keys_query + assignInNamespace("mx_keys_query", function(...) list(device_keys = list( + `@bob:example.org` = stats::setNames(list(bob_dk), "BOBDEV"))), + ns = "mx.api") + o2 <- mx.api::mx_keys_claim + # No one-time keys come back, so Bob's device is skipped and nothing + # is left. + assignInNamespace("mx_keys_claim", function(...) list(one_time_keys = list()), + ns = "mx.api") + o3 <- mx.api::mx_send_event + assignInNamespace("mx_send_event", function(...) { posted <<- posted + 1L + "$e" }, ns = "mx.api") + on.exit({ + assignInNamespace("mx_keys_query", o1, ns = "mx.api") + assignInNamespace("mx_keys_claim", o2, ns = "mx.api") + assignInNamespace("mx_send_event", o3, ns = "mx.api") + }) + acct <- mx.crypto::mxc_account_new() + store <- tempfile("sendstore2") + expect_error(mx_send_encrypted(me, acct, mx_crypto_sessions_new(), + "!r:ex", list(msgtype = "m.text", + body = "hi"), store, + member_ids = "@bob:example.org"), + "none usable") + # And nothing was posted. + expect_identical(posted, 0L) + unlink(store, recursive = TRUE) +}) + +# A room whose only member is us is a room of one, not a failure: no +# other devices were found, so there is nothing to have skipped. +local({ + me <- list(user_id = "@alice:example.org", device_id = "ALICEDEV", + server = "https://ex.invalid", token = "t") + alice_dk <- mx_crypto_device_keys(alice, "@alice:example.org", "ALICEDEV") + posted <- 0L + o1 <- mx.api::mx_keys_query + assignInNamespace("mx_keys_query", function(...) list(device_keys = list( + `@alice:example.org` = stats::setNames(list(alice_dk), "ALICEDEV"))), + ns = "mx.api") + o2 <- mx.api::mx_send_event + assignInNamespace("mx_send_event", function(...) { posted <<- posted + 1L + "$solo" }, ns = "mx.api") + on.exit({ + assignInNamespace("mx_keys_query", o1, ns = "mx.api") + assignInNamespace("mx_send_event", o2, ns = "mx.api") + }) + acct <- mx.crypto::mxc_account_new() + store <- tempfile("sendstore3") + res <- mx_send_encrypted(me, acct, mx_crypto_sessions_new(), "!r:ex", + list(msgtype = "m.text", body = "note to self"), + store, member_ids = "@alice:example.org") + expect_identical(res$event_id, "$solo") + expect_identical(posted, 1L) + unlink(store, recursive = TRUE) +}) + +# A /keys/query that could not reach Bob's server aborts the send rather +# than treating him as a user with no devices. +local({ + me <- list(user_id = "@alice:example.org", device_id = "ALICEDEV", + server = "https://ex.invalid", token = "t") + posted <- 0L + o1 <- mx.api::mx_keys_query + assignInNamespace("mx_keys_query", function(...) list( + device_keys = list(), + failures = list(`example.org` = list(errcode = "M_UNKNOWN"))), + ns = "mx.api") + o2 <- mx.api::mx_send_event + assignInNamespace("mx_send_event", function(...) { posted <<- posted + 1L + "$e" }, ns = "mx.api") + on.exit({ + assignInNamespace("mx_keys_query", o1, ns = "mx.api") + assignInNamespace("mx_send_event", o2, ns = "mx.api") + }) + acct <- mx.crypto::mxc_account_new() + store <- tempfile("sendstore4") + expect_error(mx_send_encrypted(me, acct, mx_crypto_sessions_new(), + "!r:ex", list(msgtype = "m.text", + body = "hi"), store, + member_ids = "@bob:example.org"), + "could not reach example.org") + expect_identical(posted, 0L) + unlink(store, recursive = TRUE) +}) diff --git a/man/mx_client_configure.Rd b/man/mx_client_configure.Rd index 1d34e09..3275208 100644 --- a/man/mx_client_configure.Rd +++ b/man/mx_client_configure.Rd @@ -3,8 +3,16 @@ \alias{mx_client_configure} \title{Configure and save a Matrix client} \usage{ -mx_client_configure(server, user, password, room, app = "mx.client", - path = NULL, device_id = NULL, extra = list()) +mx_client_configure( + server, + user, + password, + room, + app = "mx.client", + path = NULL, + device_id = NULL, + extra = list() +) } \arguments{ \item{server}{Character. Homeserver base URL.} diff --git a/man/mx_client_load.Rd b/man/mx_client_load.Rd index fa8b151..a1a642f 100644 --- a/man/mx_client_load.Rd +++ b/man/mx_client_load.Rd @@ -3,8 +3,12 @@ \alias{mx_client_load} \title{Load a Matrix client config} \usage{ -mx_client_load(app = "mx.client", path = NULL, - legacy_path = mx_client_legacy_config_path(app), env_var = NULL) +mx_client_load( + app = "mx.client", + path = NULL, + legacy_path = mx_client_legacy_config_path(app), + env_var = NULL +) } \arguments{ \item{app}{Character. Application namespace.} diff --git a/man/mx_crypto_claim_otks.Rd b/man/mx_crypto_claim_otks.Rd index 157ed09..725e7fb 100644 --- a/man/mx_crypto_claim_otks.Rd +++ b/man/mx_crypto_claim_otks.Rd @@ -3,12 +3,15 @@ \alias{mx_crypto_claim_otks} \title{Claim a one-time key for each device} \usage{ -mx_crypto_claim_otks(client, devices) +mx_crypto_claim_otks(client, devices, strict = FALSE) } \arguments{ \item{client}{Matrix client config.} \item{devices}{List of devices from \code{mx_crypto_known_devices()}.} + +\item{strict}{Logical. Treat a \code{failures} map as an error rather +than a warning.} } \value{ The devices with an \code{otk} field added where one was @@ -24,6 +27,10 @@ key, which must itself have come from a verified \code{device_keys} (that is, from \code{mx_crypto_known_devices()}). A key that fails is dropped with a warning and its device comes back with no \code{otk}. +\code{/keys/claim} carries the same \code{failures} map as +\code{/keys/query}, and it is handled the same way: warned about by +default, an error under \code{strict}. + } \examples{ \dontrun{ diff --git a/man/mx_crypto_encrypt_event.Rd b/man/mx_crypto_encrypt_event.Rd index f15087d..1ae5d56 100644 --- a/man/mx_crypto_encrypt_event.Rd +++ b/man/mx_crypto_encrypt_event.Rd @@ -3,8 +3,13 @@ \alias{mx_crypto_encrypt_event} \title{Encrypt event content for a room (Megolm)} \usage{ -mx_crypto_encrypt_event(megolm_out, content, room_id, sender_curve25519, - device_id) +mx_crypto_encrypt_event( + megolm_out, + content, + room_id, + sender_curve25519, + device_id +) } \arguments{ \item{megolm_out}{An outbound Megolm session.} diff --git a/man/mx_crypto_encrypt_for_devices.Rd b/man/mx_crypto_encrypt_for_devices.Rd index 10b6b03..390bbe6 100644 --- a/man/mx_crypto_encrypt_for_devices.Rd +++ b/man/mx_crypto_encrypt_for_devices.Rd @@ -3,9 +3,16 @@ \alias{mx_crypto_encrypt_for_devices} \title{Encrypt an event for an encrypted room's devices} \usage{ -mx_crypto_encrypt_for_devices(account, sessions, room_id, content, - sender_curve25519, device_id, - recipients = list(), sender_user_id = NULL) +mx_crypto_encrypt_for_devices( + account, + sessions, + room_id, + content, + sender_curve25519, + device_id, + recipients = list(), + sender_user_id = NULL +) } \arguments{ \item{account}{An mx.crypto account handle.} diff --git a/man/mx_crypto_handle_to_device.Rd b/man/mx_crypto_handle_to_device.Rd index 81e5fe4..5a0d93d 100644 --- a/man/mx_crypto_handle_to_device.Rd +++ b/man/mx_crypto_handle_to_device.Rd @@ -3,8 +3,14 @@ \alias{mx_crypto_handle_to_device} \title{Decrypt an inbound Olm to-device payload} \usage{ -mx_crypto_handle_to_device(account, my_curve25519, content, self_id = NULL, - self_ed25519 = NULL, devices = NULL) +mx_crypto_handle_to_device( + account, + my_curve25519, + content, + self_id = NULL, + self_ed25519 = NULL, + devices = NULL +) } \arguments{ \item{account}{An mx.crypto account handle.} diff --git a/man/mx_crypto_known_devices.Rd b/man/mx_crypto_known_devices.Rd index 1e5f302..bf9ebff 100644 --- a/man/mx_crypto_known_devices.Rd +++ b/man/mx_crypto_known_devices.Rd @@ -3,12 +3,15 @@ \alias{mx_crypto_known_devices} \title{List the devices (and identity keys) of some users} \usage{ -mx_crypto_known_devices(client, user_ids) +mx_crypto_known_devices(client, user_ids, strict = FALSE) } \arguments{ \item{client}{Matrix client config.} \item{user_ids}{Character vector of Matrix user ids.} + +\item{strict}{Logical. Treat a \code{failures} map as an error rather +than a warning.} } \value{ List of verified devices, each \code{list(user_id, device_id, @@ -23,6 +26,13 @@ against the identity the homeserver claims for them. A device that fails is dropped with a warning naming it, and the remaining devices are still returned: one bad device must not make a room unusable. +\code{/keys/query} answers 200 with a \code{failures} map when it could +not reach a server, and returns whatever it did manage. That is not the +same as a user having no devices, so it is never silent: by default it +warns, and with \code{strict = TRUE} it is an error. Encrypting to a +user whose devices could not be listed is how a message ends up +readable by nobody, so the send path asks for strict. + } \examples{ \dontrun{ diff --git a/man/mx_crypto_process_sync.Rd b/man/mx_crypto_process_sync.Rd index 73af12c..9cfa2b0 100644 --- a/man/mx_crypto_process_sync.Rd +++ b/man/mx_crypto_process_sync.Rd @@ -3,8 +3,14 @@ \alias{mx_crypto_process_sync} \title{Process a sync response: store room keys, decrypt room events} \usage{ -mx_crypto_process_sync(account, sessions, sync_resp, self_curve25519, - self_id = NULL, devices = NULL) +mx_crypto_process_sync( + account, + sessions, + sync_resp, + self_curve25519, + self_id = NULL, + devices = NULL +) } \arguments{ \item{account}{An mx.crypto account handle.} diff --git a/man/mx_crypto_room_key_payload.Rd b/man/mx_crypto_room_key_payload.Rd index f61d5fb..9707082 100644 --- a/man/mx_crypto_room_key_payload.Rd +++ b/man/mx_crypto_room_key_payload.Rd @@ -3,10 +3,17 @@ \alias{mx_crypto_room_key_payload} \title{Encrypt a Megolm room key to one device as a to-device payload} \usage{ -mx_crypto_room_key_payload(olm_session, sender_curve25519, - recipient_curve25519, room_id, megolm_out, - sender_user_id, sender_ed25519, recipient_user_id, - recipient_ed25519) +mx_crypto_room_key_payload( + olm_session, + sender_curve25519, + recipient_curve25519, + room_id, + megolm_out, + sender_user_id, + sender_ed25519, + recipient_user_id, + recipient_ed25519 +) } \arguments{ \item{olm_session}{An outbound Olm session diff --git a/man/mx_extract_reaction_verdict.Rd b/man/mx_extract_reaction_verdict.Rd index 44312ac..6bb16cc 100644 --- a/man/mx_extract_reaction_verdict.Rd +++ b/man/mx_extract_reaction_verdict.Rd @@ -3,8 +3,14 @@ \alias{mx_extract_reaction_verdict} \title{Extract a reaction approval verdict from sync events} \usage{ -mx_extract_reaction_verdict(sync_resp, room_id, self_id, target_event_id, - approve_keys = NULL, deny_keys = NULL) +mx_extract_reaction_verdict( + sync_resp, + room_id, + self_id, + target_event_id, + approve_keys = NULL, + deny_keys = NULL +) } \arguments{ \item{sync_resp}{Parsed \code{/sync} response.} diff --git a/man/mx_resolve_room.Rd b/man/mx_resolve_room.Rd index a9d1d4d..16dc3c3 100644 --- a/man/mx_resolve_room.Rd +++ b/man/mx_resolve_room.Rd @@ -3,8 +3,14 @@ \alias{mx_resolve_room} \title{Resolve a room id, name, or default room} \usage{ -mx_resolve_room(client, room = NULL, room_cache = NULL, fallback = TRUE, - details = FALSE, quiet = FALSE) +mx_resolve_room( + client, + room = NULL, + room_cache = NULL, + fallback = TRUE, + details = FALSE, + quiet = FALSE +) } \arguments{ \item{client}{Matrix client config.} diff --git a/man/mx_send_encrypted.Rd b/man/mx_send_encrypted.Rd index 663997e..8341bcc 100644 --- a/man/mx_send_encrypted.Rd +++ b/man/mx_send_encrypted.Rd @@ -3,8 +3,16 @@ \alias{mx_send_encrypted} \title{Send an end-to-end encrypted message to a room} \usage{ -mx_send_encrypted(client, account, sessions, room_id, content, store_dir, - recipients = NULL, member_ids = NULL) +mx_send_encrypted( + client, + account, + sessions, + room_id, + content, + store_dir, + recipients = NULL, + member_ids = NULL +) } \arguments{ \item{client}{Matrix client config.} diff --git a/man/mx_send_media.Rd b/man/mx_send_media.Rd index a8a7ecc..e92e4ca 100644 --- a/man/mx_send_media.Rd +++ b/man/mx_send_media.Rd @@ -3,9 +3,17 @@ \alias{mx_send_media} \title{Send a media file to a Matrix room} \usage{ -mx_send_media(client, path, room = NULL, body = basename(path), msgtype = NULL, - content_type = NULL, info = list(), room_cache = NULL, - dry_run = FALSE) +mx_send_media( + client, + path, + room = NULL, + body = basename(path), + msgtype = NULL, + content_type = NULL, + info = list(), + room_cache = NULL, + dry_run = FALSE +) } \arguments{ \item{client}{Matrix client config.} diff --git a/man/mx_send_table.Rd b/man/mx_send_table.Rd index f97f146..1ce6090 100644 --- a/man/mx_send_table.Rd +++ b/man/mx_send_table.Rd @@ -3,8 +3,15 @@ \alias{mx_send_table} \title{Send tabular data to a Matrix room} \usage{ -mx_send_table(client, x, room = NULL, header = TRUE, title = NULL, - room_cache = NULL, dry_run = FALSE) +mx_send_table( + client, + x, + room = NULL, + header = TRUE, + title = NULL, + room_cache = NULL, + dry_run = FALSE +) } \arguments{ \item{client}{Matrix client config.} diff --git a/man/mx_send_text.Rd b/man/mx_send_text.Rd index ea5fc94..17b627e 100644 --- a/man/mx_send_text.Rd +++ b/man/mx_send_text.Rd @@ -3,8 +3,16 @@ \alias{mx_send_text} \title{Send plain text to a Matrix room} \usage{ -mx_send_text(client, text, room = NULL, msgtype = "m.text", room_cache = NULL, - dry_run = FALSE, markdown = FALSE, mentions = NULL) +mx_send_text( + client, + text, + room = NULL, + msgtype = "m.text", + room_cache = NULL, + dry_run = FALSE, + markdown = FALSE, + mentions = NULL +) } \arguments{ \item{client}{Matrix client config.} diff --git a/man/mx_sync_update.Rd b/man/mx_sync_update.Rd index 276a085..3608a91 100644 --- a/man/mx_sync_update.Rd +++ b/man/mx_sync_update.Rd @@ -3,8 +3,14 @@ \alias{mx_sync_update} \title{Sync once and update the stored cursor} \usage{ -mx_sync_update(client, timeout = 0L, filter = NULL, save = TRUE, path = NULL, - app = NULL) +mx_sync_update( + client, + timeout = 0L, + filter = NULL, + save = TRUE, + path = NULL, + app = NULL +) } \arguments{ \item{client}{Matrix client config.} From 1dd5a3ee4cd874f848bad3fdcd2cb8663a328bb5 Mon Sep 17 00:00:00 2001 From: TroyHernandez Date: Wed, 5 Aug 2026 16:55:24 -0500 Subject: [PATCH 2/2] Count the devices the homeserver named, not the ones that verified The zero-recipient guard measured what survived mx_crypto_verify_device_map(), which drops what it cannot verify. So a room whose only other device is malformed looked exactly like a room with no other device: nothing left to encrypt to either way, and only the second of those is a reason to go ahead. The unreadable event went out with an event id attached. mx_crypto_verify_device_map() now reports every (user_id, device_id) it was handed on a "seen" attribute, verified or not, and the guard measures against that with our own device removed. A member who has no devices at all still sends: there is genuinely nobody to share a key with, and the event belongs in the room for whatever device they log in later. Both halves are tested, because the difference between them is the whole point. And an explicit recipients = list() is refused. Supplying recipients skips discovery and every guard along with it, so the same unreadable event went out for a caller who did their own lookup and came back with nothing. This is the case that escaped the last round: the tests covered devices skipped for want of a one-time key, which happens after verification, and not devices dropped during it. 160 assertions, three mutations, all caught. --- DESCRIPTION | 2 +- NEWS.md | 10 ++- R/transport.R | 56 ++++++++++++++--- inst/tinytest/test_transport.R | 110 +++++++++++++++++++++++++++++++++ man/mx_send_encrypted.Rd | 6 +- 5 files changed, 171 insertions(+), 13 deletions(-) diff --git a/DESCRIPTION b/DESCRIPTION index 6eef878..6870c29 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,7 +1,7 @@ Package: mx.client Type: Package Title: Stateful Matrix Client Helpers -Version: 0.2.0.1 +Version: 0.2.0.2 Date: 2026-08-04 Authors@R: c( person("Troy", "Hernandez", role = c("aut", "cre"), diff --git a/NEWS.md b/NEWS.md index 6cf3f07..bcc5a8b 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,4 +1,4 @@ -# mx.client 0.2.0.1 +# mx.client 0.2.0.2 ## Security @@ -16,6 +16,14 @@ event id for a message every recipient will fail to decrypt. Reaching zero now aborts. A room with no other devices at all is unaffected: that is a room of one, and sending to it is fine. + The guard counts what the homeserver named, not what survived + verification: counting survivors made a room whose only other device + is malformed indistinguishable from a room with no other device, and + only the second is safe to send to. `mx_crypto_verify_device_map()` + reports every `(user_id, device_id)` it was given on a `seen` + attribute so that distinction is available at all. An explicit + `recipients = list()` is refused too, since supplying recipients skips + discovery and every guard with it. * **HIGH**: `/keys/query` and `/keys/claim` answer 200 with a `failures` map when a server could not be reached, returning whatever they did diff --git a/R/transport.R b/R/transport.R index 0378d98..5bc0351 100644 --- a/R/transport.R +++ b/R/transport.R @@ -116,11 +116,19 @@ mx_crypto_report_failures <- function(failures, what, strict = FALSE) { # # mxc_verify_device_keys() raises on failure rather than returning FALSE, # and returns the keys it actually checked -- use those, not the raw map. +# +# Every (user_id, device_id) the homeserver named comes back on the +# "seen" attribute, verified or not. Without it a caller cannot tell a +# user with no devices from a user whose every device was dropped here, +# and those need different answers: the first is nobody to encrypt to, +# the second is everybody unreachable. mx_crypto_verify_device_map <- function(device_keys_map) { out <- list() + seen <- list() for (uid in names(device_keys_map %||% list())) { devs <- device_keys_map[[uid]] for (dev in names(devs %||% list())) { + seen[[length(seen) + 1L]] <- c(user_id = uid, device_id = dev) keys <- tryCatch( mx.crypto::mxc_verify_device_keys(devs[[dev]], uid, dev), error = function(e) { @@ -135,6 +143,7 @@ mx_crypto_verify_device_map <- function(device_keys_map) { curve25519 = keys$curve25519, ed25519 = keys$ed25519) } } + attr(out, "seen") <- seen out } @@ -234,7 +243,11 @@ mx_crypto_verify_claimed_otks <- function(devices, claimed) { #' @param content Named list. Plaintext event content. #' @param store_dir Character. Crypto store directory. #' @param recipients List of recipient devices, or NULL to discover them -#' from \code{member_ids}. +#' from \code{member_ids}. An empty list is an error: an +#' \code{m.room.encrypted} event whose room key was shared with nobody +#' is unreadable by everyone, and returning its event id would report +#' that as a successful send. Discovery finding nobody is allowed, since +#' a room whose other members have no devices is a real room. #' @param member_ids Character vector of room member user ids (used when #' \code{recipients} is NULL). #' @return List with \code{event_id} and the updated \code{sessions}. @@ -256,7 +269,16 @@ mx_send_encrypted <- function(client, account, sessions, room_id, content, # devices unknown, and an unknown device is not an absent one. Read # the successful half of a partial response and a federation blip # becomes a message nobody can read. - devs <- mx_crypto_known_devices(client, member_ids, strict = TRUE) + found <- mx_crypto_known_devices(client, member_ids, strict = TRUE) + # What the homeserver named, before verification dropped any of + # it. Counting the survivors instead made a room where Bob's only + # device is malformed look exactly like a room with no Bob in it: + # nothing left to encrypt to either way, and only one of those is + # a reason to go ahead. + others_seen <- Filter(function(sd) { + !(identical(sd[["user_id"]], client$user_id) && + identical(sd[["device_id"]], client$device_id)) + }, attr(found, "seen") %||% list()) # Skip our own device, and only our own. Device ids are scoped to a # user, not globally unique, so comparing device_id alone dropped # every other account whose device happened to share the name -- @@ -266,7 +288,7 @@ mx_send_encrypted <- function(client, account, sessions, room_id, content, !(identical(d$user_id, client$user_id) && identical(d$device_id, client$device_id)) && !is.null(d$curve25519) && nzchar(d$curve25519) - }, devs) + }, found) need <- Filter(function(d) is.null(sessions$olm[[d$curve25519]]), devs) have <- Filter(function(d) !is.null(sessions$olm[[d$curve25519]]), devs) claimed <- mx_crypto_claim_otks(client, need, strict = TRUE) @@ -284,15 +306,29 @@ mx_send_encrypted <- function(client, account, sessions, room_id, content, # Skipping some devices is the policy above. Skipping all of them # is not: the room key reaches nobody, the m.room.encrypted event # goes out anyway, and the caller is handed an event id for a - # message every recipient will fail to decrypt. A room with no - # other devices at all is different -- that is a room of one, and - # sending to it is fine. - if (length(devs) && !length(recipients)) { - stop("mx.client: every device in ", room_id, " was skipped (", - length(devs), " found, none usable). Refusing to send an ", - "encrypted event whose room key would reach nobody.", + # message every recipient will fail to decrypt. + # + # Measured against what the homeserver named, not against what + # survived verification -- otherwise a room whose one other device + # is malformed is indistinguishable from a room that has no other + # device, and the second of those is fine to send to. A member who + # has no devices at all is fine too: there is genuinely nobody to + # share a key with, and the event still belongs in the room for + # whatever device that member logs in later. + if (length(others_seen) && !length(recipients)) { + stop("mx.client: every other device in ", room_id, + " was skipped (", length(others_seen), + " named by the homeserver, none usable). Refusing to send ", + "an encrypted event whose room key would reach nobody.", call. = FALSE) } + } else if (!length(recipients)) { + # An explicit empty list is the same unreadable event, arrived at + # by a caller who did their own discovery. Nothing above runs for + # them, so say it here rather than let it through. + stop("mx.client: recipients is empty. Refusing to send an encrypted ", + "event whose room key would reach nobody; pass recipients = ", + "NULL with member_ids to have them discovered.", call. = FALSE) } out <- mx_crypto_encrypt_for_devices(account, sessions, room_id, diff --git a/inst/tinytest/test_transport.R b/inst/tinytest/test_transport.R index f19bd9d..67a71e5 100644 --- a/inst/tinytest/test_transport.R +++ b/inst/tinytest/test_transport.R @@ -269,3 +269,113 @@ local({ expect_identical(posted, 0L) unlink(store, recursive = TRUE) }) + +bob_acct <- mx.crypto::mxc_account_new() + +# ---- An unverifiable device is not an absent one ---- +# The zero-recipient guard used to count what survived verification, so a +# room whose only other device is malformed looked exactly like a room +# with no other device -- nothing left to encrypt to either way, and only +# one of those is a reason to go ahead. +# +# mx_crypto_verify_device_map() reports every (user_id, device_id) the +# homeserver named on a "seen" attribute, verified or not. +local({ + # A device whose signatures block has been stripped: named by the + # homeserver, and not verifiable. + bad <- mx_crypto_device_keys(bob_acct, "@bob:example.org", "BOBDEV") + bad$signatures <- NULL + map <- list(`@bob:example.org` = stats::setNames(list(bad), "BOBDEV")) + devs <- suppressWarnings(mx.client:::mx_crypto_verify_device_map(map)) + expect_equal(length(devs), 0L) + seen <- attr(devs, "seen") + expect_equal(length(seen), 1L) + expect_identical(seen[[1]][["user_id"]], "@bob:example.org") + expect_identical(seen[[1]][["device_id"]], "BOBDEV") +}) +# A genuinely empty map reports nothing seen, which is what lets a room +# of one still send. +expect_equal(length(attr(mx.client:::mx_crypto_verify_device_map(list()), + "seen")), 0L) +# Verified devices are reported too, so the count is of everything named. +expect_equal(length(attr(mx.client:::mx_crypto_verify_device_map(good_map), + "seen")), 1L) + +# The send refuses when the only other device could not be verified. +local({ + me <- list(user_id = "@alice:example.org", device_id = "ALICEDEV", + server = "https://ex.invalid", token = "t") + bad <- mx_crypto_device_keys(bob_acct, "@bob:example.org", "BOBDEV") + bad$signatures <- NULL + posted <- 0L + o1 <- mx.api::mx_keys_query + assignInNamespace("mx_keys_query", function(...) list(device_keys = list( + `@bob:example.org` = stats::setNames(list(bad), "BOBDEV"))), + ns = "mx.api") + o2 <- mx.api::mx_send_event + assignInNamespace("mx_send_event", function(...) { posted <<- posted + 1L + "$e" }, ns = "mx.api") + on.exit({ + assignInNamespace("mx_keys_query", o1, ns = "mx.api") + assignInNamespace("mx_send_event", o2, ns = "mx.api") + }) + acct <- mx.crypto::mxc_account_new() + store <- tempfile("sendstore5") + expect_error(suppressWarnings( + mx_send_encrypted(me, acct, mx_crypto_sessions_new(), "!r:ex", + list(msgtype = "m.text", body = "hi"), store, + member_ids = "@bob:example.org")), + "none usable") + expect_identical(posted, 0L) + unlink(store, recursive = TRUE) +}) + +# A member who has no devices at all is not that case. There is genuinely +# nobody to share a key with, and the event still belongs in the room for +# whatever device that member logs in later. +local({ + me <- list(user_id = "@alice:example.org", device_id = "ALICEDEV", + server = "https://ex.invalid", token = "t") + posted <- 0L + o1 <- mx.api::mx_keys_query + assignInNamespace("mx_keys_query", function(...) list( + device_keys = list(`@bob:example.org` = list())), ns = "mx.api") + o2 <- mx.api::mx_send_event + assignInNamespace("mx_send_event", function(...) { posted <<- posted + 1L + "$nodev" }, ns = "mx.api") + on.exit({ + assignInNamespace("mx_keys_query", o1, ns = "mx.api") + assignInNamespace("mx_send_event", o2, ns = "mx.api") + }) + acct <- mx.crypto::mxc_account_new() + store <- tempfile("sendstore6") + res <- mx_send_encrypted(me, acct, mx_crypto_sessions_new(), "!r:ex", + list(msgtype = "m.text", body = "hi"), store, + member_ids = "@bob:example.org") + expect_identical(res$event_id, "$nodev") + expect_identical(posted, 1L) + unlink(store, recursive = TRUE) +}) + +# ---- An explicit empty recipient list is refused ---- +# Discovery is skipped entirely when a caller supplies recipients, so +# every guard above is skipped with it. The event would be just as +# unreadable. +local({ + me <- list(user_id = "@alice:example.org", device_id = "ALICEDEV", + server = "https://ex.invalid", token = "t") + posted <- 0L + o <- mx.api::mx_send_event + assignInNamespace("mx_send_event", function(...) { posted <<- posted + 1L + "$e" }, ns = "mx.api") + on.exit(assignInNamespace("mx_send_event", o, ns = "mx.api")) + acct <- mx.crypto::mxc_account_new() + store <- tempfile("sendstore7") + expect_error(mx_send_encrypted(me, acct, mx_crypto_sessions_new(), + "!r:ex", list(msgtype = "m.text", + body = "hi"), store, + recipients = list()), + "recipients is empty") + expect_identical(posted, 0L) + unlink(store, recursive = TRUE) +}) diff --git a/man/mx_send_encrypted.Rd b/man/mx_send_encrypted.Rd index 8341bcc..c17115d 100644 --- a/man/mx_send_encrypted.Rd +++ b/man/mx_send_encrypted.Rd @@ -28,7 +28,11 @@ mx_send_encrypted( \item{store_dir}{Character. Crypto store directory.} \item{recipients}{List of recipient devices, or NULL to discover them -from \code{member_ids}.} +from \code{member_ids}. An empty list is an error: an +\code{m.room.encrypted} event whose room key was shared with nobody +is unreadable by everyone, and returning its event id would report +that as a successful send. Discovery finding nobody is allowed, since +a room whose other members have no devices is a real room.} \item{member_ids}{Character vector of room member user ids (used when \code{recipients} is NULL).}