diff --git a/configure.ac b/configure.ac index dece25a2..19954f1f 100644 --- a/configure.ac +++ b/configure.ac @@ -405,6 +405,8 @@ AM_CFLAGS="$AM_CFLAGS -Wimplicit-function-declaration" AM_CFLAGS="$AM_CFLAGS -Wundef" AM_CFLAGS="$AM_CFLAGS -Wfloat-equal -Wredundant-decls" AM_CFLAGS="$AM_CFLAGS -fstack-protector-strong -fno-common" +# stack arrays sized from peer input are a remote stack-exhaustion vector +AM_CFLAGS="$AM_CFLAGS -Werror=vla" AM_CFLAGS="$AM_CFLAGS -std=gnu99 -ggdb3" # GCC-specific warnings (not supported by clang) — test each one diff --git a/meson.build b/meson.build index 9c29537c..18dfe665 100644 --- a/meson.build +++ b/meson.build @@ -58,6 +58,8 @@ cc = meson.get_compiler('c') add_project_arguments([ '-Wno-deprecated-declarations', '-Wno-unused-parameter', + # stack arrays sized from peer input are a remote stack-exhaustion vector + '-Werror=vla', ], language: 'c') analyzer_args = [] diff --git a/src/command/cmd_ac.c b/src/command/cmd_ac.c index 6b24dc22..e8edc040 100644 --- a/src/command/cmd_ac.c +++ b/src/command/cmd_ac.c @@ -1955,7 +1955,10 @@ _cmd_ac_complete_params(ProfWin* window, const char* const input, gboolean previ } size_t len = strlen(input); - char parsed[len + 1]; + auto_char char* parsed = malloc(len + 1); + if (!parsed) { + return NULL; + } size_t i = 0; while (i < len) { if (input[i] == ' ') { diff --git a/src/command/cmd_funcs.c b/src/command/cmd_funcs.c index 77b6dd96..167aa6d6 100644 --- a/src/command/cmd_funcs.c +++ b/src/command/cmd_funcs.c @@ -9644,6 +9644,13 @@ cmd_url_open(ProfWin* window, const char* const command, gchar** args) return TRUE; } + // URLs come from received messages: refuse file:, javascript:, data: etc. + if (g_strcmp0(scheme, "http") != 0 && g_strcmp0(scheme, "https") != 0 + && g_strcmp0(scheme, "aesgcm") != 0) { + cons_show_error("URL scheme '%s' is not allowed, only http, https and aesgcm URLs can be opened.", scheme); + return TRUE; + } + auto_gchar gchar* cmd_template = prefs_get_string(PREF_URL_OPEN_CMD); if (cmd_template == NULL) { cons_show_error("No default `url open` command found in executables preferences."); @@ -9689,6 +9696,13 @@ cmd_url_save(ProfWin* window, const char* const command, gchar** args) return TRUE; } + // URLs come from received messages: refuse file:, javascript:, data: etc. + if (g_strcmp0(scheme, "http") != 0 && g_strcmp0(scheme, "https") != 0 + && g_strcmp0(scheme, "aesgcm") != 0) { + cons_show_error("URL scheme '%s' is not allowed, only http, https and aesgcm URLs can be saved.", scheme); + return TRUE; + } + auto_gchar gchar* cmd_template = prefs_get_string(PREF_URL_SAVE_CMD); if (cmd_template == NULL && (g_strcmp0(scheme, "http") == 0 || g_strcmp0(scheme, "https") == 0)) { _url_http_method(window, cmd_template, url, path); diff --git a/src/common.c b/src/common.c index 009c6f41..7c06b80e 100644 --- a/src/common.c +++ b/src/common.c @@ -455,6 +455,47 @@ str_xml_sanitize(const char* const str) return g_string_free(sanitized, FALSE); } +static gboolean +_is_neutralized_char(gunichar c) +{ + if (c == 0x09 || c == 0x0A || c == 0x0D) { // tab and newlines are legitimate in a body + return FALSE; + } + if (c < 0x20 || (c >= 0x7F && c <= 0x9F)) { // C0 and C1, incl. ESC: terminal escape sequences + return TRUE; + } + // bidi embeddings, overrides and isolates: they reorder what the user sees. + // LRM/RLM (U+200E/U+200F) are kept, legitimate RTL text needs them. + return (c >= 0x202A && c <= 0x202E) || (c >= 0x2066 && c <= 0x2069); +} + +/** + * Replaces control and bidi-reordering characters in untrusted text with U+FFFD. + * + * Replaced rather than dropped, so the substitution stays visible to the user. + */ +gchar* +str_neutralize_untrusted(const char* const str) +{ + if (str == NULL) { + return NULL; + } + + auto_gchar gchar* valid = g_utf8_make_valid(str, -1); + GString* out = g_string_new_len(NULL, (gssize)strlen(valid)); + + for (const char* curr = valid; *curr != '\0'; curr = g_utf8_next_char(curr)) { + gunichar c = g_utf8_get_char(curr); + if (_is_neutralized_char(c)) { + g_string_append_unichar(out, 0xFFFD); + } else { + g_string_append_unichar(out, c); + } + } + + return g_string_free(out, FALSE); +} + char* release_get_latest(void) { diff --git a/src/common.h b/src/common.h index 7abcb9fd..2f80fc4a 100644 --- a/src/common.h +++ b/src/common.h @@ -161,6 +161,7 @@ gboolean strtoi_range(const char* str, int* saveptr, int min, int max, char** er gsize g_diff_to_gsize(const void* end, const void* start); int utf8_display_len(const char* const str); gchar* str_xml_sanitize(const char* const str); +gchar* str_neutralize_untrusted(const char* const str); gboolean string_matches_one_of(const char* what, const char* is, gboolean is_can_be_null, const char* first, ...) __attribute__((sentinel)); gboolean valid_tls_policy_option(const char* is); diff --git a/src/event/server_events.c b/src/event/server_events.c index 71d7af35..eafb0d56 100644 --- a/src/event/server_events.c +++ b/src/event/server_events.c @@ -1303,4 +1303,13 @@ _clean_incoming_message(ProfMessage* message) { _cut(message, "\u200E"); _cut(message, "\u200F"); + + // runs after decryption, so OTR/PGP/OX plaintext is covered too + if (message->plain) { + auto_gchar gchar* clean = str_neutralize_untrusted(message->plain); + if (g_strcmp0(clean, message->plain) != 0) { + free(message->plain); + message->plain = strdup(clean); // message_free() frees this with free() + } + } } diff --git a/src/omemo/crypto.c b/src/omemo/crypto.c index daca6824..f4948d32 100644 --- a/src/omemo/crypto.c +++ b/src/omemo/crypto.c @@ -104,7 +104,7 @@ omemo_hmac_sha256_final_func(void* hmac_context, signal_buffer** output, void* u { gcry_error_t res; size_t mac_len = 32; - unsigned char out[mac_len]; + unsigned char out[32]; res = gcry_mac_read(hmac_context, out, &mac_len); if (res != GPG_ERR_NO_ERROR) { @@ -473,14 +473,19 @@ aes256gcm_create_secure_fragment(unsigned char* key, unsigned char* nonce) int key_size = OMEMO_AESGCM_KEY_LENGTH; int nonce_size = OMEMO_AESGCM_NONCE_LENGTH; - char* fragment = gcry_malloc_secure((nonce_size + key_size) * 2 + 1); + const size_t fragment_size = (size_t)(nonce_size + key_size) * 2 + 1; + char* fragment = gcry_malloc_secure(fragment_size); + if (fragment == NULL) { + return NULL; + } for (int i = 0; i < nonce_size; i++) { - sprintf(&(fragment[i * 2]), "%02x", nonce[i]); + g_snprintf(&fragment[i * 2], fragment_size - (size_t)i * 2, "%02x", nonce[i]); } for (int i = 0; i < key_size; i++) { - sprintf(&(fragment[(i + nonce_size) * 2]), "%02x", key[i]); + const size_t offset = (size_t)(i + nonce_size) * 2; + g_snprintf(&fragment[offset], fragment_size - offset, "%02x", key[i]); } return fragment; diff --git a/src/omemo/omemo.c b/src/omemo/omemo.c index 38157336..0969bbad 100644 --- a/src/omemo/omemo.c +++ b/src/omemo/omemo.c @@ -452,8 +452,12 @@ omemo_identity_key(unsigned char** output, size_t* length) signal_buffer* buffer = NULL; ec_public_key_serialize(&buffer, ratchet_identity_key_pair_get_public(omemo_ctx.identity_key_pair)); *length = signal_buffer_len(buffer); - *output = malloc(*length); - memcpy(*output, signal_buffer_data(buffer), *length); + // g_malloc aborts instead of returning NULL, but it returns NULL for a zero + // length, which memcpy must not be handed + *output = *length ? g_malloc(*length) : NULL; + if (*output) { + memcpy(*output, signal_buffer_data(buffer), *length); + } signal_buffer_free(buffer); } @@ -472,8 +476,10 @@ omemo_signed_prekey(unsigned char** output, size_t* length) ec_public_key_serialize(&buffer, ec_key_pair_get_public(session_signed_pre_key_get_key_pair(signed_pre_key))); SIGNAL_UNREF(signed_pre_key); *length = signal_buffer_len(buffer); - *output = malloc(*length); - memcpy(*output, signal_buffer_data(buffer), *length); + *output = *length ? g_malloc(*length) : NULL; + if (*output) { + memcpy(*output, signal_buffer_data(buffer), *length); + } signal_buffer_free(buffer); } @@ -489,8 +495,10 @@ omemo_signed_prekey_signature(unsigned char** output, size_t* length) } *length = session_signed_pre_key_get_signature_len(signed_pre_key); - *output = malloc(*length); - memcpy(*output, session_signed_pre_key_get_signature(signed_pre_key), *length); + *output = *length ? g_malloc(*length) : NULL; + if (*output) { + memcpy(*output, session_signed_pre_key_get_signature(signed_pre_key), *length); + } SIGNAL_UNREF(signed_pre_key); } diff --git a/src/pgp/ox.c b/src/pgp/ox.c index 65f7894e..4302ae63 100644 --- a/src/pgp/ox.c +++ b/src/pgp/ox.c @@ -147,13 +147,8 @@ p_ox_gpg_signcrypt(const char* const sender_barejid, const char* const recipient gpgme_set_offline(ctx, 1); gpgme_set_keylist_mode(ctx, GPGME_KEYLIST_MODE_LOCAL); - char* xmpp_jid_me = alloca((strlen(sender_barejid) + 6) * sizeof(char)); - char* xmpp_jid_recipient = alloca((strlen(recipient_barejid) + 6) * sizeof(char)); - - strcpy(xmpp_jid_me, "xmpp:"); - strcpy(xmpp_jid_recipient, "xmpp:"); - strcat(xmpp_jid_me, sender_barejid); - strcat(xmpp_jid_recipient, recipient_barejid); + auto_gchar gchar* xmpp_jid_me = g_strdup_printf("xmpp:%s", sender_barejid); + auto_gchar gchar* xmpp_jid_recipient = g_strdup_printf("xmpp:%s", recipient_barejid); gpgme_signers_clear(ctx); diff --git a/src/plugins/python_api.c b/src/plugins/python_api.c index 494fff96..9e8522f2 100644 --- a/src/plugins/python_api.c +++ b/src/plugins/python_api.c @@ -122,7 +122,7 @@ python_api_register_command(PyObject* self, PyObject* args) if (p_callback && PyCallable_Check(p_callback)) { Py_ssize_t len = PyList_Size(synopsis); - char* c_synopsis[len == 0 ? 0 : len + 1]; + char** c_synopsis = g_malloc_n((size_t)len + 1, sizeof(*c_synopsis)); Py_ssize_t i = 0; for (i = 0; i < len; i++) { PyObject* item = PyList_GetItem(synopsis, i); @@ -132,11 +132,23 @@ python_api_register_command(PyObject* self, PyObject* args) c_synopsis[len] = NULL; Py_ssize_t args_len = PyList_Size(arguments); - char* c_arguments[args_len + 1][2]; + char*(*c_arguments)[2] = g_malloc_n((size_t)args_len + 1, sizeof(*c_arguments)); for (i = 0; i < args_len; i++) { PyObject* item = PyList_GetItem(arguments, i); Py_ssize_t len2 = PyList_Size(item); if (len2 != 2) { + for (Py_ssize_t j = 0; j < len; j++) { + free(c_synopsis[j]); + } + for (Py_ssize_t j = 0; j < i; j++) { + free(c_arguments[j][0]); + free(c_arguments[j][1]); + } + g_free(c_synopsis); + g_free(c_arguments); + free(command_name_str); + free(description_str); + free(plugin_name); Py_RETURN_NONE; } PyObject* arg = PyList_GetItem(item, 0); @@ -152,7 +164,7 @@ python_api_register_command(PyObject* self, PyObject* args) c_arguments[args_len][1] = NULL; len = PyList_Size(examples); - char* c_examples[len == 0 ? 0 : len + 1]; + char** c_examples = g_malloc_n((size_t)len + 1, sizeof(*c_examples)); for (i = 0; i < len; i++) { PyObject* item = PyList_GetItem(examples, i); char* c_item = python_str_or_unicode_to_string(item); @@ -179,6 +191,9 @@ python_api_register_command(PyObject* self, PyObject* args) while (c_examples[i] != NULL) { free(c_examples[i++]); } + g_free(c_synopsis); + g_free(c_arguments); + g_free(c_examples); disable_python_threads(); } @@ -227,7 +242,7 @@ python_api_completer_add(PyObject* self, PyObject* args) log_debug("Autocomplete add %s for %s", key_str, plugin_name); Py_ssize_t len = PyList_Size(items); - char* c_items[len]; + char** c_items = g_malloc_n((size_t)len + 1, sizeof(*c_items)); // + 1 for the NULL terminator below Py_ssize_t i = 0; for (i = 0; i < len; i++) { @@ -244,6 +259,7 @@ python_api_completer_add(PyObject* self, PyObject* args) while (c_items[i] != NULL) { free(c_items[i++]); } + g_free(c_items); disable_python_threads(); free(plugin_name); @@ -267,7 +283,7 @@ python_api_completer_remove(PyObject* self, PyObject* args) log_debug("Autocomplete remove %s for %s", key_str, plugin_name); Py_ssize_t len = PyList_Size(items); - char* c_items[len]; + char** c_items = g_malloc_n((size_t)len + 1, sizeof(*c_items)); // + 1 for the NULL terminator below Py_ssize_t i = 0; for (i = 0; i < len; i++) { @@ -280,6 +296,11 @@ python_api_completer_remove(PyObject* self, PyObject* args) allow_python_threads(); api_completer_remove(plugin_name, key_str, c_items); free(key_str); + i = 0; + while (c_items[i] != NULL) { + free(c_items[i++]); + } + g_free(c_items); disable_python_threads(); free(plugin_name); diff --git a/src/ui/inputwin.c b/src/ui/inputwin.c index 1e371f5f..d986a26e 100644 --- a/src/ui/inputwin.c +++ b/src/ui/inputwin.c @@ -493,7 +493,7 @@ _inp_edited(const wint_t ch) } // printable - char bytes[MB_CUR_MAX + 1]; + char bytes[PROF_MB_CUR_MAX + 1]; // compile-time bound, the locale value is checked at startup size_t utf_len = wcrtomb(bytes, ch, &mbstate); if (utf_len == (size_t)-1) { return 0; diff --git a/src/ui/mucwin.c b/src/ui/mucwin.c index b7944609..cd67f507 100644 --- a/src/ui/mucwin.c +++ b/src/ui/mucwin.c @@ -454,22 +454,10 @@ _mucwin_print_triggers(ProfWin* window, const char* const message, GList* trigge win_appendln_highlight(window, THEME_ROOMTRIGGER, "%s", message); } else { if (first_trigger_pos > 0) { - char message_section[strlen(message) + 1]; - int i = 0; - while (i < first_trigger_pos) { - message_section[i] = message[i]; - i++; - } - message_section[i] = '\0'; + auto_gchar gchar* message_section = g_strndup(message, (gsize)first_trigger_pos); win_append_highlight(window, THEME_ROOMTRIGGER, "%s", message_section); } - char trigger_section[first_trigger_len + 1]; - int i = 0; - while (i < first_trigger_len) { - trigger_section[i] = message[first_trigger_pos + i]; - i++; - } - trigger_section[i] = '\0'; + auto_gchar gchar* trigger_section = g_strndup(&message[first_trigger_pos], (gsize)first_trigger_len); if (first_trigger_pos + first_trigger_len < (int)strlen(message)) { win_append_highlight(window, THEME_ROOMTRIGGER_TERM, "%s", trigger_section); diff --git a/src/ui/notifier.c b/src/ui/notifier.c index 9b4c676f..244e6bcc 100644 --- a/src/ui/notifier.c +++ b/src/ui/notifier.c @@ -22,6 +22,7 @@ #include #endif +#include "common.h" #include "log.h" #include "config/preferences.h" #include "ui/ui.h" @@ -122,47 +123,42 @@ _notify(const char* const message, int timeout, const char* const category) static void _notify(const char* const message, int timeout, const char* const category) { - GString* notify_command = g_string_new("terminal-notifier -title \"Profanity\" -message '"); + if (message == NULL) { + return; + } - auto_char char* escaped_single = str_replace(message, "'", "'\\''"); - - if (escaped_single[0] == '<') { - g_string_append(notify_command, "\\<"); - g_string_append(notify_command, &escaped_single[1]); - } else if (escaped_single[0] == '[') { - g_string_append(notify_command, "\\["); - g_string_append(notify_command, &escaped_single[1]); - } else if (escaped_single[0] == '(') { - g_string_append(notify_command, "\\("); - g_string_append(notify_command, &escaped_single[1]); - } else if (escaped_single[0] == '{') { - g_string_append(notify_command, "\\{"); - g_string_append(notify_command, &escaped_single[1]); + // terminal-notifier swallows a leading <, [, ( or { — escape it for its parser + auto_gchar gchar* msg = NULL; + if (message[0] == '<' || message[0] == '[' || message[0] == '(' || message[0] == '{') { + msg = g_strdup_printf("\\%s", message); } else { - g_string_append(notify_command, escaped_single); + msg = g_strdup(message); } - g_string_append(notify_command, "'"); + const gchar* argv[16]; // headroom: an added flag must not silently overrun + guint i = 0; + argv[i++] = "terminal-notifier"; + argv[i++] = "-title"; + argv[i++] = "Profanity"; + argv[i++] = "-message"; + argv[i++] = msg; - char* term_name = getenv("TERM_PROGRAM"); - char* app_id = NULL; + const char* term_name = getenv("TERM_PROGRAM"); if (g_strcmp0(term_name, "Apple_Terminal") == 0) { - app_id = "com.apple.Terminal"; + argv[i++] = "-sender"; + argv[i++] = "com.apple.Terminal"; } else if (g_strcmp0(term_name, "iTerm.app") == 0) { - app_id = "com.googlecode.iterm2"; + argv[i++] = "-sender"; + argv[i++] = "com.googlecode.iterm2"; } + argv[i] = NULL; - if (app_id) { - g_string_append(notify_command, " -sender "); - g_string_append(notify_command, app_id); + GError* err = NULL; + // argv-based spawn: the message is one argument, the shell never sees it + if (!g_spawn_async(NULL, (gchar**)argv, NULL, G_SPAWN_SEARCH_PATH, NULL, NULL, NULL, &err)) { + log_error("Could not send desktop notification: %s", err->message); + g_error_free(err); } - - int res = system(notify_command->str); - if (res == -1) { - log_error("Could not send desktop notification."); - } - - g_string_free(notify_command, TRUE); } #else static void diff --git a/src/ui/window.c b/src/ui/window.c index 4cbbbb71..daf6d770 100644 --- a/src/ui/window.c +++ b/src/ui/window.c @@ -32,6 +32,7 @@ #include #endif +#include "common.h" #include "log.h" #include "config/theme.h" #include "config/preferences.h" @@ -2115,7 +2116,7 @@ _win_print_wrapped(WINDOW* win, const char* const message, int indent, int pad_i _win_indent(win, indent + pad_indent); } - gchar copy[wordi + 1]; + gchar copy[PROF_MB_CUR_MAX + 1]; // one UTF-8 character, not the whole word g_utf8_strncpy(copy, word_ch, 1); waddstr(win, copy); diff --git a/src/xmpp/capabilities.c b/src/xmpp/capabilities.c index 3e788d80..303c09c8 100644 --- a/src/xmpp/capabilities.c +++ b/src/xmpp/capabilities.c @@ -247,14 +247,16 @@ caps_add_by_ver(const char* const ver, EntityCapabilities* caps) if (caps->features) { GSList* curr_feature = caps->features; - int num = g_slist_length(caps->features); - const gchar* features_list[num]; - int curr = 0; + // the feature count comes from a disco#info response, keep it off the stack + guint num = g_slist_length(caps->features); + const gchar** features_list = g_malloc_n(num, sizeof(*features_list)); + guint curr = 0; while (curr_feature) { features_list[curr++] = curr_feature->data; curr_feature = g_slist_next(curr_feature); } g_key_file_set_string_list(cache, ver, "features", features_list, num); + g_free(features_list); } _save_cache(); diff --git a/src/xmpp/iq.c b/src/xmpp/iq.c index 1c9869f5..f1757a24 100644 --- a/src/xmpp/iq.c +++ b/src/xmpp/iq.c @@ -1784,7 +1784,7 @@ _last_activity_get_handler(xmpp_stanza_t* const stanza) if (prefs_get_boolean(PREF_LASTACTIVITY)) { int idls_secs = (int)(ui_get_idle_time() / 1000); char str[50]; - sprintf(str, "%d", idls_secs); + g_snprintf(str, sizeof(str), "%d", idls_secs); xmpp_stanza_t* response = xmpp_iq_new(ctx, STANZA_TYPE_RESULT, xmpp_stanza_get_id(stanza)); xmpp_stanza_set_to(response, from); diff --git a/src/xmpp/message.c b/src/xmpp/message.c index 2de5ea4e..08553d2e 100644 --- a/src/xmpp/message.c +++ b/src/xmpp/message.c @@ -1562,6 +1562,20 @@ _handle_mam(xmpp_stanza_t* const stanza) const char* result_id = xmpp_stanza_get_id(result); + // XEP-0359 §6 gate, same as for live : only keep the archive + // id if the archive's owner announces stable ids + if (result_id) { + const char* archive = by ? by : from; + if (archive == NULL) { + const Jid* myjid = connection_get_jid(); + archive = myjid ? myjid->barejid : NULL; + } + if (!_stanza_id_by_trusted(archive)) { + log_debug("MAM result id dropped: archive %s does not announce XEP-0359 support", STR_MAYBE_NULL(archive)); + result_id = NULL; + } + } + GDateTime* timestamp = stanza_get_delay_from(forwarded, NULL); xmpp_stanza_t* message_stanza = xmpp_stanza_get_child_by_ns(forwarded, "jabber:client"); @@ -1658,7 +1672,7 @@ _ox_openpgp_signcrypt(xmpp_ctx_t* ctx, const char* const to, const char* const t // build rpad int randnr = (rand() % 100) + 1; - char rpad_data[randnr]; + char rpad_data[101]; for (int i = 0; i < randnr; i++) { int rchar = (rand() % 52) + 65; rpad_data[i] = (char)rchar; @@ -1747,6 +1761,10 @@ _should_ignore_based_on_silence(xmpp_stanza_t* const stanza) if (prefs_get_boolean(PREF_SILENCE_NON_ROSTER)) { const char* const from = xmpp_stanza_get_from(stanza); auto_jid Jid* from_jid = jid_create(from); + if (from_jid == NULL) { // no usable sender: treat as non-roster and ignore + log_debug("[Silence] Ignoring message with missing or invalid from attribute"); + return TRUE; + } PContact contact = roster_get_contact(from_jid->barejid); if (!contact) { log_debug("[Silence] Ignoring message from: %s", from); diff --git a/src/xmpp/omemo.c b/src/xmpp/omemo.c index 840deba5..31194f39 100644 --- a/src/xmpp/omemo.c +++ b/src/xmpp/omemo.c @@ -130,9 +130,9 @@ omemo_bundle_publish(gboolean first) iq_send_stanza(iq); xmpp_stanza_release(iq); - free(identity_key); - free(signed_prekey); - free(signed_prekey_signature); + g_free(identity_key); + g_free(signed_prekey); + g_free(signed_prekey_signature); free(id); } diff --git a/src/xmpp/presence.c b/src/xmpp/presence.c index 8ab4625d..9f5378bc 100644 --- a/src/xmpp/presence.c +++ b/src/xmpp/presence.c @@ -395,6 +395,10 @@ _presence_error_handler(xmpp_stanza_t* const stanza) } auto_jid Jid* fulljid = jid_create(from); + if (fulljid == NULL) { + log_warning("MUC error presence received with missing or invalid from attribute"); + return; + } log_info("Error joining room: %s, reason: %s", fulljid->barejid, error_cond); if (muc_active(fulljid->barejid)) { muc_leave(fulljid->barejid); @@ -451,6 +455,9 @@ _unsubscribed_handler(xmpp_stanza_t* const stanza) log_debug("Unsubscribed presence handler fired for %s", from); auto_jid Jid* from_jid = jid_create(from); + if (from_jid == NULL) { + return; + } sv_ev_subscription(from_jid->barejid, PRESENCE_UNSUBSCRIBED); autocomplete_remove(sub_requests_ac, from_jid->barejid); } @@ -466,6 +473,9 @@ _subscribed_handler(xmpp_stanza_t* const stanza) log_debug("Subscribed presence handler fired for %s", from); auto_jid Jid* from_jid = jid_create(from); + if (from_jid == NULL) { + return; + } sv_ev_subscription(from_jid->barejid, PRESENCE_SUBSCRIBED); autocomplete_remove(sub_requests_ac, from_jid->barejid); } @@ -476,6 +486,7 @@ _subscribe_handler(xmpp_stanza_t* const stanza) const char* from = xmpp_stanza_get_from(stanza); if (!from) { log_warning("Subscribe presence handler received with no from attribute"); + return; } log_debug("Subscribe presence handler fired for %s", from); @@ -728,6 +739,9 @@ _muc_user_self_handler(xmpp_stanza_t* stanza) { const char* from = xmpp_stanza_get_from(stanza); auto_jid Jid* from_jid = jid_create(from); + if (from_jid == NULL) { // caller validates, but don't rely on it + return; + } log_debug("Room self presence received from %s", from_jid->fulljid); @@ -810,6 +824,9 @@ _muc_user_occupant_handler(xmpp_stanza_t* stanza) { const char* from = xmpp_stanza_get_from(stanza); auto_jid Jid* from_jid = jid_create(from); + if (from_jid == NULL) { // caller validates, but don't rely on it + return; + } log_debug("Room presence received from %s", from_jid->fulljid); diff --git a/tests/functionaltests/functionaltests.c b/tests/functionaltests/functionaltests.c index da68145e..d7629868 100644 --- a/tests/functionaltests/functionaltests.c +++ b/tests/functionaltests/functionaltests.c @@ -238,6 +238,8 @@ main(int argc, char* argv[]) PROF_FUNC_TEST(presence_keeps_status), PROF_FUNC_TEST(presence_received), PROF_FUNC_TEST(presence_missing_resource_defaults), + PROF_FUNC_TEST(presence_error_invalid_from_no_crash), + PROF_FUNC_TEST(presence_subscription_invalid_from_no_crash), /* Disconnect - clean session termination */ PROF_FUNC_TEST(disconnect_ends_session), @@ -261,6 +263,7 @@ main(int argc, char* argv[]) PROF_FUNC_TEST(message_send), PROF_FUNC_TEST(message_receive_console), PROF_FUNC_TEST(message_receive_chatwin), + PROF_FUNC_TEST(message_invalid_from_silence_no_crash), /* XEP-0359 disco gate for stanza-id trust */ PROF_FUNC_TEST(stanza_id_dedup_fires_when_server_announces_sid0), diff --git a/tests/functionaltests/test_message.c b/tests/functionaltests/test_message.c index 402144ac..e27a3177 100644 --- a/tests/functionaltests/test_message.c +++ b/tests/functionaltests/test_message.c @@ -123,3 +123,29 @@ stanza_id_not_trusted_when_server_does_not_announce_sid0(void **state) assert_false(prof_output_exact("Got a message with duplicate (server-generated) stanza-id")); prof_timeout_reset(); } + +/* Regression test for issue #148 (REQ-INP-01): with silence.non-roster + * enabled the incoming-message filter dereferenced jid_create() without a + * NULL check, so a message with an invalid 'from' crashed the client. */ +void +message_invalid_from_silence_no_crash(void **state) +{ + prof_connect(); + + prof_input("/silence on"); + assert_true(prof_output_exact("Block all messages from JIDs that are not in the roster enabled.")); + + stbbr_send( + "" + "should be dropped, not crash" + "" + ); + + /* client is still alive: a roster contact's message comes through */ + stbbr_send( + "" + "still alive" + "" + ); + assert_true(prof_output_exact("<< chat message: Buddy1/mobile (win 2)")); +} diff --git a/tests/functionaltests/test_message.h b/tests/functionaltests/test_message.h index 924ad914..1d179918 100644 --- a/tests/functionaltests/test_message.h +++ b/tests/functionaltests/test_message.h @@ -3,3 +3,4 @@ void message_receive_console(void **state); void message_receive_chatwin(void **state); void stanza_id_dedup_fires_when_server_announces_sid0(void **state); void stanza_id_not_trusted_when_server_does_not_announce_sid0(void **state); +void message_invalid_from_silence_no_crash(void **state); diff --git a/tests/functionaltests/test_presence.c b/tests/functionaltests/test_presence.c index 8cb05a36..6e417e6b 100644 --- a/tests/functionaltests/test_presence.c +++ b/tests/functionaltests/test_presence.c @@ -289,3 +289,53 @@ presence_missing_resource_defaults(void **state) assert_true(prof_output_exact("__prof_default (15), online")); } + +/* Regression tests for issue #148 (REQ-INP-01): presence handlers used to + * dereference jid_create() results without a NULL check, so a stanza with a + * missing or invalid 'from' crashed the client. */ +void +presence_error_invalid_from_no_crash(void **state) +{ + prof_connect(); + + /* MUC join error with an invalid 'from' */ + stbbr_send( + "" + "" + "" + "" + ); + + /* same, with no 'from' at all */ + stbbr_send( + "" + "" + "" + "" + ); + + /* client is still alive and processing stanzas */ + stbbr_send( + "" + "still alive" + "" + ); + assert_true(prof_output_exact("Buddy1 (mobile) is online, \"still alive\"")); +} + +void +presence_subscription_invalid_from_no_crash(void **state) +{ + prof_connect(); + + stbbr_send(""); + stbbr_send(""); + stbbr_send(""); + + stbbr_send( + "" + "still alive" + "" + ); + assert_true(prof_output_exact("Buddy1 (mobile) is online, \"still alive\"")); +} diff --git a/tests/functionaltests/test_presence.h b/tests/functionaltests/test_presence.h index 107fa794..3648bd27 100644 --- a/tests/functionaltests/test_presence.h +++ b/tests/functionaltests/test_presence.h @@ -13,3 +13,5 @@ void presence_includes_priority(void **state); void presence_keeps_status(void **state); void presence_received(void **state); void presence_missing_resource_defaults(void **state); +void presence_error_invalid_from_no_crash(void **state); +void presence_subscription_invalid_from_no_crash(void **state); diff --git a/tests/unittests/test_common.c b/tests/unittests/test_common.c index 7e876ecf..578dc5ff 100644 --- a/tests/unittests/test_common.c +++ b/tests/unittests/test_common.c @@ -1384,3 +1384,43 @@ str_xml_sanitize__strips_illegal_characters(void** state) assert_string_equal("UTF-8: üñîçøðé and more", res5); g_free(res5); } + +void +neutralize_untrusted_keeps_plain_text(void** state) +{ + gchar* result = str_neutralize_untrusted("hello world\ttabbed\nnewline"); + assert_string_equal("hello world\ttabbed\nnewline", result); + g_free(result); +} + +void +neutralize_untrusted_replaces_escape_sequence(void** state) +{ + gchar* result = str_neutralize_untrusted("safe\x1b[31mred"); + assert_string_equal("safe\xef\xbf\xbd[31mred", result); + g_free(result); +} + +void +neutralize_untrusted_replaces_bidi_override(void** state) +{ + // U+202E RIGHT-TO-LEFT OVERRIDE + gchar* result = str_neutralize_untrusted("file\xe2\x80\xaegnp.exe"); + assert_string_equal("file\xef\xbf\xbdgnp.exe", result); + g_free(result); +} + +void +neutralize_untrusted_keeps_rtl_marks(void** state) +{ + // U+200F RIGHT-TO-LEFT MARK is legitimate in RTL text + gchar* result = str_neutralize_untrusted("\xe2\x80\x8fשלום"); + assert_string_equal("\xe2\x80\x8fשלום", result); + g_free(result); +} + +void +neutralize_untrusted_handles_null(void** state) +{ + assert_null(str_neutralize_untrusted(NULL)); +} diff --git a/tests/unittests/test_common.h b/tests/unittests/test_common.h index 277a7cdb..8ad5eac1 100644 --- a/tests/unittests/test_common.h +++ b/tests/unittests/test_common.h @@ -66,3 +66,8 @@ void release_is_new__tests__various(void** state); void str_xml_sanitize__strips_illegal_characters(void** state); #endif +void neutralize_untrusted_keeps_plain_text(void** state); +void neutralize_untrusted_replaces_escape_sequence(void** state); +void neutralize_untrusted_replaces_bidi_override(void** state); +void neutralize_untrusted_keeps_rtl_marks(void** state); +void neutralize_untrusted_handles_null(void** state); diff --git a/tests/unittests/unittests.c b/tests/unittests/unittests.c index cf01788e..0598a862 100644 --- a/tests/unittests/unittests.c +++ b/tests/unittests/unittests.c @@ -183,6 +183,11 @@ main(int argc, char* argv[]) cmocka_unit_test(jid_is_valid_user_jid__is__true_for_at_in_resource), cmocka_unit_test(jid_is_valid_user_jid__is__false_for_domain_jid), cmocka_unit_test(jid_is_valid_user_jid__is__false_for_invalid_jid), + cmocka_unit_test(jid_is_valid__boundary__localpart_length), + cmocka_unit_test(jid_is_valid__boundary__domainpart_length), + cmocka_unit_test(jid_is_valid__boundary__resourcepart_length), + cmocka_unit_test(jid_is_valid__boundary__total_length), + cmocka_unit_test(jid_is_valid__is__false_for_invalid_utf8), cmocka_unit_test(jid_is_valid__is__true_for_valid_jid), cmocka_unit_test(jid_is_valid__is__false_for_invalid_jid), cmocka_unit_test(jid_is_valid__is__false_for_null), @@ -687,6 +692,11 @@ main(int argc, char* argv[]) cmocka_unit_test(get_mentions__tests__various), cmocka_unit_test(release_is_new__tests__various), cmocka_unit_test(str_xml_sanitize__strips_illegal_characters), + cmocka_unit_test(neutralize_untrusted_keeps_plain_text), + cmocka_unit_test(neutralize_untrusted_replaces_escape_sequence), + cmocka_unit_test(neutralize_untrusted_replaces_bidi_override), + cmocka_unit_test(neutralize_untrusted_keeps_rtl_marks), + cmocka_unit_test(neutralize_untrusted_handles_null), cmocka_unit_test_setup_teardown(plugins_get_command_names__returns__no_commands, load_preferences, diff --git a/tests/unittests/xmpp/test_jid.c b/tests/unittests/xmpp/test_jid.c index e2b2ede0..0c731f29 100644 --- a/tests/unittests/xmpp/test_jid.c +++ b/tests/unittests/xmpp/test_jid.c @@ -370,3 +370,75 @@ jid_is_valid__is__false_for_empty_string(void** state) { assert_false(jid_is_valid("")); } + +/* RFC 6122 size limits: 1023 bytes per part, 3071 for the full JID */ +void +jid_is_valid__boundary__localpart_length(void** state) +{ + gchar* local_ok = g_strnfill(1023, 'a'); + gchar* jid_ok = g_strdup_printf("%s@domain", local_ok); + assert_true(jid_is_valid(jid_ok)); + + gchar* local_over = g_strnfill(1024, 'a'); + gchar* jid_over = g_strdup_printf("%s@domain", local_over); + assert_false(jid_is_valid(jid_over)); + + g_free(local_ok); + g_free(jid_ok); + g_free(local_over); + g_free(jid_over); +} + +void +jid_is_valid__boundary__domainpart_length(void** state) +{ + gchar* domain_ok = g_strnfill(1023, 'd'); + assert_true(jid_is_valid(domain_ok)); + + gchar* domain_over = g_strnfill(1024, 'd'); + assert_false(jid_is_valid(domain_over)); + + g_free(domain_ok); + g_free(domain_over); +} + +void +jid_is_valid__boundary__resourcepart_length(void** state) +{ + gchar* res_ok = g_strnfill(1023, 'r'); + gchar* jid_ok = g_strdup_printf("user@domain/%s", res_ok); + assert_true(jid_is_valid(jid_ok)); + + gchar* res_over = g_strnfill(1024, 'r'); + gchar* jid_over = g_strdup_printf("user@domain/%s", res_over); + assert_false(jid_is_valid(jid_over)); + + g_free(res_ok); + g_free(jid_ok); + g_free(res_over); + g_free(jid_over); +} + +void +jid_is_valid__boundary__total_length(void** state) +{ + // 1023 + '@' + 1023 + '/' + 1023 = 3071, the largest legal JID + gchar* local = g_strnfill(1023, 'a'); + gchar* domain = g_strnfill(1023, 'd'); + gchar* res = g_strnfill(1023, 'r'); + gchar* jid_max = g_strdup_printf("%s@%s/%s", local, domain, res); + assert_true(jid_is_valid(jid_max)); + + g_free(local); + g_free(domain); + g_free(res); + g_free(jid_max); +} + +void +jid_is_valid__is__false_for_invalid_utf8(void** state) +{ + assert_false(jid_is_valid("user\xff\xfe@domain")); + assert_false(jid_is_valid("us\xc3@domain")); // truncated multi-byte sequence + assert_false(jid_is_valid("user@domain/res\x80")); // stray continuation byte +} diff --git a/tests/unittests/xmpp/test_jid.h b/tests/unittests/xmpp/test_jid.h index 7543f903..b8aaa9e4 100644 --- a/tests/unittests/xmpp/test_jid.h +++ b/tests/unittests/xmpp/test_jid.h @@ -41,3 +41,8 @@ void jid_is_valid__is__false_for_null(void** state); void jid_is_valid__is__false_for_empty_string(void** state); #endif +void jid_is_valid__boundary__localpart_length(void** state); +void jid_is_valid__boundary__domainpart_length(void** state); +void jid_is_valid__boundary__resourcepart_length(void** state); +void jid_is_valid__boundary__total_length(void** state); +void jid_is_valid__is__false_for_invalid_utf8(void** state);