fix: receive_imf: Don't break 1:1 chat protection if received an unverified message (#4597)

[iequidoo]

fix: receive_imf: Don't break 1:1 chat protection if received the verified 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.

feat: Move unverified messages carrying current Autocrypt key from protected 1:1 chats to ad-hoc groups

TODO: This should be squashed with the previous commit if we decide to go this way.

feat: Move unverified messages which don't introduce a new Autocrypt key from protected 1:1 chats to ad-hoc groups

This prevents protected 1:1 chats from breaking by messages sent by classical MUAs.

TODO: Some `tests::verified_chats` tests are failing, but as far as i see from the log, the tests
must be fixed, the change itself looks ok. Not fixing the tests now, not sure we will merge this as
it's just POC.

[Hocuri]

For me, the big problem with keeping the protection is that it significantly reduces the security guarantees of the verified icon.

Right now, security trainers can tell users:

To determine the safety of a chat, see if there is the verified icon at the top. If it's there (and none of your devices got hacked or confiscated), then only your chat partner(s) can read your messages, and all their messages were actually sent by them.".

With this proposal, security trainers have to tell users:

To determine the safety of a chat, check if there is the verified icon at the top, and check if there is a padlock on the message. If the verification icon is there (and none of your devices got hacked or confiscated), then only your chat partner(s) can read your messages, and all their messages with a padlock were actually sent by them.

Which seems a lot harder to keep in mind.

Remember (or alternatively, disagree with me :)) that the target audience for verified 1:1 chats are non-technical at-risk users. So, some security trainer needs to be able to explain some simple rules to them when a chat is safe and when it's not.

---

If everyone but me thinks it's fine, then I'll eventually be OK with being outvoted, but I'd first like to thoroughly discuss it and make sure that everyone does think it's fine even with these arguments, plus it may be worth it to ask someone who works with at-risk users. So, I'll click "Request Changes" again. I know we don't do this often in this repo, but I think it's good to formally make sure that all concerns are heard, especially in a security-relevant area like this where our decisions matter a lot.

Edit: Improved the second security trainer explanation

[link2xt]

You can phrase it like this, only minor change is needed:

To determine the safety of a chat, see if there is the verified icon at the top. If it's there (and none of your devices got hacked or confiscated), then only your chat partner(s) can read your messages, and all their messages with a padlock were actually sent by them.".

And these unencrypted messages are very visible, see the golden test in this PR.

[iequidoo]

And these unencrypted messages are very visible, see the golden test in this PR.

+1. For some reason i thought they will appear just w/o a padlock, but after writing the test i see it's even better.

Btw, i'd change the wording of the error message from "This message is not encrypted..." to "This message is not verified..." if the message is not signed with a verified key. It doesn't matter much if a message is encrypted (there are cameras everywhere nowadays, so you always should accept the risk of reading messages by an attacker), it's more important if a message is signed.

[link2xt]

Btw, i'd change the wording of the error message from "This message is not encrypted..." to "This message is not verified..." if the message is not signed with a verified key. It doesn't matter much if a message is encrypted (there are cameras everywhere nowadays, so you always should accept the risk of reading messages by an attacker), it's more important if a message is signed.

"Encrypted" in Delta Chat means it is encrypted+signed, as in Autocrypt standard. The argument about cameras does not make any sense, there is certainly no camera over my shoulder atm.

[iequidoo]

"Encrypted" in Delta Chat means it is encrypted+signed, as in Autocrypt standard. The argument about cameras does not make any sense, there is certainly no camera over my shoulder atm.

The problem is not only cameras, but that in general the recipient can't be sure that an incoming message wasn't read by anyone else, there are many non-technical reasons, it could be a copy-pasted message, a broadcast message, it can be forwarded to somebody else by the sender later, etc. 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. And as for me, better not to use the Autocrypt wording for human-readable error/info messages, but the phrase "is not verified".

[link2xt]

As for me this particular change is a clear improvement, it fixes the bug in a minimal way.

If we decide to get rid of a "This message is not encrypted" warning then it needs more discussion with at least @Hocuri on whether the padlock is visible enough and what security properties do we want to guarantee in this case.

[link2xt]

@Hocuri What security properties are broken by this change? All outgoing messages in verified chat are still sent out encrypted with a verified key, and all incoming messages with a padlock are signed with a verified key and encrypted. Incoming unencrypted messages are clearly displayed as a warning, but do not break verification as it is not necessary to switch to unverified key, we know the receiver will still be able to decrypt it. I don't see the reason to change any wording, for me it looks like all instructions can be kept the same without problems.

[Hocuri]

What security properties are broken by this change?

By the PR as-is: None, but many users won't understand how to access the message text, which can be equally dangerous in a high-stakes situation.

In my first comment, I missed that this PR actually proposes to make the missing signature stand out more to the user, not less, sorry about that.

I actually think that this is breaking the user workflow too much:

Msg#11:  (Contact#Contact#10): [This message is not encrypted. See 'Info' for more details] [FRESH]

Many users won't know what 'Info' is, and not be able to read the message at all.

See #4597 (comment) for an alternative solution.

Edit: Elaborate further

[iequidoo]

So, @Hocuri, are you sure that breaking the existing verification currently is better than displaying ... See 'Info' for more details warnings? For me, breaking the verification makes communication less secure, i'd better tell users where this 'Info' is.

But if we want to make users happier, i'd suggest to follow this way:

1. Enable Config::SignUnencrypted by default. Maybe only for broadcast messages, if we're afraid of breaking smth else.
2. Remove the code that cleans up signatures from unencrypted messages. As we see, it's a useful information at least for verified chats.
3. Instead of displaying [This message is not encrypted. See 'Info' for more details] warnings in verified chats, move all messages which don't introduce a new Autocrypt key for the peer, to a new ad-hoc 2-member group. This will also solve the problem for unsigned broadcast messages created by older versions of DC f.e.

But even if only "1." and "2." will be done, we could merge this PR w/o a significant impact on users.

[link2xt]

In my first comment, I missed that this PR actually proposes to make the missing signature stand out more to the user, not less

This just happened to be this way, the code to display "This message is not encrypted" was not introduced in this PR.

See #4597 (comment) for an alternative solution.

The proposed solution is a solution for broadcast lists only. This fix is about not downgrading verified chats to non-verified when we know that verified key is still valid, which may happen for other reasons, e.g. receiving an unencrypted message from K-9 Mail. Encrypting broadcast lists, sorting them to other chats etc. would still be nice to have, also to avoid "This message is not encrypted" warning, but it is not the scope of this PR. We can still merge #4644 on top of this.

[link2xt]

This PR will need test changes once #4644 is merged, because it cannot rely on broadcast lists breaking 1:1 encryption anymore.

[Hocuri]

So, @Hocuri, are you sure that breaking the existing verification currently is better than displaying ... See 'Info' for more details warnings?

For me personally, no. For 99% of users, yes.

During the user testings, it became clear to me how easily users are confused by things they don't know. E.g. two users didn't notice the very big "verification broken" symbol and didn't read the info texts at all, and when I asked them about the security of the chat, they only referred to the very small padlock on the message.

Hiding the message text is breaking the flow of the user way too much, given that in many cases they didn't opt-in to the verification but just scanned a QR code. And no matter how obvious we make it how to access the message text, there will always be users that don't understand it.

This just happened to be this way, the code to display "This message is not encrypted" was not introduced in this PR.

Yes, but so far, it was only show for verified groups, where the user opted in to the extra security. And even there, I'd be nice to improve discoverability eventually, maybe "Tap to show the message".

[iequidoo]

Added an experimental commit to move unverified messages from protected 1:1 chats to ad-hoc groups. Better to review it with whitespace changes hidden.

UPD: But @Hocuri's PR #4644 is still useful, i think it has value on its own -- even if broadcast list messages are encrypted and signed with the verified key, it's more convenient to see them in a separate chat.

[link2xt]

Let's postpone this at least until #4644 is merged. Unless we have a real scenario where a message may be received unencrypted but with an Autocrypt header other than broadcast lists, what exactly we do in this case have very low impact. I can imagine it may happen with Thunderbird and K-9 Mail, but we need a real usecase. Maybe @missytake has one as a Thunderbird user, otherwise let's not try to come up with a scenario ourselves.

[iequidoo]

Let's postpone this at least until #4644 is merged. Unless we have a real scenario where a message may be received unencrypted but with an Autocrypt header other than broadcast lists, what exactly we do in this case have very low impact. I can imagine it may happen with Thunderbird and K-9 Mail, but we need a real usecase. Maybe @missytake has one as a Thunderbird user, otherwise let's not try to come up with a scenario ourselves.

Yes, a real scenario is unclear, but the second commit is a step to prevent protected 1:1 chats from breaking by classical MUA messages. I added another experimental commit for that, it is just a POC and a couple of lines

[iequidoo]

Resolved merge conflicts and reduced the diff (removed reformatting) to avoid new ones. Though #4644 is merged, i think this one is still useful, especially the 3rd commit (but it depends on the first two) as it fixes breakage of verified 1:1 chats by classical MUA's messages. Finally, it results in less warnings for users, more secure communication, there would be no need to verify the chat again, etc. The problem is that users will have diverged message threads (not clear which one to use for communication) and there are no good names for created ad-hoc groups currently -- they look like this:

Group#Chat#11: Message from bob@example.net [2 member(s)]

As for several message threads, i think generally it's ok and maybe even good -- i wonder why can't we have multiple 1:1 chats e.g. for discussing different topics. As for which one to use by default, i guess the recent one, i.e. look it up by default (also it makes sense if the peer lost the access to DC). That's why i suggested to consider any 2-member group as a 1:1 chat (which can be verified).

So, this raises further questions and unlikely to be merged as is w/o further work, but i suggest to postpone dropping this PR.

[hpk42]

Every few days, i have the problem that my chat partner sends a non-encrypted message (thunderbird) in an otherwise e2e-encrypted chat -- and i am using the 1:1-verichat android for some weeks now. It's a little annoying but no big deal -- i accept that e2e is down, read their message (usually coming with an attachment), and the next time my chat partner uses DC to send a message, protection goes up again.
There is no indication that this flow is a problem for people yet -- and also with upcoming chatmail-instances we aim to popularize DC for use with dedicated chat accounts and the problem appears even less than in the current world of "i share this account with another MUA" (which only some people do, may more heavy chatters turn to dedicated chats eventually). Moreover, we spent a lot of time to prevent and reduce "split" groups and before creating split-groups we need to have strong indication it solves a real issue of people, and not just me or some devs.

All in all i am -1 on merging this PR.

[iequidoo]

The problem is that unencrypted messages can be forged by an attacker, so even if people use dedicated accounts for DC, they are at risk of insecure communication. Maybe DKIM signature check would help here, but it doesn't solve the problem completely. Ok, nobody carries out such massive attacks on email users now, but i'd care in advance. At least, i believe, DC should provide not weaker security guarantees than other messangers supporting e2e encryption.

Or maybe we should make it clear for users that they must use verified groups for secure communication. Even for me it looks unclear, i thought that verified 1:1 chats finally (maybe in the future) will provide a way of secure communication that doesn't break for various reasons, but now i see that it's not going to happen.

I'm fine with closing this PR if it's clear for me how we are going to provide users a secure 1:1 channel of communication. But then i'd like it's obvious in UI so that people don't think e2e encryption won't break in their chats while it actually can. Initially this PR was a try to solve the problem with unencrypted broadcast messages which was solved by @Hocuri in a different way. But broadcast messages are a very particular problem and it is a just a sign that it will reappear in the future in the scope of other functionality.