Skip to content

Issue 3231 fix unsigned html alternative - #3244

Open
DenBond7 wants to merge 6 commits into
masterfrom
issue_3231_fix_unsigned-HTML-alternative
Open

Issue 3231 fix unsigned html alternative#3244
DenBond7 wants to merge 6 commits into
masterfrom
issue_3231_fix_unsigned-HTML-alternative

Conversation

@DenBond7

@DenBond7 DenBond7 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator

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/plain alternative alongside an unsigned text/html alternative.

🔒 Problem & Context

Previously, when parsing multipart/alternative emails:

  1. selectAlternativeContent did not recognize inline cleartext-signed blocks (SignedMsgBlock) in plainBlocks as signed content, causing it to fall through and select the first otherBlocks entry (unsigned text/html) for rendering.
  2. The UI presented the attacker-controlled unsigned HTML body while displaying a Signed PGP badge (verified against the plaintext alternative), violating the signature-to-content binding boundary.
  3. Evaluating signedness over nested multipart/alternative structures without selection caching resulted in $O(2^N)$ exponential recursion, allowing deeply nested alternatives to stall message processing.

🛠 What was done

  • Prioritize Signed Alternatives (PgpMsg.kt):
    Updated selectAlternativeContent to evaluate whether plainBlocks contains PGP-signed content (SIGNED_BLOCK_TYPES or isOpenPGPMimeSigned). If plainBlocks is signed and the first candidate entry in otherBlocks (the HTML alternative to be rendered) is unsigned, selectAlternativeContent now forces usePlainVersionForRendering = true.
  • Displayed Candidate Resolution (PgpMsg.kt):
    Added hasSignedDisplayedContent, which first resolves nested alternatives through getDisplayedBlocks() and then checks only the blocks that will actually be displayed. At the current alternative level, only otherBlocks.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.
  • Exponential Recursion Mitigation (AlternativeContentResolver):
    Encapsulated alternative resolution in AlternativeContentResolver backed by an IdentityHashMap selection cache. This ensures each AlternativeContentMsgBlock is evaluated at most once during traversal, reducing complexity from $O(2^N)$ to $O(N)$ and preventing performance stalling on nested structures.
  • Regression Tests (ProcessMimeMessageTest.kt & MessageDetailsFlowTest.kt):
    • Updated unit tests to verify that signed plaintext alternatives are selected for rendering over unsigned HTML alternatives, fixing HTML entity unescaping assertions.
    • Added a nested multipart/alternative regression 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.
    • Added a test case covering multipart/alternative messages with 3 alternatives (signed plaintext, unsigned 1st HTML, signed 3rd alternative) to ensure candidate evaluation logic remains secure.
    • Added a performance regression test (testProcessesDeeplyNestedAlternativesWithoutRepeatedSelection) with 25 levels of nested multipart/alternative structures to guarantee linear processing time ($O(N)$).
    • Updated UI instrumentation tests to verify that web content displays the verified signed text alongside the Signed PGP badge.

Implementation details

Before

filterBlocksViaTree ran inside:

for (block in msgBlocks)

It analyzed all MIME alternatives when determining both encryption and signature status.

For a message containing:

  • signed text/plain;
  • unsigned 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:

  1. All MIME blocks are scanned to preserve the existing encryption-status calculation.
  2. Only the blocks that will actually be displayed are resolved and checked for signatures (cached via AlternativeContentResolver).
  3. This prevents a signature from a hidden MIME alternative from being applied to displayed unsigned content. It also ensures that a signed but non-displayed child of a nested alternative cannot influence the outer alternative selection.
All MIME blocks
│
├── Check encryption across the entire MIME tree
│
├── Select the alternative that will be displayed (cached via AlternativeContentResolver)
│   └── Check signatures only for the displayed blocks
│
└── Build contentBlocks/resultBlocks
    └── Render the same selected alternative

getDisplayedBlocks() selects the content that will actually be rendered. Only those blocks are used to calculate the signature status.

The remaining loop:

for (block in msgBlocks)

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:

  • whole-message encryption analysis;
  • displayed-content signature analysis.

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):

  • Tests added or updated

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities

DenBond7 added 4 commits July 31, 2026 14:06
…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.
@DenBond7
DenBond7 marked this pull request as ready for review July 31, 2026 13:53
@DenBond7
DenBond7 requested a review from sosnovsky as a code owner July 31, 2026 13:53
@DenBond7

Copy link
Copy Markdown
Collaborator Author

@sosnovsky This one is ready

Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt Outdated
@DenBond7
DenBond7 requested a review from sosnovsky August 4, 2026 04:55
Comment thread FlowCrypt/src/main/java/com/flowcrypt/email/security/pgp/PgpMsg.kt Outdated
@DenBond7
DenBond7 requested a review from sosnovsky August 6, 2026 01:45
alternativeContentResolver: AlternativeContentResolver,
stripHtmlRootTags: Boolean = false
): FormattedContentBlockResult {
val inlineImagesByCid = mutableMapOf<String, MsgBlock>()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is one more possible security issue in line 1262:

[P1] Exclude inline images from the rejected alternative

The selection happens after plainImageBlocks has already been collected by traversing every branch with filterBlocksViaTree(allContentBlocks). With signed text/plain plus an unsigned multipart/related HTML alternative, the resolver selects the signed text, but the rejected branch’s inline image is still added to inlineImagesByCid and appended at the bottom. The message therefore displays attacker-controlled content while reporting fully signed. Collect inline images using the same alternative selection, excluding otherBlocks when 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()
        }
      }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FlowCrypt Android unsigned HTML alternative can be shown under a Signed badge

2 participants