Fix: Password Change/Reset Credential Preservation #383

Merged
thorved merged 3 commits from fix/password-change-credential-wipe into dev-1.7.3 2025-10-09 04:23:44 +00:00
2 changed files with 0 additions and 29 deletions
Showing only changes of commit c117055602 - Show all commits

View File

@@ -1317,19 +1317,15 @@ router.post("/complete-reset", async (req, res) => {
.where(eq(users.username, username));
try {
// Check if user has an active session with DEK (logged in while resetting)
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.`,
{
@@ -1351,7 +1347,6 @@ router.post("/complete-reset", async (req, res) => {
);
gemini-code-assist[bot] commented 2025-10-08 07:01:30 +00:00 (Migrated from github.com)
Review

high

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.

          await authManager.registerUser(userId, newPassword);
          authManager.logoutUser(userId);
![high](https://www.gstatic.com/codereviewagent/high-priority.svg) 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. ```suggestion await authManager.registerUser(userId, newPassword); authManager.logoutUser(userId); ```
thorved commented 2025-10-08 07:11:23 +00:00 (Migrated from github.com)
Review
  1. Logout on DEK preservation failure (High Priority)
    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
1. Logout on DEK preservation failure (High Priority) File: [users.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: Added [authManager.logoutUser(userId)](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) after [registerUser()](vscode-file://vscode-app/c:/Users/Ved/AppData/Local/Programs/Microsoft%20VS%20Code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) 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
}
} else {
// User is NOT logged in - create new DEK (data will be inaccessible)
await authManager.registerUser(userId, newPassword);
authManager.logoutUser(userId);
@@ -2049,7 +2044,6 @@ router.post("/change-password", authenticateJWT, async (req, res) => {
.set({ password_hash: newPasswordHash })
.where(eq(users.id, userId));
// Trigger database save to persist password hash
const { saveMemoryDatabaseToFile } = await import("../db/index.js");
await saveMemoryDatabaseToFile();

View File

@@ -249,7 +249,6 @@ class UserCrypto {
newPassword: string,
): Promise<boolean> {
try {
// Validate current password
const isValid = await this.validatePassword(userId, oldPassword);
if (!isValid) return false;
@@ -260,35 +259,24 @@ class UserCrypto {
const encryptedDEK = await this.getEncryptedDEK(userId);
if (!encryptedDEK) return false;
gemini-code-assist[bot] commented 2025-10-08 07:01:31 +00:00 (Migrated from github.com)
Review

medium

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.

      const dekCopy = Buffer.from(DEK);
![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg) 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); ```
gemini-code-assist[bot] commented 2025-10-08 07:01:31 +00:00 (Migrated from github.com)
Review

medium

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.

![medium](https://www.gstatic.com/codereviewagent/medium-priority.svg) 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`.
thorved commented 2025-10-08 07:11:54 +00:00 (Migrated from github.com)
Review
  1. Safer Buffer copying (Medium Priority)
    File: user-crypto.ts
    Fix: Changed from Buffer.allocUnsafe() + copy() to Buffer.from(DEK)
    Why: Buffer.from() is safer and more secure - no risk of leaking old memory data, plus it's more readable
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
thorved commented 2025-10-08 07:12:08 +00:00 (Migrated from github.com)
Review
  1. Removed dead code (Medium Priority)
    File: user-crypto.ts
    Fix: Removed unused getKEKSalt() check in resetUserPasswordWithPreservedDEK()
    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.
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.
// Decrypt DEK with old password's KEK
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 newKEK = this.deriveKEK(newPassword, newKekSalt);
// Re-encrypt the same DEK with new KEK
const newEncryptedDEK = this.encryptDEK(DEK, 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 from memory
oldKEK.fill(0);
newKEK.fill(0);
// Create a copy of DEK for the session before zeroing it out
const dekCopy = Buffer.from(DEK);
// Keep user session active with the same DEK
const now = Date.now();
const oldSession = this.userSessions.get(userId);
if (oldSession) {
@@ -301,7 +289,6 @@ class UserCrypto {
expiresAt: now + UserCrypto.SESSION_DURATION,
});
// Zero out the original DEK from memory
DEK.fill(0);
return true;
@@ -320,36 +307,26 @@ class UserCrypto {
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;
}
// 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();