Security: Implement SQLite database encryption using SQLCipher - #88
Security: Implement SQLite database encryption using SQLCipher#88anshul23102 wants to merge 2 commits into
Conversation
- Add SQLCipher encryption to protect database from unauthorized OS-level access - Derive encryption key from Electron's safeStorage API (Keychain/DPAPI) - Database file is now encrypted at rest using AES-256 - Add encryption utility module with field-level encryption support - Add encryption setup documentation with build instructions - Update database initialization to enable SQLCipher PRAGMA key - Protect sensitive data: tasks, goals, AI conversations, session records Resolves ManabBiswas#80
|
@ManabBiswas Could you please review this PR? It implements SQLCipher encryption for the database to resolve the security vulnerability described in issue #80. This PR ensures that all sensitive productivity data (tasks, goals, AI conversations, session records) is encrypted at rest using AES-256, making it unreadable by other local processes or unauthorized users. The encryption key is securely derived from Electron's safeStorage API (Keychain/DPAPI), ensuring it's never stored in plaintext on disk. I'd appreciate it if you could apply the |
ManabBiswas
left a comment
There was a problem hiding this comment.
copilot review:
Short summary / overall assessment
- Not ready to merge. I reviewed the three changed files (src/main/db/encryption.ts, src/main/db/database.ts, src/main/db/ENCRYPTION_SETUP.md). The implementation intent is good (SQLCipher via better-sqlite3 + Electron safeStorage), but there is a critical flaw in the key handling that will break or irreversibly lock access to the DB and undermines the encryption model. Several correctness and robustness issues should be addressed before merging.
What I inspected and did
- Loaded the PR metadata and diff for PR #88 and inspected:
- src/main/db/encryption.ts (new)
- src/main/db/database.ts (modified)
- src/main/db/ENCRYPTION_SETUP.md (new)
- I checked the code paths for key derivation, DB initialization and field-level encrypt/decrypt utilities.
Blocking/security/correctness issues (must fix)
- Key derivation is incorrect and will break DB access (critical)
- Problem: getDatabaseEncryptionKey() calls safeStorage.encryptString(keySeed) on a fixed seed each startup and uses the resulting ciphertext (hex substring) as the SQLCipher key.
- Why this is critical:
- safeStorage.encryptString produces an encrypted blob — not a stable secret to be used directly as a key. It may vary between calls (even for same plaintext) and is not meant to be used this way.
- The code never persists a stable encrypted key; it creates a new ciphertext on every run and immediately uses it as the DB key. That makes the DB openable only with that exact ciphertext; a different ciphertext on the next run means the key changes → DB becomes unreadable (effectively irrecoverable).
- You must generate a single random DB key and persist it (encrypted) to disk (or persist plaintext in OS secure storage) so you can recover the same key across restarts.
- Recommendation:
- Generate a random 32-byte key once (crypto.randomBytes(32)).
- Encrypt that raw key with safeStorage.encryptString() and persist the encrypted blob (e.g., base64) in a file in the app data dir, or store it in a secure storage mechanism managed for your app.
- On startup, read the persisted encrypted blob, decrypt it with safeStorage.decryptString(), and use the resulting raw key bytes for SQLCipher.
- Do NOT call safeStorage.encryptString() repeatedly on a fixed seed and use that ciphertext as the key.
- Wrong use of SQLCipher key format / missing consistent key handling
- The code supplies db.pragma(
key = 'hex:${encryptionKey}') using a hex substring taken from an encrypted blob. Even if you had deterministic hex bytes, you should ensure you're using raw key bytes (not ciphertext or base64 of ciphertext) and that the hex is the key bytes (64 hex chars = 32 bytes). - Recommendation:
- Keep the raw key as 32 bytes. When using PRAGMA key with hex, feed Buffer(key).toString('hex') (full 64 hex chars) and validate by running a simple read (e.g., PRAGMA cipher_version or SELECT count(*) FROM sqlite_master) to detect wrong key early.
- Consider using PRAGMA key = '' (passphrase mode) only if you intend to use a passphrase-derived key (then derive via PBKDF2 with salt/iterations). For a random key, use hex.
- No migration/rekey path for existing databases
- If a user already has a plain database, you need a controlled migration strategy:
- If DB exists and is unencrypted: open it, then run PRAGMA rekey = 'hex:NEWKEY' to encrypt it.
- If DB exists and is encrypted with old method: you must be able to open it with previous key and rekey or warn the user and provide recovery instructions.
- Recommendation:
- Add logic that detects whether DB is encrypted and handles:
- New install: create DB and apply key.
- Existing unencrypted DB: rekey to encrypt with new key (after taking backup).
- Wrong key: surface a clear error and instructions, don't proceed.
- Make backups before rekeying and surface user-friendly error if rekey fails.
- Add logic that detects whether DB is encrypted and handles:
- Misuse of Electron safeStorage API semantics
- safeStorage.encryptString/decryptString are fine for encrypting small blobs, but they do not provide a persistent secure key store by themselves. If you persist the encrypted blob to disk, decryptString only works when run under the same OS user (as intended). That approach is acceptable — but must be implemented correctly (persist encrypted blob, decrypt on startup).
- Current code encrypts a fixed seed and never decrypts; it treats ciphertext as the key directly — wrong.
Other important correctness/robustness issues
- encryption.ts currently imports crypto but doesn't use it — remove or use it for generating the random key.
- isEncrypted line contains Unicode escapes in the diff for && (looks like \u0026\u0026) — ensure source file shows actual && and compiles.
- getEncryptionPragma() and getDatabaseEncryptionKey() duplication — either use one canonical function (e.g., getStoredRawKey) and build PRAGMA where used.
- encryptField/decryptField fallback: when safeStorage.isEncryptionAvailable() is false they silently store plaintext. At minimum, log a clear warning and consider failing safe mode or offering a config toggle — storing plaintext silently is dangerous.
- Logging: avoid logging anything that could be considered sensitive; current logs fine, but ensure you never log keys or raw encrypted blobs.
Minor / style suggestions
- Call PRAGMA key before any other DB operation and validate the key by a quick test read.
- Add unit/integration tests covering:
- Startup with no DB
- Startup with plain DB and migration to encrypted DB
- Startup with encrypted DB and correct key
- Startup with encrypted DB and wrong/missing key (should show meaningful error)
- Document exact build steps for better-sqlite3+SQLCipher in ENCRYPTION_SETUP.md with links to a tested recipe for Linux/macOS/Windows and mention electron-rebuild if needed.
- Consider using PBKDF2 if you plan to derive the key from a passphrase (but for OS-level protection a random key encrypted by OS storage is preferable).
- Add metrics/telemetry for encryption availability (but not the key) to know how many installs can't enable encryption.
Suggested concrete changes (code sketch)
- High-level algorithm:
- On first run:
- Generate 32 random bytes: const rawKey = crypto.randomBytes(32);
- Encrypt rawKey (base64 of rawKey) with safeStorage.encryptString(rawKeyBase64)
- Persist encrypted blob (base64) to a file like app.getPath('userData') + '/enc_key'
- Use rawKey for DB PRAGMA key
- On subsequent runs:
- Read persisted encrypted blob
- Decrypt with safeStorage.decryptString(Buffer.from(storedBase64, 'base64')) => rawKeyBase64
- Convert back to Buffer and use Buffer.toString('hex') for PRAGMA key
- On first run:
- Example snippet (conceptual — adjust for your code structure):
// pseudo-code for getOrCreateDatabaseKey()
import fs from 'fs';
import path from 'path';
import { safeStorage, app } from 'electron';
import crypto from 'crypto';
const KEY_FILE = path.join(app.getPath('userData'), 'db_key.enc');
export function getOrCreateDatabaseKey(): Buffer | null {
if (!safeStorage.isEncryptionAvailable()) {
console.warn('[Encryption] safeStorage not available');
return null;
}
if (fs.existsSync(KEY_FILE)) {
const stored = fs.readFileSync(KEY_FILE, 'utf8'); // base64 of encrypted blob
const encryptedBuf = Buffer.from(stored, 'base64');
const rawBase64 = safeStorage.decryptString(encryptedBuf);
return Buffer.from(rawBase64, 'base64'); // 32 bytes
} else {
const rawKey = crypto.randomBytes(32); // 32 bytes
const rawBase64 = rawKey.toString('base64');
const encryptedBuf = safeStorage.encryptString(rawBase64); // Buffer
fs.writeFileSync(KEY_FILE, encryptedBuf.toString('base64'), {mode: 0o600});
return rawKey;
}
}- Then use:
- const rawKey = getOrCreateDatabaseKey();
- if (rawKey) db.pragma(
key = 'hex:${rawKey.toString('hex')}'); - After setting key, validate by running e.g., db.prepare("SELECT count(*) FROM sqlite_master").get();
Paste-ready review comments (copy/paste into the PR as inline comments)
- File: src/main/db/encryption.ts
-
"Blocking: The function getDatabaseEncryptionKey() currently calls safeStorage.encryptString on a fixed seed on each run and returns a substring of the ciphertext as the DB key. This will produce a different key across restarts and will make the DB unreadable. Instead you must generate a single random key, encrypt and persist that encrypted blob, and decrypt it on startup to recover the same raw key. See my suggested snippet in the review."
-
"Please persist the encrypted DB key to disk (app.getPath('userData')), with file permissions 600, and decrypt it at startup. Do not use the ciphertext of an encryptString call on a constant seed as the DB key."
-
"encryptField/decryptField: good approach, but please add a clear warning or a hard-fail if safeStorage.isEncryptionAvailable() is false (or make the UI/installer aware), because silently falling back to plaintext can lead to data exposure."
-
"Remove unused import crypto or use it to create a proper random key as suggested."
- File: src/main/db/database.ts
-
"Blocking: The call getDatabaseEncryptionKey() is being used as if it returns a stable raw key. See encryption.ts issues — as written, this will produce a new key each run and lock the DB. Please refactor per my suggestions so this call returns the same raw key across restarts."
-
"Please move the PRAGMA key call immediately after opening the DB and before any other operation (you do, but add a validation read immediately afterwards to detect wrong key early)."
-
"Add logic to detect an unencrypted existing DB and perform a safe rekey (with backup) to encrypt it on first run."
-
"Log failures clearly and stop startup if DB cannot be opened with the provided key (do not proceed)."
- Documentation: src/main/db/ENCRYPTION_SETUP.md
- "Add explicit instructions on how the app persists the encrypted key (where to store it and file permissions). Also add migration steps for existing plain DB files and a tested build recipe for each OS (electron-rebuild, SQLCipher versions, sample package.json scripts)."
Addressed all critical issues raised in the maintainer review: 1. Fixed key derivation: Generate random 32-byte key once and persist encrypted version to disk instead of calling safeStorage.encryptString on a fixed seed each startup 2. Proper key recovery: On startup, read persisted encrypted key file, decrypt it, and use for SQLCipher PRAGMA key 3. Added immediate key validation after applying encryption to detect wrong keys early 4. Improved error handling with clear failure messages if database key is invalid 5. Updated documentation with: - Exact key storage location and format - Key derivation process details - Migration strategy for existing databases - Build instructions for SQLCipher support - Testing procedures - Security considerations and recommendations 6. Added warnings when safeStorage is unavailable
|
Addressed all critical issues from your review:
The implementation now follows the exact algorithm you suggested in your review. The random key is generated once, encrypted with safeStorage, and persisted to disk so it can be recovered across restarts. |
Summary
Resolves the security vulnerability where the SQLite database is stored unencrypted at the default Electron userData path, readable by any local process.
Changes
Files Modified
src/main/db/database.ts: Enabled SQLCipher encryption during initializationsrc/main/db/encryption.ts: New encryption utility module with secure key managementsrc/main/db/ENCRYPTION_SETUP.md: Documentation for encryption setup and build configurationSecurity Impact
Build Instructions
To build with SQLCipher support:
npm install better-sqlite3 --build-from-source # or use prebuilt SQLCipher binarySee
src/main/db/ENCRYPTION_SETUP.mdfor detailed setup instructions.Resolves
Closes #80