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

Message counter sync is now broken #10643

Closed
bzbarsky-apple opened this issue Oct 19, 2021 · 4 comments
Closed

Message counter sync is now broken #10643

bzbarsky-apple opened this issue Oct 19, 2021 · 4 comments
Assignees
Labels
spec Mismatch between spec and implementation V1.0

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

After #10095 the sequence of events for a group message needing counter sync is as follows:

  1. Receive the message.
  2. Decrypt it.
  3. Start message counter sync, queue up decrypted data.
  4. When sync completes, call SessionManager::OnMessageReceived with that data.
  5. Land in SessionManager::SecureMessageDispatch.
  6. Try to decrypt the data again and I woul hope fail.

Proposed Solution

We need to either keep the dispatch path as it is now and queue up the encrypted data, or queue up unencrypted data but then the dispatch after sync completes needs to be different.

@bzbarsky-apple bzbarsky-apple added the spec Mismatch between spec and implementation label Oct 19, 2021
@bzbarsky-apple
Copy link
Contributor Author

Also, the payload header gets lost now: it gets parsed out and consumed in Decrypt, then we store only the actual payload, sans header, and on re-dispatch it's not there. So if we change the dispatch we need to make sure to keep track of the payload header too.

@holbrookt
Copy link
Contributor

@bzbarsky-apple is this still broken? Is this an issue for 1.0?

@bzbarsky-apple
Copy link
Contributor Author

@bzbarsky-apple is this still broken?

Right now it's even more broken, in that we never call into counter sync, and in fact never check message counters for group messages at all.

Is this an issue for 1.0?

That depends on whether we are trying to ship the cache-and-sync counter sync method in 1.0.

@jepenven-silabs
Copy link
Contributor

Closing it since duplicated by #11689

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec Mismatch between spec and implementation V1.0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants