chore(security): validate Content-ID before MIME/Graph use #207
@@ -111,9 +111,10 @@ internal fun buildSendMailPayload(message: OutgoingMessage, attachments: List<Se
|
||||
.put("name", attachment.file.name)
|
||||
.put("contentBytes", Base64.getEncoder().encodeToString(attachment.file.readBytes()))
|
||||
// An inline image the HTML references as `cid:contentId` — Graph renders it in the body
|
||||
// rather than listing it as a downloadable attachment.
|
||||
// rather than listing it as a downloadable attachment. The content id is sanitized of
|
||||
// control characters before it enters the payload (issue #204, defense-in-depth).
|
||||
if (attachment.isInlineImage) {
|
||||
obj.put("isInline", true).put("contentId", attachment.contentId)
|
||||
obj.put("isInline", true).put("contentId", sanitizeContentId(attachment.contentId))
|
||||
}
|
||||
items.put(obj)
|
||||
}
|
||||
|
||||
@@ -14,3 +14,12 @@ data class SendableAttachment(val file: File, val contentId: String? = null, val
|
||||
/** True only for a genuine inline image (inline flag set and a content id to reference it by). */
|
||||
val isInlineImage: Boolean get() = isInline && contentId != null
|
||||
}
|
||||
|
||||
/**
|
||||
* Strips CR/LF and other ISO control characters from a [SendableAttachment.contentId] before it is
|
||||
* emitted as a MIME `Content-ID` header ([SmtpSender]) or a Graph JSON field ([GraphSender]), so a
|
||||
* contentId that ever became attacker-influenced could not inject a header line. Today's ids are
|
||||
* app-generated (`img-<uuid>@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() }
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
|
||||
@@ -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 = "<p><img src=\"cid:logo@libremail\"></p>")
|
||||
// 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()
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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 = "<p>See <img src=\"cid:logo@libremail\"></p>",
|
||||
),
|
||||
// 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") }
|
||||
|
||||
Reference in New Issue
Block a user