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.

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

Clean up code for handling decryption failures #4126

Merged
merged 11 commits into from
Mar 22, 2024
19 changes: 8 additions & 11 deletions spec/integ/crypto/crypto.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,12 +80,12 @@ import { SecretStorageKeyDescription } from "../../../src/secret-storage";
import {
CrossSigningKey,
CryptoCallbacks,
DecryptionFailureCode,
EventShieldColour,
EventShieldReason,
KeyBackupInfo,
} from "../../../src/crypto-api";
import { E2EKeyResponder } from "../../test-utils/E2EKeyResponder";
import { DecryptionError } from "../../../src/crypto/algorithms";
import { IKeyBackup } from "../../../src/crypto/backup";
import {
createOlmAccount,
Expand Down Expand Up @@ -470,9 +470,8 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
await startClientAndAwaitFirstSync();

const awaitUISI = new Promise<void>((resolve) => {
aliceClient.on(MatrixEventEvent.Decrypted, (ev, err) => {
const error = err as DecryptionError;
if (error.code == "MEGOLM_UNKNOWN_INBOUND_SESSION_ID") {
aliceClient.on(MatrixEventEvent.Decrypted, (ev) => {
if (ev.decryptionFailureReason === DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID) {
resolve();
}
});
Expand All @@ -499,9 +498,8 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
await startClientAndAwaitFirstSync();

const awaitUnknownIndex = new Promise<void>((resolve) => {
aliceClient.on(MatrixEventEvent.Decrypted, (ev, err) => {
const error = err as DecryptionError;
if (error.code == "OLM_UNKNOWN_MESSAGE_INDEX") {
aliceClient.on(MatrixEventEvent.Decrypted, (ev) => {
if (ev.decryptionFailureReason === DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX) {
resolve();
}
});
Expand Down Expand Up @@ -532,13 +530,12 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
await aliceClient.getCrypto()!.importRoomKeys([testData.MEGOLM_SESSION_DATA]);

const awaitDecryptionError = new Promise<void>((resolve) => {
aliceClient.on(MatrixEventEvent.Decrypted, (ev, err) => {
const error = err as DecryptionError;
aliceClient.on(MatrixEventEvent.Decrypted, (ev) => {
// rust and libolm can't have an exact 1:1 mapping for all errors,
// but some errors are part of API and should match
if (
error.code != "MEGOLM_UNKNOWN_INBOUND_SESSION_ID" &&
error.code != "OLM_UNKNOWN_MESSAGE_INDEX"
ev.decryptionFailureReason !== DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID &&
ev.decryptionFailureReason !== DecryptionFailureCode.OLM_UNKNOWN_MESSAGE_INDEX
) {
resolve();
}
Expand Down
38 changes: 36 additions & 2 deletions spec/unit/models/event.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ import {
THREAD_RELATION_TYPE,
TweakName,
} from "../../../src";
import { DecryptionFailureCode } from "../../../src/crypto-api";
import { DecryptionError } from "../../../src/common-crypto/CryptoBackend";

describe("MatrixEvent", () => {
it("should create copies of itself", () => {
Expand Down Expand Up @@ -360,20 +362,50 @@ describe("MatrixEvent", () => {
});
});

it("should report decryption errors", async () => {
it("should report unknown decryption errors", async () => {
const decryptionListener = jest.fn();
encryptedEvent.addListener(MatrixEventEvent.Decrypted, decryptionListener);

const testError = new Error("test error");
const crypto = {
decryptEvent: jest.fn().mockRejectedValue(new Error("test error")),
decryptEvent: jest.fn().mockRejectedValue(testError),
} as unknown as Crypto;

await encryptedEvent.attemptDecryption(crypto);
expect(encryptedEvent.isEncrypted()).toBeTruthy();
expect(encryptedEvent.isBeingDecrypted()).toBeFalsy();
expect(encryptedEvent.isDecryptionFailure()).toBeTruthy();
expect(encryptedEvent.decryptionFailureReason).toEqual(DecryptionFailureCode.UNKNOWN_ERROR);
expect(encryptedEvent.isEncryptedDisabledForUnverifiedDevices).toBeFalsy();
expect(encryptedEvent.getContent()).toEqual({
msgtype: "m.bad.encrypted",
body: "** Unable to decrypt: Error: test error **",
});
expect(decryptionListener).toHaveBeenCalledWith(encryptedEvent, testError);
});

it("should report known decryption errors", async () => {
const decryptionListener = jest.fn();
encryptedEvent.addListener(MatrixEventEvent.Decrypted, decryptionListener);

const testError = new DecryptionError(DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID, "uisi");
const crypto = {
decryptEvent: jest.fn().mockRejectedValue(testError),
} as unknown as Crypto;

await encryptedEvent.attemptDecryption(crypto);
expect(encryptedEvent.isEncrypted()).toBeTruthy();
expect(encryptedEvent.isBeingDecrypted()).toBeFalsy();
expect(encryptedEvent.isDecryptionFailure()).toBeTruthy();
expect(encryptedEvent.decryptionFailureReason).toEqual(
DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID,
);
expect(encryptedEvent.isEncryptedDisabledForUnverifiedDevices).toBeFalsy();
expect(encryptedEvent.getContent()).toEqual({
msgtype: "m.bad.encrypted",
body: "** Unable to decrypt: DecryptionError: uisi **",
});
expect(decryptionListener).toHaveBeenCalledWith(encryptedEvent, testError);
});

it(`should report "DecryptionError: The sender has disabled encrypting to unverified devices."`, async () => {
Expand Down Expand Up @@ -423,6 +455,8 @@ describe("MatrixEvent", () => {
expect(eventAttemptDecryptionSpy).toHaveBeenCalledTimes(2);
expect(crypto.decryptEvent).toHaveBeenCalledTimes(2);
expect(encryptedEvent.getType()).toEqual("m.room.message");
expect(encryptedEvent.isDecryptionFailure()).toBe(false);
expect(encryptedEvent.decryptionFailureReason).toBe(null);
});
});

Expand Down
7 changes: 5 additions & 2 deletions spec/unit/room-state.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,11 @@ import { EventType, RelationType, UNSTABLE_MSC2716_MARKER } from "../../src/@typ
import { MatrixEvent, MatrixEventEvent } from "../../src/models/event";
import { M_BEACON } from "../../src/@types/beacon";
import { MatrixClient } from "../../src/client";
import { DecryptionError } from "../../src/crypto/algorithms";
import { defer } from "../../src/utils";
import { Room } from "../../src/models/room";
import { KnownMembership } from "../../src/@types/membership";
import { DecryptionFailureCode } from "../../src/crypto-api";
import { DecryptionError } from "../../src/common-crypto/CryptoBackend";

describe("RoomState", function () {
const roomId = "!foo:bar";
Expand Down Expand Up @@ -1040,7 +1041,9 @@ describe("RoomState", function () {
content: beacon1RelationContent,
});
jest.spyOn(failedDecryptionRelatedEvent, "isDecryptionFailure").mockReturnValue(true);
mockClient.decryptEventIfNeeded.mockRejectedValue(new DecryptionError("ERR", "msg"));
mockClient.decryptEventIfNeeded.mockRejectedValue(
new DecryptionError(DecryptionFailureCode.UNKNOWN_ERROR, "msg"),
);
// spy on event.once
const eventOnceSpy = jest.spyOn(failedDecryptionRelatedEvent, "once");

Expand Down
5 changes: 4 additions & 1 deletion spec/unit/testing.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ limitations under the License.

import { mkDecryptionFailureMatrixEvent, mkEncryptedMatrixEvent, mkMatrixEvent } from "../../src/testing";
import { EventType } from "../../src";
import { DecryptionFailureCode } from "../../src/crypto-api";

describe("testing", () => {
describe("mkMatrixEvent", () => {
Expand Down Expand Up @@ -60,6 +61,7 @@ describe("testing", () => {
expect(event.sender?.userId).toEqual("@alice:test");
expect(event.isEncrypted()).toBe(true);
expect(event.isDecryptionFailure()).toBe(false);
expect(event.decryptionFailureReason).toBe(null);
expect(event.getContent()).toEqual({ body: "blah" });
expect(event.getType()).toEqual("m.room.message");
});
Expand All @@ -70,13 +72,14 @@ describe("testing", () => {
const event = await mkDecryptionFailureMatrixEvent({
sender: "@alice:test",
roomId: "!test:room",
code: "UNKNOWN",
code: DecryptionFailureCode.UNKNOWN_ERROR,
msg: "blah",
});

expect(event.sender?.userId).toEqual("@alice:test");
expect(event.isEncrypted()).toBe(true);
expect(event.isDecryptionFailure()).toBe(true);
expect(event.decryptionFailureReason).toEqual(DecryptionFailureCode.UNKNOWN_ERROR);
expect(event.getContent()).toEqual({
body: "** Unable to decrypt: DecryptionError: blah **",
msgtype: "m.bad.encrypted",
Expand Down
46 changes: 41 additions & 5 deletions src/common-crypto/CryptoBackend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ limitations under the License.
import type { IDeviceLists, IToDeviceEvent } from "../sync-accumulator";
import { IClearEvent, MatrixEvent } from "../models/event";
import { Room } from "../models/room";
import { CryptoApi, ImportRoomKeysOpts } from "../crypto-api";
import { CryptoApi, DecryptionFailureCode, ImportRoomKeysOpts } from "../crypto-api";
import { CrossSigningInfo, UserTrustLevel } from "../crypto/CrossSigning";
import { IEncryptedEventInfo } from "../crypto/api";
import { KeyBackupInfo, KeyBackupSession } from "../crypto-api/keybackup";
Expand Down Expand Up @@ -226,10 +226,6 @@ export interface EventDecryptionResult {
* restored from backup)
*/
untrusted?: boolean;
/**
* The sender doesn't authorize the unverified devices to decrypt his messages
*/
encryptedDisabledForUnverifiedDevices?: boolean;
}

/**
Expand Down Expand Up @@ -263,3 +259,43 @@ export interface BackupDecryptor {
*/
free(): void;
}

/**
* Exception thrown when decryption fails
*
* @param code - Reason code for the failure.
*
* @param msg - user-visible message describing the problem
*
* @param details - key/value pairs reported in the logs but not shown
* to the user.
*/
export class DecryptionError extends Error {
public readonly detailedString: string;

public constructor(
public readonly code: DecryptionFailureCode,
msg: string,
details?: Record<string, string | Error>,
) {
super(msg);
this.name = "DecryptionError";
this.detailedString = detailedStringForDecryptionError(this, details);
}
}

function detailedStringForDecryptionError(err: DecryptionError, details?: Record<string, string | Error>): string {
let result = err.name + "[msg: " + err.message;

if (details) {
result +=
", " +
Object.keys(details)
.map((k) => k + ": " + details[k])
.join(", ");
}

result += "]";

return result;
}
51 changes: 51 additions & 0 deletions src/crypto-api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,57 @@ export interface CryptoApi {
deleteKeyBackupVersion(version: string): Promise<void>;
}

/** A reason code for a failure to decrypt an event. */
export enum DecryptionFailureCode {
/** Message was encrypted with a Megolm session whose keys have not been shared with us. */
MEGOLM_UNKNOWN_INBOUND_SESSION_ID = "MEGOLM_UNKNOWN_INBOUND_SESSION_ID",

/** Message was encrypted with a Megolm session which has been shared with us, but in a later ratchet state. */
OLM_UNKNOWN_MESSAGE_INDEX = "OLM_UNKNOWN_MESSAGE_INDEX",

/** Unknown or unclassified error. */
UNKNOWN_ERROR = "UNKNOWN_ERROR",

/** @deprecated only used in legacy crypto */
MEGOLM_BAD_ROOM = "MEGOLM_BAD_ROOM",

/** @deprecated only used in legacy crypto */
MEGOLM_MISSING_FIELDS = "MEGOLM_MISSING_FIELDS",

/** @deprecated only used in legacy crypto */
OLM_DECRYPT_GROUP_MESSAGE_ERROR = "OLM_DECRYPT_GROUP_MESSAGE_ERROR",

/** @deprecated only used in legacy crypto */
OLM_BAD_ENCRYPTED_MESSAGE = "OLM_BAD_ENCRYPTED_MESSAGE",

/** @deprecated only used in legacy crypto */
OLM_BAD_RECIPIENT = "OLM_BAD_RECIPIENT",

/** @deprecated only used in legacy crypto */
OLM_BAD_RECIPIENT_KEY = "OLM_BAD_RECIPIENT_KEY",

/** @deprecated only used in legacy crypto */
OLM_BAD_ROOM = "OLM_BAD_ROOM",

/** @deprecated only used in legacy crypto */
OLM_BAD_SENDER_CHECK_FAILED = "OLM_BAD_SENDER_CHECK_FAILED",

/** @deprecated only used in legacy crypto */
OLM_BAD_SENDER = "OLM_BAD_SENDER",

/** @deprecated only used in legacy crypto */
OLM_FORWARDED_MESSAGE = "OLM_FORWARDED_MESSAGE",

/** @deprecated only used in legacy crypto */
OLM_MISSING_CIPHERTEXT = "OLM_MISSING_CIPHERTEXT",

/** @deprecated only used in legacy crypto */
OLM_NOT_INCLUDED_IN_RECIPIENTS = "OLM_NOT_INCLUDED_IN_RECIPIENTS",

/** @deprecated only used in legacy crypto */
UNKNOWN_ENCRYPTION_ALGORITHM = "UNKNOWN_ENCRYPTION_ALGORITHM",
}

/**
* Options object for `CryptoApi.bootstrapCrossSigning`.
*/
Expand Down
11 changes: 6 additions & 5 deletions src/crypto/OlmDevice.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ import { Account, InboundGroupSession, OutboundGroupSession, Session, Utility }

import { logger, Logger } from "../logger";
import { IndexedDBCryptoStore } from "./store/indexeddb-crypto-store";
import * as algorithms from "./algorithms";
import { CryptoStore, IProblem, ISessionInfo, IWithheld } from "./store/base";
import { IOlmDevice, IOutboundGroupSessionKey } from "./algorithms/megolm";
import { IMegolmSessionData, OlmGroupSessionExtraData } from "../@types/crypto";
import { IMessage } from "./algorithms/olm";
import { DecryptionFailureCode } from "../crypto-api";
import { DecryptionError } from "../common-crypto/CryptoBackend";

// The maximum size of an event is 65K, and we base64 the content, so this is a
// reasonable approximation to the biggest plaintext we can encrypt.
Expand Down Expand Up @@ -1220,8 +1221,8 @@ export class OlmDevice {
this.getInboundGroupSession(roomId, senderKey, sessionId, txn, (session, sessionData, withheld) => {
if (session === null || sessionData === null) {
if (withheld) {
error = new algorithms.DecryptionError(
"MEGOLM_UNKNOWN_INBOUND_SESSION_ID",
error = new DecryptionError(
DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID,
calculateWithheldMessage(withheld),
{
session: senderKey + "|" + sessionId,
Expand All @@ -1236,8 +1237,8 @@ export class OlmDevice {
res = session.decrypt(body);
} catch (e) {
if ((<Error>e)?.message === "OLM.UNKNOWN_MESSAGE_INDEX" && withheld) {
error = new algorithms.DecryptionError(
"MEGOLM_UNKNOWN_INBOUND_SESSION_ID",
error = new DecryptionError(
DecryptionFailureCode.MEGOLM_UNKNOWN_INBOUND_SESSION_ID,
calculateWithheldMessage(withheld),
{
session: senderKey + "|" + sessionId,
Expand Down
42 changes: 3 additions & 39 deletions src/crypto/algorithms/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,45 +199,6 @@ export abstract class DecryptionAlgorithm {
public sendSharedHistoryInboundSessions?(devicesByUser: Map<string, DeviceInfo[]>): Promise<void>;
}

/**
* Exception thrown when decryption fails
*
* @param msg - user-visible message describing the problem
*
* @param details - key/value pairs reported in the logs but not shown
* to the user.
*/
export class DecryptionError extends Error {
public readonly detailedString: string;

public constructor(
public readonly code: string,
Copy link
Member

Choose a reason for hiding this comment

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

Is this a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well first of all, this class (and indeed the whole file) is supposed to be internal, so anyone using it can keep both halves when it breaks.

In any case: It would only be a breaking change for things constructing a DecryptionError: anything reading code once it is created will implicitly cast the DecryptionFailureCode to a string. And I've not been able to find any code constructing a DecryptionError outside the js-sdk.

msg: string,
details?: Record<string, string | Error>,
) {
super(msg);
this.code = code;
this.name = "DecryptionError";
this.detailedString = detailedStringForDecryptionError(this, details);
}
}

function detailedStringForDecryptionError(err: DecryptionError, details?: Record<string, string | Error>): string {
let result = err.name + "[msg: " + err.message;

if (details) {
result +=
", " +
Object.keys(details)
.map((k) => k + ": " + details[k])
.join(", ");
}

result += "]";

return result;
}

export class UnknownDeviceError extends Error {
/**
* Exception thrown specifically when we want to warn the user to consider
Expand Down Expand Up @@ -274,3 +235,6 @@ export function registerAlgorithm<P extends IParams = IParams>(
ENCRYPTION_CLASSES.set(algorithm, encryptor as new (params: IParams) => EncryptionAlgorithm);
DECRYPTION_CLASSES.set(algorithm, decryptor as new (params: DecryptionClassParams) => DecryptionAlgorithm);
}

/* Re-export for backwards compatibility. Deprecated: this is an internal class. */
export { DecryptionError } from "../../common-crypto/CryptoBackend";
Loading
Loading