Assume per-user deviceID uniqueness in encryptAndSendKeysToDevices #2136
Conversation
t3chguy
requested review from
JordanHancock-NV and
a team
and removed request for
JordanHancock-NV
There was a problem hiding this comment.
This PR looks functionally correct and will fix the issue, but see my PR comments.
To expand on the reasoning from #2135, I agree this bug shouldn't have security consequences, though the reasoning is more subtle; I don't think this would lead to a ratchet corruption and undecryptable messages.
Rather, it allows a malicious server to confuse Element Web into recording the wrong device identity key for re-sharing (forwarding) purposes. However it can only confuse it into recording the key of another device present in the room at the time the message was sent, so that device would've received the message and the message key anyway, even without this trick.
ShadowJonathan
force-pushed
the
globally-unique-deviceids
branch
from
0361645
to
ea6bd5d
Compare
|
Reviewers, I have reset this PR to @dkasak's suggested commit, as I believe that is a better approach. Approval got granted via DM. |
|
(Sonar didn't run due to missing sonar-project.properties file in this old PR) |
|
Sonar now ants 80% coverage on this new code, while I'm not sure how to achieve that. Any suggestions? Adding specific tests for this? |
* Remove unused sessionStore ([\matrix-org#2455](matrix-org#2455)). * Implement MSC3827: Filtering of `/publicRooms` by room type ([\matrix-org#2469](matrix-org#2469)). * expose latestLocationEvent on beacon model ([\matrix-org#2467](matrix-org#2467)). Contributed by @kerryarchibald. * Live location share - add start time leniency ([\matrix-org#2465](matrix-org#2465)). Contributed by @kerryarchibald. * Log real errors and not just their messages, traces are useful ([\matrix-org#2464](matrix-org#2464)). * Various changes to `src/crypto` files for correctness ([\matrix-org#2137](matrix-org#2137)). Contributed by @ShadowJonathan. * Update MSC3786 implementation: Check the `state_key` ([\matrix-org#2429](matrix-org#2429)). * Timeline needs to refresh when we see a MSC2716 marker event ([\matrix-org#2299](matrix-org#2299)). Contributed by @MadLittleMods. * Try to load keys from key backup when a message fails to decrypt ([\matrix-org#2373](matrix-org#2373)). Fixes element-hq/element-web#21026. Contributed by @duxovni. * Send call version `1` as a string ([\matrix-org#2471](matrix-org#2471)). Fixes element-hq/element-web#22629. * Fix issue with `getEventTimeline` returning undefined for thread roots in main timeline ([\matrix-org#2454](matrix-org#2454)). Fixes element-hq/element-web#22539. * Add missing `type` property on `IAuthData` ([\matrix-org#2463](matrix-org#2463)). * Clearly indicate that `lastReply` on a Thread can return falsy ([\matrix-org#2462](matrix-org#2462)). * Fix issues with getEventTimeline and thread roots ([\matrix-org#2444](matrix-org#2444)). Fixes element-hq/element-web#21613. * Live location sharing - monitor liveness of beacons yet to start ([\matrix-org#2437](matrix-org#2437)). Contributed by @kerryarchibald. * Refactor Relations to not be per-EventTimelineSet ([\matrix-org#2412](matrix-org#2412)). Fixes matrix-org#2399 and element-hq/element-web#22298. * Add tests for sendEvent threadId handling ([\matrix-org#2435](matrix-org#2435)). Fixes element-hq/element-web#22433. * Make sure `encryptAndSendKeysToDevices` assumes devices are unique per-user. ([\matrix-org#2136](matrix-org#2136)). Fixes matrix-org#2135. Contributed by @ShadowJonathan. * Don't bug the user while re-checking key backups after decryption failures ([\matrix-org#2430](matrix-org#2430)). Fixes element-hq/element-web#22416. Contributed by @duxovni.
Fixes #2135
Notes: Make sure
encryptAndSendKeysToDevicesassumes devices are unique per-user.Type: defect
Signed-off-by: Jonathan de Jong <jonathan@automatia.nl>Here's what your changelog entry will look like:
🐛 Bug Fixes
encryptAndSendKeysToDevicesassumes devices are unique per-user. (#2136). FixesencryptAndSendKeysToDevicesassumes device IDs are globally unique #2135. Contributed by @ShadowJonathan.