Skip to content

fix: generated passwords come from the platform CSPRNG - #870

Merged
blaipr merged 1 commit into
mainfrom
fix/generated-passwords-come-from-the-platform-csprng
Aug 23, 2026
Merged

fix: generated passwords come from the platform CSPRNG#870
blaipr merged 1 commit into
mainfrom
fix/generated-passwords-come-from-the-platform-csprng

Conversation

@blaipr

@blaipr blaipr commented Aug 23, 2026

Copy link
Copy Markdown
Member

The "Generate" button behind every password field — an account's password, a user's, the
sysPass master password, and the administrator account the installer creates — is
sysPass.Util.password.random(). It drew each character with

c.charAt(Math.floor(Math.random() * (c.length - 1)))

Math.random() is not a cryptographic generator. V8 seeds one xorshift128+ stream per
context, and its internal state is recoverable from a modest run of observed outputs, so
passwords generated in the same page session are predictable from one another. In a
password manager that is the wrong generator for the one value the whole product exists
to protect. It draws from crypto.getRandomValues now, with rejection sampling so the
modulo does not bias the alphabet.

Two more things in the same three lines:

* (c.length - 1) is exclusive at both ends of the multiplication, so the last character
of the assembled charset could never be produced. Which character that was depended on
which classes were enabled — the uppercase alphabet's 'Z' with the default settings.
Exercised over 200,000 draws, the replacement produces every index with a flat
distribution; the old one produced all but one.

The rejection loop — do e = f(); while (!m(e)); — regenerated the candidate until it
satisfied every enabled character class, with no bound. The length comes from an input
whose min is 1 and the default complexity requires four classes, so any length below
four made the condition unsatisfiable and froze the tab. It is bounded now.

The test is a source check, deliberately. These files are authored directly — there is no
unminified source and no build step — so it asserts against what actually ships, and the
alternative, asserting that the output looks random, is precisely the assertion that
passes for a broken generator. Math.random() is still used in the same file to mint DOM
element ids, which is a fine use for it, so the check is scoped to the generator.

Checked by putting the old expression back: two of the four fail.

The "Generate" button behind every password field — an account's password, a user's, the
sysPass master password, and the administrator account the installer creates — is
`sysPass.Util.password.random()`. It drew each character with

    c.charAt(Math.floor(Math.random() * (c.length - 1)))

`Math.random()` is not a cryptographic generator. V8 seeds one xorshift128+ stream per
context, and its internal state is recoverable from a modest run of observed outputs, so
passwords generated in the same page session are predictable from one another. In a
password manager that is the wrong generator for the one value the whole product exists
to protect. It draws from `crypto.getRandomValues` now, with rejection sampling so the
modulo does not bias the alphabet.

Two more things in the same three lines:

`* (c.length - 1)` is exclusive at both ends of the multiplication, so the last character
of the assembled charset could never be produced. Which character that was depended on
which classes were enabled — the uppercase alphabet's 'Z' with the default settings.
Exercised over 200,000 draws, the replacement produces every index with a flat
distribution; the old one produced all but one.

The rejection loop — `do e = f(); while (!m(e));` — regenerated the candidate until it
satisfied every enabled character class, with no bound. The length comes from an input
whose `min` is 1 and the default complexity requires four classes, so any length below
four made the condition unsatisfiable and froze the tab. It is bounded now.

The test is a source check, deliberately. These files are authored directly — there is no
unminified source and no build step — so it asserts against what actually ships, and the
alternative, asserting that the output looks random, is precisely the assertion that
passes for a broken generator. Math.random() is still used in the same file to mint DOM
element ids, which is a fine use for it, so the check is scoped to the generator.

Checked by putting the old expression back: two of the four fail.
@blaipr
blaipr merged commit cbf665c into main Aug 23, 2026
8 checks passed
@blaipr
blaipr deleted the fix/generated-passwords-come-from-the-platform-csprng branch August 23, 2026 23:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant