Change randomString et al to be secure

dbkr · dbkr · commit 86494c3a96d1 · 2025-01-16T14:48:36.000Z
...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.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<IEvent> {
     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
@@ -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<EmittedEvents, ClientEventHa
         this.usingExternalCrypto = opts.usingExternalCrypto ?? false;
         this.store = opts.store || new StubStore();
         this.deviceId = opts.deviceId || null;
-        this.sessionId = randomString(10);
+        this.sessionId = secureRandomString(10);
 
         const userId = opts.userId || null;
         this.credentials = { userId };
@@ -7998,7 +7998,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
      * @returns A new client secret
      */
     public generateClientSecret(): string {
-        return randomString(32);
+        return secureRandomString(32);
     }
 
     /**
diff --git a/src/crypto/key_passphrase.ts b/src/crypto/key_passphrase.ts
@@ -14,7 +14,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 { deriveRecoveryKeyFromPassphrase } from "../crypto-api/index.ts";
 
 const DEFAULT_ITERATIONS = 500000;
@@ -30,7 +30,7 @@ interface IKey {
  * @param passphrase - The passphrase to generate the key from
  */
 export async function keyFromPassphrase(passphrase: string): Promise<IKey> {
-    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
@@ -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
@@ -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<string> => {
 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
@@ -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
@@ -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<RustCryptoEvents, CryptoEventH
         if (password) {
             // Generate the key from the passphrase
             // first we generate a random salt
-            const salt = randomString(32);
+            const salt = secureRandomString(32);
             // then we derive the key from the passphrase
             const recoveryKey = await deriveRecoveryKeyFromPassphrase(
                 password,
@@ -1099,7 +1099,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, CryptoEventH
      * @returns the event id
      */
     private async sendVerificationRequestContent(roomId: string, verificationEventContent: string): Promise<string> {
-        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
@@ -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
@@ -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[] {
