Secure-Join protocol may have bugs / produce inconsistencies

[flub]

• Bob verifies Alice even if Alice's vg-member-added or vc-contact-confirm message was not signed/encrypted
• QR-joining a non-verified group relies on this bug. The current flow of messages does not allow for Bob to verify Alice in this case, but the implementation verifies Alice.

So question is how should we fix this? The protocol for QR-joining non-verified groups needs to change, but which way?

• Do we want to keep the property that QR-joining a non-verified group verifies the inviter & invitee?
• If not can we come up with a much shorter group-join option?

[flub]

For a non-secure-join we could be concerned about someone changing the first message from bob to alice. In which case we need to run a full setup-contact flow first, after which alice can join bob to the group. For this we'd probably need tweaks to the QR-code and state so alice knows there is still something to do at the end.

[hpk42]

I am not convinced yet it is true what you are claiming quite factually here :)
Could you state the line numbers in securejoin.rs where you think something goes wrong or is missing?
To me appears on first glance that the checks for proper encrypt+signed status of messages is ok for the setup-contact part of the securejoin protocols.

[flub]

https://github.com/deltachat/deltachat-core-rust/blob/8f8db0c43108e8a9386f89df23bee7ed45836a3f/src/securejoin.rs#L602 is problematic. The problem is that for bob to know alice is verified in the verified-group protocol alice needs to send a signed & encrypted vg-member-added. But instead we are trying to allow this to be not verified while later verifying alice anyway: https://github.com/deltachat/deltachat-core-rust/blob/8f8db0c43108e8a9386f89df23bee7ed45836a3f/src/securejoin.rs#L626.

From verified-group or setup-contact protocol point of view at this point we need a signed & encrypted message from alice. For joining a non-verified group alice may need to send the Chat-Group-Member-Added message plain text.

And finally I'm not sure if bob can know if they're joining a verified group or not (the line at https://github.com/deltachat/deltachat-core-rust/blob/8f8db0c43108e8a9386f89df23bee7ed45836a3f/src/securejoin.rs#L604 just always says it is not a verified group because that's its failure mode and the chat doesn't exist yet so it fails)

[hpk42]

On Wed, Jan 15, 2020 at 01:13 -0800, Floris Bruynooghe wrote: https://github.com/deltachat/deltachat-core-rust/blob/8f8db0c43108e8a9386f89df23bee7ed45836a3f/src/securejoin.rs#L602 is problematic. The problem is that for bob to know alice is verified in the verified-group protocol alice needs to send a signed & encrypted vg-member-added. But instead we are trying to allow this to be not verified while later verifying alice anyway: https://github.com/deltachat/deltachat-core-rust/blob/8f8db0c43108e8a9386f89df23bee7ed45836a3f/src/securejoin.rs#L626.

L626 Bob calling "mark_peer_as_verified()" will return an error if Alice's autocrypt-key does not match the QR-mediated key-fingerprint of Alice. So Bob will only mark Alice as verified if she has been sending the proper key (in the previous step 4 with the vc-auth-required message). There is thus no chance here that Bob will mark Alice wrongly as verified.

From verified-group or setup-contact protocol point of view at this point we need a signed & encrypted message from alice. For joining a non-verified group alice may need to send the Chat-Group-Member-Added message plain text.

An attacker could replace alice's member-added message to Bob with a different cleartext Member-added message so that a) Bob thinks he joined an opportunistic group b) Alice thinks she added Bob to a verified group Note1, however, that the verified group members, which Alice produced the QR-invite code for, will send encrypted messages to Bob's actual key. The attacker can not gain access to those messages. Note2, that Bob might send cleartext messages to - what he believes to be - the opportunistic group. But those messages will not appear as proper content in the verified chat of the verified group Alice invited for. Note3, that the attacker can not send a modified encrypted member-added message because he would need to sign it with Alice's proper key which he doesn't have. Regarding Note3, however, i think we need to modify https://github.com/deltachat/deltachat-core-rust/blob/master/src/securejoin.rs#L614 to ensure if the message is encrypted that it is signed with alice's proper key. This Note3 attack should be addressed in the current code because otherwise Bob could send an encrypted message that the attacker can read -- but Bob will do this to an opportunistic group, i.e. his chat UX does not carry a "verified" sign. Opportunistic group are naturally vulnerable to active attacks so Bob has not been mislead but it's still better to fix it.

And finally I'm not sure if bob can know if they're joining a verified group or not (the line at https://github.com/deltachat/deltachat-core-rust/blob/8f8db0c43108e8a9386f89df23bee7ed45836a3f/src/securejoin.rs#L604 just always says it is not a verified group because that's its failure mode and the chat doesn't exist yet so it fails)

Let's indeed consider extending the QR-invite code to include information whether the group-to-be-joined is a verified group or not. See #1178.

[flub]

A very rough note: the claims to this being correct mean that the https://github.com/deltachat/deltachat-core-rust/blob/8f8db0c43108e8a9386f89df23bee7ed45836a3f/src/securejoin.rs#L626 line is sufficient to verify alice and even that this can be done in the vg-member-added step of bob. This also implies that all of https://github.com/deltachat/deltachat-core-rust/blob/8f8db0c43108e8a9386f89df23bee7ed45836a3f/src/securejoin.rs#L602-L624 could be removed.

[flub]

Regarding Note3, however, i think we need to modify
https://github.com/deltachat/deltachat-core-rust/blob/master/src/securejoin.rs#L614
to ensure if the message is encrypted that it is signed with alice's proper key.

I think the encrypted_and_signed() call you point to already does that? it ensures the message is signed with the key from the QR code

[r10s]

yip, i also think, the whole vg_expect_encrypted part can be removed. it can be easily bypassed anyway and seems not to add any security.

[flub]

I believe the discussion at nextleap-project/countermitm#83 points out that when we do not check whether vc-contact-confirm|vg-member-added is signed&encrypted Bob can only mark Alice as uni-directionally verified and not as bi-directionally verified. But uni-directionally verified is not a status we or countermitm currently use (and is likely only going to cause confusion to users)

[flub]

Just a quick note I'm looking at finally resolving this. I'm investigating introducing a non-user-visible UnidirectionalVerified status (or rather start using the existing one). I'll try and describe the effect of that on the 3 scenarios: 1. setup-contact 2. join-group and 3. secure-join