dennii/Termix
1
0
Fork 0
Code Issues Pull Requests 2 Actions Packages Projects Releases 22 Wiki Activity

Fix: OIDC users losing credentials after re-login due to non-persistent encryption keys #363

Merged
thorved merged 2 commits from fix/oidc-credential-persistence into dev-1.7.2 2025-10-06 14:19:22 +00:00
Conversation 5 Commits 2 Files Changed 1 +23 -15
1 changed files with 23 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 9906b940b7 - Show all commits

38
src/backend/utils/user-crypto.ts
View File

@@ -70,13 +70,11 @@ class UserCrypto {
}
gemini-code-assist[bot] commented 2025-10-06 06:42:56 +00:00 (Migrated from github.com)
Review
Copy Link

critical

This new implementation introduces a critical race condition and contains redundant logic.

1. Critical Race Condition (Data Loss):
If two concurrent login requests for the same new OIDC user arrive, both can enter the else block to create a new DEK. The storeEncryptedDEK function performs a non-atomic select-then-update/insert, which can cause one request's DEK to overwrite the other's in the database. If a user's session uses a DEK that is different from the one stored, any data they save will be unrecoverable on their next login. This is a critical data loss scenario.

2. Redundant kekSalt Logic:
The kekSalt is generated, stored, and checked, but it's not used for OIDC key derivation (deriveOIDCSystemKey uses the userId as the salt). This adds unnecessary complexity and database writes.

Suggested Fix:
The following suggestion addresses both issues:

  • It removes the unnecessary kekSalt handling.
  • It mitigates the race condition by re-reading the DEK from the database after storing it. This "write-then-read-and-verify" check ensures the session uses the correct, persisted DEK, guaranteeing data consistency.
  async setupOIDCUserEncryption(userId: string): Promise<void> {
    const existingEncryptedDEK = await this.getEncryptedDEK(userId);

    let DEK: Buffer;

    if (existingEncryptedDEK) {
      // User already has a persisted DEK, retrieve it
      const systemKey = this.deriveOIDCSystemKey(userId);
      DEK = this.decryptDEK(existingEncryptedDEK, systemKey);
      systemKey.fill(0);
    } else {
      // First time setup - create and persist new DEK
      DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH);

      const systemKey = this.deriveOIDCSystemKey(userId);
      try {
        const encryptedDEK = this.encryptDEK(DEK, systemKey);
        await this.storeEncryptedDEK(userId, encryptedDEK);

        // MITIGATION: Read back the stored DEK to ensure we use the one that won the race.
        const storedEncryptedDEK = await this.getEncryptedDEK(userId);
        if (storedEncryptedDEK && storedEncryptedDEK.data !== encryptedDEK.data) {
          // We lost the race. Use the DEK from the database.
          DEK.fill(0); // Discard our generated DEK.
          DEK = this.decryptDEK(storedEncryptedDEK, systemKey);
        } else if (!storedEncryptedDEK) {
          // This is an unexpected state; the store operation should have worked.
          throw new Error("Failed to store and retrieve user encryption key.");
        }
      } finally {
        systemKey.fill(0);
      }
    }
![critical](https://www.gstatic.com/codereviewagent/critical.svg) This new implementation introduces a critical race condition and contains redundant logic. **1. Critical Race Condition (Data Loss):** If two concurrent login requests for the same new OIDC user arrive, both can enter the `else` block to create a new DEK. The `storeEncryptedDEK` function performs a non-atomic `select-then-update/insert`, which can cause one request's DEK to overwrite the other's in the database. If a user's session uses a DEK that is different from the one stored, any data they save will be unrecoverable on their next login. This is a critical data loss scenario. **2. Redundant `kekSalt` Logic:** The `kekSalt` is generated, stored, and checked, but it's not used for OIDC key derivation (`deriveOIDCSystemKey` uses the `userId` as the salt). This adds unnecessary complexity and database writes. **Suggested Fix:** The following suggestion addresses both issues: * It removes the unnecessary `kekSalt` handling. * It mitigates the race condition by re-reading the DEK from the database after storing it. This "write-then-read-and-verify" check ensures the session uses the correct, persisted DEK, guaranteeing data consistency. ```typescript async setupOIDCUserEncryption(userId: string): Promise<void> { const existingEncryptedDEK = await this.getEncryptedDEK(userId); let DEK: Buffer; if (existingEncryptedDEK) { // User already has a persisted DEK, retrieve it const systemKey = this.deriveOIDCSystemKey(userId); DEK = this.decryptDEK(existingEncryptedDEK, systemKey); systemKey.fill(0); } else { // First time setup - create and persist new DEK DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH); const systemKey = this.deriveOIDCSystemKey(userId); try { const encryptedDEK = this.encryptDEK(DEK, systemKey); await this.storeEncryptedDEK(userId, encryptedDEK); // MITIGATION: Read back the stored DEK to ensure we use the one that won the race. const storedEncryptedDEK = await this.getEncryptedDEK(userId); if (storedEncryptedDEK && storedEncryptedDEK.data !== encryptedDEK.data) { // We lost the race. Use the DEK from the database. DEK.fill(0); // Discard our generated DEK. DEK = this.decryptDEK(storedEncryptedDEK, systemKey); } else if (!storedEncryptedDEK) { // This is an unexpected state; the store operation should have worked. throw new Error("Failed to store and retrieve user encryption key."); } } finally { systemKey.fill(0); } } ```
thorved commented 2025-10-06 07:01:22 +00:00 (Migrated from github.com)
Review
Copy Link

✅ Changes Made

1. Race Condition Mitigation (Critical Fix)

Problem Identified

You're right - the original implementation had a classic check-then-act race condition. Two concurrent login requests could both generate different DEKs, and whichever stored last would win, leaving the other session with a mismatched key.

Race Condition Scenario:

Time  | Request A (User Login)         | Request B (User Login - Same User)
------+-------------------------------+------------------------------------
T1    | Check: No DEK exists          |
T2    | Generate Random DEK_A         |
T3    |                               | Check: No DEK exists (still!)
T4    |                               | Generate Random DEK_B (DIFFERENT!)
T5    | Store DEK_A in database       |
T6    |                               | Store DEK_B (OVERWRITES!)
T7    | Session A uses DEK_A          |
T8    |                               | Session B uses DEK_B
T9    | User creates credential       |
T10   | Credential encrypted with DEK_A|
T11   | Database now has DEK_B        |
------+-------------------------------+------------------------------------
Result: Next login uses DEK_B, but credential was encrypted with DEK_A
        ❌ DATA LOSS!

Solution Implemented

Added a "write-then-read-and-verify" pattern in setupOIDCUserEncryption():

async setupOIDCUserEncryption(userId: string): Promise<void> {
  const existingEncryptedDEK = await this.getEncryptedDEK(userId);

  let DEK: Buffer;

  if (existingEncryptedDEK) {
    // User already has a persisted DEK, retrieve it
    const systemKey = this.deriveOIDCSystemKey(userId);
    DEK = this.decryptDEK(existingEncryptedDEK, systemKey);
    systemKey.fill(0);
  } else {
    // First time setup - create and persist new DEK
    DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH);
    const systemKey = this.deriveOIDCSystemKey(userId);

    try {
      const encryptedDEK = this.encryptDEK(DEK, systemKey);
      await this.storeEncryptedDEK(userId, encryptedDEK);

      // ✅ MITIGATION: Read back the stored DEK to ensure we use the one that won the race
      const storedEncryptedDEK = await this.getEncryptedDEK(userId);
      if (
        storedEncryptedDEK &&
        storedEncryptedDEK.data !== encryptedDEK.data
      ) {
        // We lost the race. Use the DEK from the database.
        DEK.fill(0); // Discard our generated DEK
        DEK = this.decryptDEK(storedEncryptedDEK, systemKey);
      } else if (!storedEncryptedDEK) {
        // This is an unexpected state; the store operation should have worked.
        throw new Error(
          "Failed to store and retrieve user encryption key.",
        );
      }
    } finally {
      systemKey.fill(0); // Always clean up sensitive material
    }
  }

  // Load verified DEK into session
  const now = Date.now();
  this.userSessions.set(userId, {
    dataKey: Buffer.from(DEK),
    lastActivity: now,
    expiresAt: now + UserCrypto.SESSION_DURATION,
  });

  DEK.fill(0);
}

How This Prevents Data Loss:

  1. Both requests generate their own DEKs (DEK_A and DEK_B)
  2. Both attempt to store their DEK
  3. Critical Step: Both read back the stored DEK from database
  4. They compare: "Is my DEK the same as what's stored?"
  5. If different (race detected): Discard generated DEK, use the stored one
  6. Result: Both sessions end up using the same DEK (whichever won the race)
  7. ✅ Data encrypted by either session can be decrypted by future logins
## ✅ Changes Made ### 1. **Race Condition Mitigation** (Critical Fix) #### Problem Identified You're right - the original implementation had a classic **check-then-act race condition**. Two concurrent login requests could both generate different DEKs, and whichever stored last would win, leaving the other session with a mismatched key. **Race Condition Scenario:** ``` Time | Request A (User Login) | Request B (User Login - Same User) ------+-------------------------------+------------------------------------ T1 | Check: No DEK exists | T2 | Generate Random DEK_A | T3 | | Check: No DEK exists (still!) T4 | | Generate Random DEK_B (DIFFERENT!) T5 | Store DEK_A in database | T6 | | Store DEK_B (OVERWRITES!) T7 | Session A uses DEK_A | T8 | | Session B uses DEK_B T9 | User creates credential | T10 | Credential encrypted with DEK_A| T11 | Database now has DEK_B | ------+-------------------------------+------------------------------------ Result: Next login uses DEK_B, but credential was encrypted with DEK_A ❌ DATA LOSS! ``` #### Solution Implemented Added a **"write-then-read-and-verify"** pattern in `setupOIDCUserEncryption()`: ```typescript async setupOIDCUserEncryption(userId: string): Promise<void> { const existingEncryptedDEK = await this.getEncryptedDEK(userId); let DEK: Buffer; if (existingEncryptedDEK) { // User already has a persisted DEK, retrieve it const systemKey = this.deriveOIDCSystemKey(userId); DEK = this.decryptDEK(existingEncryptedDEK, systemKey); systemKey.fill(0); } else { // First time setup - create and persist new DEK DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH); const systemKey = this.deriveOIDCSystemKey(userId); try { const encryptedDEK = this.encryptDEK(DEK, systemKey); await this.storeEncryptedDEK(userId, encryptedDEK); // ✅ MITIGATION: Read back the stored DEK to ensure we use the one that won the race const storedEncryptedDEK = await this.getEncryptedDEK(userId); if ( storedEncryptedDEK && storedEncryptedDEK.data !== encryptedDEK.data ) { // We lost the race. Use the DEK from the database. DEK.fill(0); // Discard our generated DEK DEK = this.decryptDEK(storedEncryptedDEK, systemKey); } else if (!storedEncryptedDEK) { // This is an unexpected state; the store operation should have worked. throw new Error( "Failed to store and retrieve user encryption key.", ); } } finally { systemKey.fill(0); // Always clean up sensitive material } } // Load verified DEK into session const now = Date.now(); this.userSessions.set(userId, { dataKey: Buffer.from(DEK), lastActivity: now, expiresAt: now + UserCrypto.SESSION_DURATION, }); DEK.fill(0); } ``` **How This Prevents Data Loss:** 1. Both requests generate their own DEKs (DEK_A and DEK_B) 2. Both attempt to store their DEK 3. **Critical Step**: Both read back the stored DEK from database 4. They compare: "Is my DEK the same as what's stored?" 5. If different (race detected): Discard generated DEK, use the stored one 6. **Result**: Both sessions end up using the **same DEK** (whichever won the race) 7. ✅ Data encrypted by either session can be decrypted by future logins ---
async setupOIDCUserEncryption(userId: string): Promise<void> {
// Check if DEK already exists for this OIDC user
const existingKEKSalt = await this.getKEKSalt(userId);
const existingEncryptedDEK = await this.getEncryptedDEK(userId);
let DEK: Buffer;
if (existingKEKSalt && existingEncryptedDEK) {
if (existingEncryptedDEK) {
// User already has a persisted DEK, retrieve it
const systemKey = this.deriveOIDCSystemKey(userId);
DEK = this.decryptDEK(existingEncryptedDEK, systemKey);
@@ -84,19 +82,30 @@ class UserCrypto {
} else {
// First time setup - create and persist new DEK
DEK = crypto.randomBytes(UserCrypto.DEK_LENGTH);
// Generate a KEK salt for OIDC user (using a deterministic approach)
const kekSalt = await this.generateKEKSalt();
await this.storeKEKSalt(userId, kekSalt);
// Derive system key for OIDC user
const systemKey = this.deriveOIDCSystemKey(userId);
// Encrypt and store the DEK
const encryptedDEK = this.encryptDEK(DEK, systemKey);
await this.storeEncryptedDEK(userId, encryptedDEK);
try {
const encryptedDEK = this.encryptDEK(DEK, systemKey);
await this.storeEncryptedDEK(userId, encryptedDEK);
systemKey.fill(0);
// MITIGATION: Read back the stored DEK to ensure we use the one that won the race.
const storedEncryptedDEK = await this.getEncryptedDEK(userId);
if (
storedEncryptedDEK &&
storedEncryptedDEK.data !== encryptedDEK.data
) {
// We lost the race. Use the DEK from the database.
DEK.fill(0); // Discard our generated DEK.
DEK = this.decryptDEK(storedEncryptedDEK, systemKey);
} else if (!storedEncryptedDEK) {
// This is an unexpected state; the store operation should have worked.
throw new Error(
"Failed to store and retrieve user encryption key.",
);
}
} finally {
systemKey.fill(0);
}
}
const now = Date.now();
@@ -161,10 +170,9 @@ class UserCrypto {
async authenticateOIDCUser(userId: string): Promise<boolean> {
try {
const kekSalt = await this.getKEKSalt(userId);
const encryptedDEK = await this.getEncryptedDEK(userId);
if (!kekSalt || !encryptedDEK) {
if (!encryptedDEK) {
// First time login or missing encryption data - set up encryption
await this.setupOIDCUserEncryption(userId);
return true;