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

feat: add secondary verified key #4898

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented Nov 1, 2023

When a key is gossiped for the contact in a verified chat,
it is stored in the secondary verified key slot.

The messages are then encrypted to the secondary verified key
if they are also encrypted to the contact introducing this secondary key.

Chat-Group-Member-Added no longer updates the verified key.
Verified group recovery only relies on the secondary verified key.

When a message is received from a contact
signed with a secondary verified key,
secondary verified key replaces the primary verified key.
When verified key is changed for the contact
in response to receiving a message
signed with a secondary verified key,
"Setup changed" message is added
to the same chat where the message is received.

Fixes #4541

@link2xt link2xt force-pushed the link2xt/encrypt-to-secondary-key branch 3 times, most recently from 9c4d5a8 to 5421857 Compare November 3, 2023 17:56
@hpk42
Copy link
Contributor

hpk42 commented Nov 8, 2023

moving secondary keys-approach to resurrection -- let's discuss sometime if we still need them, in context of https://github.com/deltachat/securejoin/blob/main/uxscenarios/resetup.md

@hpk42 hpk42 closed this Nov 8, 2023
@link2xt link2xt reopened this Nov 9, 2023
@link2xt
Copy link
Collaborator Author

link2xt commented Nov 9, 2023

Moving out of the project resurrection.

Without the secondary key if we allow "Member added" message to overwrite both the verified key and autocrypt key, an attacker who has a verified chat with the bot can create a verified group with the bot and a victim and start changing the key of the victim for the bot by constantly "adding" the victim to a verified group. An attacker may even send the message only to the bot (add only the bot in RCPT TO) and send "member removed" immediately afterwards so the bot leaving the groups will not notify the victim about the existence of the group. This would break all the communication of the victim and the bot.

If an attacker can only change the secondary key of the victim for the bot and the bot uses secondary key only in groups shared between the victim and the attacker, chats where the attacker is not present (1:1 chat and groups for which the attacker does not know the group ID) will not be affected.

@link2xt link2xt force-pushed the link2xt/encrypt-to-secondary-key branch 4 times, most recently from 91be68b to 14897cc Compare November 9, 2023 14:23
@@ -82,13 +80,24 @@ pub struct Peerstate {
/// Fingerprint of the verified public key.
pub verified_key_fingerprint: Option<Fingerprint>,

/// The address that verified this verified key.
pub verifier: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this an introducer? Just because someone introduces a verified key does not make them the verified of that key so i think "introducer" is better, and also matches UI.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This field was simply moved around the struct. But yes, we can rename everything in a single commit on top of this adding a migration and renaming all the columns.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR introduces several new names and columns on the Rust-level -- didn't mean to suggest to rename existing database columns. But if you think database/rust-code consistency is more important than UI/core-naming then at least don't say "the address that verified this key" in a new doc comment because that's not the true meaning.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed the comments where possible.

Copy link
Contributor

@hpk42 hpk42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good to me so far apart from the "verified" naming, which should be globally "introducer" (EDIT: not so important, but at least clarify things in the doc comments then).

@link2xt
Copy link
Collaborator Author

link2xt commented Nov 9, 2023

Chat-Group-Member-Added recovery test now also works without actually looking at the header.

peerstate.verified_key_fingerprint =
peerstate.secondary_verified_key_fingerprint.take();
peerstate.verifier = peerstate.secondary_verifier.take();
peerstate.save_to_db(&context.sql).await?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we should do something similar to peerstate.handle_setup_change but without AEAP, just directly iterate over all chats with the contact and spam stock_str::contact_setup_changed(context, &self.addr).await.

@link2xt link2xt force-pushed the link2xt/encrypt-to-secondary-key branch from b8f5541 to 70d07e6 Compare November 9, 2023 19:11
@link2xt link2xt marked this pull request as ready for review November 9, 2023 21:01
@link2xt link2xt force-pushed the link2xt/encrypt-to-secondary-key branch from fcd1f0b to 3b0a1cd Compare November 9, 2023 21:06
@link2xt link2xt marked this pull request as draft November 9, 2023 21:09
@link2xt link2xt force-pushed the link2xt/encrypt-to-secondary-key branch from 3b0a1cd to 734eb00 Compare November 9, 2023 21:10
@link2xt

This comment was marked as outdated.

When a key is gossiped for the contact in a verified chat,
it is stored in the secondary verified key slot.

The messages are then encrypted to the secondary verified key
if they are also encrypted to the contact introducing this secondary key.

Chat-Group-Member-Added no longer updates the verified key.
Verified group recovery only relies on the secondary verified key.

When a message is received from a contact
signed with a secondary verified key,
secondary verified key replaces the primary verified key.
When verified key is changed for the contact
in response to receiving a message
signed with a secondary verified key,
"Setup changed" message is added
to the same chat where the message is received.
@link2xt link2xt force-pushed the link2xt/encrypt-to-secondary-key branch from 734eb00 to 6d9a7c7 Compare November 9, 2023 22:36
@link2xt link2xt marked this pull request as ready for review November 9, 2023 22:37
@link2xt link2xt changed the title Encrypt to secondary verified key feat: add secondary verified key Nov 10, 2023
@link2xt link2xt requested a review from Hocuri November 10, 2023 00:20
Copy link
Contributor

@hpk42 hpk42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm -- i would probably let the python test have some "lp.sec(...)" section titles to allow debugging the test better as the output will be a very long log of messages.

@link2xt
Copy link
Collaborator Author

link2xt commented Nov 10, 2023

lgtm -- i would probably let the python test have some "lp.sec(...)" section titles to allow debugging the test better as the output will be a very long log of messages.

There is no lp fixture in deltachat-rpc-client tests, but INFO messages have a green color easily distinguishable from purple DEBUG messages.

@link2xt link2xt merged commit ce016eb into main Nov 10, 2023
@link2xt link2xt deleted the link2xt/encrypt-to-secondary-key branch November 10, 2023 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Verified key cannot be gossiped after key change
2 participants