From b09a44173e8671cbf25aff7db73b34f947ed49cf 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 ++++++++++++++++- src/htmlbuilder.cpp | 13 +++++++++++ src/htmlbuilder.h | 9 ++++++++ tests/test_htmlbuilder.cpp | 56 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 98 insertions(+), 1 deletion(-) 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; diff --git a/src/htmlbuilder.cpp b/src/htmlbuilder.cpp index 1830383..89501ea 100644 --- a/src/htmlbuilder.cpp +++ b/src/htmlbuilder.cpp @@ -56,6 +56,19 @@ QString HtmlBuilder::namespaceCids(const QString &html, const QString &prefix) if (prefix.isEmpty()) return html; + // The '!' separator is only unambiguous if prefix itself never contains + // one: the FIRST '!' in "cid:!" is always taken as the + // separator, so a hostile Content-ID containing '!' only extends the id + // half, never collides with a different prefix. This concatenation is + // performed independently in two places (here and + // CidSchemeHandler::namespacedKey); neither trusts the other to have + // checked it, so both assert it directly. Q_ASSERT is compiled out in + // release builds — the property that matters there is pinned by + // TestHtmlBuilder::namespacedKeyRejectsPrefixContainingSeparator instead. + Q_ASSERT_X(!prefix.contains(QLatin1Char('!')), "HtmlBuilder::namespaceCids", + "cidPrefix must never contain '!': it is the separator, and a " + "prefix containing one would make the split ambiguous"); + // This runs on the sender's raw, unescaped HTML markup (not on text that // has been through toHtmlEscaped()), so no double-escaping happens here; // it is purely a URL rewrite over the existing markup. diff --git a/src/htmlbuilder.h b/src/htmlbuilder.h index ac24e28..ff62c69 100644 --- a/src/htmlbuilder.h +++ b/src/htmlbuilder.h @@ -16,6 +16,15 @@ struct ThreadRenderItem /// use the same Content-ID (cid:logo@example.org), which would collide in /// a single document, so every reference is rewritten to /// cid:!. + /// + /// Requirement on whatever generates this value: it must never contain + /// '!'. The separator that makes cid:! unambiguous is the + /// FIRST '!' in the namespaced string; that only holds if the prefix + /// half is guaranteed free of the character, since the id half is + /// attacker-controlled and may legitimately contain '!' itself. The + /// documented "m" form (e.g. "m0", "m1") satisfies this. Enforced + /// with Q_ASSERT at both places that perform this concatenation + /// (HtmlBuilder::namespaceCids and CidSchemeHandler::namespacedKey). QString cidPrefix; }; diff --git a/tests/test_htmlbuilder.cpp b/tests/test_htmlbuilder.cpp index 38699f4..86a40cd 100644 --- a/tests/test_htmlbuilder.cpp +++ b/tests/test_htmlbuilder.cpp @@ -1,4 +1,6 @@ +#include #include +#include "cidschemehandler.h" #include "htmlbuilder.h" class TestHtmlBuilder : public QObject @@ -21,6 +23,7 @@ private slots: void namespacesCidInStyleBlock(); void namespacesMultipleCidRefsOnOneLine(); void namespacesWhitespaceAroundEquals(); + void namespacedKeyRejectsPrefixContainingSeparator(); }; void TestHtmlBuilder::escapesPlainText() @@ -202,5 +205,58 @@ void TestHtmlBuilder::namespacesWhitespaceAroundEquals() QVERIFY(html.contains(QStringLiteral("cid:m0!logo@example.org"))); } +void TestHtmlBuilder::namespacedKeyRejectsPrefixContainingSeparator() +{ + // The property that actually matters, and holds even in release builds + // where Q_ASSERT is compiled out: distinct (prefix, id) pairs, drawn from + // the documented "m" generator form plus hostile Content-IDs + // (including ones containing '!', a percent-encoded '!', and several + // '!'s), always produce distinct keys, and the key always splits at its + // FIRST '!' back to exactly the original prefix. That only holds because + // prefixes generated in the "m" form are themselves '!'-free; the + // attacker only ever controls the id half, which sits after the first + // (and only guaranteed-separator) '!'. + const QStringList prefixes = { QStringLiteral("m0"), QStringLiteral("m1"), + QStringLiteral("m2"), QStringLiteral("m10") }; + const QStringList hostileIds = { + QStringLiteral("logo@example.org"), + QStringLiteral("a!b@x"), + QStringLiteral("a!b!c@x"), + QStringLiteral("%21encoded@x"), + QStringLiteral(""), + QStringLiteral("!leading@x"), + QStringLiteral("trailing!@x"), + }; + + QSet seenKeys; + for (const QString &prefix : prefixes) { + for (const QString &id : hostileIds) { + const QString key = CidSchemeHandler::namespacedKey(prefix, id); + + // Distinctness: no other (prefix, id) pair already produced this + // exact key. + QVERIFY2(!seenKeys.contains(key), + qPrintable(QStringLiteral("collision for key '%1'").arg(key))); + seenKeys.insert(key); + + // Splitting at the FIRST '!' always recovers the original + // prefix, regardless of how many '!' the hostile id contributes. + const qsizetype sep = key.indexOf(QLatin1Char('!')); + QVERIFY(sep != -1); + QCOMPARE(key.left(sep), prefix); + QCOMPARE(key.mid(sep + 1), id); + } + } + + // Same property, exercised through HtmlBuilder::namespaceCids, which + // performs the identical concatenation independently: the resulting URL + // must contain the CidSchemeHandler-computed key verbatim. + const QString html = HtmlBuilder::namespaceCids( + QStringLiteral(""), QStringLiteral("m0")); + const QString expectedKey = CidSchemeHandler::namespacedKey( + QStringLiteral("m0"), QStringLiteral("a!b@x")); + QVERIFY(html.contains(QStringLiteral("cid:%1").arg(expectedKey))); +} + QTEST_MAIN(TestHtmlBuilder) #include "test_htmlbuilder.moc" -- cgit v1.2.3