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

Fix an instance of failed to decrypt error when an in flight /keys/query fails. #3486

Merged
merged 6 commits into from
Jul 4, 2023
Merged

Fix an instance of failed to decrypt error when an in flight /keys/query fails. #3486

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
78d6f71
Fix an instance of failed to decrypt error
erikjohnston Jun 19, 2023
4aed7d7
Apply suggestions from code review
erikjohnston Jun 20, 2023
bc84fc2
Update src/crypto/algorithms/olm.ts
erikjohnston Jun 20, 2023
6c22dab
Fix line wrapping
erikjohnston Jun 20, 2023
d5b66db
Update src/crypto/algorithms/olm.ts
erikjohnston Jun 20, 2023
45fbc55
Fix null check
erikjohnston Jun 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions src/crypto/algorithms/olm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,9 +194,9 @@ class OlmDecryption extends DecryptionAlgorithm {

// check that the device that encrypted the event belongs to the user that the event claims it's from.
//
// To do this, we need to make sure that our device list is up-to-date. If the device is unknown, we can only
// assume that the device logged out and accept it anyway. Some event handlers, such as secret sharing, may be
// more strict and reject events that come from unknown devices.
// If the device is unknown then we check that we're not in the process of fetching the keys. If after that the
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved
// device is still unknown, then we can only assume that the device logged out and accept it anyway. Some event
// handlers, such as secret sharing, may be more strict and reject events that come from unknown devices.
//
// This is a defence against the following scenario:
//
Expand All @@ -209,8 +209,21 @@ class OlmDecryption extends DecryptionAlgorithm {
//
// In practice, it's not clear that the js-sdk would behave that way, so this may be only a defence in depth.

await this.crypto.deviceList.downloadKeys([event.getSender()!], false);
const senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey);
let senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey);
if (senderKeyUser == undefined) {
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved
// Await for any ongoing key query fetches for the user before trying the lookup again.
erikjohnston marked this conversation as resolved.
Show resolved Hide resolved
try {
await this.crypto.deviceList.downloadKeys([event.getSender()!], false);
} catch (e) {
throw new DecryptionError("OLM_BAD_SENDER_CHECK_FAILED", "Could not verify sender identity", {
sender: deviceKey,
err: e as Error,
});
}

senderKeyUser = this.crypto.deviceList.getUserByIdentityKey(olmlib.OLM_ALGORITHM, deviceKey);
Comment on lines +216 to +225
Copy link
Member

Choose a reason for hiding this comment

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

it rather feels like the value of this is pretty limited, given that we're happy to accept messages from unknown devices.

The only time this might do something different is if a key-fetch for another user happens to complete before that for the sender, and it turns out that other user is the rightful owner of the device. That seems a pretty remote chance to me.

@BillCarsonFr wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

(I think that logic is correct, though I'm hesitant to remove it without understanding why we added it in the first place given this is a security thing)

Copy link
Member

Choose a reason for hiding this comment

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

Regarding the logic. Here we are receiving a megolm key by succesfully decrypting an olm message.
So from that we have a megolm session that is:

  • owned by a curveKey
  • claimed to be owned by a mxid
  • claimed to be owned by a ed key

=> What we want is to validate the things that are claimed (and also sanity validate the homeserver controlled fields that are in the original event)

The only way to do that is to download the device (/keys/query), we only accepted devices correctly signed (mxid|curve|ed|device_id are signed by the ed_key). This signature "binds" the curve/ed/mxid together

So once we have the device we can check if the claimed stuff match what's in the key/query.

And typically the first time someone adds you and send you an encrypted to device you won't have yet downloaded his keys. => Makes it impossible to link to mxId at time of reception of the key.

Regarding the solution:
We need for m.room.keys to delay these checks, so not download anything at all. And instead at time of decryption check that, or when the keys are finally downloaded

Copy link
Member

Choose a reason for hiding this comment

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

Also notice that currently we can't make the difference between an not known yet device or a delete device, or a very short lived device.. (hence the red warning for message sent from a deleted device)

Copy link
Member

Choose a reason for hiding this comment

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

The only way to do that is to download the device (/keys/query),

If we have previously stored a signed copy of the device, surely there is no need to download it again?

Regarding the solution: We need for m.room.keys to delay these checks, so not download anything at all.

Yes, that's the ideal solution, but that's really not the point here: this PR is trying to make an incremental improvement to the existing implementation.

There are two proposed improvements here:

  • Erik's change as it currently stands: if we already have a signed copy of the device, there is no need to wait for any ongoing /keys/query requests to complete. Can we agree this is uncontroversial?
  • My later comment: given that - in this code - we treat an unknown device identically to a correctly-signed device, I don't really see the value in doing a /keys/query request anyway. Supposing the incoming message is from a bogus device, we're not going to get any extra information by requesting the device list from the claimed user: the bogus device isn't going to be in the list, so it is treated as "unknown".

This code has been observed to cause significant numbers of UISIs. I don't believe the current implementation offers any value, and Erik is proposing a quick win here. I think it would be a mistake to refuse it because it's not the ideal solution.

}

if (senderKeyUser !== event.getSender() && senderKeyUser != undefined) {
richvdh marked this conversation as resolved.
Show resolved Hide resolved
throw new DecryptionError("OLM_BAD_SENDER", "Message claimed to be from " + event.getSender(), {
real_sender: senderKeyUser,
Expand Down