Skip to content

Commit 0b2c467

Browse files
committed
Use stricter parsing logic for quoted-strings in addr-specs
Switched to using a stricter version of SkipQuoted that is more specifically designed for quoted-string local-parts in addr-spec tokens. The main intention is to prevent MailboxAddresses from having an Address property that contains <CR><LF> sequences which can be used to exploit `MAIL FROM` and/or `RCPT TO` SMTP commands by injecting arbitrary SMTP commands within the quoted local-part of a mailbox address.
1 parent 09f69d0 commit 0b2c467

3 files changed

Lines changed: 169 additions & 1 deletion

File tree

MimeKit/InternetAddress.cs

Lines changed: 75 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -307,6 +307,80 @@ protected virtual void OnChanged ()
307307
Changed?.Invoke (this, EventArgs.Empty);
308308
}
309309

310+
static bool SkipQuoted (byte[] text, ref int index, int endIndex, bool throwOnError)
311+
{
312+
// From rfc5322:
313+
//
314+
// qcontent = qtext / quoted-pair
315+
//
316+
// qtext = %d33 / ; Printable US-ASCII
317+
// %d35-91 / ; characters not including
318+
// %d93-126 / ; "\" or the quote character
319+
// obs-qtext
320+
//
321+
// obs-qtext = <any CHAR excepting <">, ; => may be folded
322+
// "\" & CR, and including
323+
// linear-white-space>
324+
//
325+
// quoted-pair = ("\" (VCHAR / WSP)) / obs-qp
326+
//
327+
// VCHAR = %x21-7E ; visible (printing) characters
328+
//
329+
// WSP = SP / HTAB ; white space
330+
int startIndex = index;
331+
bool escaped = false;
332+
333+
// skip over leading '"'
334+
index++;
335+
336+
while (index < endIndex) {
337+
byte c = text[index];
338+
339+
if (c == (byte) '\\') {
340+
escaped = !escaped;
341+
} else if (!escaped) {
342+
if (c == (byte) '"')
343+
break;
344+
345+
// Note: the qtext definition from rfc5322 accepts %d33, %d35-91, and %d93-126 and so you
346+
// might expect that we limit the characters to those ranges, but rfc6532 extends the qtext
347+
// definition to allow UTF-8 characters and so we need to allow for that as well. In addition
348+
// to that, obs-qtext also explicitly allows linear whitespace (SPACE %d32 and HTAB %d09), so
349+
// the only characters that are not allowed are the control characters (%d0-8 and %d10-31),
350+
// the double-quote character (%d34), and the delete character (%d127).
351+
if (c < 9 || (c > 9 && c < 32) || c == 34 || c == 127) {
352+
if (throwOnError)
353+
throw new ParseException (string.Format (CultureInfo.InvariantCulture, "Invalid character in quoted-string token at offset {0}", startIndex), startIndex, index);
354+
355+
return false;
356+
}
357+
} else {
358+
if (c < 9 || (c > 9 && c < 32) || c == 127) {
359+
if (throwOnError)
360+
throw new ParseException (string.Format (CultureInfo.InvariantCulture, "Invalid quoted-pair in quoted-string token at offset {0}", startIndex), startIndex, index);
361+
362+
return false;
363+
}
364+
365+
escaped = false;
366+
}
367+
368+
index++;
369+
}
370+
371+
if (index >= endIndex) {
372+
if (throwOnError)
373+
throw new ParseException (string.Format (CultureInfo.InvariantCulture, "Incomplete quoted-string token at offset {0}", startIndex), startIndex, index);
374+
375+
return false;
376+
}
377+
378+
// skip over the closing '"'
379+
index++;
380+
381+
return true;
382+
}
383+
310384
internal static bool TryParseLocalPart (byte[] text, ref int index, int endIndex, RfcComplianceMode compliance, bool skipTrailingCfws, bool throwOnError, [NotNullWhen (true)] out string? localpart)
311385
{
312386
using var token = new ValueStringBuilder (128);
@@ -319,7 +393,7 @@ internal static bool TryParseLocalPart (byte[] text, ref int index, int endIndex
319393
int start = index;
320394

321395
if (text[index] == (byte) '"') {
322-
if (!ParseUtils.SkipQuoted (text, ref index, endIndex, throwOnError))
396+
if (!SkipQuoted (text, ref index, endIndex, throwOnError))
323397
return false;
324398
} else if (text[index].IsAtom ()) {
325399
if (!ParseUtils.SkipAtom (text, ref index, endIndex))

UnitTests/InternetAddressTests.cs

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,42 @@ public void TestParseMailboxWithIncompleteLocalPart ()
183183
AssertParseFailure (text, false, tokenIndex, errorIndex);
184184
}
185185

186+
[Test]
187+
public void TestParseMailboxWithInvalidQuotedLocalPart ()
188+
{
189+
const string text = "\"invalid\r\nquoted\"@domain.com";
190+
int errorIndex = text.IndexOf ('\r');
191+
const int tokenIndex = 0;
192+
193+
AssertParseFailure (text, false, tokenIndex, errorIndex);
194+
}
195+
196+
[Test]
197+
public void TestParseMailboxWithInvalidQuotedPairLocalPart ()
198+
{
199+
const string text = "\"invalid\\\rquoted\"@domain.com";
200+
int errorIndex = text.IndexOf ('\r');
201+
const int tokenIndex = 0;
202+
203+
AssertParseFailure (text, false, tokenIndex, errorIndex);
204+
}
205+
206+
[Test]
207+
public void TestParseMailboxWithValidQuotedLocalPart ()
208+
{
209+
const string text = "\"\t !\\\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\"@domain.com";
210+
211+
AssertParse (text);
212+
}
213+
214+
[Test]
215+
public void TestParseMailboxWithValidUTF8QuotedLocalPart ()
216+
{
217+
const string text = "\"名がドメイン\"@domain.com";
218+
219+
AssertParse (text);
220+
}
221+
186222
[Test]
187223
public void TestParseIncompleteQuotedString ()
188224
{

UnitTests/MailboxAddressTests.cs

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,64 @@ public void TestParseMailboxWithIncompleteLocalPart ()
349349
AssertParseFailure (text, false, tokenIndex, errorIndex);
350350
}
351351

352+
[Test]
353+
public void TestParseMailboxWithInvalidQuotedLocalPart ()
354+
{
355+
const string text = "\"invalid\r\nquoted\"@domain.com";
356+
int errorIndex = text.IndexOf ('\r');
357+
const int tokenIndex = 0;
358+
359+
AssertParseFailure (text, false, tokenIndex, errorIndex);
360+
361+
try {
362+
var mailbox = new MailboxAddress ("Name", text);
363+
Assert.Fail ("Expected ParseException in .ctor");
364+
} catch (ParseException pex) {
365+
Assert.That (pex.TokenIndex, Is.EqualTo (tokenIndex), ".ctor ParseException.TokenIndex");
366+
Assert.That (pex.ErrorIndex, Is.EqualTo (errorIndex), ".ctor ParseException.ErrorIndex");
367+
} catch (Exception ex) {
368+
Assert.Fail ($"Expected ParseException, got {ex.GetType ().Name}");
369+
}
370+
}
371+
372+
[Test]
373+
public void TestParseMailboxWithInvalidQuotedPairLocalPart ()
374+
{
375+
const string text = "\"invalid\\\rquoted\"@domain.com";
376+
int errorIndex = text.IndexOf ('\r');
377+
const int tokenIndex = 0;
378+
379+
AssertParseFailure (text, false, tokenIndex, errorIndex);
380+
381+
try {
382+
var mailbox = new MailboxAddress ("Name", text);
383+
Assert.Fail ("Expected ParseException in .ctor");
384+
} catch (ParseException pex) {
385+
Assert.That (pex.TokenIndex, Is.EqualTo (tokenIndex), ".ctor ParseException.TokenIndex");
386+
Assert.That (pex.ErrorIndex, Is.EqualTo (errorIndex), ".ctor ParseException.ErrorIndex");
387+
} catch (Exception ex) {
388+
Assert.Fail ($"Expected ParseException, got {ex.GetType ().Name}");
389+
}
390+
}
391+
392+
[Test]
393+
public void TestParseMailboxWithValidQuotedLocalPart ()
394+
{
395+
const string text = "\"\t !\\\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\\\]^_`abcdefghijklmnopqrstuvwxyz{|}~\"@domain.com";
396+
397+
AssertParse (text);
398+
Assert.DoesNotThrow (() => new MailboxAddress ("Name", text), "MailboxAddress .ctor should not throw an exception.");
399+
}
400+
401+
[Test]
402+
public void TestParseMailboxWithValidUTF8QuotedLocalPart ()
403+
{
404+
const string text = "\"名がドメイン\"@domain.com";
405+
406+
AssertParse (text);
407+
Assert.DoesNotThrow (() => new MailboxAddress ("Name", text), "MailboxAddress .ctor should not throw an exception.");
408+
}
409+
352410
[Test]
353411
public void TestParseIncompleteQuotedString ()
354412
{

0 commit comments

Comments
 (0)