Skip to content

Per-participant sender keys can be processed out of order leading to InvalidKey: Decryption failed errors #2549

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

Closed
hughns opened this issue Aug 9, 2024 · 3 comments · Fixed by matrix-org/matrix-js-sdk#4345 or #2567
Assignees
Labels
T-Defect Something isn't working: bugs, crashes, hangs, vulnerabilities, or other reported problems

Comments

@hughns
Copy link
Member

hughns commented Aug 9, 2024

I have observed that sometimes EC will attempt to use an old per-sender encryption key instead of the correct one. EC is attempting to use the correct index, but the actual key is wrong.

On inspection I can see that the io.element.call.encryption_keys events are processed in the wrong order.

The io.element.call.encryption_keys events appear to be emitted in order by the js-sdk, but when they are processed by MatrixKeyProvider the resulting calls to BaseKeyProvider.onSetEncryptionKey() occur out of order.

For example with the following events in order of emitting:

{
	"call_id": "",
	"device_id": "YERKDYHWCR",
	"keys": [
	    {
	        "index": 0,
	        "key": "oldKey0"
	    },
	    {
	        "index": 1,
	        "key": "oldKey1"
	    }
	]
}
{
	"call_id": "",
	"device_id": "YERKDYHWCR",
	"keys": [
	    {
	        "index": 0,
	        "key": "newKey0"
	    }
	]
}

Then EC is trying to use oldKey0 instead of newKey0.

@hughns hughns self-assigned this Aug 9, 2024
@hughns hughns added the T-Defect Something isn't working: bugs, crashes, hangs, vulnerabilities, or other reported problems label Aug 9, 2024
@hughns
Copy link
Member Author

hughns commented Aug 9, 2024

As to how to fix this, initially I was thinking that the key requests should be simply queued and make sure that they are processed in the order that the js-sdk emits them.

However, it was pointed out to me that when we move the distribution of encryption keys to be migrated to to-device messages that we wouldn't be able to rely on the ordering of the to-device messages.

Checking the Client-Server API to-device spec it says:

the server should list the pending messages, in order of arrival, in the response body.

Which suggests that we might be able to rely on the ordering for to-device messages from local users.

However, the Server-Server API doesn't appear to say anything about EDU ordering, so we might not be able to rely on it across federation.

As such, the proposed fix is to have EC track a timestamp associated with a particular key and then make sure that we don't overwrite an older one with a newer one.

@hughns
Copy link
Member Author

hughns commented Aug 12, 2024

This will need a js-sdk bump once matrix-org/matrix-js-sdk#4345 is merged.

@hughns
Copy link
Member Author

hughns commented Aug 15, 2024

Still needs a js-sdk bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Defect Something isn't working: bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
1 participant