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.

Sign up for GitHub

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

Jump to bottom

Device verification fails if verification events are received/decrypted out of order. #21488

Closed
ara4n opened this issue Mar 20, 2022 · 1 comment
Closed

Device verification fails if verification events are received/decrypted out of order. #21488

ara4n opened this issue Mar 20, 2022 · 1 comment
Labels
A-E2EE O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect Z-WTF

Comments

@ara4n
Copy link
Member

ara4n commented Mar 20, 2022 •
edited
Loading

Steps to reproduce

  1. Run the E2E tests a bunch; note that SAS Verification tests are flakey and fail with things like:
log      | received to_device m.room_key from: @alice:localhost id: undefined 
info     | Storing megolm session S9/CNRwJbOg4dMupzNai3gygRGX+o8jyfpST+ASdVwI/0Ms331jT5qdUM1ypZLkmhARk5yMahZUmVuJ0BXXKOs8 with first index 0 
warning  | Error decrypting event (id=$ai2t4IuooBlBt9_zCt-8eqiMTNm6CddAgrE1GZpy0sI): DecryptionError[msg: The sender's device has not sent us the keys for this message., session: S9/CNRwJbOg4dMupzNai3gygRGX+o8jyfpST+ASdVwI|0Ms331jT5qdUM1ypZLkmhARk5yMahZUmVuJ0BXXKOs8] 

...

info     | Decrypted event on retry (id=$ai2t4IuooBlBt9_zCt-8eqiMTNm6CddAgrE1GZpy0sI) 
warning  | Cancelling, unexpected m.key.verification.ready verification event from @alice:localhost

This is because the verification code incorrectly assumes that it's invalid to receive an ready event when you have already started verification and received a start event - but in practice, it's completely legitimate, as the events can decrypt out of order (caused by the megolm store racing with receiving the ready timeline event in this example).

Currently the behaviour is to cancel the whole verification(!) rather than attempt to reorder the verification events back into a sensible order (or ignore no-ops, such as a ready arriving after a start).

Outcome

What did you expect?

Verification to be reliable

What happened instead?

Verification was flakey, and cancelled a valid verification on seeing a transient UISI (in the E2E tests)
@ara4n ara4n added the T-Defect label Mar 20, 2022
@ara4n ara4n changed the title Device verification fails if verification events are received out of order. Device verification fails if verification events are received/decrypted out of order. Mar 20, 2022
ara4n added a commit to matrix-org/matrix-js-sdk that referenced this issue Mar 20, 2022
@ara4n
reduce flakiness of e2e verif test
d9f0704 
it's completely valid to receive a `ready` event after having received a
`start` event as messages may be received or decrypted in any order.

partial (but possibly sufficient?) fix for element-hq/element-web#21488
@ara4n ara4n mentioned this issue Mar 20, 2022
Merged
@germain-gg germain-gg added A-E2EE S-Major Severely degrades major functionality or product features, with no satisfactory workaround O-Uncommon Most users are unlikely to come across this or unexpected workflow labels Mar 21, 2022
@MadLittleMods MadLittleMods mentioned this issue Jul 20, 2022
Closed
@richvdh
Copy link
Member

richvdh commented Jul 8, 2024

Presumably this was specific to the legacy crypto stack

@richvdh richvdh closed this as completed Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-E2EE O-Uncommon Most users are unlikely to come across this or unexpected workflow S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect Z-WTF
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@MadLittleMods @germain-gg @Johennes @ara4n @richvdh