Skip to content

[WIP] fix: Drop messages encrypted with the wrong symmetric secret#7963

Open
Hocuri wants to merge 15 commits intomainfrom
hoc/drop-incorrectly-encrypted-msgs
Open

[WIP] fix: Drop messages encrypted with the wrong symmetric secret#7963
Hocuri wants to merge 15 commits intomainfrom
hoc/drop-incorrectly-encrypted-msgs

Conversation

@Hocuri
Copy link
Collaborator

@Hocuri Hocuri commented Mar 7, 2026

The tests were originally generated with AI and then reworked. Concretely,
660642c and 191f1cf were AI-generated, everything else is handwritten.

Follow-up to #7754

@Hocuri Hocuri force-pushed the hoc/drop-incorrectly-encrypted-msgs branch from dd6b842 to a66b23a Compare March 8, 2026 10:15
@Hocuri Hocuri changed the title [WIP] fix: Drop incorrectly encrypted messages [WIP] fix: Drop messages encrypted with the wrong symmetric secret Mar 8, 2026
@Hocuri Hocuri added the blocker label Mar 8, 2026
@Hocuri Hocuri force-pushed the hoc/drop-incorrectly-encrypted-msgs branch from a66b23a to 345386d Compare March 8, 2026 10:22
@Hocuri Hocuri force-pushed the hoc/drop-incorrectly-encrypted-msgs branch from 345386d to 2171430 Compare March 8, 2026 16:26
@Hocuri Hocuri force-pushed the hoc/drop-incorrectly-encrypted-msgs branch from 0c5ce90 to db58c8f Compare March 9, 2026 15:12
@Hocuri Hocuri force-pushed the hoc/drop-incorrectly-encrypted-msgs branch from a2f94b2 to d4fbc57 Compare March 9, 2026 15:21
Comment on lines +546 to +559
if let Some(expected_sender_fingerprint) = expected_sender_fingerprint {
ensure!(
!signatures.is_empty(),
"Unsigned message is not allowed to be encrypted with this shared secret"
);
ensure!(
signatures.contains_key(&expected_sender_fingerprint.parse()?),
"This sender is not allowed to encrypt with this secret key"
);
ensure!(
signatures.len() == 1,
"Too many signatures on symm-encrypted message"
);
}
Copy link
Collaborator Author

@Hocuri Hocuri Mar 9, 2026

Choose a reason for hiding this comment

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

Here, we're returning an Error if the message was encrypted with an unexpected shared secret. This is different from what we do if decryption failed, but will lead to the same result (message trashed).

This is not great, because it could open an attack vector in the future, where something different happens depending on whether the shared secret is known, and the attacker finds a way to observe it.

It wouldn't help to set mail_raw = Vec::new(); decrypted_msg = None; is_encrypted=false; mail=Err(_) in order to do the same thing as if decryption failed, because an attacker could just make mailparse::parse_mail(&mail_raw)? fail in order to force an Err(_) being returned if the shared secret is known.

So, the only thing to mitigate this would be returning an Error from mime_parser, so that the code paths are the same.

That wouldn't actually be too hard; almost all tests pass when I do this. The only complication would be keeping the "⚠️ It seems you are using Delta Chat on multiple devices that cannot decrypt each other's outgoing messages…" message working (or just remove it).

...But, maybe things are fine as-is. What do you think @link2xt ?

If we fix this, then we can add another test:

/// Control: Test that there is a similar error when the shared secret is unknown
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_unknown_secret() -> Result<()> {
    let mut tcm = TestContextManager::new();
    let alice = &tcm.alice().await;
    let bob = &tcm.bob().await;

    test_shared_secret_decryption_ex(
        bob,
        "alice@example.net",
        "Some secret unknown to Bob",
        Some(alice),
        Some("TODO whatever the error message is"),
    )
    .await
}

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant