From 7454096cae63d87c75a676751bb07f5650280cd0 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Fri, 18 Sep 2026 16:04:37 +0200 Subject: fix: sanitise untrusted contact names on insertion A vCard FN is untrusted and ContactStore faithfully decodes `\n` to a real newline, so `Evil\nBcc: x` was inserted raw into a recipient field and flowed through splitRecipients() to MessageBuilder. contactInsertionText() only quoted a name carrying a comma or a double quote, so every other RFC 5322 special (<, >, ;, @) and any control character reached the header unguarded. The name now has control characters and whitespace runs replaced by single spaces, and every non-empty name is quoted, with `\` escaped before `"`. Quoting contains all the specials in one step. The parser is left faithful; this is fixed at the consumer/trust boundary. Tests: a name with a decoded newline inserts no control character and yields one recipient; a name with <, >, ;, @ is quoted and yields one recipient. The three tests that expected an unquoted plain name now expect the quoted form. --- src/composewindow.cpp | 40 ++++++++++++--------- tests/test_composewindow.cpp | 83 ++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 100 insertions(+), 23 deletions(-) diff --git a/src/composewindow.cpp b/src/composewindow.cpp index f62f38f..eb983d6 100644 --- a/src/composewindow.cpp +++ b/src/composewindow.cpp @@ -124,26 +124,34 @@ QStringList splitRecipients(const QString &text) /// The text a completion inserts for \p contact. /// -/// A display name containing a comma must be QUOTED, because the recipient -/// fields are comma-separated and splitRecipients() would otherwise cut the -/// name in half before the message was built. A name containing a double quote -/// is quoted too, with its quotes backslash-escaped: an unquoted `"` in an -/// address header is malformed. The backslash is escaped FIRST, or escaping the -/// quote would then double the backslashes it just introduced. +/// UNTRUSTED INPUT ARRIVES HERE. A display name comes from a vCard's FN, and +/// ContactStore::unescapeText() faithfully decodes `\n` to a real newline, so a +/// hostile card can carry `Evil\nBcc: x` and be inserted raw into a recipient +/// header. The name is sanitised and then ALWAYS quoted when non-empty, rather +/// than only for a comma or a quote: quoting contains every RFC 5322 special +/// (`<`, `>`, `;`, `@`, spaces, commas) in one step, and escaping `\` before `"` +/// is what makes the quoted form valid. Control characters become single spaces +/// and runs collapse, so a decoded newline cannot forge a header line. QString contactInsertionText(const Contact &contact) { - if (contact.name.isEmpty()) - return contact.email; - - if (contact.name.contains(QLatin1Char(',')) - || contact.name.contains(QLatin1Char('"'))) { - QString escaped = contact.name; - escaped.replace(QLatin1Char('\\'), QStringLiteral("\\\\")); - escaped.replace(QLatin1Char('"'), QStringLiteral("\\\"")); - return QStringLiteral("\"%1\" <%2>").arg(escaped, contact.email); + QString name; + name.reserve(contact.name.size()); + for (const QChar c : contact.name) { + if (c.isSpace() || c.category() == QChar::Other_Control) { + if (!name.isEmpty() && !name.endsWith(QLatin1Char(' '))) + name.append(QLatin1Char(' ')); + continue; + } + name.append(c); } + name = name.trimmed(); + + if (name.isEmpty()) + return contact.email; - return QStringLiteral("%1 <%2>").arg(contact.name, contact.email); + name.replace(QLatin1Char('\\'), QStringLiteral("\\\\")); + name.replace(QLatin1Char('"'), QStringLiteral("\\\"")); + return QStringLiteral("\"%1\" <%2>").arg(name, contact.email); } /// The span an accepted completion replaces: the comma-delimited token the diff --git a/tests/test_composewindow.cpp b/tests/test_composewindow.cpp index e2673b7..bedd793 100644 --- a/tests/test_composewindow.cpp +++ b/tests/test_composewindow.cpp @@ -114,6 +114,8 @@ private slots: void completionStillWorksAfterAComma(); void insertingACommaNameQuotesItAndKeepsOneRecipient(); void insertingANameWithAQuoteEscapesIt(); + void insertingANameWithAControlCharacterStripsIt(); + void insertingANameWithHeaderSpecialsQuotesIt(); void anEmptyNameInsertsTheBareAddress(); void noContactsLeavesTheBehaviourUnchanged(); @@ -1222,10 +1224,11 @@ void TestComposeWindow::completionOffersAContactOnTheFirstRecipient() QVERIFY2(contactPopup() && contactPopup()->isVisible(), "typing a contact's name must offer it"); QCOMPARE(contactPopup()->model()->index(0, 0).data(Qt::DisplayRole).toString(), - QStringLiteral("Alice Example ")); + QStringLiteral("\"Alice Example\" ")); acceptFirstPopupRow(); - QCOMPARE(to->text(), QStringLiteral("Alice Example ")); + QCOMPARE(to->text(), + QStringLiteral("\"Alice Example\" ")); } /// The candidate is matched on the ADDRESS as well as the name. A single @@ -1253,10 +1256,11 @@ void TestComposeWindow::completionMatchesTheAddressAsWellAsTheName() QVERIFY2(contactPopup() && contactPopup()->isVisible(), "typing an address must offer the contact"); QCOMPARE(contactPopup()->model()->index(0, 0).data(Qt::DisplayRole).toString(), - QStringLiteral("Alice Example ")); + QStringLiteral("\"Alice Example\" ")); acceptFirstPopupRow(); - QCOMPARE(to->text(), QStringLiteral("Alice Example ")); + QCOMPARE(to->text(), + QStringLiteral("\"Alice Example\" ")); } /// The case this whole task exists for: `setCompleter()` would set the @@ -1286,11 +1290,13 @@ void TestComposeWindow::completionStillWorksAfterAComma() // Bob, not Alice: the prefix is the token after the comma, so Alice must // not be offered any more. QCOMPARE(contactPopup()->model()->index(0, 0).data(Qt::DisplayRole).toString(), - QStringLiteral("Bob Example ")); + QStringLiteral("\"Bob Example\" ")); acceptFirstPopupRow(); - QCOMPARE(to->text(), - QStringLiteral("alice@example.org, Bob Example ")); + QCOMPARE( + to->text(), + QStringLiteral( + "alice@example.org, \"Bob Example\" ")); } /// A display name containing a comma must be QUOTED on insertion, or @@ -1360,6 +1366,69 @@ void TestComposeWindow::insertingANameWithAQuoteEscapesIt() QCOMPARE(message.to.size(), 1); } +/// FN is untrusted and ContactStore faithfully decodes `\n` to a real newline, +/// so a hostile card can try to smuggle a header line through a display name. +/// Nothing the completion inserts may carry a control character. +void TestComposeWindow::insertingANameWithAControlCharacterStripsIt() +{ + const Config config = configWithDrafts(); + + ComposeContext context; + context.kind = ComposeContext::Kind::New; + context.accountKey = QStringLiteral("work"); + + ComposeWindow window(context, config, m_dir->path()); + window.setContacts({ { QStringLiteral("Evil\nBcc: x"), + QStringLiteral("evil@example.org") } }); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *to = window.findChild(QStringLiteral("to")); + QVERIFY(to); + to->setFocus(); + + QTest::keyClicks(to, QStringLiteral("Evil")); + QVERIFY(contactPopup() && contactPopup()->isVisible()); + acceptFirstPopupRow(); + + for (const QChar c : to->text()) + QVERIFY2(c.category() != QChar::Other_Control, + "the inserted name must carry no control character"); + + QCOMPARE(to->text(), + QStringLiteral("\"Evil Bcc: x\" ")); + QCOMPARE(window.currentMessage().to.size(), 1); +} + +/// `<`, `>`, `;` and `@` are RFC 5322 specials. Quoting the whole name keeps +/// every one of them inside the display name instead of in the header grammar. +void TestComposeWindow::insertingANameWithHeaderSpecialsQuotesIt() +{ + const Config config = configWithDrafts(); + + ComposeContext context; + context.kind = ComposeContext::Kind::New; + context.accountKey = QStringLiteral("work"); + + ComposeWindow window(context, config, m_dir->path()); + window.setContacts({ { QStringLiteral("Weird ; @here"), + QStringLiteral("weird@example.org") } }); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *to = window.findChild(QStringLiteral("to")); + QVERIFY(to); + to->setFocus(); + + QTest::keyClicks(to, QStringLiteral("Weird")); + QVERIFY(contactPopup() && contactPopup()->isVisible()); + acceptFirstPopupRow(); + + QCOMPARE(to->text(), + QStringLiteral("\"Weird ; @here\" ")); + QCOMPARE(window.currentMessage().to.size(), 1); +} + /// A card with an address but no name completes on the address alone, with no /// empty angle brackets. void TestComposeWindow::anEmptyNameInsertsTheBareAddress() -- cgit v1.2.3