From ab50038df8e3eb0014709ec420a764230c1962c9 Mon Sep 17 00:00:00 2001
From: Richard van der Hoff <richard@matrix.org>
Date: Wed, 11 Sep 2024 13:36:28 +0100
Subject: [PATCH] Lifecycle: bail out if picklekey is missing
Currently, if we have an accesstoken which is encrypted with a picklekey, but
the picklekey has gone missing, we carry on with no access token at all. This
is sure to blow up in some way or other later on, but in a rather cryptic way.
Instead, let's bail out early.
(This will produce a "can't restore session" error, but we normally see one of
those anyway because we can't initialise the crypto store.)
---
src/Lifecycle.ts | 7 ++++---
src/utils/tokens/tokens.ts | 29 ++++++++++++++---------------
test/Lifecycle-test.ts | 23 ++++++++++++++++++++++-
3 files changed, 40 insertions(+), 19 deletions(-)
diff --git a/src/Lifecycle.ts b/src/Lifecycle.ts
index 0575d2163f..47e44c43dc 100644
--- a/src/Lifecycle.ts
+++ b/src/Lifecycle.ts
@@ -592,10 +592,11 @@ export async function restoreSessionFromStorage(opts?: { ignoreGuest?: boolean }
if (pickleKey) {
logger.log(`Got pickle key for ${userId}|${deviceId}`);
} else {
- logger.log("No pickle key available");
+ logger.log(`No pickle key available for ${userId}|${deviceId}`);
}
const decryptedAccessToken = await tryDecryptToken(pickleKey, accessToken, ACCESS_TOKEN_IV);
- const decryptedRefreshToken = await tryDecryptToken(pickleKey, refreshToken, REFRESH_TOKEN_IV);
+ const decryptedRefreshToken =
+ refreshToken && (await tryDecryptToken(pickleKey, refreshToken, REFRESH_TOKEN_IV));
const freshLogin = sessionStorage.getItem("mx_fresh_login") === "true";
sessionStorage.removeItem("mx_fresh_login");
@@ -605,7 +606,7 @@ export async function restoreSessionFromStorage(opts?: { ignoreGuest?: boolean }
{
userId: userId,
deviceId: deviceId,
- accessToken: decryptedAccessToken!,
+ accessToken: decryptedAccessToken,
refreshToken: decryptedRefreshToken,
homeserverUrl: hsUrl,
identityServerUrl: isUrl,
diff --git a/src/utils/tokens/tokens.ts b/src/utils/tokens/tokens.ts
index b6634171e0..a82259242a 100644
--- a/src/utils/tokens/tokens.ts
+++ b/src/utils/tokens/tokens.ts
@@ -68,10 +68,6 @@ async function pickleKeyToAesKey(pickleKey: string): Promise<Uint8Array> {
);
}
-const isEncryptedPayload = (token?: IEncryptedPayload | string | undefined): token is IEncryptedPayload => {
- return !!token && typeof token !== "string";
-};
-
/**
* Try to decrypt a token retrieved from storage
*
@@ -86,24 +82,27 @@ const isEncryptedPayload = (token?: IEncryptedPayload | string | undefined): tok
* @param tokenName Name of the token. Used in logging, but also used as an input when generating the actual AES key,
* so the same value must be provided to {@link persistTokenInStorage}.
*
- * @returns the decrypted token, or the plain text token. Returns undefined when token cannot be decrypted
+ * @returns the decrypted token, or the plain text token.
*/
export async function tryDecryptToken(
pickleKey: string | undefined,
- token: IEncryptedPayload | string | undefined,
+ token: IEncryptedPayload | string,
tokenName: string,
-): Promise<string | undefined> {
- if (pickleKey && isEncryptedPayload(token)) {
- const encrKey = await pickleKeyToAesKey(pickleKey);
- const decryptedToken = await decryptAES(token, encrKey, tokenName);
- encrKey.fill(0);
- return decryptedToken;
- }
- // if the token wasn't encrypted (plain string) just return it back
+): Promise<string> {
if (typeof token === "string") {
+ // Looks like an unencrypted token
return token;
}
- // otherwise return undefined
+
+ // Otherwise, it must be an encrypted token.
+ if (!pickleKey) {
+ throw new Error(`Error decrypting secret ${tokenName}: no pickle key found.`);
+ }
+
+ const encrKey = await pickleKeyToAesKey(pickleKey);
+ const decryptedToken = await decryptAES(token, encrKey, tokenName);
+ encrKey.fill(0);
+ return decryptedToken;
}
/**
diff --git a/test/Lifecycle-test.ts b/test/Lifecycle-test.ts
index b0d044c10e..6ee6cb1154 100644
--- a/test/Lifecycle-test.ts
+++ b/test/Lifecycle-test.ts
@@ -145,7 +145,12 @@ describe("Lifecycle", () => {
mockStore[tableKey] = table;
},
);
- jest.spyOn(StorageAccess, "idbDelete").mockClear().mockResolvedValue(undefined);
+ jest.spyOn(StorageAccess, "idbDelete")
+ .mockClear()
+ .mockImplementation(async (tableKey: string, key: string | string[]) => {
+ const table = mockStore[tableKey];
+ delete table?.[key as string];
+ });
};
const homeserverUrl = "https://server.org";
@@ -521,6 +526,22 @@ describe("Lifecycle", () => {
expect(await restoreSessionFromStorage()).toEqual(true);
});
+
+ it("should throw if the token was persisted with a pickle key but there is no pickle key available now", async () => {
+ initLocalStorageMock(localStorageSession);
+ initIdbMock({});
+
+ // Create a pickle key, and store it, encrypted, in IDB.
+ const pickleKey = (await PlatformPeg.get()!.createPickleKey(credentials.userId, credentials.deviceId))!;
+ localStorage.setItem("mx_has_pickle_key", "true");
+ await persistAccessTokenInStorage(credentials.accessToken, pickleKey);
+
+ // Now destroy the pickle key
+ await PlatformPeg.get()!.destroyPickleKey(credentials.userId, credentials.deviceId);
+
+ console.log("10");
+ expect(await restoreSessionFromStorage()).toEqual(true);
+ });
});
});
--
GitLab