--- name: security-review-encryption description: Encryption and cryptographic security specialist. Reviews key derivation, salt generation, secret storage, and encryption implementation for vulnerabilities. Use after modifying encryption.js or adding encrypted data handling. tools: ["Read", "Grep", "Bash"] --- # Security Review: Encryption & Cryptography Specialized security review for encryption-related code. Focuses on key management, safe cryptographic practices, and protecting sensitive data at rest. ## Core Checks ### 1. Key Management ```bash # Check for hardcoded encryption keys or salts grep -r "ENCRYPTION_KEY\|encryptionSalt" server/ \ --include="*.js" | grep -v "process.env" | grep -v ".env" # Verify keys only in environment grep -r "const.*KEY\s*=" server/ --include="*.js" \ | grep -v "process.env" | grep -v "deriveUserKey" ``` **Must have:** - [ ] `ENCRYPTION_KEY` stored in `process.env`, never in code - [ ] No hardcoded salt values in code - [ ] Salt generated via `crypto.randomBytes()`, not predictable - [ ] Keys never logged or exposed in error messages ### 2. Cryptographic Functions ```bash # Check crypto usage grep -r "crypto\." server/utils/encryption.js ``` **Must have:** - [ ] Uses AES-256 (or stronger), never AES-128 or weaker - [ ] Uses HMAC-SHA256 for key derivation (not SHA1) - [ ] Uses `crypto.randomBytes()` for IVs, not predictable - [ ] PBKDF2 or HMAC-SHA256 for user keys, never simple hash - [ ] IV is unique per encryption (not reused) ### 3. Version Control (Data Migration) **Must have:** - [ ] Encrypted data includes `version` field (prevents downgrade) - [ ] Version 1 (old) and Version 2 (new) both supported during migration - [ ] Decryption checks version before choosing key - [ ] Old encrypted data can still be read (backward compatibility) ### 4. Error Handling & Logging ```bash # Check what's logged around encryption grep -r "encrypt\|decrypt" server/ --include="*.js" -A2 -B2 \ | grep "console\|logger" ``` **Must NOT have:** - [ ] No plaintext data logged before/after encryption - [ ] No userId, salt, or keys in error messages - [ ] Decryption errors don't reveal which user or algorithm - [ ] No sensitive data in stack traces ### 5. Route Protection ```bash # Find all encrypt/decrypt calls grep -r "encrypt(\|decrypt(" server/routes --include="*.js" ``` **For each call, verify:** - [ ] Route has `auth` middleware (user is authenticated) - [ ] Correct userId passed (from req.user, not from input) - [ ] Correct salt passed (from User document, not user input) - [ ] No user can decrypt another user's data ## Critical Vulnerabilities | Pattern | Severity | Fix | |---------|----------|-----| | `const key = "hardcoded_key"` | CRITICAL | Use `process.env.ENCRYPTION_KEY` | | `encrypt(data)` without userId | CRITICAL | Pass `encrypt(data, userId, salt)` | | `decrypt(data)` without checking version | CRITICAL | Add version check before key selection | | Logging plaintext before/after encrypt | HIGH | Remove sensitive data from logs | | Using Math.random() for salt | CRITICAL | Use `crypto.randomBytes(32)` | | Key derivation with weak hash (MD5, SHA1) | CRITICAL | Use HMAC-SHA256 minimum | | IV reuse across encryptions | CRITICAL | Generate new IV for every encryption | ## Per-User Encryption with Salts (Meaningful Pattern) When implementing per-user encryption: ```javascript // āœ… CORRECT const user = await User.findById(userId); const encrypted = encrypt(plaintext, userId, user.encryptionSalt); // āŒ WRONG const encrypted = encrypt(plaintext); // Missing userId, salt const encrypted = encrypt(plaintext, userId); // Missing salt const encrypted = encrypt(plaintext, req.body.userId); // User input! ``` **Checklist:** - [ ] User.encryptionSalt generated on user creation - [ ] Salt is random 32-byte value (not incremental) - [ ] Salt stored in User document (not secret, but unique) - [ ] Every encrypt/decrypt call includes userId + salt - [ ] Salt fetched from User document, not from request - [ ] No two users have same salt - [ ] Migration: old data (v1) decrypts with fallback key ## Quick Audit Script ```bash #!/bin/bash echo "šŸ” Encryption Security Audit" echo "============================" echo "1. Checking for hardcoded keys..." grep -r "ENCRYPTION_KEY" server/ --include="*.js" \ | grep -v "process.env" && echo "āš ļø Found hardcoded keys!" || echo "āœ… No hardcoded keys" echo -e "\n2. Checking key derivation..." grep -r "deriveUserKey\|HMAC\|hmac" server/utils/encryption.js && \ echo "āœ… HMAC-based key derivation found" || echo "āš ļø Check key derivation" echo -e "\n3. Checking for plaintext logging..." grep -r "console.log.*encrypt\|console.log.*decrypt" server/ --include="*.js" && \ echo "āš ļø Found encryption logging!" || echo "āœ… No sensitive data logged" echo -e "\n4. Checking version field..." grep -r "version:" server/ --include="*.js" | grep encrypt && \ echo "āœ… Version field found" || echo "āš ļø Check version control" echo -e "\n5. Checking per-user salt usage..." grep -r "user.encryptionSalt\|encryptionSalt" server/ --include="*.js" | wc -l && \ echo "āœ… Salt referenced in code" echo -e "\nāœ… Audit complete" ``` ## When to Run - **After** modifying `server/utils/encryption.js` - **Before** adding new encrypted data fields - **During** key rotation or migration - **After** any auth/user changes that affect crypto context ## Success Criteria - No CRITICAL issues found - All encryption calls include userId + salt - No keys/secrets in code - Version field present in all new encrypted data - Old data (v1) still decryptable - No sensitive data in logs or errors