From 6d4fcb8fd597c39ba0b7f404e64f500b11b75e1c Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 2 Aug 2026 17:49:48 +0200 Subject: fix: enforce !-free cidPrefix invariant at both concatenation sites The cid: namespacing scheme (cid:!) is only unambiguous because the prefix half is guaranteed free of '!': the first '!' in the result is always the separator, so an attacker-controlled Content-ID containing '!' only extends the id half rather than colliding with a different prefix. That invariant previously existed only as a comment. Add Q_ASSERT_X at both independent call sites that perform this concatenation (CidSchemeHandler::namespacedKey and HtmlBuilder::namespaceCids) so a future prefix generator that violates it traps in debug builds, per Task 5's precedent of not letting one unit's correctness depend silently on another's future behaviour. Since Q_ASSERT compiles out in release, pin the property that actually matters release builds too test: distinct (prefix, id) pairs across a documented "m" prefix set and hostile Content-IDs (containing '!', percent- encoded '!', empty, leading/trailing '!') never collide, and the key always splits at its first '!' back to the exact original prefix. --- src/cidschemehandler.h | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) (limited to 'src/cidschemehandler.h') diff --git a/src/cidschemehandler.h b/src/cidschemehandler.h index 56e6ea1..1231fc6 100644 --- a/src/cidschemehandler.h +++ b/src/cidschemehandler.h @@ -20,8 +20,27 @@ public: void setParts(const QHash &parts) { m_parts = parts; } /// Builds the namespaced key HtmlBuilder's rewritten URLs will request. + /// + /// The '!' separator disambiguates a hostile Content-ID from the prefix + /// only because prefix is guaranteed '!'-free: the FIRST '!' in the + /// result is always the separator, so an attacker-controlled contentId + /// containing '!' (even several) cannot make one message's key collide + /// with another's, it only extends the id half after that first '!'. + /// This is asserted here rather than merely documented, since two call + /// sites (this one and HtmlBuilder::namespaceCids) perform the same + /// concatenation independently and neither should trust the other to + /// have checked it. Q_ASSERT is compiled out in release builds; the + /// property that matters there (distinct pairs never collide, and the + /// key always splits at its first '!' back to the original prefix) is + /// pinned by a test instead, since it holds unconditionally regardless + /// of whether this assertion fires. static QString namespacedKey(const QString &prefix, const QString &contentId) - { return prefix + QLatin1Char('!') + contentId; } + { + Q_ASSERT_X(!prefix.contains(QLatin1Char('!')), "CidSchemeHandler::namespacedKey", + "cidPrefix must never contain '!': it is the separator, and a " + "prefix containing one would make the split ambiguous"); + return prefix + QLatin1Char('!') + contentId; + } void requestStarted(QWebEngineUrlRequestJob *job) override; -- cgit v1.2.3