diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 8078bfe..25d4a02 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -17,3 +17,10 @@ **Vulnerability:** The WebUI server was missing standard HTTP security headers (X-Frame-Options, X-Content-Type-Options, Content-Security-Policy, etc.), potentially exposing it to clickjacking, MIME-sniffing, and XSS attacks. **Learning:** Express.js does not include security headers by default. Explicit middleware is required to set them. **Prevention:** Always include security headers middleware in the Express app setup pipeline to set X-Content-Type-Options, X-Frame-Options, Referrer-Policy, and Content-Security-Policy headers. + +## 2026-02-05 - Plaintext Password Migration Pattern +**Vulnerability:** The application stored the WebUI password in plaintext in `config.json`, allowing anyone with file access to read it. +**Learning:** Security upgrades often require migration strategies for existing data. A dual-strategy works best: +1. **Startup Migration:** Check and migrate data when the service initializes. +2. **Opportunistic Migration:** If startup migration is bypassed (e.g., config hot-reload), migrate when the legacy data is successfully used (e.g., on successful login). +**Prevention:** Always store passwords hashed (e.g., PBKDF2, Argon2). When upgrading, ensure backward compatibility by detecting the data format (plaintext vs hash) and upgrading it transparently. diff --git a/src/main/webui/server/AuthManager.ts b/src/main/webui/server/AuthManager.ts index ca333c8..59d23f5 100644 --- a/src/main/webui/server/AuthManager.ts +++ b/src/main/webui/server/AuthManager.ts @@ -58,6 +58,74 @@ export class AuthManager { this.startSessionCleanup(); } + /** + * Initialize authentication manager and migrate password if needed + */ + public initialize(): void { + const config = this.configManager.getConfig(); + const currentPassword = config.WebUIPassword; + + // Check if password needs migration (is not hashed) + if (currentPassword && !currentPassword.startsWith('pbkdf2:')) { + console.log('Migrating WebUI password to secure hash...'); + const hashedPassword = this.hashPassword(currentPassword); + this.configManager.set('WebUIPassword', hashedPassword); + } + } + + /** + * Hash a password using PBKDF2 + */ + private hashPassword(password: string): string { + const salt = crypto.randomBytes(16).toString('hex'); + const iterations = 10000; + const keylen = 64; + const digest = 'sha512'; + + const hash = crypto.pbkdf2Sync(password, salt, iterations, keylen, digest).toString('hex'); + return `pbkdf2:${digest}:${iterations}:${salt}:${hash}`; + } + + /** + * Verify a password against a stored hash (or plaintext for legacy) + */ + private verifyPassword(password: string, storedPassword: string): boolean { + // Handle legacy plaintext passwords + if (!storedPassword.startsWith('pbkdf2:')) { + const passwordBuffer = Buffer.from(password); + const storedBuffer = Buffer.from(storedPassword); + if (passwordBuffer.length !== storedBuffer.length) { + return false; + } + return crypto.timingSafeEqual(passwordBuffer, storedBuffer); + } + + try { + const parts = storedPassword.split(':'); + if (parts.length !== 5) { + return false; + } + + const [scheme, digest, iterationsStr, salt, expectedHash] = parts; + if (scheme !== 'pbkdf2') { + return false; + } + + const iterations = parseInt(iterationsStr, 10); + const keylen = Buffer.from(expectedHash, 'hex').length; + + const hash = crypto.pbkdf2Sync(password, salt, iterations, keylen, digest).toString('hex'); + + const hashBuffer = Buffer.from(hash, 'hex'); + const expectedHashBuffer = Buffer.from(expectedHash, 'hex'); + + return crypto.timingSafeEqual(hashBuffer, expectedHashBuffer); + } catch (error) { + console.error('Password verification error:', error); + return false; + } + } + /** * Validate login credentials and generate token */ @@ -75,19 +143,21 @@ export class AuthManager { // Check if password matches // Ensure password is a string to prevent Buffer.from throwing on undefined/null const passwordInput = typeof request.password === 'string' ? request.password : ''; - const passwordBuffer = Buffer.from(passwordInput); - const serverPasswordBuffer = Buffer.from(serverPassword); - if ( - passwordBuffer.length !== serverPasswordBuffer.length || - !crypto.timingSafeEqual(passwordBuffer, serverPasswordBuffer) - ) { + if (!this.verifyPassword(passwordInput, serverPassword)) { return { success: false, message: 'Invalid password', }; } + // Opportunistic migration: if we just successfully logged in with a plaintext password, hash it now + if (!serverPassword.startsWith('pbkdf2:')) { + console.log('Migrating WebUI password to secure hash after successful login'); + const hashedPassword = this.hashPassword(serverPassword); + this.configManager.set('WebUIPassword', hashedPassword); + } + // Generate session token const token = this.generateToken(request.rememberMe || false); @@ -281,7 +351,7 @@ export class AuthManager { return { hasPassword: config.WebUIPasswordRequired && !!config.WebUIPassword, - defaultPassword: config.WebUIPassword === 'changeme', + defaultPassword: this.verifyPassword('changeme', config.WebUIPassword), authRequired: config.WebUIPasswordRequired, }; } diff --git a/src/main/webui/server/WebUIManager.ts b/src/main/webui/server/WebUIManager.ts index d5590a1..1e0c5ff 100644 --- a/src/main/webui/server/WebUIManager.ts +++ b/src/main/webui/server/WebUIManager.ts @@ -309,6 +309,9 @@ export class WebUIManager extends EventEmitter { } try { + // Ensure authentication system is initialized (and passwords migrated) + this.authManager.initialize(); + const config = this.configManager.getConfig(); // Check if WebUI is enabled diff --git a/src/main/webui/server/__tests__/AuthManager.test.ts b/src/main/webui/server/__tests__/AuthManager.test.ts index 1c87c68..256fe1c 100644 --- a/src/main/webui/server/__tests__/AuthManager.test.ts +++ b/src/main/webui/server/__tests__/AuthManager.test.ts @@ -90,16 +90,52 @@ describe('AuthManager', () => { rememberMe: false, }); + // Expect calls for both WebUIPassword (migration) and WebUISecret expect(mockConfigManager.set).toHaveBeenCalledWith('WebUISecret', expect.any(String)); }); it('should use existing WebUISecret if present', async () => { mockConfig.WebUISecret = 'existing-secret'; + // Use a hashed password to prevent opportunistic migration from triggering 'set' + mockConfig.WebUIPassword = 'pbkdf2:sha512:10000:somesalt:somehash'; + // We need to mock verifyPassword internal logic or just rely on it returning false? + // Wait, verifyPassword calculates hash. If I put a fake hash, verification fails. + + // Easier: Let migration happen, but expect 'WebUISecret' NOT to be set. + mockConfig.WebUIPassword = 'securepassword123'; + await authManager.validateLogin({ password: 'securepassword123', rememberMe: false, }); + expect(mockConfigManager.set).toHaveBeenCalledWith('WebUIPassword', expect.stringMatching(/^pbkdf2:/)); + expect(mockConfigManager.set).not.toHaveBeenCalledWith('WebUISecret', expect.any(String)); + }); + + it('should migrate plaintext password on login', async () => { + mockConfig.WebUIPassword = 'securepassword123'; + + const result = await authManager.validateLogin({ + password: 'securepassword123', + rememberMe: false, + }); + + expect(result.success).toBe(true); + expect(mockConfigManager.set).toHaveBeenCalledWith('WebUIPassword', expect.stringMatching(/^pbkdf2:/)); + }); + }); + + describe('initialize', () => { + it('should migrate plaintext password', () => { + mockConfig.WebUIPassword = 'securepassword123'; + authManager.initialize(); + expect(mockConfigManager.set).toHaveBeenCalledWith('WebUIPassword', expect.stringMatching(/^pbkdf2:/)); + }); + + it('should not migrate already hashed password', () => { + mockConfig.WebUIPassword = 'pbkdf2:sha512:10000:salt:hash'; + authManager.initialize(); expect(mockConfigManager.set).not.toHaveBeenCalled(); }); });