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

Encryption should not hinder verification #2734

Merged
merged 8 commits into from
Oct 18, 2022
Merged

Encryption should not hinder verification #2734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3ec8058
encryption should not hinder verification
BillCarsonFr Oct 5, 2022
8d29d5f
Reduce duplicated code
duxovni Oct 15, 2022
df089ed
Merge branch 'develop' into bca/e2ee_hinders_verif
duxovni Oct 15, 2022
c9b51f4
Fix typescript strict error
duxovni Oct 15, 2022
409d0e9
Test sending verification events to unverified devices
duxovni Oct 15, 2022
5ed1a8c
Fix typescript strict errors
duxovni Oct 15, 2022
490304b
Fix even more ts strict errors
duxovni Oct 15, 2022
29cb9c8
Fix every TS strict error related to crypto being undefined
duxovni Oct 15, 2022
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
143 changes: 119 additions & 24 deletions spec/unit/crypto/algorithms/megolm.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,9 +493,9 @@ describe("MegolmDecryption", function() {
bobClient1.initCrypto(),
bobClient2.initCrypto(),
]);
const aliceDevice = aliceClient.crypto.olmDevice;
const bobDevice1 = bobClient1.crypto.olmDevice;
const bobDevice2 = bobClient2.crypto.olmDevice;
const aliceDevice = aliceClient.crypto!.olmDevice;
const bobDevice1 = bobClient1.crypto!.olmDevice;
const bobDevice2 = bobClient2.crypto!.olmDevice;

const encryptionCfg = {
"algorithm": "m.megolm.v1.aes-sha2",
Expand Down Expand Up @@ -532,10 +532,10 @@ describe("MegolmDecryption", function() {
},
};

aliceClient.crypto.deviceList.storeDevicesForUser(
aliceClient.crypto!.deviceList.storeDevicesForUser(
"@bob:example.com", BOB_DEVICES,
);
aliceClient.crypto.deviceList.downloadKeys = async function(userIds) {
aliceClient.crypto!.deviceList.downloadKeys = async function(userIds) {
return this.getDevicesFromStore(userIds);
};

Expand All @@ -551,7 +551,7 @@ describe("MegolmDecryption", function() {
body: "secret",
},
});
await aliceClient.crypto.encryptEvent(event, room);
await aliceClient.crypto!.encryptEvent(event, room);

expect(aliceClient.sendToDevice).toHaveBeenCalled();
const [msgtype, contentMap] = mocked(aliceClient.sendToDevice).mock.calls[0];
Expand Down Expand Up @@ -583,6 +583,100 @@ describe("MegolmDecryption", function() {
bobClient2.stopClient();
});

it("does not block unverified devices when sending verification events", async function() {
const aliceClient = (new TestClient(
"@alice:example.com", "alicedevice",
)).client;
const bobClient = (new TestClient(
"@bob:example.com", "bobdevice",
)).client;
await Promise.all([
aliceClient.initCrypto(),
bobClient.initCrypto(),
]);
const bobDevice = bobClient.crypto!.olmDevice;

const encryptionCfg = {
"algorithm": "m.megolm.v1.aes-sha2",
};
const roomId = "!someroom";
const room = new Room(roomId, aliceClient, "@alice:example.com", {});

const bobMember = new RoomMember(roomId, "@bob:example.com");
room.getEncryptionTargetMembers = async function() {
return [bobMember];
};
room.setBlacklistUnverifiedDevices(true);
aliceClient.store.storeRoom(room);
await aliceClient.setRoomEncryption(roomId, encryptionCfg);

const BOB_DEVICES: Record<string, IDevice> = {
bobdevice: {
algorithms: [olmlib.OLM_ALGORITHM, olmlib.MEGOLM_ALGORITHM],
keys: {
"ed25519:bobdevice": bobDevice.deviceEd25519Key,
"curve25519:bobdevice": bobDevice.deviceCurve25519Key,
},
verified: 0,
known: true,
},
};

aliceClient.crypto!.deviceList.storeDevicesForUser(
"@bob:example.com", BOB_DEVICES,
);
aliceClient.crypto!.deviceList.downloadKeys = async function(userIds) {
// @ts-ignore private
return this.getDevicesFromStore(userIds);
};

await bobDevice.generateOneTimeKeys(1);
const oneTimeKeys = await bobDevice.getOneTimeKeys();
const signedOneTimeKeys: Record<string, { key: string, signatures: object }> = {};
for (const keyId in oneTimeKeys.curve25519) {
if (oneTimeKeys.curve25519.hasOwnProperty(keyId)) {
const k = {
key: oneTimeKeys.curve25519[keyId],
signatures: {},
};
signedOneTimeKeys["signed_curve25519:" + keyId] = k;
await bobClient.crypto!.signObject(k);
break;
}
}

aliceClient.claimOneTimeKeys = jest.fn().mockResolvedValue({
one_time_keys: {
'@bob:example.com': {
bobdevice: signedOneTimeKeys,
},
},
failures: {},
});

aliceClient.sendToDevice = jest.fn().mockResolvedValue({});

const event = new MatrixEvent({
type: "m.key.verification.start",
sender: "@alice:example.com",
room_id: roomId,
event_id: "$event",
content: {
from_device: "alicedevice",
method: "m.sas.v1",
transaction_id: "transactionid",
},
});
await aliceClient.crypto!.encryptEvent(event, room);

expect(aliceClient.sendToDevice).toHaveBeenCalled();
const [msgtype] = mocked(aliceClient.sendToDevice).mock.calls[0];
expect(msgtype).toEqual("m.room.encrypted");

aliceClient.stopClient();
bobClient.stopClient();
});

it("notifies devices when unable to create olm session", async function() {
const aliceClient = (new TestClient(
"@alice:example.com", "alicedevice",
Expand All @@ -594,8 +688,8 @@ describe("MegolmDecryption", function() {
aliceClient.initCrypto(),
bobClient.initCrypto(),
]);
const aliceDevice = aliceClient.crypto.olmDevice;
const bobDevice = bobClient.crypto.olmDevice;
const aliceDevice = aliceClient.crypto!.olmDevice;
const bobDevice = bobClient.crypto!.olmDevice;

const encryptionCfg = {
"algorithm": "m.megolm.v1.aes-sha2",
Expand Down Expand Up @@ -632,10 +726,11 @@ describe("MegolmDecryption", function() {
},
};

aliceClient.crypto.deviceList.storeDevicesForUser(
aliceClient.crypto!.deviceList.storeDevicesForUser(
"@bob:example.com", BOB_DEVICES,
);
aliceClient.crypto.deviceList.downloadKeys = async function(userIds) {
aliceClient.crypto!.deviceList.downloadKeys = async function(userIds) {
// @ts-ignore private
return this.getDevicesFromStore(userIds);
};

Expand All @@ -654,7 +749,7 @@ describe("MegolmDecryption", function() {
event_id: "$event",
content: {},
});
await aliceClient.crypto.encryptEvent(event, aliceRoom);
await aliceClient.crypto!.encryptEvent(event, aliceRoom);

expect(aliceClient.sendToDevice).toHaveBeenCalled();
const [msgtype, contentMap] = mocked(aliceClient.sendToDevice).mock.calls[0];
Expand Down Expand Up @@ -685,10 +780,10 @@ describe("MegolmDecryption", function() {
aliceClient.initCrypto(),
bobClient.initCrypto(),
]);
const bobDevice = bobClient.crypto.olmDevice;
const bobDevice = bobClient.crypto!.olmDevice;

const aliceEventEmitter = new TypedEventEmitter<ClientEvent.ToDeviceEvent, any>();
aliceClient.crypto.registerEventHandlers(aliceEventEmitter);
aliceClient.crypto!.registerEventHandlers(aliceEventEmitter);

const roomId = "!someroom";

Expand All @@ -705,7 +800,7 @@ describe("MegolmDecryption", function() {
},
}));

await expect(aliceClient.crypto.decryptEvent(new MatrixEvent({
await expect(aliceClient.crypto!.decryptEvent(new MatrixEvent({
type: "m.room.encrypted",
sender: "@bob:example.com",
event_id: "$event",
Expand All @@ -732,7 +827,7 @@ describe("MegolmDecryption", function() {
},
}));

await expect(aliceClient.crypto.decryptEvent(new MatrixEvent({
await expect(aliceClient.crypto!.decryptEvent(new MatrixEvent({
type: "m.room.encrypted",
sender: "@bob:example.com",
event_id: "$event",
Expand Down Expand Up @@ -762,10 +857,10 @@ describe("MegolmDecryption", function() {
]);

const aliceEventEmitter = new TypedEventEmitter<ClientEvent.ToDeviceEvent, any>();
aliceClient.crypto.registerEventHandlers(aliceEventEmitter);
aliceClient.crypto!.registerEventHandlers(aliceEventEmitter);

aliceClient.crypto.downloadKeys = jest.fn();
const bobDevice = bobClient.crypto.olmDevice;
aliceClient.crypto!.downloadKeys = jest.fn();
const bobDevice = bobClient.crypto!.olmDevice;

const roomId = "!someroom";

Expand All @@ -788,7 +883,7 @@ describe("MegolmDecryption", function() {
setTimeout(resolve, 100);
});

await expect(aliceClient.crypto.decryptEvent(new MatrixEvent({
await expect(aliceClient.crypto!.decryptEvent(new MatrixEvent({
type: "m.room.encrypted",
sender: "@bob:example.com",
event_id: "$event",
Expand Down Expand Up @@ -820,7 +915,7 @@ describe("MegolmDecryption", function() {
setTimeout(resolve, 100);
});

await expect(aliceClient.crypto.decryptEvent(new MatrixEvent({
await expect(aliceClient.crypto!.decryptEvent(new MatrixEvent({
type: "m.room.encrypted",
sender: "@bob:example.com",
event_id: "$event",
Expand Down Expand Up @@ -850,10 +945,10 @@ describe("MegolmDecryption", function() {
bobClient.initCrypto(),
]);
const aliceEventEmitter = new TypedEventEmitter<ClientEvent.ToDeviceEvent, any>();
aliceClient.crypto.registerEventHandlers(aliceEventEmitter);
aliceClient.crypto!.registerEventHandlers(aliceEventEmitter);

const bobDevice = bobClient.crypto.olmDevice;
aliceClient.crypto.downloadKeys = jest.fn();
const bobDevice = bobClient.crypto!.olmDevice;
aliceClient.crypto!.downloadKeys = jest.fn();

const roomId = "!someroom";

Expand All @@ -875,7 +970,7 @@ describe("MegolmDecryption", function() {
setTimeout(resolve, 100);
});

await expect(aliceClient.crypto.decryptEvent(new MatrixEvent({
await expect(aliceClient.crypto!.decryptEvent(new MatrixEvent({
type: "m.room.encrypted",
sender: "@bob:example.com",
event_id: "$event",
Expand Down
4 changes: 4 additions & 0 deletions src/@types/event.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ export enum EventType {
KeyVerificationCancel = "m.key.verification.cancel",
KeyVerificationMac = "m.key.verification.mac",
KeyVerificationDone = "m.key.verification.done",
KeyVerificationKey = "m.key.verification.key",
KeyVerificationAccept = "m.key.verification.accept",
// XXX this event is not yet supported by js-sdk
KeyVerificationReady = "m.key.verification.ready",
// use of this is discouraged https://matrix.org/docs/spec/client_server/r0.6.1#m-room-message-feedback
RoomMessageFeedback = "m.room.message.feedback",
Reaction = "m.reaction",
Expand Down
37 changes: 34 additions & 3 deletions src/crypto/algorithms/megolm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import { DeviceInfo } from "../deviceinfo";
import { IOlmSessionResult } from "../olmlib";
import { DeviceInfoMap } from "../DeviceList";
import { MatrixEvent } from "../../models/event";
import { EventType, MsgType } from '../../@types/event';
import { IEventDecryptionResult, IMegolmSessionData, IncomingRoomKeyRequest } from "../index";
import { RoomKeyRequestState } from '../OutgoingRoomKeyRequestManager';
import { OlmGroupSessionExtraData } from "../../@types/crypto";
Expand Down Expand Up @@ -1019,7 +1020,12 @@ class MegolmEncryption extends EncryptionAlgorithm {
}
}

const [devicesInRoom, blocked] = await this.getDevicesInRoom(room);
/**
* When using in-room messages and the room has encryption enabled,
* clients should ensure that encryption does not hinder the verification.
*/
const forceDistributeToUnverified = this.isVerificationEvent(eventType, content);
const [devicesInRoom, blocked] = await this.getDevicesInRoom(room, forceDistributeToUnverified);

// check if any of these devices are not yet known to the user.
// if so, warn the user so they can verify or ignore.
Expand Down Expand Up @@ -1053,6 +1059,26 @@ class MegolmEncryption extends EncryptionAlgorithm {
return encryptedContent;
}

private isVerificationEvent(eventType: string, content: object): boolean {
switch (eventType) {
case EventType.KeyVerificationCancel:
case EventType.KeyVerificationDone:
case EventType.KeyVerificationMac:
case EventType.KeyVerificationStart:
case EventType.KeyVerificationKey:
case EventType.KeyVerificationReady:
case EventType.KeyVerificationAccept: {
return true;
}
case EventType.RoomMessage: {
return content['msgtype'] === MsgType.KeyVerificationRequest;
}
default: {
return false;
}
}
}

/**
* Forces the current outbound group session to be discarded such
* that another one will be created next time an event is sent.
Expand Down Expand Up @@ -1119,14 +1145,19 @@ class MegolmEncryption extends EncryptionAlgorithm {
* Get the list of unblocked devices for all users in the room
*
* @param {module:models/room} room
* @param forceDistributeToUnverified if set to true will include the unverified devices
* even if setting is set to block them (useful for verification)
*
* @return {Promise} Promise which resolves to an array whose
* first element is a map from userId to deviceId to deviceInfo indicating
* the devices that messages should be encrypted to, and whose second
* element is a map from userId to deviceId to data indicating the devices
* that are in the room but that have been blocked
*/
private async getDevicesInRoom(room: Room): Promise<[DeviceInfoMap, IBlockedMap]> {
private async getDevicesInRoom(
room: Room,
forceDistributeToUnverified = false,
): Promise<[DeviceInfoMap, IBlockedMap]> {
const members = await room.getEncryptionTargetMembers();
const roomMembers = members.map(function(u) {
return u.userId;
Expand Down Expand Up @@ -1161,7 +1192,7 @@ class MegolmEncryption extends EncryptionAlgorithm {
const deviceTrust = this.crypto.checkDeviceTrust(userId, deviceId);

if (userDevices[deviceId].isBlocked() ||
(!deviceTrust.isVerified() && isBlacklisting)
(!deviceTrust.isVerified() && isBlacklisting && !forceDistributeToUnverified)
) {
if (!blocked[userId]) {
blocked[userId] = {};
Expand Down
13 changes: 7 additions & 6 deletions src/crypto/verification/Base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ limitations under the License.
*/

import { MatrixEvent } from '../../models/event';
import { EventType } from '../../@types/event';
import { logger } from '../../logger';
import { DeviceInfo } from '../deviceinfo';
import { newTimeoutError } from "./Error";
Expand Down Expand Up @@ -182,13 +183,13 @@ export class VerificationBase<
} else if (e.getType() === this.expectedEvent) {
// if we receive an expected m.key.verification.done, then just
// ignore it, since we don't need to do anything about it
if (this.expectedEvent !== "m.key.verification.done") {
if (this.expectedEvent !== EventType.KeyVerificationDone) {
this.expectedEvent = undefined;
this.rejectEvent = undefined;
this.resetTimer();
this.resolveEvent(e);
}
} else if (e.getType() === "m.key.verification.cancel") {
} else if (e.getType() === EventType.KeyVerificationCancel) {
const reject = this.reject;
this.reject = undefined;
// there is only promise to reject if verify has been called
Expand Down Expand Up @@ -241,20 +242,20 @@ export class VerificationBase<
const sender = e.getSender();
if (sender !== this.userId) {
const content = e.getContent();
if (e.getType() === "m.key.verification.cancel") {
if (e.getType() === EventType.KeyVerificationCancel) {
content.code = content.code || "m.unknown";
content.reason = content.reason || content.body
|| "Unknown reason";
this.send("m.key.verification.cancel", content);
this.send(EventType.KeyVerificationCancel, content);
} else {
this.send("m.key.verification.cancel", {
this.send(EventType.KeyVerificationCancel, {
code: "m.unknown",
reason: content.body || "Unknown reason",
});
}
}
} else {
this.send("m.key.verification.cancel", {
this.send(EventType.KeyVerificationCancel, {
code: "m.unknown",
reason: e.toString(),
});
Expand Down
Loading