fix: verify passwords server-side instead of running argon2 in the browser#3468
Conversation
…owser The UI ran a synchronous WASM Argon2id verify on the main thread when creating a backup, restoring a backup, and changing the master password. At the parameters StartOS hashes with (rfc9106_low_mem: Argon2id, 64 MiB, t=3) that blocks the tab for seconds on desktop and 20-25s on mobile, where it reads as a hung browser. Creating a backup onto a target whose existing backup used a different password ran it three times over. The server already performed the identical argon2 verification on every one of these calls -- backup_all's check_password, BackupMountGuard::mount's load_metadata, and reset_password_impl -- natively, in milliseconds. All three flows now rely on it, and @start9labs/argon2 is dropped from the web entirely. backup_all's two password checks were indistinguishable to the client: both came back as IncorrectPassword. Add ErrorKind::BackupPasswordMismatch for the mount failure so the UI can still tell "wrong master password" apart from "this target's existing backup used a different password" and prompt for the original. Also drop tuiAutoFocus from every site, and stop the shared prompt dialog from submitting as a form -- it now sets autocapitalize="off" and submits via its own buttons, so mobile keyboards stop capitalizing passwords and browser password managers stop offering to save a backup password as a saved credential. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dr-bonez
left a comment
There was a problem hiding this comment.
Ideally we shouldn't return the hash to the frontend in the first place if we're doing this
…tend Now that the browser no longer verifies passwords, the two hashes we shipped it are pure attack surface. `serverInfo.passwordHash` was a copy of the master password's argon2 hash living in the *public* database, so it replicated to every authenticated client. It existed solely for the client-side verify. Drop the field; the real hash stays in the private database. No migration needed -- `Current::pre_init` round-trips the db through the typed `Database` model on every boot (both the Equal and Less branches), and with no `deny_unknown_fields` that drops the removed key from existing databases on the next start. Worse, `StartOsRecoveryInfo` -- returned by `getBackupTargets`, `setup.cifs.verify` and the disk listing -- carried each backup's `passwordHash` *and* its `wrappedKey`: the backup's encryption key sealed under that password. A client holding both can crack the hash offline and then unwrap the actual backup key. It couldn't simply skip_serializing them: the same struct is written back to the backup drive as unencrypted-metadata.json, so that would blank the hash and wrapped key on disk and make the backup undecryptable. Split it -- on-disk `BackupUnencryptedMetadata` keeps the key material and never leaves the server; the ts-exported `StartOsRecoveryInfo` is now just hostname/version/timestamp, and `recovery_info` maps between them. Also drop the dead `passwordHash` write from the v0_3_6_alpha_0 migration, so the hash never lands in the public db even transiently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Agreed — done in de0c3b0. And it was worse than just the hash:
Two things that weren't obvious going in:
No migration needed. One thing to flag: I couldn't compile Rust locally on this machine, so the |
ts-rs propagates Rust `///` doc comments into the generated TypeScript as a TSDoc block (see CheckPortV6Res / RangeBindInfo). The doc comment added to StartOsRecoveryInfo therefore emits four lines the hand-edited binding was missing, which is exactly the drift ts-bindings-check reported. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`check:tunnel` was defined but missing from the aggregate script, so the StartTunnel web app was never type-checked by `npm run check` (only its i18n was). It passes clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three things that each cost a CI round trip on this branch: - osBindings/index.ts is built from a shell glob, so its order follows LC_COLLATE. CI runs under C; regenerating under a UTF-8 locale reorders all 336 export lines and fails the Generated Artifacts gate on pure churn. - ts-rs propagates a Rust `///` doc comment into the generated .ts as a TSDoc block, so adding one to a #[ts(export)] type is not a comment-only change. - Bindings can only come from compiling the crate (the exporters are #[test] fns the derive macro generates inside it), and that build is opt-level=3 by way of [profile.test] -- so cap it rather than hunting for a lighter command. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| ### Changed | ||
|
|
||
| - **Dialogs no longer steal focus when they open.** The Add Device, Add Subnet, | ||
| and Change Password dialogs no longer autofocus their first field, which on |
|
|
||
| ### Changed | ||
|
|
||
| - **The inbound-rule dialog no longer steals focus when it opens**, which on |
Summary
On mobile, entering the master password when creating a backup froze the whole browser for 20–25 seconds — long enough to read as a device crash. Same on Change Password. It was not
tuiAutoFocus; it was argon2.The UI called
verify()from@start9labs/argon2— a synchronous WASM Argon2id hash on the main thread — in three places: backup create, backup restore, and change password. StartOS hashes the master password withargon2::Config::rfc9106_low_mem()= Argon2id, 64 MiB, t=3, p=1, so that's ~192 MiB of memory-hard mixing, single-threaded, with the main thread blocked throughout. It can't be made fast client-side: the shipped WASM is already an optimized 45 KB release build,p=1rules out threading, and SIMD would only buy 2–4×.Worse, creating a backup onto a target whose existing backup used a different password ran it three times (master hash → target hash → original password), and Change Password ran it even when a cheaper validation had already failed — so mistyping the confirmation field cost you 20–25s before being told the passwords don't match. None of this reproduces against the mock server, whose fixture hash is
m=1024,t=1.The server already did the identical check on every one of these calls, natively, in milliseconds:
backup_all→check_password, before the backup task spawns (backup_bulk.rs)BackupMountGuard::mount→load_metadata→check_password(disk/mount/backup.rs)getBackupInfo(backup/target/mod.rs)reset_password_impl→argon2::verify_encoded(auth.rs)So all four client-side verifies are deleted and
@start9labs/argon2is dropped from the web entirely (dependency, wasm asset, and theinitArgon()bootstrap — which the setup-wizard ran on every boot despite never callingverify()).The one thing that needed a backend change
backup_allruns two password checks and both returnedErrorKind::IncorrectPassword, so the client couldn't tell "you typed your master password wrong" from "this target's existing backup was encrypted with a different password — ask for the original". AddedErrorKind::BackupPasswordMismatch(81) for the mount failure, which reaches the client as the RPC error code.ErrorKindhas noTSderive, so no binding regeneration was needed.Prompt semantics are preserved exactly: a password the server rejects leaves the prompt open to retry (previously
verifyPassword'sEMPTY+take(1); nowTaskService.run'sPromise<boolean>+filter(Boolean)+take(1)— notePromptModal.submit()callsnext(), notcomplete(), so thetake(1)is what closes the dialog).Also in this PR
tuiAutoFocusremoved from every site — the shared prompt dialog, the refresh alert, the OS-update dialog, the marketplace package drawer, the setup wizard's password page, three StartTunnel dialogs, and StartWRT's inbound-rule dialog. On mobile it raised the keyboard the instant a sheet opened, over the dialog's own buttons.autocapitalize="off"(mobile keyboards were capitalizing the first character of the backup password) and submits via its own buttons / Enter rather than a<form>submit, so browser password managers stop offering to save a backup encryption password as a saved credential. The setup wizard's Unlock Backup prompt gets the sameautocapitalizetreatment.Notes
portal/routes/backups/tree (@TODO 041, scheduled backup jobs) is kept — only its argon2 call is stripped, so un-parking it in 0.4.1 doesn't silently reintroduce the freeze.npm run check(all projects) +check:tunnel, and successful builds of ui, setup-wizard, tunnel, wrt, and brochure. Rust is left to CI.check:tunnelis defined inpackage.jsonbut missing from the aggregatecheckscript, sonpm run checknever typechecks the StartTunnel web app. It passes clean today; not fixed here.🤖 Generated with Claude Code