[WIP] fix: Drop messages encrypted with the wrong symmetric secret#7963
[WIP] fix: Drop messages encrypted with the wrong symmetric secret#7963
Conversation
dd6b842 to
a66b23a
Compare
a66b23a to
345386d
Compare
This also removes a test that tested that load_shared_secrets() loads an old BobState. This is fine; the same test also directly tests loading the BobState.
345386d to
2171430
Compare
0c5ce90 to
db58c8f
Compare
a2f94b2 to
d4fbc57
Compare
| 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" | ||
| ); | ||
| } |
There was a problem hiding this comment.
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
}
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