From 86494c3a96d144b5ca2c154b396c9e4807fb2f05 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 16 Jan 2025 14:48:36 +0000 Subject: [PATCH] Change randomString et al to be secure ...and renames them, removing the special lowercase and uppercase versions and exporting the underlying function instead. Any apps that use these will either need to take the speed hit from secure random functions and use the new ones, or write their own insecure versions. The lowercase and uppercasde verisons were used exactly once each in element-web and never in js-sdk itself. The underlying function is very simple and exporting just this gives more flexibility with fewer exports. --- spec/unit/interactive-auth.spec.ts | 4 +- spec/unit/matrixrtc/MatrixRTCSession.spec.ts | 6 +-- spec/unit/matrixrtc/mocks.ts | 4 +- spec/unit/randomstring.spec.ts | 47 +++++++++++-------- spec/unit/secret-storage.spec.ts | 4 +- spec/unit/thread-utils.spec.ts | 4 +- src/client.ts | 6 +-- src/crypto/key_passphrase.ts | 4 +- .../verification/request/ToDeviceChannel.ts | 4 +- src/oidc/authorize.ts | 10 ++-- src/randomstring.ts | 41 +++++++++------- src/rust-crypto/rust-crypto.ts | 6 +-- src/secret-storage.ts | 4 +- src/webrtc/call.ts | 4 +- 14 files changed, 80 insertions(+), 68 deletions(-) diff --git a/spec/unit/interactive-auth.spec.ts b/spec/unit/interactive-auth.spec.ts index 0d4177f55..e867788ff 100644 --- a/spec/unit/interactive-auth.spec.ts +++ b/spec/unit/interactive-auth.spec.ts @@ -20,7 +20,7 @@ import { logger } from "../../src/logger"; import { InteractiveAuth, AuthType } from "../../src/interactive-auth"; import { HTTPError, MatrixError } from "../../src/http-api"; import { sleep } from "../../src/utils"; -import { randomString } from "../../src/randomstring"; +import { secureRandomString } from "../../src/randomstring"; // Trivial client object to test interactive auth // (we do not need TestClient here) @@ -502,7 +502,7 @@ describe("InteractiveAuth", () => { const doRequest = jest.fn(); const stateUpdated = jest.fn(); const requestEmailToken = jest.fn(); - const sid = randomString(24); + const sid = secureRandomString(24); requestEmailToken.mockImplementation(() => sleep(500, { sid })); const ia = new InteractiveAuth({ diff --git a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts index 7e50d9522..801c2f145 100644 --- a/spec/unit/matrixrtc/MatrixRTCSession.spec.ts +++ b/spec/unit/matrixrtc/MatrixRTCSession.spec.ts @@ -19,7 +19,7 @@ import { KnownMembership } from "../../../src/@types/membership"; import { DEFAULT_EXPIRE_DURATION, SessionMembershipData } from "../../../src/matrixrtc/CallMembership"; import { MatrixRTCSession, MatrixRTCSessionEvent } from "../../../src/matrixrtc/MatrixRTCSession"; import { EncryptionKeysEventContent } from "../../../src/matrixrtc/types"; -import { randomString } from "../../../src/randomstring"; +import { secureRandomString } from "../../../src/randomstring"; import { flushPromises } from "../../test-utils/flushPromises"; import { makeMockRoom, makeMockRoomState, membershipTemplate } from "./mocks"; @@ -98,7 +98,7 @@ describe("MatrixRTCSession", () => { }); it("safely ignores events with no memberships section", () => { - const roomId = randomString(8); + const roomId = secureRandomString(8); const event = { getType: jest.fn().mockReturnValue(EventType.GroupCallMemberPrefix), getContent: jest.fn().mockReturnValue({}), @@ -133,7 +133,7 @@ describe("MatrixRTCSession", () => { }); it("safely ignores events with junk memberships section", () => { - const roomId = randomString(8); + const roomId = secureRandomString(8); const event = { getType: jest.fn().mockReturnValue(EventType.GroupCallMemberPrefix), getContent: jest.fn().mockReturnValue({ memberships: ["i am a fish"] }), diff --git a/spec/unit/matrixrtc/mocks.ts b/spec/unit/matrixrtc/mocks.ts index b5aa70960..3b35a2fab 100644 --- a/spec/unit/matrixrtc/mocks.ts +++ b/spec/unit/matrixrtc/mocks.ts @@ -16,7 +16,7 @@ limitations under the License. import { EventType, MatrixEvent, Room } from "../../../src"; import { SessionMembershipData } from "../../../src/matrixrtc/CallMembership"; -import { randomString } from "../../../src/randomstring"; +import { secureRandomString } from "../../../src/randomstring"; type MembershipData = SessionMembershipData[] | SessionMembershipData | {}; @@ -41,7 +41,7 @@ export const membershipTemplate: SessionMembershipData = { }; export function makeMockRoom(membershipData: MembershipData): Room { - const roomId = randomString(8); + const roomId = secureRandomString(8); // Caching roomState here so it does not get recreated when calling `getLiveTimeline.getState()` const roomState = makeMockRoomState(membershipData, roomId); const room = { diff --git a/spec/unit/randomstring.spec.ts b/spec/unit/randomstring.spec.ts index 526edfacf..8dec2c6c0 100644 --- a/spec/unit/randomstring.spec.ts +++ b/spec/unit/randomstring.spec.ts @@ -16,10 +16,11 @@ limitations under the License. import { decodeBase64 } from "../../src/base64"; import { - randomLowercaseString, - randomString, - randomUppercaseString, + secureRandomString, secureRandomBase64Url, + secureRandomStringFrom, + LOWERCASE, + UPPERCASE, } from "../../src/randomstring"; describe("Random strings", () => { @@ -33,34 +34,40 @@ describe("Random strings", () => { expect(decoded).toHaveLength(n); }); - it.each([8, 16, 32])("randomString generates string of %i characters", (n: number) => { - const rand1 = randomString(n); - const rand2 = randomString(n); + it.each([8, 16, 32])("secureRandomString generates string of %i characters", (n: number) => { + const rand1 = secureRandomString(n); + const rand2 = secureRandomString(n); expect(rand1).not.toEqual(rand2); expect(rand1).toHaveLength(n); }); - it.each([8, 16, 32])("randomLowercaseString generates lowercase string of %i characters", (n: number) => { - const rand1 = randomLowercaseString(n); - const rand2 = randomLowercaseString(n); + it.each([8, 16, 32])( + "secureRandomStringFrom generates lowercase string of %i characters when given lowercase", + (n: number) => { + const rand1 = secureRandomStringFrom(n, LOWERCASE); + const rand2 = secureRandomStringFrom(n, LOWERCASE); - expect(rand1).not.toEqual(rand2); + expect(rand1).not.toEqual(rand2); - expect(rand1).toHaveLength(n); + expect(rand1).toHaveLength(n); - expect(rand1.toLowerCase()).toEqual(rand1); - }); + expect(rand1.toLowerCase()).toEqual(rand1); + }, + ); - it.each([8, 16, 32])("randomUppercaseString generates lowercase string of %i characters", (n: number) => { - const rand1 = randomUppercaseString(n); - const rand2 = randomUppercaseString(n); + it.each([8, 16, 32])( + "secureRandomStringFrom generates uppercase string of %i characters when given uppercase", + (n: number) => { + const rand1 = secureRandomStringFrom(n, UPPERCASE); + const rand2 = secureRandomStringFrom(n, UPPERCASE); - expect(rand1).not.toEqual(rand2); + expect(rand1).not.toEqual(rand2); - expect(rand1).toHaveLength(n); + expect(rand1).toHaveLength(n); - expect(rand1.toUpperCase()).toEqual(rand1); - }); + expect(rand1.toUpperCase()).toEqual(rand1); + }, + ); }); diff --git a/spec/unit/secret-storage.spec.ts b/spec/unit/secret-storage.spec.ts index 9caaa7458..e3c736429 100644 --- a/spec/unit/secret-storage.spec.ts +++ b/spec/unit/secret-storage.spec.ts @@ -27,7 +27,7 @@ import { ServerSideSecretStorageImpl, trimTrailingEquals, } from "../../src/secret-storage"; -import { randomString } from "../../src/randomstring"; +import { secureRandomString } from "../../src/randomstring"; import { SecretInfo } from "../../src/secret-storage.ts"; import { AccountDataEvents, ClientEvent, MatrixEvent, TypedEventEmitter } from "../../src"; import { defer, IDeferred } from "../../src/utils"; @@ -179,7 +179,7 @@ describe("ServerSideSecretStorageImpl", function () { it("should return true for a correct key check", async function () { const secretStorage = new ServerSideSecretStorageImpl({} as AccountDataClient, {}); - const myKey = new TextEncoder().encode(randomString(32)); + const myKey = new TextEncoder().encode(secureRandomString(32)); const { iv, mac } = await calculateKeyCheck(myKey); const keyInfo: SecretStorageKeyDescriptionAesV1 = { diff --git a/spec/unit/thread-utils.spec.ts b/spec/unit/thread-utils.spec.ts index eba63d86e..8bab97bd8 100644 --- a/spec/unit/thread-utils.spec.ts +++ b/spec/unit/thread-utils.spec.ts @@ -15,12 +15,12 @@ limitations under the License. */ import { IEvent } from "../../src"; -import { randomString } from "../../src/randomstring"; +import { secureRandomString } from "../../src/randomstring"; import { getRelationsThreadFilter } from "../../src/thread-utils"; function makeEvent(relatesToEvent: string, relType: string): Partial { return { - event_id: randomString(10), + event_id: secureRandomString(10), type: "m.room.message", content: { "msgtype": "m.text", diff --git a/src/client.ts b/src/client.ts index 0e49b1609..22647bf42 100644 --- a/src/client.ts +++ b/src/client.ts @@ -160,7 +160,7 @@ import { Visibility, } from "./@types/partials.ts"; import { EventMapper, eventMapperFor, MapperOpts } from "./event-mapper.ts"; -import { randomString } from "./randomstring.ts"; +import { secureRandomString } from "./randomstring.ts"; import { BackupManager, IKeyBackup, IKeyBackupCheck, IPreparedKeyBackupVersion, TrustInfo } from "./crypto/backup.ts"; import { DEFAULT_TREE_POWER_LEVELS_TEMPLATE, MSC3089TreeSpace } from "./models/MSC3089TreeSpace.ts"; import { ISignatures } from "./@types/signed.ts"; @@ -1346,7 +1346,7 @@ export class MatrixClient extends TypedEventEmitter { - const salt = randomString(32); + const salt = secureRandomString(32); const key = await deriveRecoveryKeyFromPassphrase(passphrase, salt, DEFAULT_ITERATIONS); diff --git a/src/crypto/verification/request/ToDeviceChannel.ts b/src/crypto/verification/request/ToDeviceChannel.ts index 34bf6f51a..97e3c313a 100644 --- a/src/crypto/verification/request/ToDeviceChannel.ts +++ b/src/crypto/verification/request/ToDeviceChannel.ts @@ -15,7 +15,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -import { randomString } from "../../../randomstring.ts"; +import { secureRandomString } from "../../../randomstring.ts"; import { logger } from "../../../logger.ts"; import { CANCEL_TYPE, @@ -283,7 +283,7 @@ export class ToDeviceChannel implements IVerificationChannel { * @returns the transaction id */ public static makeTransactionId(): string { - return randomString(32); + return secureRandomString(32); } } diff --git a/src/oidc/authorize.ts b/src/oidc/authorize.ts index 7e3692dfb..d8b6c91a1 100644 --- a/src/oidc/authorize.ts +++ b/src/oidc/authorize.ts @@ -17,7 +17,7 @@ limitations under the License. import { IdTokenClaims, Log, OidcClient, SigninResponse, SigninState, WebStorageStateStore } from "oidc-client-ts"; import { logger } from "../logger.ts"; -import { randomString } from "../randomstring.ts"; +import { secureRandomString } from "../randomstring.ts"; import { OidcError } from "./error.ts"; import { BearerTokenResponse, @@ -52,7 +52,7 @@ export type AuthorizationParams = { * @returns scope */ export const generateScope = (deviceId?: string): string => { - const safeDeviceId = deviceId ?? randomString(10); + const safeDeviceId = deviceId ?? secureRandomString(10); return `openid urn:matrix:org.matrix.msc2967.client:api:* urn:matrix:org.matrix.msc2967.client:device:${safeDeviceId}`; }; @@ -79,9 +79,9 @@ const generateCodeChallenge = async (codeVerifier: string): Promise => { export const generateAuthorizationParams = ({ redirectUri }: { redirectUri: string }): AuthorizationParams => ({ scope: generateScope(), redirectUri, - state: randomString(8), - nonce: randomString(8), - codeVerifier: randomString(64), // https://tools.ietf.org/html/rfc7636#section-4.1 length needs to be 43-128 characters + state: secureRandomString(8), + nonce: secureRandomString(8), + codeVerifier: secureRandomString(64), // https://tools.ietf.org/html/rfc7636#section-4.1 length needs to be 43-128 characters }); /** diff --git a/src/randomstring.ts b/src/randomstring.ts index d0cc9e675..b448a025d 100644 --- a/src/randomstring.ts +++ b/src/randomstring.ts @@ -17,9 +17,9 @@ limitations under the License. import { encodeUnpaddedBase64Url } from "./base64.ts"; -const LOWERCASE = "abcdefghijklmnopqrstuvwxyz"; -const UPPERCASE = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; -const DIGITS = "0123456789"; +export const LOWERCASE = "abcdefghijklmnopqrstuvwxyz"; +export const UPPERCASE = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; +export const DIGITS = "0123456789"; export function secureRandomBase64Url(len: number): string { const key = new Uint8Array(len); @@ -28,24 +28,29 @@ export function secureRandomBase64Url(len: number): string { return encodeUnpaddedBase64Url(key); } -export function randomString(len: number): string { - return randomStringFrom(len, UPPERCASE + LOWERCASE + DIGITS); +/** + * Generates a random string of uppercase and lowercase letters plus digits using a + * cryptographically secure random number generator. + * @param len The length of the string to generate + * @returns Random string of uppercase and lowercase letters plus digits of length `len` + */ +export function secureRandomString(len: number): string { + return secureRandomStringFrom(len, UPPERCASE + LOWERCASE + DIGITS); } -export function randomLowercaseString(len: number): string { - return randomStringFrom(len, LOWERCASE); -} - -export function randomUppercaseString(len: number): string { - return randomStringFrom(len, UPPERCASE); -} - -function randomStringFrom(len: number, chars: string): string { +/** + * Generate a cryptographically secure random string using characters given + * @param len The length of the string to generate + * @param chars The characters to use in the random string. + * @returns Random string of characters of length `len` + */ +export function secureRandomStringFrom(len: number, chars: string): string { + const positions = new Uint32Array(chars.length); let ret = ""; - - for (let i = 0; i < len; ++i) { - ret += chars.charAt(Math.floor(Math.random() * chars.length)); + crypto.getRandomValues(positions); + for (let i = 0; i < len; i++) { + const currentCharPlace = positions[i % chars.length] % chars.length; + ret += chars[currentCharPlace]; } - return ret; } diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts index 02fe6270a..f66402f56 100644 --- a/src/rust-crypto/rust-crypto.ts +++ b/src/rust-crypto/rust-crypto.ts @@ -79,7 +79,7 @@ import { EventType, MsgType } from "../@types/event.ts"; import { TypedEventEmitter } from "../models/typed-event-emitter.ts"; import { decryptionKeyMatchesKeyBackupInfo, RustBackupManager } from "./backup.ts"; import { TypedReEmitter } from "../ReEmitter.ts"; -import { randomString } from "../randomstring.ts"; +import { secureRandomString } from "../randomstring.ts"; import { ClientStoppedError } from "../errors.ts"; import { ISignatures } from "../@types/signed.ts"; import { decodeBase64, encodeBase64 } from "../base64.ts"; @@ -956,7 +956,7 @@ export class RustCrypto extends TypedEventEmitter { - const txId = randomString(32); + const txId = secureRandomString(32); // Send the verification request content to the DM room const { event_id: eventId } = await this.http.authedRequest<{ event_id: string }>( Method.Put, diff --git a/src/secret-storage.ts b/src/secret-storage.ts index 565e9ead2..4e20f3a7a 100644 --- a/src/secret-storage.ts +++ b/src/secret-storage.ts @@ -23,7 +23,7 @@ limitations under the License. import { TypedEventEmitter } from "./models/typed-event-emitter.ts"; import { ClientEvent, ClientEventHandlerMap } from "./client.ts"; import { MatrixEvent } from "./models/event.ts"; -import { randomString } from "./randomstring.ts"; +import { secureRandomString } from "./randomstring.ts"; import { logger } from "./logger.ts"; import encryptAESSecretStorageItem from "./utils/encryptAESSecretStorageItem.ts"; import decryptAESSecretStorageItem from "./utils/decryptAESSecretStorageItem.ts"; @@ -435,7 +435,7 @@ export class ServerSideSecretStorageImpl implements ServerSideSecretStorage { // Create a unique key id. XXX: this is racey. if (!keyId) { do { - keyId = randomString(32); + keyId = secureRandomString(32); } while (await this.accountDataAdapter.getAccountDataFromServer(`m.secret_storage.key.${keyId}`)); } diff --git a/src/webrtc/call.ts b/src/webrtc/call.ts index c1203e247..6984bada4 100644 --- a/src/webrtc/call.ts +++ b/src/webrtc/call.ts @@ -29,7 +29,7 @@ import { checkObjectHasKeys, isNullOrUndefined, recursivelyAssign } from "../uti import { MatrixEvent } from "../models/event.ts"; import { EventType, TimelineEvents, ToDeviceMessageId } from "../@types/event.ts"; import { RoomMember } from "../models/room-member.ts"; -import { randomString } from "../randomstring.ts"; +import { secureRandomString } from "../randomstring.ts"; import { MCallReplacesEvent, MCallAnswer, @@ -277,7 +277,7 @@ export class CallError extends Error { } export function genCallID(): string { - return Date.now().toString() + randomString(16); + return Date.now().toString() + secureRandomString(16); } function getCodecParamMods(isPtt: boolean): CodecParamsMod[] {