From c0d5ebffb249718153a50f7a7b7806e36fe2aaf3 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 06:27:27 +0000 Subject: [PATCH] fix(portduino): fail closed when markup escaping returns NULL escapedNotificationBody() returned its input unescaped if g_markup_escape_text() gave back NULL, which would hand libnotify the exact attacker-controlled string the function exists to neutralize. The branch is unreachable for the input we pass - already sanitized to valid UTF-8, and GLib documents no NULL return for it - but an error path whose fallback is the unsafe action is the wrong shape for a helper on this boundary. Drop the body instead. Also note in the doc comment why running this on the caller's thread does not weaken the rule that the worker owns every libnotify call: it is a pure GLib string function that touches no libnotify or DBus state. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013EZstXBJtzRyBmUD1h2FLs --- src/modules/ExternalNotificationModule.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/modules/ExternalNotificationModule.cpp b/src/modules/ExternalNotificationModule.cpp index 5aee852a48..a1b13c070f 100644 --- a/src/modules/ExternalNotificationModule.cpp +++ b/src/modules/ExternalNotificationModule.cpp @@ -682,11 +682,19 @@ static std::string sanitizedMeshText(const char *src, size_t len) /// escaping is unconditional: a server without body-markup renders the entities literally, which is /// cosmetic, while failing to escape one that has it is not, and mesh text carries no markup worth /// preserving. Only the body needs this; the spec gives the summary no markup. +/// +/// Runs on the caller's thread rather than the worker's. That does not weaken the rule that the +/// worker owns every libnotify call: this is a pure GLib string function, touching no libnotify or +/// DBus state, and GLib has been thread-safe since 2.32. static std::string escapedNotificationBody(const std::string &text) { gchar *escaped = g_markup_escape_text(text.data(), (gssize)text.size()); - if (!escaped) - return text; + if (!escaped) { + // Fail closed. Returning the input here would hand libnotify the one string this function + // exists to neutralize, so drop the body instead - it cannot happen for the already + // sanitized input we pass, and if it ever does, losing a body beats injecting one. + return "[unprintable]"; + } std::string out(escaped); g_free(escaped); return out;