fix(security): salt password hashes with scrypt (CWE-328)

[sebastiondev]

Summary

Fredy currently stores user passwords as unsalted SHA-256 digests (see lib/services/security/hash.js on master, which is just crypto.createHash('sha256').update(password).digest('hex')). This is CWE-328 (Use of Weak Hash) combined with CWE-916 (Password Hash With Insufficient Computational Effort) and CWE-759 (Missing Salt).

Any time the users table leaks — for example via the admin backup download, a filesystem disclosure, a stolen volume snapshot, or a misconfigured Docker mount — the stored hashes can be reversed in seconds with a precomputed rainbow table, or brute-forced trivially on commodity GPUs (SHA-256 sustains billions of guesses per second). Because self-hosted users often reuse passwords, the practical impact of a users-table leak is credential theft that extends well beyond Fredy itself.

Fix

lib/services/security/hash.js is rewritten to:

• Hash new passwords with scrypt (N=32768, r=8, p=1, 16-byte random salt, 64-byte key), serialized as scrypt$N$r$p$saltHex$hashHex so parameters can be evolved later without a schema change.
• Expose verify(password, stored) with constant-time comparison (crypto.timingSafeEqual).
• Keep a legacy branch in verify() that still accepts existing 64-char hex SHA-256 hashes, so no user is locked out on upgrade.
• Add needsRehash(stored) — returns true for legacy hashes or weaker scrypt parameters.
• In loginRoute, on a successful login against a legacy hash, transparently re-hash with the new scheme and persist. No forced password reset, no admin action required — existing installs migrate silently as users log in.

hash() and verify() are now async; every call site (index.js, generalSettingsRoute, loginRoute, userRoute, userStorage.upsertUser / ensureAdminUserExists / ensureDemoUserExists) is updated to await them.

Tests

test/services/hash.test.js adds coverage for:

• salt uniqueness (two hashes of the same password differ, and neither is a bare hex digest);
• verify() accepts the correct password and rejects a wrong one;
• legacy unsalted SHA-256 hashes still verify (backwards compatibility);
• needsRehash() returns true for legacy and false for freshly-produced hashes;
• malformed / truncated hash strings are rejected without throwing.

yarn test passes locally; lint is clean.

Security analysis

Threat model: an attacker who obtains a copy of the users table (e.g. through the existing admin backup download endpoint, a leaked SQLite file, or a compromised backup). Before this fix, that read-only exposure is sufficient to recover plaintext passwords in minutes. After this fix, scrypt with a per-user salt raises the offline attack cost from "seconds on a laptop" to "computationally infeasible for any reasonable password", which is a genuine capability reduction beyond what the leak precondition alone provides. Timing-safe comparison also removes a theoretical side-channel on the verify path.

Adversarial review

Before submitting, we tried to disprove this finding. The main counter-arguments we considered were: (1) "the users table is only readable by admins, so leakage is already game-over" — but a leaked backup, stolen disk image, or accidental repo commit are all realistic exposure paths that don't require live admin access, and the shared-password blast radius extends beyond Fredy. (2) "Node/Fastify or the DB layer already provides password protection" — it doesn't; the hashing is entirely in lib/services/security/hash.js and was custom SHA-256. (3) "This is just a hardening nit" — CWE-328 with no salt is not a hardening nit; it converts a database read into plaintext credentials. The fix is minimal, backwards compatible, and covered by tests.

Proof of concept

Reproduce on master before the fix:

// node --input-type=module
import crypto from 'crypto';
// This is exactly what lib/services/security/hash.js does on master:
const stored = crypto.createHash('sha256').update('admin').digest('hex');
console.log(stored);
// -> 8c6976e5b5410415bde908bd4dee15dfb167a9c873fc4bb8a81f6f2ab448a918
// Google that hex string; the plaintext "admin" is the first hit.
// Same story for any users table exported via the admin backup: every
// row's `password` column is a rainbow-table lookup away from plaintext.

After this PR, the stored value looks like scrypt$32768$8$1$<32 hex salt>$<128 hex hash>, is unique per user even when passwords collide, and is not present in any rainbow table.

To confirm the migration path end-to-end: start a pre-fix Fredy, create a user, upgrade to this branch, log in with the same password — login succeeds and the stored hash is rewritten to the scrypt$… form on that request (observable via a quick SELECT password FROM users before and after).

---

_{Discovered by the Sebastion AI GitHub App.}

[orangecoding]

Some checks are failing. please check

[sebastiondev]

Thanks for the ping. The failing check was yarn format:check — lib/services/storage/userStorage.js wasn't Prettier-formatted. Fixed in 9be16eb (ran prettier --write on the file; no logic change). The test job was already green. CI should pass on the next run.