Issue 3231 fix unsigned html alternative - #3244
Conversation
…HTML - Ensure selectAlternativeContent renders signed text/plain alternative when text/html is unsigned. - Prevent displaying attacker-controlled unsigned HTML under a valid Signed PGP badge. - Update unit and UI regression tests for signed-plaintext-unsigned-html-alternative fixtures.
|
@sosnovsky This one is ready |
…rnative selection
| alternativeContentResolver: AlternativeContentResolver, | ||
| stripHtmlRootTags: Boolean = false | ||
| ): FormattedContentBlockResult { | ||
| val inlineImagesByCid = mutableMapOf<String, MsgBlock>() |
There was a problem hiding this comment.
there is one more possible security issue in line 1262:
[P1] Exclude inline images from the rejected alternative
The selection happens after
plainImageBlockshas already been collected by traversing every branch withfilterBlocksViaTree(allContentBlocks). With signedtext/plainplus an unsignedmultipart/relatedHTML alternative, the resolver selects the signed text, but the rejected branch’s inline image is still added toinlineImagesByCidand appended at the bottom. The message therefore displays attacker-controlled content while reporting fully signed. Collect inline images using the same alternative selection, excludingotherBlockswhen the plain branch wins, and add an image regression case.
with such fix:
...
val imagesAtTheBottom = mutableListOf<MsgBlock>()
val plainImageBlocks = alternativeContentResolver.getInlineImageBlocksForRendering(
allContentBlocks
)
for (plainImageBlock in plainImageBlocks) { ... }
...
fun getInlineImageBlocksForRendering(blocks: List<MsgBlock>): List<MsgBlock> =
blocks.flatMap { block ->
when {
block is AlternativeContentMsgBlock -> {
val selection = select(block)
val selectedBlocks = when {
selection.usePlainVersionForRendering -> selection.displayedBlocks
!hasSignedDisplayedContent(selection.displayedBlocks) -> block.otherBlocks
else -> selection.displayedBlocks + block.otherBlocks.drop(1).filter {
MimeUtils.isPlainImgAtt(it) && it.isOpenPGPMimeSigned
}
}
getInlineImageBlocksForRendering(selectedBlocks)
}
MimeUtils.isPlainImgAtt(block) -> listOf(block)
else -> emptyList()
}
}
This PR resolves a security vulnerability where an unsigned HTML alternative body could be displayed while simultaneously presenting a Signed PGP badge when an email contains a cleartext/inline signed
text/plainalternative alongside an unsignedtext/htmlalternative.🔒 Problem & Context
Previously, when parsing
multipart/alternativeemails:selectAlternativeContentdid not recognize inline cleartext-signed blocks (SignedMsgBlock) inplainBlocksas signed content, causing it to fall through and select the firstotherBlocksentry (unsignedtext/html) for rendering.multipart/alternativestructures without selection caching resulted in🛠 What was done
PgpMsg.kt):Updated
selectAlternativeContentto evaluate whetherplainBlockscontains PGP-signed content (SIGNED_BLOCK_TYPESorisOpenPGPMimeSigned). IfplainBlocksis signed and the first candidate entry inotherBlocks(the HTML alternative to be rendered) is unsigned,selectAlternativeContentnow forcesusePlainVersionForRendering = true.PgpMsg.kt):Added
hasSignedDisplayedContent, which first resolves nested alternatives throughgetDisplayedBlocks()and then checks only the blocks that will actually be displayed. At the current alternative level, onlyotherBlocks.take(1)is considered because it represents the rendering candidate. This prevents hidden or later signed parts inside nested alternatives from incorrectly influencing the outer alternative selection.AlternativeContentResolver):Encapsulated alternative resolution in
AlternativeContentResolverbacked by anIdentityHashMapselection cache. This ensures eachAlternativeContentMsgBlockis evaluated at most once during traversal, reducing complexity fromProcessMimeMessageTest.kt&MessageDetailsFlowTest.kt):multipart/alternativeregression test where the nested displayed HTML is unsigned but a later hidden nested part is signed. The test verifies that the valid signed outer plaintext is still selected and that hidden signed content cannot influence the selection.multipart/alternativemessages with 3 alternatives (signed plaintext, unsigned 1st HTML, signed 3rd alternative) to ensure candidate evaluation logic remains secure.testProcessesDeeplyNestedAlternativesWithoutRepeatedSelection) with 25 levels of nestedmultipart/alternativestructures to guarantee linear processing time ($O(N)$).Implementation details
Before
filterBlocksViaTreeran inside:It analyzed all MIME alternatives when determining both encryption and signature status.
For a message containing:
text/plain;text/html;the function detected the signature in the plaintext alternative, while the renderer selected the HTML alternative.
As a result, unsigned HTML content was displayed with a Signed badge.
After
The processing is now split into three sequential phases:
AlternativeContentResolver).getDisplayedBlocks()selects the content that will actually be rendered. Only those blocks are used to calculate the signature status.The remaining loop:
now only separates content blocks from result blocks for formatting.
Why
Signature status must describe the content that is actually displayed.
Moving signature analysis outside the original loop separates:
This prevents a signature from a hidden MIME alternative from being applied to the alternative selected for rendering.
close #3231
Tests (delete all except exactly one):
To be filled by reviewers
I have reviewed that this PR... (tick whichever items you personally focused on during this review):