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

Broadcast messages trigger ProtectionStatus::ProtectionBroken state #4597

Closed
link2xt opened this issue Aug 2, 2023 · 9 comments · Fixed by #4644
Closed

Broadcast messages trigger ProtectionStatus::ProtectionBroken state #4597

link2xt opened this issue Aug 2, 2023 · 9 comments · Fixed by #4644
Assignees
Labels
bug Something is not working discussion good first issue Good for newcomers verified-chats

Comments

@link2xt
Copy link
Collaborator

link2xt commented Aug 2, 2023

Broadcast list messages are always sent unencrypted. If Alice has a verified 1:1 chat with Bob and Bob sends a message to a broadcast list with Alice, Alice receives unencrypted message, gets a warning info message "Bob sent a message from another device. Tap to learn more." and 1:1 chat with Bob on Alice device is switched to "protection broken" state with a banner "Bob sent a message from another device" replaces the message input field.

Autocrypt header that Bob sends still contains the verified key, so Alice can detect that Bob is still sending from an old device that has the key rather than "another device" with a new key. On the other hand, we can still interpret this as Bob setting up another device which does not have Alice's key, but transferred the old key, e.g. via Autocrypt Setup Message, but this is unlikely.

This can also be triggered by using K-9 mail and manually switching off the encryption for a single mail without changing the encryption preference.

A receiver-side solution to this issue is to avoid triggering ProtectionBroken in the case when Autocrypt key has not changed, because 1:1 chat protection is about guaranteeing that outgoing messages are encrypted with a verified key, and this is not changed by incoming message that contains a verified key in the Autocrypt header and enabled encryption preference. Once there is a test that encryption is protection status of 1:1 chat is not degraded in this particular scenario, the issue can be closed.

There is a related bugreport about broadcast list messages resetting the ephemeral timer: #2775

There have also been discussions of enabling encryption for broadcast lists on the sender side and possible metadata leaks (OpenPGP key IDs or number of encrypted message recipients leaked) with several proposed ways to do it, e.g. sending one message for all unencrypted recipients and one message for all recipients that support encryption or sending a single message to each member to avoid leaking the number of recipients. These discussions are outside of the scope of this issue.

@link2xt link2xt added discussion bug Something is not working verified-chats good first issue Good for newcomers and removed discussion labels Aug 2, 2023
@hpk42
Copy link
Contributor

hpk42 commented Aug 7, 2023

A receiver-side solution to this issue is to avoid triggering ProtectionBroken in the case when Autocrypt key has not changed, because 1:1 chat protection is about guaranteeing that outgoing messages are encrypted with a verified key, and this is not changed by incoming message that contains a verified key in the Autocrypt header and enabled encryption preference.

The "verified" mark on 1:1 chats is not just about outgoing messages but also about being sure that, as long as you see the verified icon in the title, also incoming messages are properly encrypted+signed. At least that's true for verified group chats and by analogy we should try to make it true also for 1:1 as much as feasible.

Nevertheless, and after some in-person discussion, i think it's probably fine to follow your suggestion as a kind of temporary mitigation until we can do encrypted broadcast messages. So if an incoming 1:1 message carries an autocrypt header with the key we expect from this sender, then we don't show a warning but just display the message without the padlock. But maybe @Hocuri has more thoughts on this.

@Hocuri
Copy link
Collaborator

Hocuri commented Aug 7, 2023

I think we need a proper solution before we release verified 1:1 chats. @link2xt's suggestion does mean that the "verified" icon is downgraded quite a bit since an attacker could send a faked message into the chat.

My feeling is that leaking public key fingerprints in a broadcast list is fine, leaking the email addresses of all recipients would not be fine.

@link2xt
Copy link
Collaborator Author

link2xt commented Aug 8, 2023

an attacker could send a faked message into the chat

The message will not have a padlock and the attacker will not be able to read any responses. We also already do not degrade verified 1:1 chat for outgoing unencrypted messages. If this is a problem, maybe missing padlock could be made more visible if the chat is in a verified state, e.g. display red padlock instead of missing one on the UI side, but the chat should not be degraded to unverified if the incoming message contains verified key in the Autocrypt header as we can be sure we can still send the messages encrypted with the verified key and the receiver will be able to read them.

For broadcast encryption I suggest to open another issue as it does not have a solution yet.

@Hocuri
Copy link
Collaborator

Hocuri commented Aug 9, 2023

We also already do not degrade verified 1:1 chat for outgoing unencrypted messages.

Outgoing unencrypted messages can't be sent by an attacker though.

If this is a problem, maybe missing padlock could be made more visible if the chat is in a verified state, e.g. display red padlock instead of missing one on the UI side

I'm not convinced users will understand this, and also it's quite some effort (requiring changes in all UIs) given that it's just meant to be a temporary solution - I guess that we want to go for encrypted broadcast lists eventually?

FTR (without wanting to start a discussion about this): If it should turn out that encrypting is too difficult, yet another solution would be to make our broadcast lists more channel-like and create an extra chat on the receiver side. Code-wise that's very easy to implement, just need to add a List-Id header when sending the message. This does require new UX discussions though, like, what do we call them? Broadcast lists? Mailing lists? Channels? (IIRC when implementing Broadcast Lists, we said that we'll postpone the decision of whether they should be channel-like or broadcast-like, since it's an experimental feature, anyway)

@link2xt
Copy link
Collaborator Author

link2xt commented Aug 9, 2023

Outgoing unencrypted messages can't be sent by an attacker though.

They can be sent by an attacker if the server does not check the From: field with some spam filter or if the attacker controls your server rather than the server of the contact.

given that it's just meant to be a temporary solution

It's not a temporary solution, because no matter what you do on the sender side or how secure your servers are it is always possible for the contact to send you an unencrypted mail with Autocrypt header back from an old DC version, from K-9 mail and so on. The issue is only about changing how DC reacts to receiving such message in a verified 1:1 chat.

@iequidoo
Copy link
Collaborator

iequidoo commented Aug 10, 2023

It's not a temporary solution, because no matter what you do on the sender side or how secure your servers are it is always possible for the contact to send you an unencrypted mail with Autocrypt header back from an old DC version, from K-9 mail and so on. The issue is only about changing how DC reacts to receiving such message in a verified 1:1 chat.

That's why i suggested to move unverified (i.e. unsigned messages -- the encryption doesn't matter) to separate 2-member ad-hoc groups in #4188 (see "Solution 1.1" and the following discussion). This can be done w/o much work in UI also. And it looks not quite right to me that we have the only 1:1 chat, it's a try to merge in a single thread everything that can't be merged by nature. Better we could have many 1:1 chats, but just would look up the most recent one by default.

Anyway, the solution with displaying a red/broken padlock looks acceptable and probably more simple in the current design.

EDIT: But as for unencrypted incoming messages that doesn't change the Autocrypt key (moreover, if they are signed with it), i agree that we should keep the protection. And if they are unsigned, currently the best option is to make that fact more noticeable.

@iequidoo iequidoo self-assigned this Aug 17, 2023
iequidoo added a commit that referenced this issue Aug 24, 2023
Also don't set protection to ProtectionBroken if it's already set to it.
iequidoo added a commit that referenced this issue Aug 25, 2023
Also don't set protection to ProtectionBroken if it's already set to it.
iequidoo added a commit that referenced this issue Aug 25, 2023
Also don't set protection to ProtectionBroken if it already is.

Co-authored-by: Hocuri <hocuri@gmx.de>
iequidoo added a commit that referenced this issue Aug 25, 2023
Also don't set protection to ProtectionBroken if it already is.

Co-authored-by: Hocuri <hocuri@gmx.de>
iequidoo added a commit that referenced this issue Aug 26, 2023
…ified Autocrypt key (#4597)

For example, broadcast list messages are sent unencrypted, but their Autocrypt header still contains
the verified key. Thus we're sure that we still can encrypt to the peer and there's no need to break
the existing protection.
iequidoo added a commit that referenced this issue Aug 26, 2023
Also don't set protection to ProtectionBroken if it already is.

Co-authored-by: Hocuri <hocuri@gmx.de>
iequidoo added a commit that referenced this issue Aug 26, 2023
…ified Autocrypt key (#4597)

For example, broadcast list messages are sent unencrypted, but their Autocrypt header still contains
the verified key. Thus we're sure that we still can encrypt to the peer and there's no need to break
the existing protection.
@Hocuri
Copy link
Collaborator

Hocuri commented Aug 26, 2023

Outgoing unencrypted messages can't be sent by an attacker though.

They can be sent by an attacker if the server does not check the From: field with some spam filter or if the attacker controls your server rather than the server of the contact.

It seems unlikely that this would benefit an attacker - I know that I didn't send this message, so even in the absence of a warning, I know that something is wrong. If it should turn out that it's actually useful to an attacker, then we should reconsider this rule, and e.g. move the message to a 2-member ad-hoc group as suggested by @iequidoo.


Since we now have a PR for keeping the protection when an unsigned message comes in, let's continue the discussion whether we should do this there.

iequidoo added a commit that referenced this issue Aug 27, 2023
…ified Autocrypt key (#4597)

For example, broadcast list messages are sent unencrypted, but their Autocrypt header still contains
the verified key. Thus we're sure that we still can encrypt to the peer and there's no need to break
the existing protection.
@Hocuri
Copy link
Collaborator

Hocuri commented Aug 30, 2023

@hpk42, @r10s and me had a video call yesterday, what do you think about this?

Independently of this issue, it would be nice to show broadcast lists as a separate chat on the receiver's side. Back in the days, we decided to show them in the 1:1 chat because that's what WhatsApp does, but

  1. Even in WhatsApp, they seem not to be that polished or widely used, and apparently people would prefer if they were shown as a chat
  2. We got feedback that it would be nicer if they were shown as a chat, plus some problems like this would be fixed.

Then this issue here will go away.

In any case, we're not in a hurry since the next release with verified 1:1 chats is quite far out, so no need for temporary workarounds.

Independently, we can try to encrypt broadcast lists if this only leaks key fingerprints and no email addresses.

Broadcast lists will still stay experimental for now.

Alternative

One other idea @hpk42 had:

If the sender signs the outgoing message (but doesn't encrypt), then the receiver can also be sure that this is not an attack, and it could be OK to show them (without a padlock + without a warning) in the verified 1:1 chat. It would still be unfortunate that it's one more case besides unencrypted outgoing messages where unencrypted messages are shown in the verified 1:1 chat, since it might lead people to overestimate the security, but

@iequidoo
Copy link
Collaborator

iequidoo commented Aug 30, 2023

Independently of this issue, it would be nice to show broadcast lists as a separate chat on the receiver's side. Back in the days, we decided to show them in the 1:1 chat because that's what WhatsApp does, but

  1. Even in WhatsApp, they seem not to be that polished or widely used, and apparently people would prefer if they were shown as a chat

  2. We got feedback that it would be nicer if they were shown as a chat, plus some problems like this would be fixed.

+1. I'd also prefer broadcast messages to be shown as separate chats. But the problem discussed here isn't only about broadcast messages, but about all signed (verified) unencrypted messages. If we fix it in common, this particular bug goes away too.

One other idea @hpk42 had:

If the sender signs the outgoing message (but doesn't encrypt), then the receiver can also be sure that this is not an attack, and it could be OK to show them (without a padlock + without a warning) in the verified 1:1 chat. It would still be unfortunate that it's one more case besides unencrypted outgoing messages where unencrypted messages are shown in the verified 1:1 chat, since it might lead people to overestimate the security, but

+1. Moreover, i already proposed that in #4632 (comment):

It's more important that incoming messages are signed (verified) and outgoing ones are encrypted. So, i think it's ok to display unencrypted signed incoming messages just w/o a padlock, but as for not properly signed (unverified) ones, better to keep the current behaviour with displaying the error.

But afair, the current code just removes all signatures if the message isn't encrypted. I have no idea why that was done initially, but it's easy to fix, probably even just removing this extra code.

EDIT: Another problem is that currently we don't sign unencrypted messages. I.e. by default -- we have Config::SignUnencrypted for that. I remember that there are some opinions that it's useless in real life and just creates more cases to care of, but anyway we should handle such messages in some reasonable way, so i think it still makes sense.

iequidoo added a commit that referenced this issue Sep 1, 2023
…ified Autocrypt key (#4597)

For example, broadcast list messages are sent unencrypted, but their Autocrypt header still contains
the verified key. Thus we're sure that we still can encrypt to the peer and there's no need to break
the existing protection.
iequidoo added a commit that referenced this issue Sep 8, 2023
…ified Autocrypt key (#4597)

For example, broadcast list messages are sent unencrypted, but their Autocrypt header still contains
the verified key. Thus we're sure that we still can encrypt to the peer and there's no need to break
the existing protection.
iequidoo added a commit that referenced this issue Oct 1, 2023
…ified Autocrypt key (#4597)

For example, broadcast list messages are sent unencrypted, but their Autocrypt header still contains
the verified key. Thus we're sure that we still can encrypt to the peer and there's no need to break
the existing protection.
iequidoo added a commit that referenced this issue Oct 26, 2023
…ified Autocrypt key (#4597)

For example, broadcast list messages are sent unencrypted, but their Autocrypt header still contains
the verified key. Thus we're sure that we still can encrypt to the peer and there's no need to break
the existing protection.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working discussion good first issue Good for newcomers verified-chats
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants