From bff059ab656e5d5ff2ab5b55b5c11db87b0bd276 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Thu, 26 Sep 2024 23:07:11 +0100 Subject: [PATCH 1/6] Add labs option to exclude unverified devices Add a labs option which will, when set, switch into the "invisible crypto" mode of refusing to send keys to, or decrypt messages from, devices that have not been signed by their owner. --- src/MatrixClientPeg.ts | 4 ++ src/i18n/strings/en_EN.json | 2 + src/settings/Settings.tsx | 11 ++++++ .../DeviceIsolationModeController.ts | 37 +++++++++++++++++++ .../components/structures/MatrixChat-test.tsx | 1 + .../DeviceIsolationModeController-test.ts | 33 +++++++++++++++++ test/test-utils/test-utils.ts | 1 + 7 files changed, 89 insertions(+) create mode 100644 src/settings/controllers/DeviceIsolationModeController.ts create mode 100644 test/settings/controllers/DeviceIsolationModeController-test.ts diff --git a/src/MatrixClientPeg.ts b/src/MatrixClientPeg.ts index 87ad8ec0cb..0ba2ec65d7 100644 --- a/src/MatrixClientPeg.ts +++ b/src/MatrixClientPeg.ts @@ -42,6 +42,7 @@ import PlatformPeg from "./PlatformPeg"; import { formatList } from "./utils/FormattingUtils"; import SdkConfig from "./SdkConfig"; import { Features } from "./settings/Settings"; +import { setDeviceIsolationMode } from "./settings/controllers/DeviceIsolationModeController.ts"; export interface IMatrixClientCreds { homeserverUrl: string; @@ -343,6 +344,9 @@ class MatrixClientPegClass implements IMatrixClientPeg { }); StorageManager.setCryptoInitialised(true); + + setDeviceIsolationMode(this.matrixClient, SettingsStore.getValue("feature_exclude_insecure_devices")); + // TODO: device dehydration and whathaveyou return; } diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 70089dcf3b..64680a93a3 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -1427,6 +1427,8 @@ "dynamic_room_predecessors": "Dynamic room predecessors", "dynamic_room_predecessors_description": "Enable MSC3946 (to support late-arriving room archives)", "element_call_video_rooms": "Element Call video rooms", + "exclude_insecure_devices": "Exclude insecure devices when sending/receiving messages", + "exclude_insecure_devices_description": "When this mode is enabled, encrypted messages will not be shared with unverified devices, and messages from unverified devices will be shown as an error. Note that if you enable this mode, you may be unable to communicate with users who have not verified their devices.", "experimental_description": "Feeling experimental? Try out our latest ideas in development. These features are not finalised; they may be unstable, may change, or may be dropped altogether. Learn more.", "experimental_section": "Early previews", "extended_profiles_msc_support": "Requires your server to support MSC4133", diff --git a/src/settings/Settings.tsx b/src/settings/Settings.tsx index 2fadb53dde..76bb109cac 100644 --- a/src/settings/Settings.tsx +++ b/src/settings/Settings.tsx @@ -11,6 +11,7 @@ import React, { ReactNode } from "react"; import { UNSTABLE_MSC4133_EXTENDED_PROFILES } from "matrix-js-sdk/src/matrix"; import { _t, _td, TranslationKey } from "../languageHandler"; +import DeviceIsolationModeController from "./controllers/DeviceIsolationModeController.ts"; import { NotificationBodyEnabledController, NotificationsEnabledController, @@ -309,6 +310,16 @@ export const SETTINGS: { [setting: string]: ISetting } = { supportedLevelsAreOrdered: true, default: false, }, + "feature_exclude_insecure_devices": { + isFeature: true, + labsGroup: LabGroup.Encryption, + controller: new DeviceIsolationModeController(), + displayName: _td("labs|exclude_insecure_devices"), + description: _td("labs|exclude_insecure_devices_description"), + supportedLevels: LEVELS_DEVICE_ONLY_SETTINGS_WITH_CONFIG_PRIORITISED, + supportedLevelsAreOrdered: true, + default: false, + }, "useOnlyCurrentProfiles": { supportedLevels: LEVELS_ACCOUNT_SETTINGS, displayName: _td("settings|disable_historical_profile"), diff --git a/src/settings/controllers/DeviceIsolationModeController.ts b/src/settings/controllers/DeviceIsolationModeController.ts new file mode 100644 index 0000000000..03fee77742 --- /dev/null +++ b/src/settings/controllers/DeviceIsolationModeController.ts @@ -0,0 +1,37 @@ +/* +Copyright 2024 New Vector Ltd. +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only +Please see LICENSE files in the repository root for full details. +*/ + +import { AllDevicesIsolationMode, OnlySignedDevicesIsolationMode } from "matrix-js-sdk/src/crypto-api"; +import { MatrixClient } from "matrix-js-sdk/src/matrix"; + +import SettingController from "./SettingController"; +import { MatrixClientPeg } from "../../MatrixClientPeg"; +import { SettingLevel } from "../SettingLevel"; + +/** + * A controller for the "exclude_insecure_devices" setting, which will + * update the crypto stack's device isolation mode on change. + */ +export default class DeviceIsolationModeController extends SettingController { + public onChange(level: SettingLevel, roomId: string, newValue: any): void { + setDeviceIsolationMode(MatrixClientPeg.safeGet(), newValue); + } +} + +/** + * Set the crypto stack's device isolation mode based on the current value of the + * "exclude_insecure_devices" setting. + * + * @param client - MatrixClient to update to the new setting. + * @param settingValue - value of the "exclude_insecure_devices" setting. + */ +export function setDeviceIsolationMode(client: MatrixClient, settingValue: boolean): void { + client + .getCrypto() + ?.setDeviceIsolationMode( + settingValue ? new OnlySignedDevicesIsolationMode() : new AllDevicesIsolationMode(true), + ); +} diff --git a/test/components/structures/MatrixChat-test.tsx b/test/components/structures/MatrixChat-test.tsx index 1003d1d167..83b4feea3a 100644 --- a/test/components/structures/MatrixChat-test.tsx +++ b/test/components/structures/MatrixChat-test.tsx @@ -1002,6 +1002,7 @@ describe("", () => { getUserVerificationStatus: jest .fn() .mockResolvedValue(new UserVerificationStatus(false, false, false)), + setDeviceIsolationMode: jest.fn(), }; loginClient.isCryptoEnabled.mockReturnValue(true); loginClient.getCrypto.mockReturnValue(mockCrypto as any); diff --git a/test/settings/controllers/DeviceIsolationModeController-test.ts b/test/settings/controllers/DeviceIsolationModeController-test.ts new file mode 100644 index 0000000000..089a8ddfc8 --- /dev/null +++ b/test/settings/controllers/DeviceIsolationModeController-test.ts @@ -0,0 +1,33 @@ +/* +Copyright 2024 New Vector Ltd. +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only +Please see LICENSE files in the repository root for full details. +*/ + +import { AllDevicesIsolationMode, OnlySignedDevicesIsolationMode } from "matrix-js-sdk/src/crypto-api"; + +import { stubClient } from "../../test-utils"; +import DeviceIsolationModeController from "../../../src/settings/controllers/DeviceIsolationModeController.ts"; +import { SettingLevel } from "../../../src/settings/SettingLevel"; + +describe("DeviceIsolationModeController", () => { + afterEach(() => { + jest.resetAllMocks(); + }); + + describe("tracks enabling and disabling", () => { + it("on sets signed device isolation mode", () => { + const cli = stubClient(); + const controller = new DeviceIsolationModeController(); + controller.onChange(SettingLevel.DEVICE, "", true); + expect(cli.getCrypto()?.setDeviceIsolationMode).toHaveBeenCalledWith(new OnlySignedDevicesIsolationMode()); + }); + + it("off sets all device isolation mode", () => { + const cli = stubClient(); + const controller = new DeviceIsolationModeController(); + controller.onChange(SettingLevel.DEVICE, "", false); + expect(cli.getCrypto()?.setDeviceIsolationMode).toHaveBeenCalledWith(new AllDevicesIsolationMode(true)); + }); + }); +}); diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts index b0c3dda279..ebfc6b221b 100644 --- a/test/test-utils/test-utils.ts +++ b/test/test-utils/test-utils.ts @@ -131,6 +131,7 @@ export function createTestClient(): MatrixClient { resetKeyBackup: jest.fn(), isEncryptionEnabledInRoom: jest.fn(), getVerificationRequestsToDeviceInProgress: jest.fn().mockReturnValue([]), + setDeviceIsolationMode: jest.fn(), }), getPushActionsForEvent: jest.fn(), From 76b89508247643ae65e49854b74abe295c566f8a Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 27 Sep 2024 00:57:28 +0100 Subject: [PATCH 2/6] DecryptionFailureBody: better error messages Improve the error messages shown for messages from insecure devices. --- .../messages/_DecryptionFailureBody.pcss | 21 ++++++++++++ .../views/messages/DecryptionFailureBody.tsx | 34 +++++++++++++++++-- src/i18n/strings/en_EN.json | 2 ++ .../messages/DecryptionFailureBody-test.tsx | 28 +++++++++++++++ .../DecryptionFailureBody-test.tsx.snap | 15 ++++++++ 5 files changed, 98 insertions(+), 2 deletions(-) diff --git a/res/css/views/messages/_DecryptionFailureBody.pcss b/res/css/views/messages/_DecryptionFailureBody.pcss index 5dfdd7b7ae..7841bbc84c 100644 --- a/res/css/views/messages/_DecryptionFailureBody.pcss +++ b/res/css/views/messages/_DecryptionFailureBody.pcss @@ -10,3 +10,24 @@ Please see LICENSE files in the repository root for full details. color: $secondary-content; font-style: italic; } + +/* Formatting for the "Verified identity has changed" error */ +.mx_DecryptionFailureVerifiedIdentityChanged > span { + /* Show it in red */ + color: $e2e-warning-color; + /* TODO: background colour? */ + + /* With a red border */ + border: 1px solid $e2e-warning-color; + border-radius: $font-16px; + + /* Some space inside the border */ + padding: 4px 12px 4px 8px; + + /* some space between the (!) icon and text */ + display: inline-flex; + gap: 8px; + + /* Center vertically */ + align-items: center; +} diff --git a/src/components/views/messages/DecryptionFailureBody.tsx b/src/components/views/messages/DecryptionFailureBody.tsx index d6e46267af..9cdc2eac7b 100644 --- a/src/components/views/messages/DecryptionFailureBody.tsx +++ b/src/components/views/messages/DecryptionFailureBody.tsx @@ -6,6 +6,7 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only Please see LICENSE files in the repository root for full details. */ +import classNames from "classnames"; import React, { forwardRef, ForwardRefExoticComponent, useContext } from "react"; import { MatrixEvent } from "matrix-js-sdk/src/matrix"; import { DecryptionFailureCode } from "matrix-js-sdk/src/crypto-api"; @@ -13,8 +14,9 @@ import { DecryptionFailureCode } from "matrix-js-sdk/src/crypto-api"; import { _t } from "../../../languageHandler"; import { IBodyProps } from "./IBodyProps"; import { LocalDeviceVerificationStateContext } from "../../../contexts/LocalDeviceVerificationStateContext"; +import { Icon as WarningBadgeIcon } from "../../../../res/img/compound/error-16px.svg"; -function getErrorMessage(mxEvent: MatrixEvent, isVerified: boolean | undefined): string { +function getErrorMessage(mxEvent: MatrixEvent, isVerified: boolean | undefined): string | React.JSX.Element { switch (mxEvent.decryptionFailureReason) { case DecryptionFailureCode.MEGOLM_KEY_WITHHELD_FOR_UNVERIFIED_DEVICE: return _t("timeline|decryption_failure|blocked"); @@ -32,16 +34,44 @@ function getErrorMessage(mxEvent: MatrixEvent, isVerified: boolean | undefined): break; case DecryptionFailureCode.HISTORICAL_MESSAGE_USER_NOT_JOINED: + // TODO: event should be hidden instead of showing this error. + // To be revisited as part of https://github.com/element-hq/element-meta/issues/2449 return _t("timeline|decryption_failure|historical_event_user_not_joined"); + + case DecryptionFailureCode.SENDER_IDENTITY_PREVIOUSLY_VERIFIED: + return ( + + + {_t("timeline|decryption_failure|sender_identity_previously_verified")} + + ); + + case DecryptionFailureCode.UNSIGNED_SENDER_DEVICE: + // TODO: event should be hidden instead of showing this error. + // To be revisited as part of https://github.com/element-hq/element-meta/issues/2449 + return _t("timeline|decryption_failure|sender_unsigned_device"); } return _t("timeline|decryption_failure|unable_to_decrypt"); } +/** Get an extra CSS class, specific to the decryption failure reason */ +function errorClassName(mxEvent: MatrixEvent): string | null { + switch (mxEvent.decryptionFailureReason) { + case DecryptionFailureCode.SENDER_IDENTITY_PREVIOUSLY_VERIFIED: + return "mx_DecryptionFailureVerifiedIdentityChanged"; + + default: + return null; + } +} + // A placeholder element for messages that could not be decrypted export const DecryptionFailureBody = forwardRef(({ mxEvent }, ref): React.JSX.Element => { const verificationState = useContext(LocalDeviceVerificationStateContext); + const classes = classNames("mx_DecryptionFailureBody", "mx_EventTile_content", errorClassName(mxEvent)); + return ( -
+
{getErrorMessage(mxEvent, verificationState)}
); diff --git a/src/i18n/strings/en_EN.json b/src/i18n/strings/en_EN.json index 64680a93a3..5a0b9d7cac 100644 --- a/src/i18n/strings/en_EN.json +++ b/src/i18n/strings/en_EN.json @@ -3303,6 +3303,8 @@ "historical_event_no_key_backup": "Historical messages are not available on this device", "historical_event_unverified_device": "You need to verify this device for access to historical messages", "historical_event_user_not_joined": "You don't have access to this message", + "sender_identity_previously_verified": "Verified identity has changed", + "sender_unsigned_device": "Encrypted by a device not verified by its owner.", "unable_to_decrypt": "Unable to decrypt message" }, "disambiguated_profile": "%(displayName)s (%(matrixId)s)", diff --git a/test/components/views/messages/DecryptionFailureBody-test.tsx b/test/components/views/messages/DecryptionFailureBody-test.tsx index 8ba4503446..021e58d071 100644 --- a/test/components/views/messages/DecryptionFailureBody-test.tsx +++ b/test/components/views/messages/DecryptionFailureBody-test.tsx @@ -103,4 +103,32 @@ describe("DecryptionFailureBody", () => { // Then expect(container).toHaveTextContent("You don't have access to this message"); }); + + it("should handle messages from users who change identities after verification", async () => { + // When + const event = await mkDecryptionFailureMatrixEvent({ + code: DecryptionFailureCode.SENDER_IDENTITY_PREVIOUSLY_VERIFIED, + msg: "User previously verified", + roomId: "fakeroom", + sender: "fakesender", + }); + const { container } = customRender(event); + + // Then + expect(container).toMatchSnapshot(); + }); + + it("should handle messages from unverified devices", async () => { + // When + const event = await mkDecryptionFailureMatrixEvent({ + code: DecryptionFailureCode.UNSIGNED_SENDER_DEVICE, + msg: "Unsigned device", + roomId: "fakeroom", + sender: "fakesender", + }); + const { container } = customRender(event); + + // Then + expect(container).toHaveTextContent("Encrypted by a device not verified by its owner"); + }); }); diff --git a/test/components/views/messages/__snapshots__/DecryptionFailureBody-test.tsx.snap b/test/components/views/messages/__snapshots__/DecryptionFailureBody-test.tsx.snap index 22e44fd16a..b2ba5b2a2e 100644 --- a/test/components/views/messages/__snapshots__/DecryptionFailureBody-test.tsx.snap +++ b/test/components/views/messages/__snapshots__/DecryptionFailureBody-test.tsx.snap @@ -19,3 +19,18 @@ exports[`DecryptionFailureBody Should display "Unable to decrypt message" 1`] =
`; + +exports[`DecryptionFailureBody should handle messages from users who change identities after verification 1`] = ` +
+
+ +
+ Verified identity has changed + +
+
+`; From 8ad7a4f9e75181b2f07e8149a3e451743676e7ac Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 27 Sep 2024 01:00:35 +0100 Subject: [PATCH 3/6] playwright: factor out `createSecondBotDevice` utility --- playwright/e2e/crypto/event-shields.spec.ts | 24 ++++++++------------- playwright/e2e/crypto/utils.ts | 11 ++++++++++ 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/playwright/e2e/crypto/event-shields.spec.ts b/playwright/e2e/crypto/event-shields.spec.ts index 98142089fb..fa9d1959da 100644 --- a/playwright/e2e/crypto/event-shields.spec.ts +++ b/playwright/e2e/crypto/event-shields.spec.ts @@ -6,12 +6,16 @@ SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only Please see LICENSE files in the repository root for full details. */ -import { Page } from "@playwright/test"; - import { expect, test } from "../../element-web-test"; -import { autoJoin, createSharedRoomWithUser, enableKeyBackup, logIntoElement, logOutOfElement, verify } from "./utils"; -import { Bot } from "../../pages/bot"; -import { HomeserverInstance } from "../../plugins/homeserver"; +import { + autoJoin, + createSecondBotDevice, + createSharedRoomWithUser, + enableKeyBackup, + logIntoElement, + logOutOfElement, + verify, +} from "./utils"; test.describe("Cryptography", function () { test.use({ @@ -296,13 +300,3 @@ test.describe("Cryptography", function () { }); }); }); - -async function createSecondBotDevice(page: Page, homeserver: HomeserverInstance, bob: Bot) { - const bobSecondDevice = new Bot(page, homeserver, { - bootstrapSecretStorage: false, - bootstrapCrossSigning: false, - }); - bobSecondDevice.setCredentials(await homeserver.loginUser(bob.credentials.userId, bob.credentials.password)); - await bobSecondDevice.prepareClient(); - return bobSecondDevice; -} diff --git a/playwright/e2e/crypto/utils.ts b/playwright/e2e/crypto/utils.ts index a4d9769eb9..c198633733 100644 --- a/playwright/e2e/crypto/utils.ts +++ b/playwright/e2e/crypto/utils.ts @@ -377,3 +377,14 @@ export async function awaitVerifier( return verificationRequest.verifier; }); } + +/** Log in a second device for the given bot user */ +export async function createSecondBotDevice(page: Page, homeserver: HomeserverInstance, bob: Bot) { + const bobSecondDevice = new Bot(page, homeserver, { + bootstrapSecretStorage: false, + bootstrapCrossSigning: false, + }); + bobSecondDevice.setCredentials(await homeserver.loginUser(bob.credentials.userId, bob.credentials.password)); + await bobSecondDevice.prepareClient(); + return bobSecondDevice; +} From 70b592bc4f6355a49d8d3ad6a16181de7b91cc99 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Fri, 27 Sep 2024 01:01:02 +0100 Subject: [PATCH 4/6] Playwright test for messages from insecure devices --- .../e2e/crypto/invisible-crypto.spec.ts | 56 +++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 playwright/e2e/crypto/invisible-crypto.spec.ts diff --git a/playwright/e2e/crypto/invisible-crypto.spec.ts b/playwright/e2e/crypto/invisible-crypto.spec.ts new file mode 100644 index 0000000000..c53bacd32c --- /dev/null +++ b/playwright/e2e/crypto/invisible-crypto.spec.ts @@ -0,0 +1,56 @@ +/* +Copyright 2024 New Vector Ltd. + +SPDX-License-Identifier: AGPL-3.0-only OR GPL-3.0-only +Please see LICENSE files in the repository root for full details. +*/ + +import { expect, test } from "../../element-web-test"; +import { autoJoin, createSecondBotDevice, createSharedRoomWithUser, verify } from "./utils"; +import { bootstrapCrossSigningForClient } from "../../pages/client.ts"; + +/** Tests for the "invisible crypto" behaviour -- i.e., when the "exclude insecure devices" setting is enabled */ +test.describe("Invisible cryptography", () => { + test.use({ + displayName: "Alice", + botCreateOpts: { displayName: "Bob" }, + labsFlags: ["feature_exclude_insecure_devices"], + }); + + test("Messages fail to decrypt when sender is previously verified", async ({ + page, + bot: bob, + user: aliceCredentials, + app, + homeserver, + }) => { + await app.client.bootstrapCrossSigning(aliceCredentials); + await autoJoin(bob); + + // create an encrypted room + const testRoomId = await createSharedRoomWithUser(app, bob.credentials.userId, { + name: "TestRoom", + initial_state: [ + { + type: "m.room.encryption", + state_key: "", + content: { + algorithm: "m.megolm.v1.aes-sha2", + }, + }, + ], + }); + + // Verify Bob + await verify(app, bob); + + // Bob logs in a new device and resets cross-signing + const bobSecondDevice = await createSecondBotDevice(page, homeserver, bob); + await bootstrapCrossSigningForClient(await bobSecondDevice.prepareClient(), bob.credentials, true); + + /* should show an error for a message from a previously verified device */ + await bobSecondDevice.sendMessage(testRoomId, "test encrypted from user that was previously verified"); + const lastTile = page.locator(".mx_EventTile_last"); + await expect(lastTile).toContainText("Verified identity has changed"); + }); +}); From f4efe45c6db97c73b794af616f6ed29c246659ee Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 30 Sep 2024 11:36:38 +0100 Subject: [PATCH 5/6] fixup! DecryptionFailureBody: better error messages Use compound colour tokens, and add a background colour. --- res/css/views/messages/_DecryptionFailureBody.pcss | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/res/css/views/messages/_DecryptionFailureBody.pcss b/res/css/views/messages/_DecryptionFailureBody.pcss index 7841bbc84c..257b14f344 100644 --- a/res/css/views/messages/_DecryptionFailureBody.pcss +++ b/res/css/views/messages/_DecryptionFailureBody.pcss @@ -14,11 +14,11 @@ Please see LICENSE files in the repository root for full details. /* Formatting for the "Verified identity has changed" error */ .mx_DecryptionFailureVerifiedIdentityChanged > span { /* Show it in red */ - color: $e2e-warning-color; - /* TODO: background colour? */ + color: var(--cpd-color-text-critical-primary); + background-color: var(--cpd-color-bg-critical-subtle); /* With a red border */ - border: 1px solid $e2e-warning-color; + border: 1px solid var(--cpd-color-border-critical-subtle); border-radius: $font-16px; /* Some space inside the border */ From 04d91105a143d4a2d3bae26137719441c9038ec8 Mon Sep 17 00:00:00 2001 From: Richard van der Hoff Date: Mon, 30 Sep 2024 12:51:15 +0100 Subject: [PATCH 6/6] fixup! DecryptionFailureBody: better error messages Use compound spacing tokens --- res/css/views/messages/_DecryptionFailureBody.pcss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/res/css/views/messages/_DecryptionFailureBody.pcss b/res/css/views/messages/_DecryptionFailureBody.pcss index 257b14f344..64a09be7ef 100644 --- a/res/css/views/messages/_DecryptionFailureBody.pcss +++ b/res/css/views/messages/_DecryptionFailureBody.pcss @@ -22,11 +22,11 @@ Please see LICENSE files in the repository root for full details. border-radius: $font-16px; /* Some space inside the border */ - padding: 4px 12px 4px 8px; + padding: var(--cpd-space-1x) var(--cpd-space-3x) var(--cpd-space-1x) var(--cpd-space-2x); /* some space between the (!) icon and text */ display: inline-flex; - gap: 8px; + gap: var(--cpd-space-2x); /* Center vertically */ align-items: center;