Merge pull request #4621 from matrix-org/dbkr/secure_random_string

Change randomString et al to be secure

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({

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"] }),

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 = {

spec/unit/randomstring.spec.ts

@@ -16,13 +16,18 @@ limitations under the License.
 
 import { decodeBase64 } from "../../src/base64";
 import {
-    randomLowercaseString,
+    secureRandomString,
-    randomString,
-    randomUppercaseString,
     secureRandomBase64Url,
+    secureRandomStringFrom,
+    LOWERCASE,
+    UPPERCASE,
 } from "../../src/randomstring";
 
 describe("Random strings", () => {
+    afterEach(() => {
+        jest.restoreAllMocks();
+    });
+
     it.each([8, 16, 32])("secureRandomBase64 generates %i valid base64 bytes", (n: number) => {
         const randb641 = secureRandomBase64Url(n);
         const randb642 = secureRandomBase64Url(n);
@@ -33,34 +38,77 @@ describe("Random strings", () => {
         expect(decoded).toHaveLength(n);
     });
 
-    it.each([8, 16, 32])("randomString generates string of %i characters", (n: number) => {
+    it.each([8, 16, 32])("secureRandomString generates string of %i characters", (n: number) => {
-        const rand1 = randomString(n);
+        const rand1 = secureRandomString(n);
-        const rand2 = randomString(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) => {
+    it.each([8, 16, 32])(
-        const rand1 = randomLowercaseString(n);
+        "secureRandomStringFrom generates lowercase string of %i characters when given lowercase",
-        const rand2 = randomLowercaseString(n);
+        (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])(
+        "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).toHaveLength(n);
+
+            expect(rand1.toUpperCase()).toEqual(rand1);
+        },
+    );
+
+    it("throws if given character set less than 2 characters", () => {
+        expect(() => secureRandomStringFrom(8, "a")).toThrow();
     });
 
-    it.each([8, 16, 32])("randomUppercaseString generates lowercase string of %i characters", (n: number) => {
+    it("throws if given character set more than 256 characters", () => {
-        const rand1 = randomUppercaseString(n);
+        const charSet = Array.from({ length: 257 }, (_, i) => "a").join("");
-        const rand2 = randomUppercaseString(n);
 
-        expect(rand1).not.toEqual(rand2);
+        expect(() => secureRandomStringFrom(8, charSet)).toThrow();
+    });
 
-        expect(rand1).toHaveLength(n);
+    it("throws if given length less than 1", () => {
+        expect(() => secureRandomStringFrom(0, "abc")).toThrow();
+    });
 
-        expect(rand1.toUpperCase()).toEqual(rand1);
+    it("throws if given length more than 32768", () => {
+        expect(() => secureRandomStringFrom(32769, "abc")).toThrow();
+    });
+
+    it("asks for more entropy if given entropy is unusable", () => {
+        // This is testing the internal implementation details of the function rather
+        // than strictly the public API. The intention is to have some assertion that
+        // the rejection sampling to make the distribution even over all possible characters
+        // is doing what it's supposed to do.
+
+        // mock once to fill with 255 the first time: 255 should be unusable because
+        // we give 10 possible characters below and 256 is not evenly divisible by 10, so
+        // this should force it to call for more entropy.
+        jest.spyOn(globalThis.crypto, "getRandomValues").mockImplementationOnce((arr) => {
+            if (arr === null) throw new Error("Buffer is null");
+            new Uint8Array(arr.buffer).fill(255);
+            return arr;
+        });
+
+        secureRandomStringFrom(8, "0123456789");
+        expect(globalThis.crypto.getRandomValues).toHaveBeenCalledTimes(2);
     });
 });

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 = {

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",

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);
     }
 
     /**

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);
 

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);
     }
 }
 

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),
+    state: secureRandomString(8),
-    nonce: randomString(8),
+    nonce: secureRandomString(8),
-    codeVerifier: randomString(64), // https://tools.ietf.org/html/rfc7636#section-4.1 length needs to be 43-128 characters
+    codeVerifier: secureRandomString(64), // https://tools.ietf.org/html/rfc7636#section-4.1 length needs to be 43-128 characters
 });
 
 /**

src/randomstring.ts

@@ -17,9 +17,23 @@ limitations under the License.
 
 import { encodeUnpaddedBase64Url } from "./base64.ts";
 
-const LOWERCASE = "abcdefghijklmnopqrstuvwxyz";
+/**
-const UPPERCASE = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
+ * String representing the lowercase latin alphabet for use in {@link secureRandomStringFrom}
-const DIGITS = "0123456789";
+ * (can be combined with other such exports or other characters by appending strings)
+ */
+export const LOWERCASE = "abcdefghijklmnopqrstuvwxyz";
+
+/**
+ * String representing the uppercase latin alphabet for use in secureRandomStringFrom
+ * (can be combined with other such exports or other characters by appending strings)
+ */
+export const UPPERCASE = "ABCDEFGHIJKLMNOPQRSTUVWXYZ";
+
+/**
+ * String representing the arabic numerals for use in secureRandomStringFrom
+ * (can be combined with other such exports or other characters by appending strings)
+ */
+export const DIGITS = "0123456789";
 
 export function secureRandomBase64Url(len: number): string {
     const key = new Uint8Array(len);
@@ -28,24 +42,62 @@ 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);
+ * Generate a cryptographically secure random string using characters given.
-}
+ *
-
+ * @param len - The length of the string to generate (must be positive and less than 32768).
-export function randomUppercaseString(len: number): string {
+ * @param chars - The characters to use in the random string (between 2 and 256 characters long).
-    return randomStringFrom(len, UPPERCASE);
+ * @returns Random string of characters of length `len`.
-}
+ */
-
+export function secureRandomStringFrom(len: number, chars: string): string {
-function randomStringFrom(len: number, chars: string): string {
+    // This is intended for latin strings so 256 possibilities should be more than enough and
-    let ret = "";
+    // means we can use random bytes, minimising the amount of entropy we need to ask for.
-
+    if (chars.length < 2 || chars.length > 256) {
-    for (let i = 0; i < len; ++i) {
+        throw new Error("Character set must be between 2 and 256 characters long");
-        ret += chars.charAt(Math.floor(Math.random() * chars.length));
     }
 
-    return ret;
+    if (len < 1 || len > 32768) {
+        throw new Error("Requested random string length must be between 1 and 32768");
+    }
+
+    // We'll generate random unsigned bytes, so get the largest number less than 256 that is a multiple
+    // of the length of the character set: We'll need to discard any random values that are larger than
+    // this as we can't possibly map them onto the character set while keeping each character equally
+    // likely to be chosen (minus 1 to convert to indices in a string). (Essentially, we're using a d8
+    // to choose between 7 possibilities and re-rolling on an 8, keeping all 7 outcomes equally likely.)
+    // Our random values must be strictly less than this
+    const randomValueCutoff = 256 - (256 % chars.length);
+
+    // Grab 30% more entropy than we need. This should be enough that we can discard the values that are
+    // too high without having to go back and grab more unless we're super unlucky.
+    const entropyBuffer = new Uint8Array(Math.floor(len * 1.3));
+    // Mark all of this buffer as used to start with (we haven't populated it with entropy yet) so it will
+    // be filled on the first iteration.
+    let entropyBufferPos = entropyBuffer.length;
+
+    const result = [];
+    while (result.length < len) {
+        if (entropyBufferPos === entropyBuffer.length) {
+            globalThis.crypto.getRandomValues(entropyBuffer);
+            entropyBufferPos = 0;
+        }
+
+        const randomByte = entropyBuffer[entropyBufferPos++];
+
+        if (randomByte < randomValueCutoff) {
+            result.push(chars[randomByte % chars.length]);
+        }
+    }
+
+    return result.join("");
 }

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,

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}`));
         }
 

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[] {