[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-commits
Subject: KDE/kdepimlibs/kmime
From: Thomas McGuire <mcguire () kde ! org>
Date: 2009-12-26 22:00:21
Message-ID: 1261864821.733460.31912.nullmailer () svn ! kde ! org
[Download RAW message or body]
SVN commit 1066360 by tmcguire:
Prevent spoofing of the From address by using Unicode direction control chars
in the display name.
Make sure to balance the Bidi control characters so that the direction changes
can not flow over to the addrspec.
We balance the characters when parsing the mailbox or addresslist.
Add some tests for that.
Note that this only fixes KMime-based clients, a fix for KMail will follow soon.
M +5 -3 kmime_header_parsing.cpp
M +48 -0 kmime_util.cpp
M +43 -0 kmime_util.h
M +27 -0 tests/kmime_message_test.cpp
M +1 -0 tests/kmime_message_test.h
M +39 -0 tests/kmime_util_test.cpp
M +2 -0 tests/kmime_util_test.h
--- trunk/KDE/kdepimlibs/kmime/kmime_header_parsing.cpp #1066359:1066360
@@ -136,14 +136,14 @@
void Mailbox::setName( const QString &name )
{
- mDisplayName = name;
+ mDisplayName = balanceBidiState( name );
}
void Mailbox::setNameFrom7Bit( const QByteArray &name,
const QByteArray &defaultCharset )
{
QByteArray cs;
- mDisplayName = decodeRFC2047String( name, cs, defaultCharset, false );
+ setName( decodeRFC2047String( name, cs, defaultCharset, false ) );
}
bool Mailbox::hasAddress() const
@@ -1194,7 +1194,9 @@
return false;
}
- result.displayName = maybeDisplayName;
+ // KDE5 TODO: Don't expose displayName as public, but rather add setter for it that
+ // automatically calls balanceBidiState
+ result.displayName = balanceBidiState( maybeDisplayName );
// get obs-mbox-list (may contain empty entries):
scursor++;
--- trunk/KDE/kdepimlibs/kmime/kmime_util.cpp #1066359:1066360
@@ -32,6 +32,7 @@
#include <klocale.h>
#include <kcharsets.h>
#include <kcodecs.h>
+#include <kdebug.h>
#include <QtCore/QList>
#include <QtCore/QString>
@@ -558,4 +559,51 @@
}
}
+KMIME_EXPORT QString balanceBidiState( const QString &input )
+{
+ const int LRO = 0x202D;
+ const int RLO = 0x202E;
+ const int LRE = 0x202A;
+ const int RLE = 0x202B;
+ const int PDF = 0x202C;
+
+ QString result = input;
+
+ int openDirChangers = 0;
+ int numPDFsRemoved = 0;
+ for ( int i = 0; i < input.length(); i++ ) {
+ const ushort &code = input.at( i ).unicode();
+ if ( code == LRO || code == RLO || code == LRE || code == RLE ) {
+ openDirChangers++;
+ }
+ else if ( code == PDF ) {
+ if ( openDirChangers > 0 ) {
+ openDirChangers--;
+ }
+ else {
+ // One PDF too much, remove it
+ kWarning() << "Possible Unicode spoofing (unexpected PDF) detected in" << input;
+ result.remove( i - numPDFsRemoved, 1 );
+ numPDFsRemoved++;
+ }
+ }
+ }
+
+ if ( openDirChangers > 0 ) {
+ kWarning() << "Possible Unicode spoofing detected in" << input;
+
+ // At PDF chars to the end until the correct state is restored.
+ // As a special exception, when encountering quoted strings, place the PDF before
+ // the last quote.
+ for ( int i = openDirChangers; i > 0; i-- ) {
+ if ( result.endsWith( '"' ) )
+ result.insert( result.length() - 1, QChar( PDF ) );
+ else
+ result += QChar( PDF );
+ }
+ }
+
+ return result;
+}
+
} // namespace KMime
--- trunk/KDE/kdepimlibs/kmime/kmime_util.h #1066359:1066360
@@ -258,6 +258,49 @@
*/
KMIME_EXPORT extern void addQuotes( QByteArray &str, bool forceQuotes );
+/**
+ * Makes sure that the bidirectional state at the end of the string is the
+ * same as at the beginning of the string.
+ *
+ * This is useful so that Unicode control characters that can change the text
+ * direction can not spill over to following strings.
+ *
+ * As an example, consider a mailbox in the form "display name" <local@domain.com>.
+ * If the display name here contains unbalanced control characters that change the
+ * text direction, it would also have an effect on the addrspec, which could lead to
+ * spoofing.
+ *
+ * By passing the display name to this function, one can make sure that no change of
+ * the bidi state can spill over to the next strings, in this case the addrspec.
+ *
+ * Example: The string "Hello <RLO>World" is unbalanced, as it contains a right-to-left
+ * override character, which is never followed by a <PDF>, the "pop directional
+ * formatting" character. This function adds the missing <PDF> at the end, and
+ * the output of this function would be "Hello <RLO>World<PDF>".
+ *
+ * Example of spoofing:
+ * Consider "Firstname Lastname<RLO>" <moc.mitciv@attacker.com>. Because of the RLO,
+ * it is displayed as "Firstname Lastname <moc.rekcatta@victim.com>", which spoofs the
+ * domain name.
+ * By passing "Firstname Lastname<RLO>" to this function, one can balance the <RLO>,
+ * leading to "Firstname Lastname<RLO><PDF>", so the whole mailbox is displayed
+ * correctly as "Firstname Lastname" <moc.mitciv@attacker.com> again.
+ *
+ * See http://unicode.org/reports/tr9 for more information on bidi control chars.
+ *
+ * @param input the display name of a mailbox, which is checked for unbalanced Unicode
+ * direction control characters
+ * @return the display name which now contains a balanced state of direction control
+ * characters
+ *
+ * Note that this function does not do any parsing related to mailboxes, it only works
+ * on plain strings. Therefore, passing the complete mailbox will not lead to any results,
+ * only the display name should be passed.
+ *
+ * @since 4.5
+ */
+KMIME_EXPORT QString balanceBidiState( const QString &input );
+
} // namespace KMime
#endif /* __KMIME_UTIL_H__ */
--- trunk/KDE/kdepimlibs/kmime/tests/kmime_message_test.cpp #1066359:1066360
@@ -244,3 +244,30 @@
QVERIFY( cd );
QCOMPARE( cd->filename(), QString( "jaselka 1.docx" ) );
}
+
+void MessageTest::testBidiSpoofing()
+{
+ const QString RLO( QChar( 0x202E ) );
+ const QString PDF( QChar( 0x202C ) );
+
+ const QByteArray senderAndRLO =
+ encodeRFC2047String( "\"Sender" + RLO + "\" <sender@test.org>", "utf-8" );
+
+ // The display name of the "From" has an RLO, make sure the KMime parser balances it
+ QByteArray data =
+ "From: " + senderAndRLO + "\n"
+ "\n"
+ "Body";
+
+ KMime::Message msg;
+ msg.setContent( data );
+ msg.parse();
+
+ const QString expectedDisplayName = "\"Sender" + RLO + PDF + "\"";
+ const QString expectedMailbox = expectedDisplayName + " <sender@test.org>";
+ QCOMPARE( msg.from()->addresses().count(), 1 );
+ QCOMPARE( msg.from()->asUnicodeString(), expectedMailbox );
+ QCOMPARE( msg.from()->displayNames().first(), expectedDisplayName );
+ QCOMPARE( msg.from()->mailboxes().first().name(), expectedDisplayName );
+ QCOMPARE( msg.from()->mailboxes().first().address().data(), "sender@test.org" );
+}
--- trunk/KDE/kdepimlibs/kmime/tests/kmime_message_test.h #1066359:1066360
@@ -33,6 +33,7 @@
void testWronglyFoldedHeaders();
void missingHeadersTest();
void testBug219749();
+ void testBidiSpoofing();
};
--- trunk/KDE/kdepimlibs/kmime/tests/kmime_util_test.cpp #1066359:1066360
@@ -61,3 +61,42 @@
// missing space after ':'
QCOMPARE( extractHeader( "From:<toma@kovoks.nl>", "From" ), QByteArray( "<toma@kovoks.nl>" ) );
}
+
+void KMimeUtilTest::testBalanceBidiState()
+{
+ QFETCH( QString, input );
+ QFETCH( QString, expResult );
+
+ QCOMPARE( balanceBidiState( input ), expResult );
+}
+
+void KMimeUtilTest::testBalanceBidiState_data()
+{
+ QTest::addColumn<QString>( "input" );
+ QTest::addColumn<QString>( "expResult" );
+
+ const QString LRO( QChar( 0x202D ) );
+ const QString RLO( QChar( 0x202E ) );
+ const QString LRE( QChar( 0x202A ) );
+ const QString RLE( QChar( 0x202B ) );
+ const QString PDF( QChar( 0x202C ) );
+
+ QTest::newRow( "" ) << "Normal" << "Normal";
+ QTest::newRow( "" ) << RLO + "Balanced" + PDF << RLO + "Balanced" + PDF;
+ QTest::newRow( "" ) << RLO + "MissingPDF1" << RLO + "MissingPDF1" + PDF;
+ QTest::newRow( "" ) << "\"" + RLO + "Quote\"" << "\"" + RLO + "Quote" + PDF + "\"";
+ QTest::newRow( "" ) << "MissingPDF2" + RLO << "MissingPDF2" + RLO + PDF;
+ QTest::newRow( "" ) << RLO + "MultipleRLO" + RLO << RLO + "MultipleRLO" + RLO + PDF + PDF;
+ QTest::newRow( "" ) << LRO + "Mixed" + LRE + RLE + RLO + "Bla"
+ << LRO + "Mixed" + LRE + RLE + RLO + "Bla" + PDF.repeated( 4 );
+ QTest::newRow( "" ) << RLO + "TooManyPDF" + PDF + RLO + PDF + PDF
+ << RLO + "TooManyPDF" + PDF + RLO + PDF;
+ QTest::newRow( "" ) << PDF + "WrongOrder" + RLO
+ << "WrongOrder" + RLO + PDF;
+ QTest::newRow( "" ) << "ComplexOrder" + RLO + PDF + PDF + RLO
+ << "ComplexOrder" + RLO + PDF + RLO + PDF;
+ QTest::newRow( "" ) << "ComplexOrder2" + RLO + PDF + PDF + PDF + RLO + PDF + PDF + PDF
+ << "ComplexOrder2" + RLO + PDF + RLO + PDF;
+ QTest::newRow( "" ) << PDF + PDF + PDF + "ComplexOrder3" + PDF + PDF + RLO + PDF + PDF + PDF
+ << "ComplexOrder3" + RLO + PDF;
+}
\ No newline at end of file
--- trunk/KDE/kdepimlibs/kmime/tests/kmime_util_test.h #1066359:1066360
@@ -27,6 +27,8 @@
private Q_SLOTS:
void testUnfoldHeader();
void testExtractHeader();
+ void testBalanceBidiState();
+ void testBalanceBidiState_data();
};
#endif
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic