diff --git a/app/src/main/kotlin/org/libremail/mail/GraphSender.kt b/app/src/main/kotlin/org/libremail/mail/GraphSender.kt index 936dc54..a183458 100644 --- a/app/src/main/kotlin/org/libremail/mail/GraphSender.kt +++ b/app/src/main/kotlin/org/libremail/mail/GraphSender.kt @@ -111,9 +111,10 @@ internal fun buildSendMailPayload(message: OutgoingMessage, attachments: List@libremail`) and contain none, so this is a no-op for them — it is + * defense-in-depth against a future change letting external values reach [contentId] (issue #204). + */ +internal fun sanitizeContentId(raw: String?): String = raw.orEmpty().filterNot { it.isISOControl() } diff --git a/app/src/main/kotlin/org/libremail/mail/SmtpSender.kt b/app/src/main/kotlin/org/libremail/mail/SmtpSender.kt index 8e4f650..a0d260f 100644 --- a/app/src/main/kotlin/org/libremail/mail/SmtpSender.kt +++ b/app/src/main/kotlin/org/libremail/mail/SmtpSender.kt @@ -121,7 +121,9 @@ class SmtpSender @Inject constructor() { /** An inline image part: attached bytes, a `Content-ID` the HTML's `cid:` matches, inline disposition. */ private fun inlinePart(attachment: SendableAttachment): MimeBodyPart = MimeBodyPart().apply { attachFile(attachment.file) - contentID = "<${attachment.contentId}>" + // Sanitize before it becomes a header value: a CR/LF in the content id would otherwise inject + // additional MIME header lines into this part (issue #204, defense-in-depth). + contentID = "<${sanitizeContentId(attachment.contentId)}>" setDisposition(MimeBodyPart.INLINE) } diff --git a/app/src/main/kotlin/org/libremail/richtext/RichTextHtml.kt b/app/src/main/kotlin/org/libremail/richtext/RichTextHtml.kt index 7553f0e..ea18904 100644 --- a/app/src/main/kotlin/org/libremail/richtext/RichTextHtml.kt +++ b/app/src/main/kotlin/org/libremail/richtext/RichTextHtml.kt @@ -27,7 +27,7 @@ object RichTextHtml { /** The `font-family`/`font-size` declarations of the base-style wrapper (possibly empty). */ private fun baseCss(base: RichBaseStyle): String = listOfNotNull( - base.fontCss?.let { "font-family:$it" }, + base.fontCss?.let(::safeFontFamily)?.let { "font-family:$it" }, base.fontSizePt?.let { "font-size:${it}pt" }, ).joinToString(";") @@ -197,7 +197,8 @@ private fun appendRun(sb: StringBuilder, content: RichTextContent, a: Int, b: In /** Merges the parameterized styles active on a run into one CSS declaration list (maybe empty). */ private fun inlineCss(styles: List): String { val parts = ArrayList() - styles.firstNotNullOfOrNull { it as? RichStyle.FontFamily }?.let { parts.add("font-family:${it.css}") } + styles.firstNotNullOfOrNull { it as? RichStyle.FontFamily }?.css?.let(::safeFontFamily) + ?.let { parts.add("font-family:$it") } styles.firstNotNullOfOrNull { it as? RichStyle.FontSize }?.let { parts.add("font-size:${it.pt}pt") } styles.firstNotNullOfOrNull { it as? RichStyle.FontColor }?.let { parts.add("color:${cssColor(it.argb)}") } styles.firstNotNullOfOrNull { it as? RichStyle.Highlight } @@ -215,3 +216,16 @@ internal fun cssColor(argb: Int): String = "#" + (argb and RGB_MASK).toString(HE internal fun escape(s: String): String = s.replace("&", "&").replace("<", "<").replace(">", ">") internal fun escapeAttr(s: String): String = escape(s).replace("\"", """) + +/** The characters a font-family stack legitimately uses: letters, digits, spaces, commas, quotes, etc. */ +private val SAFE_FONT_FAMILY = Regex("[A-Za-z0-9 ,._'\"-]*") + +/** + * Emits a `font-family` value only when every character is one a real font stack uses, so a value + * that ever carried CSS metacharacters (notably `;` or `:`) can't inject a sibling declaration into + * the raw `style` attribute — an unrecognized value is dropped rather than emitted (issue #205, + * defense-in-depth: not reachable today, since the picker only offers the fixed `FontRegistry` stacks + * and reply/forward flattens sender HTML first). Every bundled `FontRegistry` stack is within this + * set, so the built-in fonts round-trip unchanged. + */ +private fun safeFontFamily(css: String): String? = css.takeIf { SAFE_FONT_FAMILY.matches(it) } diff --git a/app/src/test/kotlin/org/libremail/mail/GraphSenderTest.kt b/app/src/test/kotlin/org/libremail/mail/GraphSenderTest.kt index 087899d..f1757de 100644 --- a/app/src/test/kotlin/org/libremail/mail/GraphSenderTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/GraphSenderTest.kt @@ -127,4 +127,21 @@ class GraphSenderTest { image.delete() } } + + @Test + fun `an inline image contentId is stripped of control characters in the payload`() { + val image = File.createTempFile("graph-inline", ".png").apply { writeText("PNGDATA") } + try { + val message = message(to = "a@x.com").copy(bodyHtml = "

") + // A crafted content id with CR/LF and a control char; only the printable text must remain. + val inline = SendableAttachment(image, contentId = "logo@libremail\r\nevil", isInline = true) + val contentId = JSONObject(buildSendMailPayload(message, listOf(inline))) + .getJSONObject("message").getJSONArray("attachments").getJSONObject(0) + .getString("contentId") + assertEquals("logo@libremailevil", contentId) + assertFalse('\r' in contentId || '\n' in contentId, "contentId=$contentId") + } finally { + image.delete() + } + } } diff --git a/app/src/test/kotlin/org/libremail/mail/SmtpSenderTest.kt b/app/src/test/kotlin/org/libremail/mail/SmtpSenderTest.kt index b3fe3d1..6eaeb81 100644 --- a/app/src/test/kotlin/org/libremail/mail/SmtpSenderTest.kt +++ b/app/src/test/kotlin/org/libremail/mail/SmtpSenderTest.kt @@ -4,6 +4,8 @@ package org.libremail.mail import com.icegreen.greenmail.util.GreenMail import com.icegreen.greenmail.util.GreenMailUtil import com.icegreen.greenmail.util.ServerSetupTest +import jakarta.mail.Multipart +import jakarta.mail.Part import kotlinx.coroutines.test.runTest import org.junit.After import org.junit.Before @@ -13,6 +15,7 @@ import org.libremail.domain.model.OutgoingMessage import org.libremail.domain.model.SmtpParams import java.io.File import kotlin.test.assertEquals +import kotlin.test.assertFalse import kotlin.test.assertNull import kotlin.test.assertTrue @@ -166,6 +169,62 @@ class SmtpSenderTest { image.delete() } + @Test + fun `a Content-ID carrying CRLF cannot inject a MIME header line`() = runTest { + val image = File.createTempFile("libremail-inline", ".png").apply { writeText("PNGDATA") } + val params = SmtpParams( + host = "127.0.0.1", + port = greenMail.smtp.port, + security = MailSecurity.NONE, + username = "sender@example.org", + secret = "secret", + useXoauth2 = false, + ) + + sender.send( + params = params, + from = "sender@example.org", + message = OutgoingMessage( + accountId = "x", + to = "bob@example.org", + subject = "Inline injection", + body = "See image", + bodyHtml = "

See

", + ), + // A crafted content id that, unsanitized, would break out of the Content-ID header and add + // its own `X-Injected` header line to the inline part. + attachments = listOf( + SendableAttachment(image, contentId = "logo@libremail\r\nX-Injected: evil", isInline = true), + ), + ) + + greenMail.waitForIncomingEmail(1) + val received = greenMail.receivedMessages.single() + // The CR/LF is stripped, so the crafted text never becomes its own header on any MIME part... + assertFalse(hasHeaderAnywhere(received, "X-Injected"), "Content-ID CR/LF must not inject a header") + // ...and the Content-ID that is emitted stays on a single line. + val cid = contentIdAnywhere(received) + assertTrue(cid != null && '\r' !in cid && '\n' !in cid, "Content-ID must be a single line: $cid") + image.delete() + } + + /** True if [part] or any nested MIME part carries a header named [name]. */ + private fun hasHeaderAnywhere(part: Part, name: String): Boolean { + if (!part.getHeader(name).isNullOrEmpty()) return true + val content = runCatching { part.content }.getOrNull() + return content is Multipart && (0 until content.count).any { hasHeaderAnywhere(content.getBodyPart(it), name) } + } + + /** The first `Content-ID` header found on [part] or any nested MIME part, or null. */ + private fun contentIdAnywhere(part: Part): String? { + part.getHeader("Content-ID")?.firstOrNull()?.let { return it } + val content = runCatching { part.content }.getOrNull() + if (content is Multipart) { + for (i in 0 until content.count) contentIdAnywhere(content.getBodyPart(i))?.let { return it } + } + return null + } + @Test fun `send delivers a message with an attachment`() = runTest { val file = File.createTempFile("libremail-report", ".txt").apply { writeText("quarterly numbers") } diff --git a/app/src/test/kotlin/org/libremail/richtext/RichTextHtmlTest.kt b/app/src/test/kotlin/org/libremail/richtext/RichTextHtmlTest.kt index 79bbecf..e5d6bbb 100644 --- a/app/src/test/kotlin/org/libremail/richtext/RichTextHtmlTest.kt +++ b/app/src/test/kotlin/org/libremail/richtext/RichTextHtmlTest.kt @@ -254,6 +254,40 @@ class RichTextHtmlTest { assertRoundTrips(content) } + @Test + fun `an unsafe inline font-family value is dropped instead of injected raw (issue 205)`() { + val content = RichTextContent( + text = "x", + spans = listOf( + RichSpan(0, 1, RichStyle.FontFamily("Arial; color:red")), + RichSpan(0, 1, RichStyle.Bold), + ), + ) + val html = RichTextHtml.toHtml(content) + // The smuggled sibling declaration must never reach the emitted style attribute... + assertFalse(html.contains("color:red"), html) + assertFalse(html.contains("Arial; color:red"), html) + assertFalse(html.contains("font-family"), html) + // ...but the run's other styling is unaffected. + assertTrue(html.contains("x"), html) + } + + @Test + fun `an unsafe base-style font-family is dropped from the wrapper (issue 205)`() { + val content = RichTextContent("hi", baseStyle = RichBaseStyle(fontCss = "Arial: red;}", fontSizePt = 12)) + val html = RichTextHtml.toHtml(content) + assertFalse(html.contains("font-family"), html) + // The safe font-size declaration in the same wrapper still emits. + assertTrue(html.contains("font-size:12pt"), html) + } + + @Test + fun `a registry font stack with quotes and commas still emits its font-family (issue 205)`() { + // The bundled FontRegistry stacks are all within the safe set, so built-in fonts are unaffected. + val content = RichTextContent("x", spans = listOf(RichSpan(0, 1, RichStyle.FontFamily("'Inter', sans-serif")))) + assertTrue(RichTextHtml.toHtml(content).contains("font-family:'Inter', sans-serif")) + } + @Test fun `base style wrapper adds no stray text or newlines`() { val restored = RichTextHtml.fromHtml("

a
b

")