Merge pull request #5001 from k9mail/no_encoding_in_MimeHeader

No encoding in MimeHeader

app/core/src/main/java/com/fsck/k9/autocrypt/AutocryptTransferMessageCreator.kt

@@ -20,8 +20,8 @@ class AutocryptTransferMessageCreator(private val stringProvider: AutocryptStrin
             val subjectText = stringProvider.transferMessageSubject()
             val messageText = stringProvider.transferMessageBody()
 
-            val textBodyPart = MimeBodyPart(TextBody(messageText))
+            val textBodyPart = MimeBodyPart.create(TextBody(messageText))
-            val dataBodyPart = MimeBodyPart(BinaryMemoryBody(data, "7bit"))
+            val dataBodyPart = MimeBodyPart.create(BinaryMemoryBody(data, "7bit"))
             dataBodyPart.setHeader(MimeHeader.HEADER_CONTENT_TYPE, "application/autocrypt-setup")
             dataBodyPart.setHeader(MimeHeader.HEADER_CONTENT_DISPOSITION, "attachment; filename=\"autocrypt-setup-message\"")
 
@@ -29,7 +29,7 @@ class AutocryptTransferMessageCreator(private val stringProvider: AutocryptStrin
             messageBody.addBodyPart(textBodyPart)
             messageBody.addBodyPart(dataBodyPart)
 
-            val message = MimeMessage()
+            val message = MimeMessage.create()
             MimeMessageHelper.setBody(message, messageBody)
 
             val nowDate = Date()

app/core/src/main/java/com/fsck/k9/message/MessageBuilder.java

@@ -26,6 +26,7 @@ import com.fsck.k9.mail.MessagingException;
 import com.fsck.k9.mail.internet.MessageIdGenerator;
 import com.fsck.k9.mail.internet.MimeBodyPart;
 import com.fsck.k9.mail.internet.MimeHeader;
+import com.fsck.k9.mail.internet.MimeHeaderEncoder;
 import com.fsck.k9.mail.internet.MimeMessage;
 import com.fsck.k9.mail.internet.MimeMessageHelper;
 import com.fsck.k9.mail.internet.MimeMultipart;
@@ -83,7 +84,7 @@ public abstract class MessageBuilder {
     protected MimeMessage build() throws MessagingException {
         //FIXME: check arguments
 
-        MimeMessage message = new MimeMessage();
+        MimeMessage message = MimeMessage.create();
 
         buildHeader(message);
         buildBody(message);
@@ -108,7 +109,8 @@ public abstract class MessageBuilder {
         }
 
         if (!K9.isHideUserAgent()) {
-            message.setHeader("User-Agent", resourceProvider.userAgent());
+            String encodedUserAgent = MimeHeaderEncoder.encode("User-Agent", resourceProvider.userAgent());
+            message.setHeader("User-Agent", encodedUserAgent);
         }
 
         final String replyTo = identity.getReplyTo();
@@ -166,8 +168,8 @@ public abstract class MessageBuilder {
             composedMimeMessage.setSubType("alternative");
             // Let the receiver select either the text or the HTML part.
             bodyPlain = buildText(isDraft, SimpleMessageFormat.TEXT);
-            composedMimeMessage.addBodyPart(new MimeBodyPart(bodyPlain, "text/plain"));
+            composedMimeMessage.addBodyPart(MimeBodyPart.create(bodyPlain, "text/plain"));
-            composedMimeMessage.addBodyPart(new MimeBodyPart(body, "text/html"));
+            composedMimeMessage.addBodyPart(MimeBodyPart.create(body, "text/html"));
 
             if (hasAttachments) {
                 // If we're HTML and have attachments, we have a MimeMultipart container to hold the
@@ -175,7 +177,7 @@ public abstract class MessageBuilder {
                 // (composedMimeMessage) with the user's composed messages, and subsequent parts for
                 // the attachments.
                 MimeMultipart mp = createMimeMultipart();
-                mp.addBodyPart(new MimeBodyPart(composedMimeMessage));
+                mp.addBodyPart(MimeBodyPart.create(composedMimeMessage));
                 addAttachmentsToMessage(mp);
                 MimeMessageHelper.setBody(message, mp);
             } else {
@@ -186,7 +188,7 @@ public abstract class MessageBuilder {
             // Text-only message.
             if (hasAttachments) {
                 MimeMultipart mp = createMimeMultipart();
-                mp.addBodyPart(new MimeBodyPart(body, "text/plain"));
+                mp.addBodyPart(MimeBodyPart.create(body, "text/plain"));
                 addAttachmentsToMessage(mp);
                 MimeMessageHelper.setBody(message, mp);
             } else {
@@ -231,7 +233,7 @@ public abstract class MessageBuilder {
             }
 
             Body body = new TempFileBody(attachment.getFileName());
-            MimeBodyPart bp = new MimeBodyPart(body);
+            MimeBodyPart bp = MimeBodyPart.create(body);
 
             addContentType(bp, attachment.getContentType(), attachment.getName());
             addContentDisposition(bp, attachment.getName(), attachment.getSize());

app/core/src/main/java/com/fsck/k9/message/PgpMessageBuilder.java

@@ -27,6 +27,7 @@ import com.fsck.k9.mail.Message.RecipientType;
 import com.fsck.k9.mail.MessagingException;
 import com.fsck.k9.mail.filter.EOLConvertingOutputStream;
 import com.fsck.k9.mail.internet.BinaryTempFileBody;
+import com.fsck.k9.mail.internet.Headers;
 import com.fsck.k9.mail.internet.MessageIdGenerator;
 import com.fsck.k9.mail.internet.MimeBodyPart;
 import com.fsck.k9.mail.internet.MimeHeader;
@@ -219,7 +220,7 @@ public class PgpMessageBuilder extends MessageBuilder {
             messageContentBodyPart.setHeader(MimeHeader.HEADER_CONTENT_TYPE,
                     messageContentBodyPart.getContentType() + "; protected-headers=\"v1\"");
             messageContentBodyPart.setHeader(MimeHeader.SUBJECT, subjects[0]);
-            currentProcessedMimeMessage.setHeader(MimeHeader.SUBJECT, resourceProvider.encryptedSubject());
+            currentProcessedMimeMessage.setSubject(resourceProvider.encryptedSubject());
         }
     }
 
@@ -394,7 +395,7 @@ public class PgpMessageBuilder extends MessageBuilder {
         multipartSigned.setSubType("signed");
         multipartSigned.addBodyPart(signedBodyPart);
         multipartSigned.addBodyPart(
-                new MimeBodyPart(new BinaryMemoryBody(signedData, MimeUtil.ENC_7BIT),
+                MimeBodyPart.create(new BinaryMemoryBody(signedData, MimeUtil.ENC_7BIT),
                         "application/pgp-signature; name=\"signature.asc\""));
         MimeMessageHelper.setBody(currentProcessedMimeMessage, multipartSigned);
 
@@ -413,8 +414,8 @@ public class PgpMessageBuilder extends MessageBuilder {
     private void mimeBuildEncryptedMessage(@NonNull Body encryptedBodyPart) throws MessagingException {
         MimeMultipart multipartEncrypted = createMimeMultipart();
         multipartEncrypted.setSubType("encrypted");
-        multipartEncrypted.addBodyPart(new MimeBodyPart(new TextBody("Version: 1"), "application/pgp-encrypted"));
+        multipartEncrypted.addBodyPart(MimeBodyPart.create(new TextBody("Version: 1"), "application/pgp-encrypted"));
-        MimeBodyPart encryptedPart = new MimeBodyPart(encryptedBodyPart, "application/octet-stream; name=\"encrypted.asc\"");
+        MimeBodyPart encryptedPart = MimeBodyPart.create(encryptedBodyPart, "application/octet-stream; name=\"encrypted.asc\"");
         encryptedPart.addHeader(MimeHeader.HEADER_CONTENT_DISPOSITION, "inline; filename=\"encrypted.asc\"");
         multipartEncrypted.addBodyPart(encryptedPart);
         MimeMessageHelper.setBody(currentProcessedMimeMessage, multipartEncrypted);

mail/common/src/main/java/com/fsck/k9/mail/internet/MimeBodyPart.java

@@ -27,6 +27,20 @@ public class MimeBodyPart extends BodyPart {
     private final MimeHeader mHeader;
     private Body mBody;
 
+    /**
+     * Creates an instance that will check the header field syntax when adding headers.
+     */
+    public static MimeBodyPart create(Body body) throws MessagingException {
+        return new MimeBodyPart(body, null, true);
+    }
+
+    /**
+     * Creates an instance that will check the header field syntax when adding headers.
+     */
+    public static MimeBodyPart create(Body body, String contentType) throws MessagingException {
+        return new MimeBodyPart(body, contentType, true);
+    }
+
     public MimeBodyPart() throws MessagingException {
         this(null);
     }
@@ -36,7 +50,12 @@ public class MimeBodyPart extends BodyPart {
     }
 
     public MimeBodyPart(Body body, String contentType) throws MessagingException {
+        this(body, contentType, false);
+    }
+
+    private MimeBodyPart(Body body, String contentType, boolean checkHeaders) throws MessagingException {
         mHeader = new MimeHeader();
+        mHeader.setCheckHeaders(checkHeaders);
         if (contentType != null) {
             addHeader(MimeHeader.HEADER_CONTENT_TYPE, contentType);
         }

mail/common/src/main/java/com/fsck/k9/mail/internet/MimeHeader.kt

@@ -17,6 +17,8 @@ class MimeHeader {
     val headers: List<Header>
         get() = fields.map { Header(it.name, it.value) }
 
+    var checkHeaders = false
+
     fun clear() {
         fields.clear()
     }
@@ -26,11 +28,13 @@ class MimeHeader {
     }
 
     fun addHeader(name: String, value: String) {
-        val field = NameValueField(name, MimeUtility.foldAndEncode(value))
+        requireValidHeader(name, value)
+        val field = NameValueField(name, value)
         fields.add(field)
     }
 
     fun addRawHeader(name: String, raw: String) {
+        requireValidRawHeader(name, raw)
         val field = RawField(name, raw)
         fields.add(field)
     }
@@ -76,21 +80,34 @@ class MimeHeader {
     }
 
     private fun Appendable.appendNameValueField(field: Field) {
-        val value = field.value
-        val encodedValue = if (hasToBeEncoded(value)) {
-            EncoderUtil.encodeEncodedWord(value)
-        } else {
-            value
-        }
-
         append(field.name)
         append(": ")
-        append(encodedValue)
+        append(field.value)
     }
 
-    // encode non printable characters except LF/CR/TAB codes.
+    private fun requireValidHeader(name: String, value: String) {
-    private fun hasToBeEncoded(text: String): Boolean {
+        if (checkHeaders) {
-        return text.any { !it.isVChar() && !it.isWspOrCrlf() }
+            checkHeader(name, value)
+        }
+    }
+
+    private fun requireValidRawHeader(name: String, raw: String) {
+        if (checkHeaders) {
+            if (!raw.startsWith(name)) throw AssertionError("Raw header value needs to start with header name")
+            val delimiterIndex = raw.indexOf(':')
+            val value = if (delimiterIndex == raw.lastIndex) "" else raw.substring(delimiterIndex + 1).trimStart()
+
+            checkHeader(name, value)
+        }
+    }
+
+    private fun checkHeader(name: String, value: String) {
+        try {
+            MimeHeaderChecker.checkHeader(name, value)
+        } catch (e: MimeHeaderParserException) {
+            // Use AssertionError so we crash the app
+            throw AssertionError("Invalid header", e)
+        }
     }
 
     companion object {

mail/common/src/main/java/com/fsck/k9/mail/internet/MimeHeaderChecker.kt

@@ -0,0 +1,115 @@
+package com.fsck.k9.mail.internet
+
+/**
+ * Check unstructured header field syntax.
+ *
+ * This does not allow the obsolete syntax. Only use this for messages constructed by K-9 Mail, not incoming messages.
+ *
+ * See RFC 5322
+ * ```
+ * optional-field =  field-name ":" unstructured CRLF
+ * field-name     =  1*ftext
+ * ftext          =  %d33-57 / %d59-126  ; Printable US-ASCII characters not including ":".
+ *
+ * unstructured   =  (*([FWS] VCHAR) *WSP) / obs-unstruct
+ * FWS            =  ([*WSP CRLF] 1*WSP) / obs-FWS  ; Folding white space
+ * ```
+ */
+object MimeHeaderChecker {
+    fun checkHeader(name: String, value: String) {
+        if (!name.isValidFieldName()) {
+            throw MimeHeaderParserException("Header name contains characters not allowed: $name")
+        }
+
+        val initialLineLength = name.length + 2 // name + colon + space
+        UnstructuredHeaderChecker(value, initialLineLength).checkHeaderValue()
+    }
+
+    private fun String.isValidFieldName() = all { it.isFieldText() }
+
+    private fun Char.isFieldText() = isVChar() && this != ':'
+}
+
+private class UnstructuredHeaderChecker(val input: String, initialLineLength: Int) {
+    private val endIndex = input.length
+    private var currentIndex = 0
+    private var lineLength = initialLineLength
+
+    fun checkHeaderValue() {
+        while (!endReached()) {
+            val char = peek()
+            when {
+                char == CR -> {
+                    expectCr()
+                    expectLf()
+
+                    if (lineLength > 1000) {
+                        throw MimeHeaderParserException("Line exceeds 998 characters", currentIndex - 1)
+                    }
+                    lineLength = 0
+
+                    expectWsp()
+                    skipWsp()
+                    expectVChar()
+                }
+                char.isVChar() || char.isWsp() -> {
+                    skipVCharAndWsp()
+                }
+                else -> {
+                    throw MimeHeaderParserException("Unexpected character (${char.toInt()})", currentIndex)
+                }
+            }
+        }
+
+        if (lineLength > 998) {
+            throw MimeHeaderParserException("Line exceeds 998 characters", currentIndex)
+        }
+    }
+
+    private fun expectCr() = expect("CR", CR)
+
+    private fun expectLf() = expect("LF", LF)
+
+    private fun expectVChar() = expect("VCHAR") { it.isVChar() }
+
+    private fun expectWsp() = expect("WSP") { it.isWsp() }
+
+    private fun skipWsp() {
+        while (!endReached() && peek().isWsp()) {
+            skip()
+        }
+    }
+
+    private fun skipVCharAndWsp() {
+        while (!endReached() && peek().let { it.isVChar() || it.isWsp() }) {
+            skip()
+        }
+    }
+
+    private fun endReached() = currentIndex >= endIndex
+
+    private fun peek(): Char {
+        if (currentIndex >= input.length) {
+            throw MimeHeaderParserException("End of input reached unexpectedly", currentIndex)
+        }
+
+        return input[currentIndex]
+    }
+
+    private fun skip() {
+        currentIndex++
+        lineLength++
+    }
+
+    private fun expect(displayInError: String, character: Char) {
+        expect(displayInError) { it == character }
+    }
+
+    private inline fun expect(displayInError: String, predicate: (Char) -> Boolean) {
+        if (!endReached() && predicate(peek())) {
+            skip()
+        } else {
+            throw MimeHeaderParserException("Expected $displayInError", currentIndex)
+        }
+    }
+}

mail/common/src/main/java/com/fsck/k9/mail/internet/MimeHeaderEncoder.kt

@@ -0,0 +1,26 @@
+package com.fsck.k9.mail.internet
+
+object MimeHeaderEncoder {
+    @JvmStatic
+    fun encode(name: String, value: String): String {
+        // TODO: Fold long text that provides enough opportunities for folding and doesn't contain any characters that
+        //  need to be encoded.
+        return if (hasToBeEncoded(name, value)) {
+            EncoderUtil.encodeEncodedWord(value)
+        } else {
+            value
+        }
+    }
+
+    private fun hasToBeEncoded(name: String, value: String): Boolean {
+        return exceedsRecommendedLineLength(name, value) || charactersNeedEncoding(value)
+    }
+
+    private fun exceedsRecommendedLineLength(name: String, value: String): Boolean {
+        return name.length + 2 /* colon + space */ + value.length > RECOMMENDED_MAX_LINE_LENGTH
+    }
+
+    private fun charactersNeedEncoding(text: String): Boolean {
+        return text.any { !it.isVChar() && !it.isWspOrCrlf() }
+    }
+}

mail/common/src/main/java/com/fsck/k9/mail/internet/MimeHeaderParser.kt

@@ -183,4 +183,4 @@ class MimeHeaderParser(private val input: String) {
     }
 }
 
-class MimeHeaderParserException(message: String, val errorIndex: Int) : RuntimeException(message)
+class MimeHeaderParserException(message: String, val errorIndex: Int = -1) : RuntimeException(message)

mail/common/src/main/java/com/fsck/k9/mail/internet/MimeMessage.java

@@ -75,7 +75,19 @@ public class MimeMessage extends Message {
         return mimeMessage;
     }
 
+    /**
+     * Creates an instance that will check the header field syntax when adding headers.
+     */
+    public static MimeMessage create() {
+        return new MimeMessage(true);
+    }
+
     public MimeMessage() {
+        this(false);
+    }
+
+    private MimeMessage(boolean checkHeaders) {
+        mHeader.setCheckHeaders(checkHeaders);
     }
 
     /**
@@ -261,12 +273,13 @@ public class MimeMessage extends Message {
      */
     @Override
     public String getSubject() {
-        return MimeUtility.unfoldAndDecode(getFirstHeader("Subject"), this);
+        return MimeUtility.unfoldAndDecode(getFirstHeader(MimeHeader.SUBJECT), this);
     }
 
     @Override
     public void setSubject(String subject) {
-        setHeader("Subject", subject);
+        String encodedSubject = MimeHeaderEncoder.encode(MimeHeader.SUBJECT, subject);
+        setHeader(MimeHeader.SUBJECT, encodedSubject);
     }
 
     @Override

mail/common/src/main/java/com/fsck/k9/mail/internet/MimeUtility.java

@@ -914,11 +914,6 @@ public class MimeUtility {
         return decode(unfold(s), message);
     }
 
-    // TODO implement proper foldAndEncode
-    public static String foldAndEncode(String s) {
-        return s;
-    }
-
     /**
      * Returns the named parameter of a header field.
      *

mail/common/src/test/java/com/fsck/k9/mail/internet/MimeHeaderCheckerTest.kt

@@ -0,0 +1,210 @@
+package com.fsck.k9.mail.internet
+
+import org.junit.Assert.fail
+import org.junit.Test
+
+class MimeHeaderCheckerTest {
+    @Test
+    fun emptyValue() {
+        assertValidHeader("Subject: ")
+    }
+
+    @Test
+    fun blankValue() {
+        assertValidHeader("Subject:  ")
+    }
+
+    @Test
+    fun textEndingInSpace() {
+        assertValidHeader("Subject: Text ")
+    }
+
+    @Test
+    fun textContainingSpaces() {
+        assertValidHeader("Subject: Text containing spaces")
+    }
+
+    @Test
+    fun allVisibleCharacters() {
+        val text = (33..126).map { it.toChar() }.joinToString("")
+        assertValidHeader("Subject: $text")
+    }
+
+    @Test
+    fun blankFirstLine() {
+        assertValidHeader("Subject: \r\n Two")
+    }
+
+    @Test
+    fun twoLines() {
+        assertValidHeader("Subject: One\r\n Two")
+    }
+
+    @Test
+    fun threeLines() {
+        assertValidHeader("Subject: One\r\n Two\r\n Three")
+    }
+
+    @Test
+    fun secondLineStartingWithTab() {
+        assertValidHeader("Subject: One\r\n\tTwo")
+    }
+
+    @Test
+    fun secondLineStartingWithMultipleWhitespace() {
+        assertValidHeader("Subject: One\r\n \t Two")
+    }
+
+    @Test
+    fun singleLineAtMaximumLineLength() {
+        val longText = "x".repeat(998 /* text limit */ - 4 /* Test */ - 2 /* colon, space */)
+        assertValidHeader("Test: $longText")
+    }
+
+    @Test
+    fun firstLineAtMaximumLineLength() {
+        val longText = "x".repeat(998 /* text limit */ - 4 /* Test */ - 2 /* colon, space */)
+        assertValidHeader("Test: $longText\r\n Text")
+    }
+
+    @Test
+    fun middleLineAtMaximumLineLength() {
+        val longText = "x".repeat(998 - 1 /* space */)
+        assertValidHeader("Test: One\r\n $longText\r\n Three")
+    }
+
+    @Test
+    fun lastLineAtMaximumLineLength() {
+        val longText = "x".repeat(998 - 1 /* space */)
+        assertValidHeader("Test: One\r\n $longText")
+    }
+
+    @Test
+    fun colonInHeaderName() {
+        assertInvalidHeader("Header:Name: Text")
+    }
+
+    @Test
+    fun nonAsciiCharacterInHeaderName() {
+        assertInvalidHeader("Sübject: Text")
+    }
+
+    @Test
+    fun headerNameExceedingLineLimit() {
+        val longName = "x".repeat(998 - 2 /* space, colon */ + 1)
+        assertInvalidHeader("$longName: ")
+    }
+
+    @Test
+    fun nonAsciiCharacter() {
+        assertInvalidHeader("Subject: ö")
+    }
+
+    @Test
+    fun nonVisibleCharacter() {
+        assertInvalidHeader("Subject: \u0007")
+    }
+
+    @Test
+    fun endingInCR() {
+        assertInvalidHeader("Subject: Text\r")
+    }
+
+    @Test
+    fun endingInLF() {
+        assertInvalidHeader("Subject: Text\n")
+    }
+
+    @Test
+    fun endingInCRLF() {
+        assertInvalidHeader("Subject: Text\r\n")
+    }
+
+    @Test
+    fun lineBreakNotFollowedByWhitespace() {
+        assertInvalidHeader("Subject: One\r\nTwo")
+    }
+
+    @Test
+    fun singleCR() {
+        assertInvalidHeader("Subject: One\rTwo")
+    }
+
+    @Test
+    fun singleCrFollowedByWhitespace() {
+        assertInvalidHeader("Subject: One\r Two")
+    }
+
+    @Test
+    fun consecutiveCRs() {
+        assertInvalidHeader("Subject: \r\r\n Two")
+    }
+
+    @Test
+    fun singleLF() {
+        assertInvalidHeader("Subject: One\nTwo")
+    }
+
+    @Test
+    fun singleLfFollowedByWhitespace() {
+        assertInvalidHeader("Subject: One\n Two")
+    }
+
+    @Test
+    fun consecutiveLFs() {
+        assertInvalidHeader("Subject: \r\n\n Two")
+    }
+
+    @Test
+    fun consecutiveLineBreaks() {
+        assertInvalidHeader("Subject: One\r\n\r\n Two")
+    }
+
+    @Test
+    fun blankMiddleLine() {
+        assertInvalidHeader("Subject: One\r\n \r\n Two")
+    }
+
+    @Test
+    fun endsWithBlankLine() {
+        assertInvalidHeader("Subject: One\r\n ")
+    }
+
+    @Test
+    fun singleLineExceedingLineLength() {
+        val longText = "x".repeat(998 /* text limit */ - 4 /* Test */ - 2 /* colon, space */ + 1)
+        assertInvalidHeader("Test: $longText")
+    }
+
+    @Test
+    fun firstLineExceedingLineLength() {
+        val longText = "x".repeat(998 /* text limit */ - 4 /* Test */ - 2 /* colon, space */ + 1)
+        assertInvalidHeader("Test: $longText\r\n Text")
+    }
+
+    @Test
+    fun middleLineExceedingLineLength() {
+        val longText = "x".repeat(998 - 1 /* space */ + 1)
+        assertInvalidHeader("Test: One\r\n $longText\r\n Three")
+    }
+
+    @Test
+    fun lastLineExceedingLineLength() {
+        val longText = "x".repeat(998 - 1 /* space */ + 1)
+        assertInvalidHeader("Test: One\r\n $longText")
+    }
+
+    private fun assertValidHeader(header: String) {
+        val (name, value) = header.split(": ", limit = 2)
+        MimeHeaderChecker.checkHeader(name, value)
+    }
+
+    private fun assertInvalidHeader(header: String) {
+        val (name, value) = header.split(": ", limit = 2)
+        try {
+            MimeHeaderChecker.checkHeader(name, value)
+            fail("Expected exception")
+        } catch (expected: MimeHeaderParserException) {
+        }
+    }
+}