From 9e0d0ed46664bcfe9bc3197be2d794fc1c7a5142 Mon Sep 17 00:00:00 2001 From: Maximilian Wuttke Date: Mon, 29 Mar 2021 02:47:35 +0200 Subject: [PATCH 1/3] OMEMO: Remove duplicate session initalisation The function `omemo_start_session` was effectively called twice in the `/msg` command: Once in `chatwin_new` and afterwards in `cmd_msg`. I've removed the second call. --- src/command/cmd_funcs.c | 24 ++++++++---------------- src/ui/chatwin.c | 14 ++++++++++++-- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index 8f463a6c..b140de53 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -2151,30 +2151,22 @@ cmd_msg(ProfWin* window, const char* const command, gchar** args) ProfChatWin* chatwin = wins_get_chat(barejid); if (!chatwin) { + // NOTE: This will also start the new OMEMO session + // and send a MAM request. chatwin = chatwin_new(barejid); } ui_focus_win((ProfWin*)chatwin); -#ifdef HAVE_OMEMO - gboolean is_otr_secure = FALSE; - -#ifdef HAVE_LIBOTR - is_otr_secure = otr_is_secure(barejid); -#endif // HAVE_LIBOTR - - if (omemo_automatic_start(barejid) && is_otr_secure) { - win_println(window, THEME_DEFAULT, "!", "Chat could be either OMEMO or OTR encrypted. Use '/omemo start %s' or '/otr start %s' to start a session.", usr, usr); - return TRUE; - } else if (omemo_automatic_start(barejid)) { - omemo_start_session(barejid); - chatwin->is_omemo = TRUE; - } -#endif // HAVE_OMEMO - if (msg) { + // FIXME [OMEMO] We can't be sure whether the + // bundles have already been receieved. Thus, it is + // possible (and probable) that the recipent can't + // encrypt the message. cl_ev_send_msg(chatwin, msg, NULL); } else { #ifdef HAVE_LIBOTR + // Start the OTR session after this (i.e. the + // first) message was sent if (otr_is_secure(barejid)) { chatwin_otr_secured(chatwin, otr_is_trusted(barejid)); } diff --git a/src/ui/chatwin.c b/src/ui/chatwin.c index a52af306..e1ec1968 100644 --- a/src/ui/chatwin.c +++ b/src/ui/chatwin.c @@ -80,12 +80,22 @@ chatwin_new(const char* const barejid) } } + // We start a new OMEMO session if this contact has been configured accordingly. + // However, if OTR is *also* configured, ask the user to choose between OMEMO and OTR. #ifdef HAVE_OMEMO - if (omemo_automatic_start(barejid)) { + gboolean is_otr_secure = FALSE; +#ifdef HAVE_LIBOTR + is_otr_secure = otr_is_secure(barejid); +#endif // HAVE_LIBOTR + if (omemo_automatic_start(barejid) && is_otr_secure) { + win_println(window, THEME_DEFAULT, "!", "This chat could be either OMEMO or OTR encrypted, but not both. " + "Use '/omemo start' or '/otr start' to select the encryption method."); + } else if (omemo_automatic_start(barejid)) { + // Start the OMEMO session omemo_start_session(barejid); chatwin->is_omemo = TRUE; } -#endif +#endif // HAVE_OMEMO if (prefs_get_boolean(PREF_MAM)) { iq_mam_request(chatwin); From e8664e27303e6028bc335e33172d84025ff3fa30 Mon Sep 17 00:00:00 2001 From: Maximilian Wuttke Date: Mon, 29 Mar 2021 21:53:51 +0200 Subject: [PATCH 2/3] OMEMO: Fail if message keys couldn't be encrypted for any recipient device If the message (key) can't be encrypted for any device, sending the message is refused and an informative error message is presented to the user. Also, don't encrypt for the same device, since the OMEMO XEP disallows this. --- src/command/cmd_funcs.c | 15 +++++++-------- src/omemo/omemo.c | 20 ++++++++++++++++++++ 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index b140de53..90c41dfd 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -2151,22 +2151,21 @@ cmd_msg(ProfWin* window, const char* const command, gchar** args) ProfChatWin* chatwin = wins_get_chat(barejid); if (!chatwin) { - // NOTE: This will also start the new OMEMO session - // and send a MAM request. + // NOTE: This will also start the new OMEMO session and send a MAM request. chatwin = chatwin_new(barejid); } ui_focus_win((ProfWin*)chatwin); if (msg) { - // FIXME [OMEMO] We can't be sure whether the - // bundles have already been receieved. Thus, it is - // possible (and probable) that the recipent can't - // encrypt the message. + // NOTE: In case the message is OMEMO encrypted, we can't be sure + // whether the key bundles of the recipient have already been + // received. In the case that *no* bundles have been received yet, + // the message won't be sent, and an error is shown to the user. + // Other cases are not handled here. cl_ev_send_msg(chatwin, msg, NULL); } else { #ifdef HAVE_LIBOTR - // Start the OTR session after this (i.e. the - // first) message was sent + // Start the OTR session after this (i.e. the first) message was sent if (otr_is_secure(barejid)) { chatwin_otr_secured(chatwin, otr_is_trusted(barejid)); } diff --git a/src/omemo/omemo.c b/src/omemo/omemo.c index a552cd57..495842bf 100644 --- a/src/omemo/omemo.c +++ b/src/omemo/omemo.c @@ -705,6 +705,7 @@ omemo_on_message_send(ProfWin* win, const char* const message, gboolean request_ memcpy(key_tag, key, AES128_GCM_KEY_LENGTH); memcpy(key_tag + AES128_GCM_KEY_LENGTH, tag, AES128_GCM_TAG_LENGTH); + // List of barejids of the recipients of this message GList* recipients = NULL; if (muc) { ProfMucWin* mucwin = (ProfMucWin*)win; @@ -727,6 +728,7 @@ omemo_on_message_send(ProfWin* win, const char* const message, gboolean request_ omemo_ctx.identity_key_store.recv = false; + // Encrypt keys for the recipients GList* recipients_iter; for (recipients_iter = recipients; recipients_iter != NULL; recipients_iter = recipients_iter->next) { GList* recipient_device_id = NULL; @@ -772,6 +774,17 @@ omemo_on_message_send(ProfWin* win, const char* const message, gboolean request_ g_list_free_full(recipients, free); + // Don't send the message if no key could be encrypted. + // (Since none of the recipients would be able to read the message.) + if (keys == NULL) { + win_println(win, THEME_ERROR, "!", "This message cannot be decrypted for any recipient.\n" + "You should trust your recipients' device fingerprint(s) using \"/omemo fingerprint trust FINGERPRINT\".\n" + "It could also be that the key bundle of the recipient(s) have not been received. " + "In this case, you could try \"omemo end\", \"omemo start\", and send the message again."); + goto out; + } + + // Encrypt keys for the sender if (!muc) { GList* sender_device_id = g_hash_table_lookup(omemo_ctx.device_list, jid->barejid); for (device_ids_iter = sender_device_id; device_ids_iter != NULL; device_ids_iter = device_ids_iter->next) { @@ -784,6 +797,12 @@ omemo_on_message_send(ProfWin* win, const char* const message, gboolean request_ .device_id = GPOINTER_TO_INT(device_ids_iter->data) }; + // Don't encrypt for this device (according to + // ). + if (address.device_id == omemo_ctx.device_id) { + continue; + } + res = session_cipher_create(&cipher, omemo_ctx.store, &address, omemo_ctx.signal); if (res != 0) { log_error("[OMEMO] cannot create cipher for %s device id %d", address.name, address.device_id); @@ -808,6 +827,7 @@ omemo_on_message_send(ProfWin* win, const char* const message, gboolean request_ } } + // Send the message if (muc) { ProfMucWin* mucwin = (ProfMucWin*)win; assert(mucwin->memcheck == PROFMUCWIN_MEMCHECK); From 025c2fb1e0ed0915c90d4371c2f96ce9f9c8f957 Mon Sep 17 00:00:00 2001 From: Maximilian Wuttke Date: Mon, 29 Mar 2021 22:10:18 +0200 Subject: [PATCH 3/3] Msg sending: don't write to chatwin nor to log if sending failed Currently, only `chat_log_omemo_msg_out` can fail (i.e. return `NULL` instead of a stanza id). In this case, the message is neither printed to the chat window nor added to the log (since it wasn't sent). --- src/event/client_events.c | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/event/client_events.c b/src/event/client_events.c index 6db19463..bf519feb 100644 --- a/src/event/client_events.c +++ b/src/event/client_events.c @@ -134,27 +134,33 @@ cl_ev_send_msg_correct(ProfChatWin* chatwin, const char* const msg, const char* if (chatwin->is_omemo) { #ifdef HAVE_OMEMO char* id = omemo_on_message_send((ProfWin*)chatwin, plugin_msg, request_receipt, FALSE, replace_id); - chat_log_omemo_msg_out(chatwin->barejid, plugin_msg, NULL); - log_database_add_outgoing_chat(id, chatwin->barejid, plugin_msg, replace_id, PROF_MSG_ENC_OMEMO); - chatwin_outgoing_msg(chatwin, plugin_msg, id, PROF_MSG_ENC_OMEMO, request_receipt, replace_id); - free(id); + if (id != NULL) { + chat_log_omemo_msg_out(chatwin->barejid, plugin_msg, NULL); + log_database_add_outgoing_chat(id, chatwin->barejid, plugin_msg, replace_id, PROF_MSG_ENC_OMEMO); + chatwin_outgoing_msg(chatwin, plugin_msg, id, PROF_MSG_ENC_OMEMO, request_receipt, replace_id); + free(id); + } #endif } else if (chatwin->is_ox) { #ifdef HAVE_LIBGPGME // XEP-0373: OpenPGP for XMPP char* id = message_send_chat_ox(chatwin->barejid, plugin_msg, request_receipt, replace_id); - chat_log_pgp_msg_out(chatwin->barejid, plugin_msg, NULL); - log_database_add_outgoing_chat(id, chatwin->barejid, plugin_msg, replace_id, PROF_MSG_ENC_OX); - chatwin_outgoing_msg(chatwin, plugin_msg, id, PROF_MSG_ENC_OX, request_receipt, replace_id); - free(id); + if (id != NULL) { + chat_log_pgp_msg_out(chatwin->barejid, plugin_msg, NULL); + log_database_add_outgoing_chat(id, chatwin->barejid, plugin_msg, replace_id, PROF_MSG_ENC_OX); + chatwin_outgoing_msg(chatwin, plugin_msg, id, PROF_MSG_ENC_OX, request_receipt, replace_id); + free(id); + } #endif } else if (chatwin->pgp_send) { #ifdef HAVE_LIBGPGME char* id = message_send_chat_pgp(chatwin->barejid, plugin_msg, request_receipt, replace_id); - chat_log_pgp_msg_out(chatwin->barejid, plugin_msg, NULL); - log_database_add_outgoing_chat(id, chatwin->barejid, plugin_msg, replace_id, PROF_MSG_ENC_PGP); - chatwin_outgoing_msg(chatwin, plugin_msg, id, PROF_MSG_ENC_PGP, request_receipt, replace_id); - free(id); + if (id != NULL) { + chat_log_pgp_msg_out(chatwin->barejid, plugin_msg, NULL); + log_database_add_outgoing_chat(id, chatwin->barejid, plugin_msg, replace_id, PROF_MSG_ENC_PGP); + chatwin_outgoing_msg(chatwin, plugin_msg, id, PROF_MSG_ENC_PGP, request_receipt, replace_id); + free(id); + } #endif } else { gboolean handled = FALSE;