Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decrypt inline PGP in text/plain parts #5777

Open
link2xt opened this issue Jul 15, 2024 · 12 comments
Open

Decrypt inline PGP in text/plain parts #5777

link2xt opened this issue Jul 15, 2024 · 12 comments
Labels
enhancement New feature or request

Comments

@link2xt
Copy link
Collaborator

link2xt commented Jul 15, 2024

Delta Chat currently sends PGP/MIME messages and does not support PGP/Inline.

This has been requested before as an issue and on the forum:

For minimal support we need to detect text/plain parts that start with ----BEGIN PGP MESSAGE----- and end with ----END PGP MESSAGE----- and replace them with the decrypted contents if possible. This should not affect "encrypted" (padlock) status of the message, no signatures need to be verified (the message will be displayed as unprotected anyway) and no error should be reported if decryption fails, just keep the text part as is. Also no need to support messages which have only some part encrypted. PGP/Inline can also encrypt attachments, decrypting them is also out of scope for this issue.

@link2xt link2xt added the enhancement New feature or request label Jul 15, 2024
@iequidoo
Copy link
Collaborator

the message will be displayed as unprotected anyway

What do you mean by this if presence of a padlock shouldn't be affected by the message content anyway?

@link2xt
Copy link
Collaborator Author

link2xt commented Jul 16, 2024

the message will be displayed as unprotected anyway

What do you mean by this if presence of a padlock shouldn't be affected by the message content anyway?

If the message consists of a single text/plain part and the only contents of this part is an inline PGP message encrypted and signed by a valid key someone may expect that it should have a padlock. In Delta Chat it will not have a padlock and will be treated the same as unencrypted message.

@iequidoo
Copy link
Collaborator

I'd also suggest that if a text/plain part only containing an inline PGP message is inside another signed part in turn (e.g. inside encrypted+signed message, though i have no idea why this can happen), then don't decrypt this inline PGP message at all if we aren't going to check its signatures, otherwise the decrypted inline message with no valid signatures can be shown with a padlock.

@link2xt
Copy link
Collaborator Author

link2xt commented Jul 17, 2024

otherwise the decrypted inline message with no valid signatures can be shown with a padlock.

If the whole message is encrypted+signed with PGP/MIME, then it should have a padlock. The message has valid signatures, it is signed as a whole. If it is double-encrypted for some reason, it does not make it less secure.

If there is an inline PGP inside a text part it can as well be decrypted, I see no problem with displaying this message with a padlock. No need to complicate the code with conditions to skip decrypting text/plain parts if the message was encrypted+signed outside.

Anyway, such messages are likely never sent.

@link2xt
Copy link
Collaborator Author

link2xt commented Jul 17, 2024

Thinking more about security of this, it seems to enable replay attacks. Someone can take an encrypted message and send it as inline PGP to someone who can decrypt it. They will decrypt the message and treat as insecure message, potentially replying to it in plaintext.

This feature should be disabled for bots and should not replace inline PGP with plaintext if decrypted plaintext looks like encrypted MIME message, e.g. has a line starting with from: or mime-version: before the first blank line.

@iequidoo
Copy link
Collaborator

If the whole message is encrypted+signed with PGP/MIME, then it should have a padlock. The message has valid signatures, it is signed as a whole.

It's signed as a whole, but then the user should see exactly what is [properly] signed (i.e. not a decrypted part of it). Although such double-encrypted messages are not expected, it's only a matter of checking Mimeparser::was_encrypted() afaiu.

Thinking more about security of this, it seems to enable replay attacks. Someone can take an encrypted message and send it as inline PGP to someone who can decrypt it. They will decrypt the message and treat as insecure message, potentially replying to it in plaintext.

So, signatures of an inline PGP still must be checked. Otherwise, even if the user won't reply to the message citing it, they would think that the sender knows the message content which is also a successful attack. If there's no valid signature corresponding to the From address, such an inline PGP must be shown without decryption.

This feature [...] should not replace inline PGP with plaintext if decrypted plaintext looks like encrypted MIME message

This doesn't help if the inline PGP is taken from another message with this inline PGP, so maybe don't complicate the code with such checks? It seems that checking signatures is sufficient as PGP messages are signed-then-encrypted as far as i remember.

@link2xt
Copy link
Collaborator Author

link2xt commented Jul 20, 2024

So, signatures of an inline PGP still must be checked.

In cases described on the forum such as using Mailvelope we are not going to have the key because Mailvelope does not support Autocrypt: mailvelope/mailvelope#535

So given that we cannot check the signature, maybe indeed better only decrypt inline PGP if the message is not a PGP/MIME message.

To indicate that the message was decrypted it is also fine to keep ----BEGIN PGP MESSAGE----- and ----END PGP MESSAGE-----. For advanced users who copy-paste the text to gpg anyway this is not surprising and will discourage replaying messages as they will not look normal.

@iequidoo
Copy link
Collaborator

iequidoo commented Jul 20, 2024

Then i suggest instead of ----BEGIN PGP MESSAGE----- prepend the message with a warning like "The sender may not be the author of this decrypted message and may not know its content". Then it's fine to decrypt even "double-encrypted" messages shown with padlock.

@link2xt
Copy link
Collaborator Author

link2xt commented Jul 20, 2024

a warning like "The sender may not be the author of this decrypted message and may not know its content"

Well, that is a bit too much for a non-practical attack that is probably never executed. An attacker can also achieve the same by simply relaying a PGP/MIME message that does not have protected From:, it will be displayed without any warning at all, just without a padlock. So protecting against PGP/Inline relaying does not gain much. Besides, this scary warning will need to be translated.

To summarize, inline PGP should be decrypted if the message is within a text/plain MIME part (could be the root part), spans the whole MIME part (starts with ----BEGIN PGP MESSAGE----- and ends with -----END PGP MESSAGE-----), root message is not signed/encrypted (i.e. it is not a PGP/MIME message). The signatures should not be checked and the message should have no padlock.

@iequidoo
Copy link
Collaborator

An attacker can also achieve the same by simply relaying a PGP/MIME message that does not have protected From:, it will be displayed without any warning at all, just without a padlock.

It can even have protected From:, but if it doesn't have valid signatures (in our view, because we e.g. don't yet know the author's pubkey which may be attached in the same message btw, or the Autocrypt header was stripped by an attacker), such a message will be just shown w/o a padlock. Also if we quote this message later, the quote won't be protected. While all this isn't about protected chats, it's still a successful attack because the user won't be aware that the message is actually confidential, moreover quoting such a message reveals its content to the whole network. This breaks the capability of contacting the user "anonymously" (i.e. w/o the previous two-side key exchange) and looks like an interoperability issue. Ok, the chat isn't protected, so e2ee isn't guaranteed, but at least i'd expect some warning in this case that makes me to take e.g. Thunderbird to gain full control over what's happening. Currently i can't just publish my Delta Chat PGP key and say "contact me confidentially using this".

@link2xt
Copy link
Collaborator Author

link2xt commented Jul 21, 2024

It can even have protected From:

As far as I understand there is no place for protected From: with inline PGP, decrypted text is just plaintext without any headers.

Also if we quote this message later, the quote won't be protected.

This is already the case for encrypted but not signed messages, e.g. if someone uses a client without Autocrypt support to send a PGP/MIME message to Delta Chat this can also happen. I suggest we ignore this in this issue to avoid expanding the scope. For PGP/MIME this will be solved once we protect Autocrypt header so it will be impossible to decrypt but fail to verify the signature. For PGP/Inline I am not sure we need to care that much, if the sender wants to have the most security including the message not being quoted they should have switched from PGP/inline to PGP/mime long ago.

@iequidoo
Copy link
Collaborator

iequidoo commented Jul 21, 2024

As far as I understand there is no place for protected From: with inline PGP, decrypted text is just plaintext without any headers.

No, i'm talking about current behaviour:

            if let (Some(inner_from), true) = (inner_from, !signatures.is_empty()) {
                if addr_cmp(&inner_from.addr, &from.addr) {
                    from_is_signed = true;
                    from = inner_from;
                } else {
                    // There is a From: header in the encrypted &                                                                                                                                                    
                    // signed part, but it doesn't match the outer one.                                                                                                                                              
                    // This _might_ be because the sender's mail server                                                                                                                                              
                    // replaced the sending address, e.g. in a mailing list.                                                                                                                                         
                    // Or it's because someone is doing some replay attack.                                                                                                                                          
                    // Resending encrypted messages via mailing lists                                                                                                                                                
                    // without reencrypting is not useful anyway,                                                                                                                                                    
                    // so we return an error below.                                                                                                                                                                  
                    warn!(
                        context,
                        "From header in signed part doesn't match the outer one",
                    );

                    // Return an error from the parser.                                                                                                                                                              
                    // This will result in creating a tombstone                                                                                                                                                      
                    // and no further message processing                                                                                                                                                             
                    // as if the MIME structure is broken.                                                                                                                                                           
                    bail!("From header is forged");

So, if we don't find valid signatures, we accept the message. I think we should reject it as well, otherwise the outer From can be forged and if someone tries to contact the user confidentially attaching a public key, the user would see the wrong address and may reply to that address. Moreover, the reply would be unencrypted. (EDIT: fixed in 04fd2cd.) That's why i also think we should warn users about encrypted messages whose signatures can't be checked, but this is a separate topic.

This is already the case for encrypted but not signed messages, e.g. if someone uses a client without Autocrypt support to send a PGP/MIME message to Delta Chat this can also happen.

Yes, or for messages signed with an attached key, or if the Autocrypt header was stripped off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants