feat(security): decouple the login verifier from the AES vault key
The zero-knowledge verifier sent to /login used to be the raw PBKDF2 output in hex — i.e. the exact bytes of the AES key that encrypts every entry. Intercepting a /login body (loopback, but still) handed over the vault key. This introduces a decoupled scheme where the transmitted verifier is a one-way function of the key. New auth-hash scheme - users.hash_algo 'pbkdf2-sha256-v2': the client sends verifier = SHA256(keyHex + "pmserver/auth-verifier/v2") instead of keyHex. Stored form is still SHA256(verifier) (identical server wrap to 'pbkdf2-sha256'), so only the algo LABEL differs — it tells the client which verifier formula to use. Verification needs no new server branch (VerifierToStoredHash already SHA256-wraps any non-legacy verifier). - The AES key (cryptoKey) stays hex(PBKDF2) for EVERY algo, so entries remain decryptable and switching schemes never re-encrypts data. Adoption: new-registration + master-pw-change only - Register and change-master-password write v2. Existing accounts keep their algo until they rotate — the login/reauth migration signal now fires only for LEGACY 'pbkdf2' (was: anything != CURRENT), so sha256/v2 accounts are never force-migrated (which would have downgraded v2 → sha256 via migrate-kdf). Client (js/app.js): algo-aware everywhere - verifierFromKeyHex(keyHex, algo) central helper; deriveKeyAndVerifier / computeVerifier take an algo arg. state.hashAlgo caches the account scheme, set from /login/challenge, register, change-master, the quick-unlock / PIN cold-start blobs, and the /recovery-key/redeem response. All ~12 verifier sites updated (login, register, reauth ×4, change-master current+new, migrate-kdf, quick-unlock + PIN cold-start, recovery-mode current verifier). Safety invariant: unknown/empty hashAlgo → key hex → byte-identical to the old behaviour, so every pre-decoupling account (and every existing quick-unlock / PIN blob without the new field) keeps working unchanged. Verified: existing account + pre-change quick-unlock still unlocks; a master-pw change now writes 'pbkdf2-sha256-v2' in vault.db. Server: recovery redeem returns hashAlgo; register + change-master store the decoupled algo; login + reauth migration signal narrowed to legacy. Also: BuildAssets.ps1 pipes $null into node --check so the JS syntax gate can't block on stdin in the Delphi pre-build environment. Addresses CODE_AUDIT.md section 1.1. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This commit is contained in:
r-zakarya
@@ -47,6 +47,15 @@ const
|
||||
// during /login to compute the comparison.
|
||||
HASH_ALGO_LEGACY = 'pbkdf2';
|
||||
HASH_ALGO_CURRENT = 'pbkdf2-sha256';
|
||||
// 'pbkdf2-sha256-v2' : DECOUPLED. Same stored form as CURRENT
|
||||
// (SHA256 of the client verifier), but the client's transmitted
|
||||
// verifier is now SHA256(keyHex + domain) instead of keyHex — so the
|
||||
// /login body no longer carries the raw AES vault key. Used by new
|
||||
// registrations and by every master-pw change. Existing accounts stay
|
||||
// on their current algo until they rotate (no forced migration).
|
||||
// Verification is identical to CURRENT (VerifierToStoredHash wraps any
|
||||
// non-legacy verifier in SHA256), so no new verify branch is needed.
|
||||
HASH_ALGO_DECOUPLED = 'pbkdf2-sha256-v2';
|
||||
|
||||
DEFAULT_FOLDERS: array[0..4] of string = ('All', 'Social', 'Banking', 'Work', 'Personal');
|
||||
|
||||
@@ -235,10 +244,16 @@ begin
|
||||
LQ.Free;
|
||||
end;
|
||||
|
||||
// New ZK registrations land on the DECOUPLED scheme; the plaintext
|
||||
// fallback (legacy clients) stays on CURRENT. VerifierToStoredHash
|
||||
// wraps both the same way (SHA256), so only the stored algo LABEL
|
||||
// differs — it's what tells the client which verifier formula to use.
|
||||
var LRegAlgo := HASH_ALGO_CURRENT;
|
||||
if LVerifier <> '' then
|
||||
begin
|
||||
// ZK path: use the client-supplied salt + iters + verifier as-is.
|
||||
LHash := VerifierToStoredHash(LVerifier, HASH_ALGO_CURRENT);
|
||||
LRegAlgo := HASH_ALGO_DECOUPLED;
|
||||
LHash := VerifierToStoredHash(LVerifier, LRegAlgo);
|
||||
end
|
||||
else
|
||||
begin
|
||||
@@ -253,7 +268,8 @@ begin
|
||||
LQ.Connection := DB.Connection;
|
||||
LQ.SQL.Text :=
|
||||
'INSERT INTO users (username, password_hash, salt, hash_algo, kdf_iterations) ' +
|
||||
'VALUES (:u, :h, :s, ''' + HASH_ALGO_CURRENT + ''', :it)';
|
||||
'VALUES (:u, :h, :s, :algo, :it)';
|
||||
LQ.ParamByName('algo').AsString := LRegAlgo;
|
||||
LQ.ParamByName('u').AsString := LUser;
|
||||
LQ.ParamByName('h').AsString := LHash;
|
||||
LQ.ParamByName('s').AsString := LSalt;
|
||||
@@ -396,12 +412,15 @@ begin
|
||||
LogAudit(LUserId, 'login', LIP);
|
||||
// Signal migration whenever EITHER:
|
||||
// - the user's iteration count is below the target (KDF bump needed), OR
|
||||
// - the user's hash_algo is not the current scheme (format upgrade needed
|
||||
// to remove the AES-key-in-vault.db architectural flaw).
|
||||
// The client then calls /migrate-kdf which fixes both in one atomic step.
|
||||
// - the user is on the LEGACY 'pbkdf2' scheme (stored hash = raw key hex;
|
||||
// upgrade to SHA256-wrapped to remove the AES-key-in-vault.db flaw).
|
||||
// NOTE: we deliberately do NOT signal for 'pbkdf2-sha256' or the newer
|
||||
// 'pbkdf2-sha256-v2' (decoupled) — those are already SHA256-wrapped at
|
||||
// rest, and forcing sha256 → v2 is out of scope (v2 is adopted only on
|
||||
// register / master-pw change, never force-migrated at login).
|
||||
SendAuthSuccess(AResponse, LUserId, LToken, LSalt, LCSRF, LKdfIters,
|
||||
(LKdfIters < PBKDF2_ITERATIONS_TARGET) or
|
||||
not SameText(LAlgo, HASH_ALGO_CURRENT));
|
||||
SameText(LAlgo, HASH_ALGO_LEGACY));
|
||||
end;
|
||||
|
||||
// ===== /logout ===============================================================
|
||||
@@ -535,8 +554,11 @@ begin
|
||||
var LObj := TJSONObject.Create;
|
||||
LObj.AddPair('message', 'OK');
|
||||
LObj.AddPair('kdfIterations', TJSONNumber.Create(LKdfIters));
|
||||
// Same rule as HandleLogin: only KDF-bump or LEGACY format triggers
|
||||
// migration. sha256 / v2 accounts are left as-is (v2 must not be
|
||||
// force-downgraded to sha256 by migrate-kdf).
|
||||
if (LKdfIters < PBKDF2_ITERATIONS_TARGET) or
|
||||
not SameText(LAlgo, HASH_ALGO_CURRENT) then
|
||||
SameText(LAlgo, HASH_ALGO_LEGACY) then
|
||||
begin
|
||||
var LMig := TJSONObject.Create;
|
||||
LMig.AddPair('target', TJSONNumber.Create(PBKDF2_ITERATIONS_TARGET));
|
||||
@@ -883,9 +905,14 @@ begin
|
||||
end;
|
||||
|
||||
// Step 3: compute the new auth hash. ZK path: just wrap the
|
||||
// client-supplied newVerifier. Plaintext: derive server-side.
|
||||
// client-supplied newVerifier (rotating onto the DECOUPLED scheme).
|
||||
// Plaintext: derive server-side (stays CURRENT).
|
||||
var LNewAlgo := HASH_ALGO_CURRENT;
|
||||
if LNewVerifier <> '' then
|
||||
LNewHash := VerifierToStoredHash(LNewVerifier, HASH_ALGO_CURRENT)
|
||||
begin
|
||||
LNewAlgo := HASH_ALGO_DECOUPLED;
|
||||
LNewHash := VerifierToStoredHash(LNewVerifier, LNewAlgo);
|
||||
end
|
||||
else
|
||||
LNewHash := ComputeAuthHashCurrent(LNewPwd, LNewSalt, PBKDF2_ITERATIONS_TARGET);
|
||||
|
||||
@@ -905,7 +932,7 @@ begin
|
||||
LQ.ParamByName('h').AsString := LNewHash;
|
||||
LQ.ParamByName('s').AsString := LNewSalt;
|
||||
LQ.ParamByName('it').AsInteger := PBKDF2_ITERATIONS_TARGET;
|
||||
LQ.ParamByName('algo').AsString := HASH_ALGO_CURRENT;
|
||||
LQ.ParamByName('algo').AsString := LNewAlgo;
|
||||
LQ.ParamByName('uid').AsInteger := LUserId;
|
||||
LQ.ExecSQL;
|
||||
finally
|
||||
|
||||
@@ -273,7 +273,7 @@ procedure HandleRedeem(ARequest: TIdHTTPRequestInfo;
|
||||
var
|
||||
LBody, LObj: TJSONObject;
|
||||
LUser, LCode, LCodeHash, LIP, LStoredHash, LKdfSalt, LWrappedKey, LWrappedIv,
|
||||
LSalt, LToken, LCSRF: string;
|
||||
LSalt, LToken, LCSRF, LAlgo: string;
|
||||
LUserId, LKdfIters, LCurrentUses, LNewUses: Integer;
|
||||
LQ: TFDQuery;
|
||||
begin
|
||||
@@ -309,7 +309,7 @@ begin
|
||||
LQ.Connection := DB.Connection;
|
||||
// Join to users to look up by username + verify the code in one shot.
|
||||
LQ.SQL.Text :=
|
||||
'SELECT u.id, u.salt, u.kdf_iterations, ' +
|
||||
'SELECT u.id, u.salt, u.kdf_iterations, u.hash_algo, ' +
|
||||
' rk.code_hash, rk.kdf_salt, rk.wrapped_key, rk.wrapped_iv, rk.remaining_uses ' +
|
||||
'FROM users u ' +
|
||||
'LEFT JOIN recovery_keys rk ON rk.user_id = u.id ' +
|
||||
@@ -326,6 +326,7 @@ begin
|
||||
LUserId := LQ.FieldByName('id').AsInteger;
|
||||
LSalt := LQ.FieldByName('salt').AsString;
|
||||
LKdfIters := LQ.FieldByName('kdf_iterations').AsInteger;
|
||||
LAlgo := LQ.FieldByName('hash_algo').AsString;
|
||||
LStoredHash := LQ.FieldByName('code_hash').AsString;
|
||||
LKdfSalt := LQ.FieldByName('kdf_salt').AsString;
|
||||
LWrappedKey := LQ.FieldByName('wrapped_key').AsString;
|
||||
@@ -401,6 +402,7 @@ begin
|
||||
LObj.AddPair('csrfToken', LCSRF);
|
||||
LObj.AddPair('salt', LSalt);
|
||||
LObj.AddPair('kdfIterations', TJSONNumber.Create(LKdfIters));
|
||||
LObj.AddPair('hashAlgo', LAlgo);
|
||||
LObj.AddPair('wrappedKey', LWrappedKey);
|
||||
LObj.AddPair('wrappedIv', LWrappedIv);
|
||||
LObj.AddPair('kdfSalt', LKdfSalt);
|
||||
|
||||
@@ -91,7 +91,10 @@ if ($jsFiles) {
|
||||
foreach ($jf in $jsFiles) {
|
||||
Log "Syntax check: $($jf.Relative)"
|
||||
# --check prints errors to stderr and returns non-zero on failure.
|
||||
$out = & $node.Source --check $jf.FullPath 2>&1
|
||||
# Pipe $null into node so it can NEVER block waiting on stdin
|
||||
# (some Windows node shims read stdin when launched from a
|
||||
# non-interactive pre-build event, which would hang the build).
|
||||
$out = $null | & $node.Source --check $jf.FullPath 2>&1
|
||||
if ($LASTEXITCODE -ne 0) {
|
||||
Log "JS SYNTAX ERROR in $($jf.Relative):"
|
||||
Log ($out | Out-String)
|
||||
|
||||
Binary file not shown.
Reference in New Issue
Block a user