Change randomString et al to be secure #4621

dbkr · 2025-01-16T15:10:44Z

...and renames them, removing the special lowercase and uppercase versions and exporting the underlying function instead.

I don't believe any of these uses are speed-sensitive, so I don't really see much value in maintaining separate, fast, insecure versions. I've just changed everything to use the secure versions.

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. Either way, they deliberately have different names so apps will need to make a choice rather then just get moved to the secure versions.

The lowercase and uppercase versions 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.

Checklist

Tests written for new code (and old code if feasible).
New or updated public/exported symbols have accurate TSDoc documentation.
Linter and other CI checks pass.
Sign-off given on the changes (see CONTRIBUTING.md).

...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.

Because the js-sdk methods are changing and there's no reason for these not to use the secure versions. The dedicated upper/lower functions were *only* used in this one case, so this should do the exact same thing with the one exported function. Requires matrix-org/matrix-js-sdk#4621 (merge both together)

hughns

The MatrixRTC and WebRTC changes look good to me.

richvdh

Any apps that use these

Are these functions supposed to be exported to apps? They don't appear to be exported via the public entry points to the library and in consequence aren't documented. I'd go so far as to say this is not an X-Breaking-Change: apps using internal functions within the js-sdk don't get to rely on the semver telling them if those internal functions are changing.

If you want them to be exported, I'd suggest that this would be a good time to fix that.

dbkr · 2025-01-17T13:07:24Z

Mmm, yeah - we had been assuming that it was, but no, they should not really be exported as far as I'm concerned: I don't believe it's in the js-sdk remit to provide random string utilities (obviously element web is a special case as ever).

src/randomstring.ts

richvdh · 2025-01-17T14:24:59Z

src/randomstring.ts

-
-    for (let i = 0; i < len; ++i) {
-        ret += chars.charAt(Math.floor(Math.random() * chars.length));
+    crypto.getRandomValues(positions);


at line 26, we use globalThis.crypto. It would be good to remember why we use globalThis there, and to be consistent one way or the other.

richvdh · 2025-01-17T14:49:26Z

src/randomstring.ts

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


I'm far from convinced this algorithm is correct; in particular afaict you're generating a random number for each character in the input set, rather than the desired length of the output string.

Please add some comments.

Well spotted. If we do find a bug I unit test reproducing it would be awesome. You might be able to mock crypto to get predictable results?

After some back & forth with crypto, I've updated the algorithm. The conclusion seemed to be to prioritise strict correctness so hopefully this is an acceptably comprehensible version. I have made the unit test assert a little about the behaviour but haven't gone as far as asserting specific outputs for specific random values which would really be nailing down the exact algorithm. However we could do this assert that it picks all values exactly equal number of times (for certain specific inputs).

andybalaam

No comments beyond what Rich said, but please do address the question about whether it works the way we expect it to.

andybalaam · 2025-01-20T12:15:57Z

src/randomstring.ts

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


Well spotted. If we do find a bug I unit test reproducing it would be awesome. You might be able to mock crypto to get predictable results?

dkasak · 2025-01-20T20:45:02Z

src/randomstring.ts

+    // 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;


Suggested change

const maxRandValue = Math.floor(255 / chars.length) * chars.length - 1;

const maxRandValue = 255 - 255 % chars.length - 1;

We can avoid Math.floor using modular arithmetic.

Fixed, and adjusted to be 256 which is what it surely should have been, and don't subtract 1 because we're already using < (rather than <=) below.

Yes. I started writing an elaborate explanation of why the bounds were wrong, but you beat me to it by grokking it yourself beforehand. :)

Also, the reason we don't need the - 1 is because taking the value modulo chars.length ensures that the largest possible resulting index is chars.length - 1.

also I think this was just wrong in that it was subtracting 1 unnercessarily because we already used < rather than <= below.

richvdh

LGTM other than nitpicking about documentation

src/randomstring.ts

Co-authored-by: Richard van der Hoff <[email protected]>

* Switch to secure random strings Because the js-sdk methods are changing and there's no reason for these not to use the secure versions. The dedicated upper/lower functions were *only* used in this one case, so this should do the exact same thing with the one exported function. Requires matrix-org/matrix-js-sdk#4621 (merge both together) * Change remaining instances of randomString which I somehow entirely missed the first time. * Fix import order

dbkr · 2025-01-21T13:54:45Z

Force merging because it won't pass element web tests without the corresponding web PR

* Switch to secure random strings Because the js-sdk methods are changing and there's no reason for these not to use the secure versions. The dedicated upper/lower functions were *only* used in this one case, so this should do the exact same thing with the one exported function. Requires matrix-org/matrix-js-sdk#4621 (merge both together) * Change remaining instances of randomString which I somehow entirely missed the first time. * Fix import order

* Switch to secure random strings Because the js-sdk methods are changing and there's no reason for these not to use the secure versions. The dedicated upper/lower functions were *only* used in this one case, so this should do the exact same thing with the one exported function. Requires matrix-org/matrix-js-sdk#4621 (merge both together) * Change remaining instances of randomString which I somehow entirely missed the first time. * Fix import order (cherry picked from commit 56eafc9)

* Switch to secure random strings Because the js-sdk methods are changing and there's no reason for these not to use the secure versions. The dedicated upper/lower functions were *only* used in this one case, so this should do the exact same thing with the one exported function. Requires matrix-org/matrix-js-sdk#4621 (merge both together) * Change remaining instances of randomString which I somehow entirely missed the first time. * Fix import order (cherry picked from commit 56eafc9) Co-authored-by: David Baker <[email protected]>

dbkr added T-Defect X-Breaking-Change labels Jan 16, 2025

github-actions bot deployed to PR Documentation Preview January 16, 2025 15:11 View deployment

dbkr mentioned this pull request Jan 16, 2025

Switch to secure random strings element-hq/element-web#29013

Merged

4 tasks

dbkr added T-Defect and removed T-Defect labels Jan 16, 2025

dbkr marked this pull request as ready for review January 16, 2025 17:15

dbkr requested review from a team as code owners January 16, 2025 17:15

dbkr requested review from andybalaam, uhoreg, toger5 and florianduros January 16, 2025 17:15

florianduros approved these changes Jan 17, 2025

View reviewed changes

hughns approved these changes Jan 17, 2025

View reviewed changes

richvdh reviewed Jan 17, 2025

View reviewed changes

dbkr removed the X-Breaking-Change label Jan 17, 2025

richvdh requested changes Jan 17, 2025

View reviewed changes

andybalaam approved these changes Jan 20, 2025

View reviewed changes

Use a random impl with rejection sampling

35fe7bc

github-actions bot deployed to PR Documentation Preview January 20, 2025 18:15 View deployment

dbkr requested a review from richvdh January 20, 2025 18:24

Document exports

1f1d6f0

github-actions bot deployed to PR Documentation Preview January 20, 2025 18:28 View deployment

dkasak reviewed Jan 20, 2025

View reviewed changes

Use modulo arithmetic instead

f13967a

also I think this was just wrong in that it was subtracting 1 unnercessarily because we already used < rather than <= below.

github-actions bot deployed to PR Documentation Preview January 21, 2025 09:53 View deployment

richvdh approved these changes Jan 21, 2025

View reviewed changes

src/randomstring.ts Outdated Show resolved Hide resolved

src/randomstring.ts Outdated Show resolved Hide resolved

Add link

8bffa39

Co-authored-by: Richard van der Hoff <[email protected]>

github-actions bot deployed to PR Documentation Preview January 21, 2025 13:30 View deployment

Fix tsdoc formatting & consistency

ea67d39

Co-authored-by: Richard van der Hoff <[email protected]>

github-actions bot deployed to PR Documentation Preview January 21, 2025 13:33 View deployment

dbkr added this pull request to the merge queue Jan 21, 2025

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 21, 2025

dbkr merged commit 424c258 into develop Jan 21, 2025
30 checks passed

dbkr deleted the dbkr/secure_random_string branch January 21, 2025 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change randomString et al to be secure #4621

Change randomString et al to be secure #4621

dbkr commented Jan 16, 2025

hughns left a comment

richvdh left a comment

dbkr commented Jan 17, 2025

richvdh Jan 17, 2025

richvdh Jan 17, 2025

andybalaam Jan 20, 2025

dbkr Jan 20, 2025

andybalaam left a comment

andybalaam Jan 20, 2025

dkasak Jan 20, 2025

dbkr Jan 21, 2025

dkasak Jan 21, 2025 •

edited

Loading

richvdh left a comment

dbkr commented Jan 21, 2025

	const maxRandValue = Math.floor(255 / chars.length) * chars.length - 1;
	const maxRandValue = 255 - 255 % chars.length - 1;

Change randomString et al to be secure #4621

Change randomString et al to be secure #4621

Conversation

dbkr commented Jan 16, 2025

Checklist

hughns left a comment

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

dbkr commented Jan 17, 2025

richvdh Jan 17, 2025

Choose a reason for hiding this comment

richvdh Jan 17, 2025

Choose a reason for hiding this comment

andybalaam Jan 20, 2025

Choose a reason for hiding this comment

dbkr Jan 20, 2025

Choose a reason for hiding this comment

andybalaam left a comment

Choose a reason for hiding this comment

andybalaam Jan 20, 2025

Choose a reason for hiding this comment

dkasak Jan 20, 2025

Choose a reason for hiding this comment

dbkr Jan 21, 2025

Choose a reason for hiding this comment

dkasak Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

richvdh left a comment

Choose a reason for hiding this comment

dbkr commented Jan 21, 2025

dkasak Jan 21, 2025 •

edited

Loading