Skip to content

Commit

Permalink
Refactor & make base64 functions browser-safe
Browse files Browse the repository at this point in the history
We had two identical sets of base64 functions in the js-sdk, both
using Buffer which isn't really available in the browser unless you're
using an old webpack (ie. what element-web uses). This PR:

 * Takes the crypto base64 file and moves it out of crypto (because
   we use base64 for much more than just crypto)
 * Makes them work in a browser without the Buffer global
 * Removes the other base64 functions
 * Changes everything to use the new common ones
 * Adds a comment explaining why the function is kinda ugly and how
   soul destroyingly awful the JS ecosystem is.
 * Runs the tests with both impls
 * Changes the test to not just test the decoder against the encoder
 * Adds explicit support & tests for (decoding) base64Url (I'll add an
   encode method later, no need for that to go in this PR too).
  • Loading branch information
dbkr committed Oct 20, 2023
1 parent c026495 commit 31f3855
Show file tree
Hide file tree
Showing 16 changed files with 140 additions and 103 deletions.
40 changes: 34 additions & 6 deletions spec/unit/crypto/base64.spec.ts → spec/unit/base64.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,16 +15,44 @@ limitations under the License.
*/

import { TextEncoder, TextDecoder } from "util";
import NodeBuffer from "node:buffer";

import { decodeBase64, encodeBase64, encodeUnpaddedBase64 } from "../../../src/common-crypto/base64";
import { decodeBase64, encodeBase64, encodeUnpaddedBase64 } from "../../src/base64";

describe.each(["browser", "node"])("Crypto Base64 encoding (%s)", (env) => {
let origBuffer = Buffer;

beforeAll(() => {
if (env === "browser") {
origBuffer = Buffer;
// @ts-ignore
// eslint-disable-next-line no-global-assign
Buffer = undefined;

global.atob = NodeBuffer.atob;
global.btoa = NodeBuffer.btoa;
}
});

afterAll(() => {
// eslint-disable-next-line no-global-assign
Buffer = origBuffer;
// @ts-ignore
global.atob = undefined;
// @ts-ignore
global.btoa = undefined;
});

describe("Crypto Base64 encoding", () => {
it("Should decode properly encoded data", async () => {
const toEncode = "encoding hello world";
const encoded = encodeBase64(new TextEncoder().encode(toEncode));
const decoded = new TextDecoder().decode(decodeBase64(encoded));
const decoded = new TextDecoder().decode(decodeBase64("ZW5jb2RpbmcgaGVsbG8gd29ybGQ="));

expect(decoded).toStrictEqual("encoding hello world");
});

it("Should decode URL-safe base64", async () => {
const decoded = new TextDecoder().decode(decodeBase64("Pz8_Pz8="));

expect(decoded).toStrictEqual(toEncode);
expect(decoded).toStrictEqual("?????");
});

it("Encode unpadded should not have padding", async () => {
Expand Down
9 changes: 5 additions & 4 deletions spec/unit/crypto/secrets.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import { DeviceInfo } from "../../../src/crypto/deviceinfo";
import { ISignatures } from "../../../src/@types/signed";
import { ICurve25519AuthData } from "../../../src/crypto/keybackup";
import { SecretStorageKeyDescription, SECRET_STORAGE_ALGORITHM_V1_AES } from "../../../src/secret-storage";
import { decodeBase64 } from "../../../src/base64";

async function makeTestClient(
userInfo: { userId: string; deviceId: string },
Expand Down Expand Up @@ -275,13 +276,13 @@ describe("Secrets", function () {

describe("bootstrap", function () {
// keys used in some of the tests
const XSK = new Uint8Array(olmlib.decodeBase64("3lo2YdJugHjfE+Or7KJ47NuKbhE7AAGLgQ/dc19913Q="));
const XSK = new Uint8Array(decodeBase64("3lo2YdJugHjfE+Or7KJ47NuKbhE7AAGLgQ/dc19913Q="));
const XSPubKey = "DRb8pFVJyEJ9OWvXeUoM0jq/C2Wt+NxzBZVuk2nRb+0";
const USK = new Uint8Array(olmlib.decodeBase64("lKWi3hJGUie5xxHgySoz8PHFnZv6wvNaud/p2shN9VU="));
const USK = new Uint8Array(decodeBase64("lKWi3hJGUie5xxHgySoz8PHFnZv6wvNaud/p2shN9VU="));
const USPubKey = "CUpoiTtHiyXpUmd+3ohb7JVxAlUaOG1NYs9Jlx8soQU";
const SSK = new Uint8Array(olmlib.decodeBase64("1R6JVlXX99UcfUZzKuCDGQgJTw8ur1/ofgPD8pp+96M="));
const SSK = new Uint8Array(decodeBase64("1R6JVlXX99UcfUZzKuCDGQgJTw8ur1/ofgPD8pp+96M="));
const SSPubKey = "0DfNsRDzEvkCLA0gD3m7VAGJ5VClhjEsewI35xq873Q";
const SSSSKey = new Uint8Array(olmlib.decodeBase64("XrmITOOdBhw6yY5Bh7trb/bgp1FRdIGyCUxxMP873R0="));
const SSSSKey = new Uint8Array(decodeBase64("XrmITOOdBhw6yY5Bh7trb/bgp1FRdIGyCUxxMP873R0="));

it("bootstraps when no storage or cross-signing keys locally", async function () {
const key = new Uint8Array(16);
Expand Down
2 changes: 1 addition & 1 deletion spec/unit/crypto/verification/secret_request.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ limitations under the License.

import "../../../olm-loader";
import { MatrixClient, MatrixEvent } from "../../../../src/matrix";
import { encodeBase64 } from "../../../../src/crypto/olmlib";
import { encodeBase64 } from "../../../../src/base64";
import "../../../../src/crypto"; // import this to cycle-break
import { CrossSigningInfo } from "../../../../src/crypto/CrossSigning";
import { VerificationRequest } from "../../../../src/crypto/verification/request/VerificationRequest";
Expand Down
2 changes: 1 addition & 1 deletion spec/unit/rendezvous/ecdhv2.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ limitations under the License.
import "../../olm-loader";
import { RendezvousFailureReason, RendezvousIntent } from "../../../src/rendezvous";
import { MSC3903ECDHPayload, MSC3903ECDHv2RendezvousChannel } from "../../../src/rendezvous/channels";
import { decodeBase64 } from "../../../src/crypto/olmlib";
import { decodeBase64 } from "../../../src/base64";
import { DummyTransport } from "./DummyTransport";

function makeTransport(name: string) {
Expand Down
2 changes: 1 addition & 1 deletion spec/unit/rendezvous/rendezvous.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
MSC3886SimpleHttpRendezvousTransportDetails,
} from "../../../src/rendezvous/transports";
import { DummyTransport } from "./DummyTransport";
import { decodeBase64 } from "../../../src/crypto/olmlib";
import { decodeBase64 } from "../../../src/base64";
import { logger } from "../../../src/logger";
import { DeviceInfo } from "../../../src/crypto/deviceinfo";

Expand Down
79 changes: 79 additions & 0 deletions src/base64.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
Copyright 2023 The Matrix.org Foundation C.I.C.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

/**
* Base64 encoding and decoding utilities
*/

/**
* Encode a typed array of uint8 as base64.
* @param uint8Array - The data to encode.
* @returns The base64.
*/
export function encodeBase64(uint8Array: ArrayBuffer | Uint8Array): string {
// A brief note on the state of base64 encoding in Javascript.
// As of 2023, there is still no common native impl between both browsers and
// node. Older Webpack provides an impl for Buffer and there is a polyfill class
// for it. There are also plenty of pure js impls, eg. base64-js which has 2336
// dependents at current count. Using this would probably be fine although it's
// a little under-docced and run by an individual. The node impl works fine,
// the browser impl works but predates Uint8Array and so only uses strings.
// Right now, switching between native (or polyfilled) impls like this feels
// like the least bad option, but... *shrugs*.
if (typeof Buffer === "function") {
return Buffer.from(uint8Array).toString("base64");
} else if (typeof btoa === "function" && uint8Array instanceof Uint8Array) {
// ArrayBuffer is a node concept so the param should always be a Uint8Array on
// the browser. We need to check because ArrayBuffers don't have reduce.
return btoa(uint8Array.reduce((acc, current) => acc + String.fromCharCode(current), ""));
} else {
throw new Error("No base64 impl found!");
}
}

/**
* Encode a typed array of uint8 as unpadded base64.
* @param uint8Array - The data to encode.
* @returns The unpadded base64.
*/
export function encodeUnpaddedBase64(uint8Array: ArrayBuffer | Uint8Array): string {
return encodeBase64(uint8Array).replace(/={1,2}$/, "");
}

/**
* Decode a base64 string to a typed array of uint8.
* @param base64 - The base64 to decode.
* @returns The decoded data.
*/
export function decodeBase64(base64: string): Uint8Array {
// See encodeBase64 for a short treatise on base64 en/decoding in JS
if (typeof Buffer === "function") {
return Buffer.from(base64, "base64");
} else if (typeof atob === "function") {
const itFunc = function* (): Generator<number> {
const decoded = atob(
// built-in atob doesn't support base64url: convert so we support either
base64.replace("-", "+").replace("_", "/"),
);
for (let i = 0; i < decoded.length; ++i) {
yield decoded.charCodeAt(i);
}
};
return Uint8Array.from(itFunc());
} else {
throw new Error("No base64 impl found!");
}
}
2 changes: 1 addition & 1 deletion src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ import { Direction, EventTimeline } from "./models/event-timeline";
import { IActionsObject, PushProcessor } from "./pushprocessor";
import { AutoDiscovery, AutoDiscoveryAction } from "./autodiscovery";
import * as olmlib from "./crypto/olmlib";
import { decodeBase64, encodeBase64 } from "./crypto/olmlib";
import { decodeBase64, encodeBase64 } from "./base64";
import { IExportedDevice as IExportedOlmDevice } from "./crypto/OlmDevice";
import { IOlmDevice } from "./crypto/algorithms/megolm";
import { TypedReEmitter } from "./ReEmitter";
Expand Down
46 changes: 0 additions & 46 deletions src/common-crypto/base64.ts

This file was deleted.

3 changes: 2 additions & 1 deletion src/crypto/CrossSigning.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ limitations under the License.
*/

import type { PkSigning } from "@matrix-org/olm";
import { decodeBase64, encodeBase64, IObject, pkSign, pkVerify } from "./olmlib";
import { IObject, pkSign, pkVerify } from "./olmlib";
import { logger } from "../logger";
import { IndexedDBCryptoStore } from "../crypto/store/indexeddb-crypto-store";
import { decryptAES, encryptAES } from "./aes";
Expand All @@ -31,6 +31,7 @@ import { ISignatures } from "../@types/signed";
import { CryptoStore, SecretStorePrivateKeys } from "./store/base";
import { ServerSideSecretStorage, SecretStorageKeyDescription } from "../secret-storage";
import { DeviceVerificationStatus, UserVerificationStatus as UserTrustLevel } from "../crypto-api";
import { decodeBase64, encodeBase64 } from "../base64";

// backwards-compatibility re-exports
export { UserTrustLevel };
Expand Down
2 changes: 1 addition & 1 deletion src/crypto/aes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

import { decodeBase64, encodeBase64 } from "./olmlib";
import { decodeBase64, encodeBase64 } from "../base64";
import { subtleCrypto, crypto, TextEncoder } from "./crypto";

// salt for HKDF, with 8 bytes of zeros
Expand Down
2 changes: 1 addition & 1 deletion src/crypto/dehydration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ limitations under the License.
import anotherjson from "another-json";

import type { IDeviceKeys, IOneTimeKey } from "../@types/crypto";
import { decodeBase64, encodeBase64 } from "./olmlib";
import { decodeBase64, encodeBase64 } from "../base64";
import { IndexedDBCryptoStore } from "../crypto/store/indexeddb-crypto-store";
import { decryptAES, encryptAES } from "./aes";
import { logger } from "../logger";
Expand Down
21 changes: 11 additions & 10 deletions src/crypto/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ import {
import { Device, DeviceMap } from "../models/device";
import { deviceInfoToDevice } from "./device-converter";
import { ClientPrefix, MatrixError, Method } from "../http-api";
import { decodeBase64, encodeBase64 } from "../base64";

/* re-exports for backwards compatibility */
export type {
Expand Down Expand Up @@ -500,7 +501,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
await this.secretStorage.store("m.megolm_backup.v1", fixedKey, [keys![0]]);
}

return olmlib.decodeBase64(fixedKey || storedKey);
return decodeBase64(fixedKey || storedKey);
}

// try to get key from app
Expand Down Expand Up @@ -1050,7 +1051,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
newKeyId = await createSSSS(opts, backupKey);

// store the backup key in secret storage
await secretStorage.store("m.megolm_backup.v1", olmlib.encodeBase64(backupKey!), [newKeyId]);
await secretStorage.store("m.megolm_backup.v1", encodeBase64(backupKey!), [newKeyId]);

// The backup is trusted because the user provided the private key.
// Sign the backup with the cross-signing key so the key backup can
Expand Down Expand Up @@ -1094,7 +1095,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
);
// write the key to 4S
const privateKey = decodeRecoveryKey(info.recovery_key);
await secretStorage.store("m.megolm_backup.v1", olmlib.encodeBase64(privateKey));
await secretStorage.store("m.megolm_backup.v1", encodeBase64(privateKey));

// create keyBackupInfo object to add to builder
const data: IKeyBackupInfo = {
Expand Down Expand Up @@ -1122,7 +1123,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
const keyId = newKeyId || oldKeyId;
await secretStorage.store("m.megolm_backup.v1", fixedBackupKey, keyId ? [keyId] : null);
}
const decodedBackupKey = new Uint8Array(olmlib.decodeBase64(fixedBackupKey || sessionBackupKey));
const decodedBackupKey = new Uint8Array(decodeBase64(fixedBackupKey || sessionBackupKey));
builder.addSessionBackupPrivateKeyToCache(decodedBackupKey);
} else if (this.backupManager.getKeyBackupEnabled()) {
// key backup is enabled but we don't have a session backup key in SSSS: see if we have one in
Expand All @@ -1137,7 +1138,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
return;
}
logger.info("Got session backup key from cache/user that wasn't in SSSS: saving to SSSS");
await secretStorage.store("m.megolm_backup.v1", olmlib.encodeBase64(backupKey));
await secretStorage.store("m.megolm_backup.v1", encodeBase64(backupKey));
}

const operation = builder.buildOperation();
Expand Down Expand Up @@ -1178,7 +1179,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap

// write the key to 4S
const privateKey = info.privateKey;
await this.secretStorage.store("m.megolm_backup.v1", olmlib.encodeBase64(privateKey));
await this.secretStorage.store("m.megolm_backup.v1", encodeBase64(privateKey));
await this.storeSessionBackupPrivateKey(privateKey);

await this.backupManager.checkAndStart();
Expand Down Expand Up @@ -1301,13 +1302,13 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap

// make sure we have a Uint8Array, rather than a string
if (typeof encodedKey === "string") {
key = new Uint8Array(olmlib.decodeBase64(fixBackupKey(encodedKey) || encodedKey));
key = new Uint8Array(decodeBase64(fixBackupKey(encodedKey) || encodedKey));
await this.storeSessionBackupPrivateKey(key);
}
if (encodedKey && typeof encodedKey === "object" && "ciphertext" in encodedKey) {
const pickleKey = Buffer.from(this.olmDevice.pickleKey);
const decrypted = await decryptAES(encodedKey, pickleKey, "m.megolm_backup.v1");
key = olmlib.decodeBase64(decrypted);
key = decodeBase64(decrypted);
}
return key;
}
Expand All @@ -1323,7 +1324,7 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
throw new Error(`storeSessionBackupPrivateKey expects Uint8Array, got ${key}`);
}
const pickleKey = Buffer.from(this.olmDevice.pickleKey);
const encryptedKey = await encryptAES(olmlib.encodeBase64(key), pickleKey, "m.megolm_backup.v1");
const encryptedKey = await encryptAES(encodeBase64(key), pickleKey, "m.megolm_backup.v1");
return this.cryptoStore.doTxn("readwrite", [IndexedDBCryptoStore.STORE_ACCOUNT], (txn) => {
this.cryptoStore.storeSecretStorePrivateKey(txn, "m.megolm_backup.v1", encryptedKey);
});
Expand Down Expand Up @@ -4196,7 +4197,7 @@ export function fixBackupKey(key?: string): string | null {
return null;
}
const fixedKey = Uint8Array.from(key.split(","), (x) => parseInt(x));
return olmlib.encodeBase64(fixedKey);
return encodeBase64(fixedKey);
}

/**
Expand Down
Loading

0 comments on commit 31f3855

Please sign in to comment.