Skip to content

fix: ignore verification error if the chat is not protected yet #6883

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

Merged
merged 2 commits into from
May 31, 2025

Conversation

link2xt
Copy link
Collaborator

@link2xt link2xt commented May 30, 2025

If we receive a message from non-verified contact
in a non-protected chat with a Chat-Verified header, there is no need to upgrade the chat
to verified and display an error.

If it was an attack, an attacker could
just not send the Chat-Verified header.
Most of the time, however, it is just
message reordering.

Closes #6408

Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

The change LGTM, but shouldn't we additionally change this code:

    let create_protected = if mime_parser.get_header(HeaderDef::ChatVerified).is_some() {
        if let VerifiedEncryption::NotVerified(err) = verified_encryption {
            warn!(context, "Verification problem: {err:#}.");
            let s = format!("{err}. See 'Info' for more details");
            mime_parser.replace_msg_by_error(&s);
        }
        ProtectionStatus::Protected
    } else {
        ProtectionStatus::Unprotected
    };

in create_group()?

Because right now, probably the following still results in the bug (even though I didn't test):

  • Bob and Charlie are already verified
  • Bob and Alice are already verified
  • Bob and Alice create a verified group
  • Bob manually adds Charlie to the verified group, but Charlie doesn't receive the message yet
  • Alice sends a .xdc to Charlie

@link2xt
Copy link
Collaborator Author

link2xt commented May 31, 2025

but shouldn't we additionally change this code

I changed the code and extended existing test.

If we receive a message from non-verified contact
in a non-protected chat with a Chat-Verified header,
there is no need to upgrade the chat
to verified and display an error.

If it was an attack, an attacker could
just not send the Chat-Verified header.
Most of the time, however, it is just
message reordering.
@link2xt link2xt force-pushed the link2xt/verified-editor-reordering branch from 69191f5 to 5322cb1 Compare May 31, 2025 11:30
Copy link
Collaborator

@Hocuri Hocuri left a comment

Choose a reason for hiding this comment

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

Nice and simple solution for this problem!

@link2xt
Copy link
Collaborator Author

link2xt commented May 31, 2025

There is a Python test broken with a second commit, need to adjust it, will merge then.

@link2xt link2xt force-pushed the link2xt/verified-editor-reordering branch from 5322cb1 to 554fed5 Compare May 31, 2025 12:16
@link2xt link2xt force-pushed the link2xt/verified-editor-reordering branch from 554fed5 to 1cf520c Compare May 31, 2025 12:30
@link2xt link2xt merged commit becb83f into main May 31, 2025
53 of 54 checks passed
@link2xt link2xt deleted the link2xt/verified-editor-reordering branch May 31, 2025 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Race condition with group editor bot
2 participants