From 86494c3a96d144b5ca2c154b396c9e4807fb2f05 Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 16 Jan 2025 14:48:36 +0000 Subject: [PATCH 1/6] 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 0d4177f557a..e867788ff35 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 7e50d952230..801c2f145fc 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 b5aa7096017..3b35a2faba2 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 526edfacfcd..8dec2c6c06d 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 9caaa74580e..e3c736429e1 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 eba63d86e8e..8bab97bd870 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 0e49b16091c..22647bf4290 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 34bf6f51ab5..97e3c313ae2 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 7e3692dfb94..d8b6c91a136 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 d0cc9e675d2..b448a025d3a 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 02fe6270a17..f66402f5602 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 565e9ead295..4e20f3a7aee 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 c1203e24707..6984bada4af 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[] { From 35fe7bc60a959ab6c1eda9454db81324c09efde5 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 20 Jan 2025 18:13:26 +0000 Subject: [PATCH 2/6] Use a random impl with rejection sampling --- spec/unit/randomstring.spec.ts | 41 ++++++++++++++++++++++++++++ src/randomstring.ts | 49 +++++++++++++++++++++++++++------- 2 files changed, 81 insertions(+), 9 deletions(-) diff --git a/spec/unit/randomstring.spec.ts b/spec/unit/randomstring.spec.ts index 8dec2c6c06d..2413ca9588a 100644 --- a/spec/unit/randomstring.spec.ts +++ b/spec/unit/randomstring.spec.ts @@ -24,6 +24,10 @@ import { } 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); @@ -70,4 +74,41 @@ describe("Random strings", () => { expect(rand1.toUpperCase()).toEqual(rand1); }, ); + + it("throws if given character set less than 2 characters", () => { + expect(() => secureRandomStringFrom(8, "a")).toThrow(); + }); + + it("throws if given character set more than 256 characters", () => { + const charSet = Array.from({ length: 257 }, (_, i) => "a").join(""); + + expect(() => secureRandomStringFrom(8, charSet)).toThrow(); + }); + + 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(); + }); + + 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); + }); }); diff --git a/src/randomstring.ts b/src/randomstring.ts index b448a025d3a..c379c630974 100644 --- a/src/randomstring.ts +++ b/src/randomstring.ts @@ -40,17 +40,48 @@ export function secureRandomString(len: number): 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. + * @param len The length of the string to generate (must be positive and less than 32768) + * @param chars The characters to use in the random string (between 2 and 256 characters long). * @returns Random string of characters of length `len` */ export function secureRandomStringFrom(len: number, chars: string): string { - const positions = new Uint32Array(chars.length); - let ret = ""; - crypto.getRandomValues(positions); - for (let i = 0; i < len; i++) { - const currentCharPlace = positions[i % chars.length] % chars.length; - ret += chars[currentCharPlace]; + // This is intended for latin strings so 256 possibilities should be more than enough and + // means we can use random bytes, minimising the amount of entropy we need to ask for. + if (chars.length < 2 || chars.length > 256) { + throw new Error("Character set must be between 2 and 256 characters long"); + } + + 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.) + const maxRandValue = Math.floor(255 / chars.length) * chars.length - 1; + + // 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 < maxRandValue) { + result.push(chars[randomByte % chars.length]); + } } - return ret; + + return result.join(""); } From 1f1d6f0bc8169b509e7fd07bea421559f8e84639 Mon Sep 17 00:00:00 2001 From: David Baker Date: Mon, 20 Jan 2025 18:27:40 +0000 Subject: [PATCH 3/6] Document exports --- src/randomstring.ts | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/randomstring.ts b/src/randomstring.ts index c379c630974..f38af24eb2f 100644 --- a/src/randomstring.ts +++ b/src/randomstring.ts @@ -17,8 +17,22 @@ limitations under the License. import { encodeUnpaddedBase64Url } from "./base64.ts"; +/** + * String representing the lowercase latin alphabet for use in secureRandomStringFrom + * (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 { From f13967aaecdf47197b610b415ded10f36c17fcb8 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 21 Jan 2025 09:50:55 +0000 Subject: [PATCH 4/6] Use modulo arithmetic instead also I think this was just wrong in that it was subtracting 1 unnercessarily because we already used < rather than <= below. --- src/randomstring.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/randomstring.ts b/src/randomstring.ts index f38af24eb2f..5701a0a1f32 100644 --- a/src/randomstring.ts +++ b/src/randomstring.ts @@ -74,7 +74,8 @@ export function secureRandomStringFrom(len: number, chars: string): string { // 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.) - const maxRandValue = Math.floor(255 / chars.length) * chars.length - 1; + // 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. @@ -92,7 +93,7 @@ export function secureRandomStringFrom(len: number, chars: string): string { const randomByte = entropyBuffer[entropyBufferPos++]; - if (randomByte < maxRandValue) { + if (randomByte < randomValueCutoff) { result.push(chars[randomByte % chars.length]); } } From 8bffa39eb9aac2bbb4010b20968f334b55e6257b Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 21 Jan 2025 13:29:44 +0000 Subject: [PATCH 5/6] Add link Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/randomstring.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/randomstring.ts b/src/randomstring.ts index 5701a0a1f32..71988059839 100644 --- a/src/randomstring.ts +++ b/src/randomstring.ts @@ -18,7 +18,7 @@ limitations under the License. import { encodeUnpaddedBase64Url } from "./base64.ts"; /** - * String representing the lowercase latin alphabet for use in secureRandomStringFrom + * String representing the lowercase latin alphabet for use in {@link secureRandomStringFrom} * (can be combined with other such exports or other characters by appending strings) */ export const LOWERCASE = "abcdefghijklmnopqrstuvwxyz"; From ea67d3977e5df62e3333876ec39026a9a6f77024 Mon Sep 17 00:00:00 2001 From: David Baker Date: Tue, 21 Jan 2025 13:32:28 +0000 Subject: [PATCH 6/6] Fix tsdoc formatting & consistency Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> --- src/randomstring.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/randomstring.ts b/src/randomstring.ts index 71988059839..7d71512c6a1 100644 --- a/src/randomstring.ts +++ b/src/randomstring.ts @@ -53,10 +53,11 @@ export function secureRandomString(len: number): string { } /** - * 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) - * @param chars The characters to use in the random string (between 2 and 256 characters long). - * @returns Random string of characters of length `len` + * 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). + * @param chars - The characters to use in the random string (between 2 and 256 characters long). + * @returns Random string of characters of length `len`. */ export function secureRandomStringFrom(len: number, chars: string): string { // This is intended for latin strings so 256 possibilities should be more than enough and