Fix: Password Change/Reset Credential Preservation #383
@@ -18,7 +18,6 @@ import QRCode from "qrcode";
|
|||||||
import type { Request, Response } from "express";
|
import type { Request, Response } from "express";
|
||||||
import { authLogger } from "../../utils/logger.js";
|
import { authLogger } from "../../utils/logger.js";
|
||||||
import { AuthManager } from "../../utils/auth-manager.js";
|
import { AuthManager } from "../../utils/auth-manager.js";
|
||||||
import { UserCrypto } from "../../utils/user-crypto.js";
|
|
||||||
import { DataCrypto } from "../../utils/data-crypto.js";
|
import { DataCrypto } from "../../utils/data-crypto.js";
|
||||||
import { LazyFieldEncryption } from "../../utils/lazy-field-encryption.js";
|
import { LazyFieldEncryption } from "../../utils/lazy-field-encryption.js";
|
||||||
|
|
||||||
@@ -1318,8 +1317,52 @@ router.post("/complete-reset", async (req, res) => {
|
|||||||
.where(eq(users.username, username));
|
.where(eq(users.username, username));
|
||||||
|
|
||||||
try {
|
try {
|
||||||
await authManager.registerUser(userId, newPassword);
|
// Check if user has an active session with DEK (logged in while resetting)
|
||||||
authManager.logoutUser(userId);
|
const hasActiveSession = authManager.isUserUnlocked(userId);
|
||||||
|
|
||||||
|
if (hasActiveSession) {
|
||||||
|
// User is logged in - preserve their data by keeping the same DEK
|
||||||
|
// This happens when user resets password from settings page
|
||||||
|
const success = await authManager.resetUserPasswordWithPreservedDEK(
|
||||||
|
userId,
|
||||||
|
newPassword,
|
||||||
|
);
|
||||||
|
|
||||||
|
if (!success) {
|
||||||
|
// If preservation fails, fall back to new DEK (will lose data)
|
||||||
|
authLogger.warn(
|
||||||
|
`Failed to preserve DEK during password reset for ${username}. Creating new DEK - data will be lost.`,
|
||||||
|
{
|
||||||
|
operation: "password_reset_preserve_failed",
|
||||||
|
userId,
|
||||||
|
username,
|
||||||
|
},
|
||||||
|
);
|
||||||
|
await authManager.registerUser(userId, newPassword);
|
||||||
|
} else {
|
||||||
|
authLogger.success(
|
||||||
|
`Password reset completed for user: ${username}. Data preserved using existing session.`,
|
||||||
|
{
|
||||||
|
operation: "password_reset_data_preserved",
|
||||||
|
userId,
|
||||||
|
|
|||||||
|
username,
|
||||||
|
},
|
||||||
|
);
|
||||||
|
}
|
||||||
|
} else {
|
||||||
|
// User is NOT logged in - create new DEK (data will be inaccessible)
|
||||||
|
await authManager.registerUser(userId, newPassword);
|
||||||
|
authManager.logoutUser(userId);
|
||||||
|
|
||||||
|
authLogger.warn(
|
||||||
|
`Password reset completed for user: ${username}. Existing encrypted data is now inaccessible and will need to be re-entered.`,
|
||||||
|
{
|
||||||
|
operation: "password_reset_data_inaccessible",
|
||||||
|
userId,
|
||||||
|
username,
|
||||||
|
},
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
await db
|
await db
|
||||||
.update(users)
|
.update(users)
|
||||||
@@ -1329,15 +1372,6 @@ router.post("/complete-reset", async (req, res) => {
|
|||||||
totp_backup_codes: null,
|
totp_backup_codes: null,
|
||||||
})
|
})
|
||||||
.where(eq(users.id, userId));
|
.where(eq(users.id, userId));
|
||||||
|
|
||||||
authLogger.warn(
|
|
||||||
`Password reset completed for user: ${username}. Existing encrypted data is now inaccessible and will need to be re-entered.`,
|
|
||||||
{
|
|
||||||
operation: "password_reset_data_inaccessible",
|
|
||||||
userId,
|
|
||||||
username,
|
|
||||||
},
|
|
||||||
);
|
|
||||||
} catch (encryptionError) {
|
} catch (encryptionError) {
|
||||||
authLogger.error(
|
authLogger.error(
|
||||||
"Failed to re-encrypt user data after password reset",
|
"Failed to re-encrypt user data after password reset",
|
||||||
@@ -2014,6 +2048,10 @@ router.post("/change-password", authenticateJWT, async (req, res) => {
|
|||||||
.set({ password_hash: newPasswordHash })
|
.set({ password_hash: newPasswordHash })
|
||||||
.where(eq(users.id, userId));
|
.where(eq(users.id, userId));
|
||||||
|
|
||||||
|
// Trigger database save to persist password hash
|
||||||
|
const { saveMemoryDatabaseToFile } = await import("../db/index.js");
|
||||||
|
await saveMemoryDatabaseToFile();
|
||||||
|
|
||||||
authLogger.success("User password changed successfully", {
|
authLogger.success("User password changed successfully", {
|
||||||
operation: "password_change_success",
|
operation: "password_change_success",
|
||||||
userId,
|
userId,
|
||||||
|
|||||||
@@ -295,6 +295,16 @@ class AuthManager {
|
|||||||
newPassword,
|
newPassword,
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
async resetUserPasswordWithPreservedDEK(
|
||||||
|
userId: string,
|
||||||
|
newPassword: string,
|
||||||
|
): Promise<boolean> {
|
||||||
|
return await this.userCrypto.resetUserPasswordWithPreservedDEK(
|
||||||
|
userId,
|
||||||
|
newPassword,
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
export { AuthManager, type AuthenticationResult, type JWTPayload };
|
export { AuthManager, type AuthenticationResult, type JWTPayload };
|
||||||
|
|||||||
@@ -1,6 +1,6 @@
|
|||||||
import crypto from "crypto";
|
import crypto from "crypto";
|
||||||
import { getDb } from "../database/db/index.js";
|
import { getDb } from "../database/db/index.js";
|
||||||
import { settings, users } from "../database/db/schema.js";
|
import { settings } from "../database/db/schema.js";
|
||||||
import { eq } from "drizzle-orm";
|
import { eq } from "drizzle-orm";
|
||||||
import { databaseLogger } from "./logger.js";
|
import { databaseLogger } from "./logger.js";
|
||||||
|
|
||||||
@@ -249,6 +249,7 @@ class UserCrypto {
|
|||||||
newPassword: string,
|
newPassword: string,
|
||||||
): Promise<boolean> {
|
): Promise<boolean> {
|
||||||
try {
|
try {
|
||||||
|
// Validate current password
|
||||||
const isValid = await this.validatePassword(userId, oldPassword);
|
const isValid = await this.validatePassword(userId, oldPassword);
|
||||||
if (!isValid) return false;
|
if (!isValid) return false;
|
||||||
|
|
||||||
@@ -259,23 +260,114 @@ class UserCrypto {
|
|||||||
const encryptedDEK = await this.getEncryptedDEK(userId);
|
const encryptedDEK = await this.getEncryptedDEK(userId);
|
||||||
|
While A safer and more readable approach is to use 
While `Buffer.allocUnsafe()` followed by `DEK.copy()` is functionally correct, using `Buffer.allocUnsafe()` is generally discouraged in security-sensitive code as it can be a source of vulnerabilities (like leaking old data from memory) if not handled with extreme care.
A safer and more readable approach is to use `Buffer.from(buffer)`, which creates a secure copy of the buffer.
```suggestion
const dekCopy = Buffer.from(DEK);
```
This block of code fetches and validates the old This is not just dead code; it performs an unnecessary database query and could cause the function to fail unexpectedly for certain user types (e.g., OIDC users) who might not have a 
This block of code fetches and validates the old `kekSalt`, but it's never used. The purpose of this function is to re-encrypt an existing DEK from an active session using a *new* KEK derived from the new password. The old password's KEK and salt are not required.
This is not just dead code; it performs an unnecessary database query and could cause the function to fail unexpectedly for certain user types (e.g., OIDC users) who might not have a `kekSalt`.
2. Safer Buffer copying (Medium Priority)
File: [user-crypto.ts](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
Fix: Changed from [Buffer.allocUnsafe() + copy()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) to [Buffer.from(DEK)](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
Why: [Buffer.from()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) is safer and more secure - no risk of leaking old memory data, plus it's more readable
3. Removed dead code (Medium Priority)
File: [user-crypto.ts](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
Fix: Removed unused [getKEKSalt()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) check in [resetUserPasswordWithPreservedDEK()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html)
Why: The old KEK salt is not needed - we're using an existing DEK from the session and only need to create a new KEK salt for the new password. This also prevents failures for OIDC users who don't have a KEK salt.
|
|||||||
if (!encryptedDEK) return false;
|
if (!encryptedDEK) return false;
|
||||||
|
|
||||||
|
// Decrypt DEK with old password's KEK
|
||||||
const DEK = this.decryptDEK(encryptedDEK, oldKEK);
|
const DEK = this.decryptDEK(encryptedDEK, oldKEK);
|
||||||
|
|
||||||
|
// The DEK stays the same - only the KEK changes with the new password
|
||||||
|
// This preserves all encrypted user data
|
||||||
|
|
||||||
|
// Generate new KEK from new password
|
||||||
const newKekSalt = await this.generateKEKSalt();
|
const newKekSalt = await this.generateKEKSalt();
|
||||||
const newKEK = this.deriveKEK(newPassword, newKekSalt);
|
const newKEK = this.deriveKEK(newPassword, newKekSalt);
|
||||||
|
|
||||||
|
// Re-encrypt the same DEK with new KEK
|
||||||
const newEncryptedDEK = this.encryptDEK(DEK, newKEK);
|
const newEncryptedDEK = this.encryptDEK(DEK, newKEK);
|
||||||
|
|
||||||
|
// Store new KEK salt and new encrypted DEK
|
||||||
await this.storeKEKSalt(userId, newKekSalt);
|
await this.storeKEKSalt(userId, newKekSalt);
|
||||||
await this.storeEncryptedDEK(userId, newEncryptedDEK);
|
await this.storeEncryptedDEK(userId, newEncryptedDEK);
|
||||||
|
|
||||||
|
// Trigger database save to persist the encryption key changes
|
||||||
|
const { saveMemoryDatabaseToFile } = await import("../database/db/index.js");
|
||||||
|
await saveMemoryDatabaseToFile();
|
||||||
|
|
||||||
|
// Clean up sensitive data from memory
|
||||||
oldKEK.fill(0);
|
oldKEK.fill(0);
|
||||||
newKEK.fill(0);
|
newKEK.fill(0);
|
||||||
DEK.fill(0);
|
|
||||||
|
|
||||||
this.logoutUser(userId);
|
// Create a copy of DEK for the session before zeroing it out
|
||||||
|
const dekCopy = Buffer.allocUnsafe(DEK.length);
|
||||||
|
DEK.copy(dekCopy);
|
||||||
|
|
||||||
|
// Keep user session active with the same DEK
|
||||||
|
const now = Date.now();
|
||||||
|
const oldSession = this.userSessions.get(userId);
|
||||||
|
if (oldSession) {
|
||||||
|
oldSession.dataKey.fill(0);
|
||||||
|
}
|
||||||
|
|
||||||
|
this.userSessions.set(userId, {
|
||||||
|
dataKey: dekCopy,
|
||||||
|
lastActivity: now,
|
||||||
|
expiresAt: now + UserCrypto.SESSION_DURATION,
|
||||||
|
});
|
||||||
|
|
||||||
|
// Zero out the original DEK from memory
|
||||||
|
DEK.fill(0);
|
||||||
|
|
||||||
return true;
|
return true;
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
|
databaseLogger.error("Password change failed", error, {
|
||||||
|
operation: "password_change_error",
|
||||||
|
userId,
|
||||||
|
error: error instanceof Error ? error.message : "Unknown error",
|
||||||
|
});
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
async resetUserPasswordWithPreservedDEK(
|
||||||
|
userId: string,
|
||||||
|
newPassword: string,
|
||||||
|
): Promise<boolean> {
|
||||||
|
try {
|
||||||
|
// This method preserves the existing DEK by re-encrypting it with the new password
|
||||||
|
// Used for password reset when user is logged in
|
||||||
|
|
||||||
|
// Check if user has an active session with DEK
|
||||||
|
const existingDEK = this.getUserDataKey(userId);
|
||||||
|
if (!existingDEK) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
const kekSalt = await this.getKEKSalt(userId);
|
||||||
|
if (!kekSalt) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
// Generate new KEK from new password
|
||||||
|
const newKekSalt = await this.generateKEKSalt();
|
||||||
|
const newKEK = this.deriveKEK(newPassword, newKekSalt);
|
||||||
|
|
||||||
|
// Re-encrypt the existing DEK with new KEK
|
||||||
|
const newEncryptedDEK = this.encryptDEK(existingDEK, newKEK);
|
||||||
|
|
||||||
|
// Store new KEK salt and new encrypted DEK
|
||||||
|
await this.storeKEKSalt(userId, newKekSalt);
|
||||||
|
await this.storeEncryptedDEK(userId, newEncryptedDEK);
|
||||||
|
|
||||||
|
// Trigger database save to persist the encryption key changes
|
||||||
|
const { saveMemoryDatabaseToFile } = await import(
|
||||||
|
"../database/db/index.js"
|
||||||
|
);
|
||||||
|
await saveMemoryDatabaseToFile();
|
||||||
|
|
||||||
|
// Clean up sensitive data
|
||||||
|
newKEK.fill(0);
|
||||||
|
|
||||||
|
// Update session activity timestamp
|
||||||
|
const session = this.userSessions.get(userId);
|
||||||
|
if (session) {
|
||||||
|
session.lastActivity = Date.now();
|
||||||
|
}
|
||||||
|
|
||||||
|
return true;
|
||||||
|
} catch (error) {
|
||||||
|
databaseLogger.error("Password reset with preserved DEK failed", error, {
|
||||||
|
operation: "password_reset_preserve_error",
|
||||||
|
userId,
|
||||||
|
error: error instanceof Error ? error.message : "Unknown error",
|
||||||
|
});
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
In the failure scenario for preserving the DEK, the user's session is not terminated after re-registering the user with a new DEK. This leaves the user in an inconsistent state, where their active session contains a stale DEK that no longer corresponds to the encrypted data on disk. This could lead to unexpected errors.
To ensure a clean state, the user should be logged out, similar to the flow for a non-logged-in password reset.
File: users.ts
Fix: Added authManager.logoutUser(userId) after registerUser() in the failure scenario
Why: When DEK preservation fails and a new DEK is created, the old session with stale DEK must be terminated to avoid inconsistent state