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,
-    randomString,
-    randomUppercaseString,
+    secureRandomString,
     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) => {
-        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);
+        },
+    );
+
+    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.toLowerCase()).toEqual(rand1);
+            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) => {
-        const rand1 = randomUppercaseString(n);
-        const rand2 = randomUppercaseString(n);
+    it("throws if given character set more than 256 characters", () => {
+        const charSet = Array.from({ length: 257 }, (_, i) => "a").join("");
 
-        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();
+    });
+
+    it("throws if given length more than 32768", () => {
+        expect(() => secureRandomStringFrom(32769, "abc")).toThrow();
+    });
 
-        expect(rand1.toUpperCase()).toEqual(rand1);
+    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),
-    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
 });
 
 /**